feat(api): serve retained document content and versions - #860
Conversation
Signed-off-by: wangzifei <wangzifei@cit.group.hk>
|
Read it end to end. CI approved — fork PRs need that here after every push. This is careful work, and the parts I went looking for trouble in are the parts you got right. Authentication. A new route that serves raw bytes is where I expected to find the hole. Instead there are two independent checks: Ingest tokens are rejected structurally rather than by a check that could be forgotten: A scoped token gets the same The transaction commits before the response is built.
Two things. 1. 2. One sentence missing from the Limits section, which is otherwise the right section. You say the route "buffers within the existing upload cap", and Neither blocks. Once CI reports, and with the index rows added, this looks good to me. |
|
Cross-posting on #860 and #863: these two implement the same feature from #859 and .route("/documents/{id}/content", get(documents_routes::content))
.route("/documents/{id}/versions", get(documents_routes::versions))Neither exists on Worth a maintainer call on which one to keep before either gets more review |
WaylandYang
left a comment
There was a problem hiding this comment.
Reviewed against #859's spec and side by side with #863 (the other implementation
of the same issue — see my note there). This is the one that should land.
It covers the requirement the other branch misses: session or scoped PAT at
Viewer level, with ingest tokens rejected. The DocumentReader extractor is the
right shape — AuthUser can't do this because it treats every bearer as a JWT,
and the comment says exactly that. Holding FOR NO KEY UPDATE from the ledger read
through the blob read closes the replacement/purge window instead of asking the
client to retry, and re-checking purged_at under the lock is the detail that
makes it actually hold. Resolving the default version through document_versions
rather than trusting documents.sha256 directly, and treating a missing ledger row
as an invariant error rather than a 404, matches the issue's "500 not 404"
semantics. Header construction that fails loudly on a bad MIME or an unencodable
filename is preferable to silently emitting a wrong ETag. ADR 0052 says all of
this out loud, including the compatibility boundary on reusing current display
metadata for historical bytes.
Verified on a merge of this branch into current dev (after #833/#845/#864
landed), against pgvector/pgvector:pg16 on a fresh database:
cargo test -p utopia-server documents → 12 passed, 0 failed
(all five new cases ran, including
content_reads_keep_viewer_access_pat_scope_and_reject_source_tokens)
cargo test -p utopia-store → 329 passed, 0 failed
cargo clippy --workspace --all-targets -- -D warnings → clean
One thing for a follow-up rather than this PR: none of these tests run in CI.
documents_routes_tests gates on test_db::url(), so in the backend job (no
database) all of them skip and report green, and the migrations job's server
filters are api::mcp::tests, api::chat::, retrieval:: — documents isn't
among them. That's true of the existing documents tests on dev too, not just
these five. A one-line step in the migrations job, the way #845 added one for
api::chat::, would make the PAT/purge/invariant cases actually load-bearing.
Happy to open that separately.
One small thing worth a follow-up from the #863 side, which I've suggested there:
recomputing SHA-256 over the served bytes. This PR checks size_bytes against the
ledger; the digest check is the stronger guarantee for an auditor and is cheap to
add on top.
LGTM.
What
Adds a versioned read contract for retained originals:
GET /api/v1/documents/{id}/contentserves the current or requested?version=Nbytes from the document ledger.GET /api/v1/documents/{id}/versionslistsversion,sha256,size_bytes, andingested_at.410 Gone; soft-deleted retained documents remain readable until purge.Content-Disposition.utp_pat_personal access tokens scoped to the knowledge base; source ingest tokens are rejected.The content transaction takes
SELECT ... FOR NO KEY UPDATEand holds it through the blob read, so replacement or purge cannot move or remove the selected blob after the ledger check. Commit happens after the blob read is complete. A ledger entry whose blob is unavailable returns500as an invariant failure rather than masking it as a normal client error.ADR 0052 records the API/lifecycle boundary and why the ledger is the source of truth.
Why
The document API currently exposes metadata while versions and original bytes stay internal. This makes auditable retention observable to clients, lets callers retrieve the exact recorded original, and closes #859.
Closes #859
Testing
Against Postgres 17 with
UTOPIA_TEST_REQUIRE_DB=1andUTOPIA_DATABASE_URLset:Results:
cargo fmtcompleted without changes.cargo clippypassed with warnings denied.