Why: - ADR-0002 already answered three of the four questions plan 002 listed as "decisions needed"; the fourth -- what happens to a manually edited point when its file is re-uploaded -- was left for a Phase 6 test to force. Deciding it in code rather than in the ADR would invert this repo's rule. Changes: - ADR-0002 gains "Re-ingestion versus manual edits": the new file wins, surviving points are overwritten with an incremented version, absent points are flagged inactive rather than removed, and manually created points sit past the ingested chunk_index range so the existing sweep covers them. - Plan 002's stale decisions section becomes a pointer table; its audit scope is pinned to both ADR-0009 tables. Impact: - Clobbered edits are recoverable from point_audit_events, not from Qdrant: the deterministic point ID cannot hold both versions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
271 lines
15 KiB
Markdown
271 lines
15 KiB
Markdown
# 002. Point CRUD and keyword-search implementation plan
|
||
|
||
## Purpose
|
||
|
||
This plan covers the milestone named at the end of
|
||
[plan 001](001-ingestion-vertical-slice.md): direct, fine-grained management of
|
||
individual Qdrant points through `/v1/points`, plus filter/keyword search over
|
||
them. Ingestion (plan 001) writes points in bulk; this slice lets a human or
|
||
admin frontend read, edit, reorder, and soft-delete them one at a time, under
|
||
the same tenant isolation.
|
||
|
||
This is an implementation plan, not an Architecture Decision Record. The ADRs
|
||
explain why the collection schema, endpoints, and isolation rules are what they
|
||
are; this document defines order, scope, and verification criteria.
|
||
|
||
## Prerequisite
|
||
|
||
Plan 001 is complete through Phase 6, so this prerequisite is satisfied. It
|
||
required plan 001 through **Phase 5** before Phase 3 of this plan starts.
|
||
Specifically this plan depends on: the `chunks` collection and its
|
||
payload indexes actually existing, API-key authentication and `AuthContext`
|
||
tenant derivation, the application-lifetime Qdrant client from the FastAPI
|
||
lifespan, and the request-lifetime `AsyncSession` wiring. Phases 1–2 below
|
||
(schemas and the read paths) can be built against the collection alone and do
|
||
not need the full ingestion path.
|
||
|
||
## Architecture baseline
|
||
|
||
| System | Responsibility |
|
||
|---|---|
|
||
| FastAPI | HTTP boundary, auth, tenant derivation, request/response schemas, scope checks. |
|
||
| Points application service | Point CRUD semantics: soft delete, ordering, neighbor relinking, optimistic concurrency. |
|
||
| Qdrant | The only store for point content, vectors, and payload. |
|
||
| Postgres | Audit of mutating operations. It does **not** hold a mirror of point state. |
|
||
|
||
The controlling ADRs are:
|
||
|
||
- [ADR-0001](../adr/0001-ingestion-pipeline-and-collection-schema.md): the
|
||
`chunks` collection, payload schema, payload indexes, deterministic point IDs,
|
||
`order_id` as a fractional float, and the
|
||
`previous_chunk_id`/`next_chunk_id` adjacency pointers.
|
||
- [ADR-0002](../adr/0002-chunk-crud-and-search-api.md): the CRUD operation set,
|
||
soft-delete-by-default, reorder semantics, keyword search vs. semantic search,
|
||
and `version`-based optimistic concurrency.
|
||
- [ADR-0008](../adr/0008-rest-api-and-fastapi-boundary.md): the `/v1/points`
|
||
REST surface, `GET /v1/files/{file_id}/points`,
|
||
`DELETE /v1/files/{file_id}`, and the `points:read`/`points:write` scopes.
|
||
- [ADR-0012](../adr/0012-application-resource-lifetime-and-dependency-ownership.md):
|
||
the Qdrant client is application-lifetime and injected, never constructed per
|
||
request.
|
||
- [ADR-0015](../adr/0015-modular-monolith-package-architecture.md): routes call
|
||
`application/points/`, which calls a port implemented in
|
||
`infrastructure/qdrant/`. Routers never build Qdrant filters or call the SDK.
|
||
- [ADR-0016](../adr/0016-testing-strategy-and-quality-gates.md): unit tests
|
||
against a fake point port; Testcontainers Qdrant for the adapter.
|
||
|
||
ADR-0001 through 0004 are Accepted; 0008, 0012, 0015, and 0016 are still
|
||
Proposed. Treat the proposed ones as the implementation baseline only once the
|
||
project owner accepts them, and update the ADR rather than diverging silently.
|
||
|
||
## Scope
|
||
|
||
### In scope
|
||
|
||
- `POST /v1/points`, `GET /v1/points/{point_id}`, `PUT /v1/points/{point_id}`,
|
||
`PATCH /v1/points/{point_id}/payload`, `DELETE /v1/points/{point_id}`.
|
||
- `GET /v1/points?file_id=...` (scroll, `order_by: order_id`, paginated),
|
||
`GET /v1/points/count`, `GET /v1/points/search?q=...`.
|
||
- `PATCH /v1/points/{point_id}/order` with correct neighbor relinking.
|
||
- `POST /v1/points/batch` for bulk multi-operation edits.
|
||
- `GET /v1/files/{file_id}/points` and `DELETE /v1/files/{file_id}`
|
||
(bulk soft delete of a file's points), from ADR-0008.
|
||
- Soft delete as the default for every delete path, with neighbor relinking.
|
||
- Optimistic concurrency on every mutating path via the `version` payload field.
|
||
- Audit rows in Postgres for mutating operations: both ADR-0009 tables,
|
||
`api_request_logs` (one row per API call, written from the request middleware)
|
||
and `point_audit_events` with the real `api_request_log_id` foreign key.
|
||
- Automated tests for tenant isolation, pointer integrity, concurrency
|
||
conflicts, and pagination.
|
||
|
||
### Explicitly out of scope
|
||
|
||
- Hybrid dense+sparse retrieval, RRF fusion, and late-interaction rerank
|
||
(ADR-0003/0005) — that is the next plan, 003. `GET /v1/points/search` here is
|
||
**keyword/filter matching only**; do not let it grow a semantic mode.
|
||
- Re-embedding a point on content edit (see the open decision below).
|
||
- Hard deletion / compliance purge endpoints.
|
||
- The LangGraph conversational API (ADR-0006/0007).
|
||
- A frontend.
|
||
|
||
## Required invariants
|
||
|
||
1. `tenant_id` comes from the authenticated context and is injected into every
|
||
filter server-side — on reads *and* writes, on every code path. A
|
||
`tenant_id`, or any payload key that would override it, appearing in a
|
||
request body or query string is rejected, never honored.
|
||
2. Cross-tenant access returns `404`, not `403` — the same rule plan 001 applies
|
||
to files. A caller must not be able to probe for the existence of another
|
||
tenant's point IDs.
|
||
3. Soft delete is the default: `is_active=false` + `deleted_at` via
|
||
`set_payload`. Points are never removed from Qdrant by any endpoint in this
|
||
slice.
|
||
4. Reads exclude inactive points unless the caller explicitly opts in.
|
||
5. Any operation that changes a point's position or removes it from the sequence
|
||
— insert, reorder, delete — updates the affected neighbors'
|
||
`previous_chunk_id`/`next_chunk_id` in the **same** `points/batch` request.
|
||
A partial relink is a defect: ADR-0003's context-window expansion walks these
|
||
pointers.
|
||
6. Point IDs stay derived from `file_id` + the immutable `chunk_index`.
|
||
Reordering changes `order_id` only, never the point ID.
|
||
7. Every mutating operation is guarded by the `version` payload field via
|
||
Qdrant's `update_filter`, and increments it. A stale write returns `409`,
|
||
it does not silently clobber.
|
||
8. `order_id` is a fractional float. Inserting or moving a point assigns a value
|
||
between its two new neighbors; it never renumbers siblings.
|
||
9. Routers contain no Qdrant SDK calls and no filter construction. The Qdrant
|
||
client is injected from the lifespan (ADR-0012).
|
||
|
||
## Decisions resolved before implementation
|
||
|
||
An earlier revision of this plan listed three open decisions here. All are now
|
||
settled, and one further question this plan deferred to a Phase 6 test has been
|
||
settled too. They are recorded in the ADRs — these lines are a pointer, not a
|
||
second source of truth.
|
||
|
||
| Question | Resolution | Recorded in |
|
||
|---|---|---|
|
||
| Re-embedding on content edit | Re-embed inline, reusing ingestion's ports and bounds and its `502`/`504` codes. The re-embed happens *before* the version-guarded write, so a stale edit still `409`s rather than re-embedding for nothing. | ADR-0002, "Re-embedding on content edit" |
|
||
| Fractional-key exhaustion | No renormalize endpoint in this slice. Log `points.order_id.gap_low` under a safety threshold; reject with `409` and a distinct error code if the gap would collapse onto a neighbor value. Recovery is a runbook operation. | ADR-0002, "`order_id` gap exhaustion" |
|
||
| Batch semantics | All-or-nothing, capped at 100 operations. Every operation's `version` precondition is validated before any is applied; one failure rejects the whole request and nothing reaches Qdrant. | ADR-0002, "`POST /points/batch` semantics" |
|
||
| Re-ingestion versus manual edits | The newly uploaded file wins. Surviving points are overwritten in place with an incremented `version`; points absent from the new version are flagged inactive, never removed; manually created points sit past the ingested `chunk_index` range and are swept by the same rule. Clobbered content is recorded in `point_audit_events` as `reingest_overwrite`. | ADR-0002, "Re-ingestion versus manual edits" |
|
||
|
||
Phase 6's cross-slice end-to-end test therefore *verifies* the re-ingestion rule
|
||
rather than forcing the decision.
|
||
|
||
## Build order
|
||
|
||
### Phase 1: Point contracts and the port
|
||
|
||
1. Define the payload model in `src/application/points/` mirroring ADR-0001's
|
||
field list exactly, with the reserved/server-owned fields (`tenant_id`,
|
||
`version`, `chunk_index`, `deleted_at`) separated from caller-writable ones.
|
||
2. Define the point port: get, upsert, set payload, scroll, count, keyword
|
||
search, batch. Tenant filter is a required parameter on every method — not an
|
||
optional argument a caller can forget.
|
||
3. Implement the Qdrant adapter behind the port in
|
||
`src/infrastructure/qdrant/`, using the injected application-lifetime client.
|
||
4. Add a fake port implementation for unit tests, with ordering and version
|
||
behavior faithful enough to test the service against.
|
||
5. Define the Pydantic request/response schemas in `src/api/schemas/`. Vectors
|
||
are returned only when explicitly requested (ADR-0008).
|
||
|
||
**Exit criteria:** the service layer can be unit-tested end to end against the
|
||
fake; a Testcontainers Qdrant test confirms the adapter's filter construction
|
||
and `order_by` scroll match the fake's semantics.
|
||
|
||
### Phase 2: Read paths
|
||
|
||
1. `GET /v1/points/{point_id}` — tenant-filtered retrieve; `404` when the point
|
||
belongs to another tenant or does not exist.
|
||
2. `GET /v1/points?file_id=...` — scroll with `order_by: order_id`, stable
|
||
pagination, `is_active: true` implied.
|
||
3. `GET /v1/files/{file_id}/points` — same listing, addressed by file.
|
||
4. `GET /v1/points/count` — tenant/domain-filtered count.
|
||
5. `GET /v1/points/search?q=...` — full-text payload match on `content` plus
|
||
structured filters. Name the response and docstring so it cannot be mistaken
|
||
for semantic retrieval.
|
||
6. Enforce `points:read` scope on all of the above.
|
||
7. Tests: tenant isolation returns `404`; inactive points are excluded by
|
||
default and included on explicit opt-in; pagination does not skip or repeat
|
||
under a concurrent insert.
|
||
|
||
**Exit criteria:** a tenant can list and search only its own active points, in
|
||
display order, with correct paging.
|
||
|
||
### Phase 3: Soft delete and neighbor relinking
|
||
|
||
1. Implement the relinking primitive in the service: given a point leaving the
|
||
sequence, compute the neighbor payload updates and emit them with the
|
||
deactivation in one `points/batch` call.
|
||
2. `DELETE /v1/points/{point_id}` — soft delete plus relink.
|
||
3. `DELETE /v1/files/{file_id}` — bulk soft delete of a file's active points.
|
||
The whole file leaves the sequence, so the boundary pointers must end
|
||
consistent (typically all null within that file).
|
||
4. Deleting an already-inactive point is a no-op success, not a `404` and not a
|
||
second relink.
|
||
5. Tests: after deleting a middle point, its old neighbors point at each other;
|
||
after deleting the first point, the new first has a null
|
||
`previous_chunk_id`; a full traversal of a file's pointer chain after a
|
||
series of deletes visits every active point exactly once and never enters an
|
||
inactive one.
|
||
|
||
**Exit criteria:** no delete path can leave a stale or dangling pointer, and
|
||
none removes a point from Qdrant.
|
||
|
||
### Phase 4: Create, replace, and payload update
|
||
|
||
1. `POST /v1/points` — create one point. Server assigns `tenant_id`, `version`,
|
||
and the derived point ID; the caller supplies content, position, and
|
||
metadata. Insert relinks neighbors like a reorder does.
|
||
2. `PUT /v1/points/{point_id}` — upsert with `update_only`, guarded by
|
||
`version` via `update_filter`; `409` on a stale version. Apply the
|
||
re-embedding decision above.
|
||
3. `PATCH /v1/points/{point_id}/payload` — payload-only update. Reject attempts
|
||
to write server-owned fields.
|
||
4. Enforce `points:write` scope; write an audit row per mutation
|
||
(tenant, actor, point, operation, resulting version).
|
||
5. Tests: concurrent writers at the same version — one succeeds, one gets `409`
|
||
and does not mutate; a `tenant_id` or `version` in the request body is
|
||
rejected; a create lands in the right sequence position.
|
||
|
||
**Exit criteria:** manual edits and ingestion re-runs cannot silently clobber
|
||
each other; server-owned fields are unwritable through any endpoint.
|
||
|
||
### Phase 5: Reorder and batch
|
||
|
||
1. `PATCH /v1/points/{point_id}/order` — assign a new fractional `order_id`
|
||
between the new neighbors and relink up to four points in one
|
||
`points/batch`, per ADR-0002.
|
||
2. Handle the boundary moves (to first, to last) and the no-op move
|
||
(already in that position).
|
||
3. Implement the fractional-gap decision from above.
|
||
4. `POST /v1/points/batch` with the semantics decided above, including the
|
||
per-request operation cap.
|
||
5. Tests: a randomized sequence of inserts, moves, and deletes leaves the
|
||
`order_id` ordering and the pointer chain agreeing with each other after
|
||
every step — this property test is the main defense for this phase.
|
||
|
||
**Exit criteria:** display order derived from `order_id` and traversal order
|
||
derived from the pointer chain are identical for every file, after any sequence
|
||
of mutations.
|
||
|
||
### Phase 6: Integration tests, operations, and documentation
|
||
|
||
1. Testcontainers Qdrant integration tests for each endpoint's real filter and
|
||
ordering behavior, isolated per test by unique collection or tenant keys.
|
||
2. An end-to-end test crossing plan 001 and this slice: ingest a CSV, list its
|
||
points, reorder one, soft-delete another, re-upload the same file, and assert
|
||
the manual edits interact with re-ingestion exactly as ADR-0002's
|
||
"Re-ingestion versus manual edits" specifies.
|
||
3. Structured logging at the mutation boundary with stable event names
|
||
(`points.updated`, `points.reordered`, `points.soft_deleted`) carrying
|
||
`request_id`, `tenant_id`, `file_id`, and the resulting version.
|
||
4. Extend the operator runbook: how to inspect a file's point sequence, how to
|
||
spot a broken pointer chain, and how to recover one.
|
||
5. Update the README and ADR-0002 with anything this implementation settled.
|
||
|
||
**Exit criteria:** every endpoint has an integration test against real Qdrant,
|
||
and an operator can diagnose an ordering problem from the logs and the runbook.
|
||
|
||
## Definition of done
|
||
|
||
This slice is done when, in local Compose and under automated test:
|
||
|
||
```text
|
||
ingest a CSV (plan 001)
|
||
-> GET /v1/points?file_id=... lists its active points in order_id order
|
||
-> PATCH .../order moves one point; order_id and the
|
||
previous/next chain still agree
|
||
-> PUT /v1/points/{id} edits content under a version guard;
|
||
a stale write gets 409
|
||
-> DELETE /v1/points/{id} soft-deletes and relinks neighbors
|
||
-> GET /v1/points/search?q=... keyword-matches content within the tenant
|
||
-> every one of the above returns 404, not 403, for another tenant
|
||
```
|
||
|
||
The next plan (003) is agent hybrid retrieval — three prefetches, RRF fusion,
|
||
late-interaction rerank, and context-window expansion over the pointer chain
|
||
this slice is responsible for keeping correct (ADR-0003, ADR-0005). Do not pull
|
||
any of it into this plan.
|