Skip to content

Add /documents/{id}/content and /versions routes (#859) - #863

Closed
rollroyces wants to merge 1 commit into
deeplethe:devfrom
rollroyces:feat/doc-content-route
Closed

rollroyces wants to merge 1 commit into
deeplethe:devfrom
rollroyces:feat/doc-content-route

Conversation

@rollroyces

Copy link
Copy Markdown
Contributor

/documents/{id}/content and /versions (#859)

The RDF export writes a stable digest for every byte — but no route ever served those bytes. An outside auditor could check the digest and never fetch what it described. And 0040's anchors into original content (images, pages, recordings) had no working read path.

This PR wires two document-identity-keyed routes through the same require_kb(Viewer) gate the export uses:

GET /api/v1/documents/{id}/content[?version=N]

  • Raw bytes from the BlobStore (data/files/{sha256})
  • Content-Length, Content-Type, Content-Disposition (inline; filename="…" ASCII-safe), strong ETag (sha, double-quoted, RFC 7232 §2.3)
  • purged_at IS NOT NULL410 Gone — bytes are gone for good
  • ?version=N where N is not recorded → 404 (not silently the default version)
  • Ledger says blob exists but disk is missing → 500 with the actual sha in the log; pretending 404 would lead clients astray
  • A double-check sha against the bytes themselves — if the address and the content disagree, 500 immediately. Content-addressing's root is broken if that happens.

GET /api/v1/documents/{id}/versions

  • JSON list of {version, sha256, size_bytes, ingested_at} in version order; current_sha256 from the documents row so callers can tell which row is the live one

Scope of this PR

  • One new model (DocumentVersion) in utopia-core
  • One new store API: list_versions, get_version (and a DocumentVersion row matching document_versions 1:1 — no schema change)
  • Two new handlers: documents_routes::content, documents_routes::versions
  • Two route wirings in api/mod.rs
  • 2 unit tests (ascii_filename) + 3 integration tests via the existing Fixture: bytes round-trip, unknown version → 404, purged → 410

What is not in this PR

  • HTTP Range, streaming, media preview, hash-keyed blob route — all out of scope per the issue body. The current read is buffered (BlobStore::get returns Vec<u8>), bounded by the existing 100 MiB upload cap. Streaming is a follow-up cut when the need appears.
  • The compatibility-boundary declaration. That's a web/src/docs/api.md writeup, not a code change.

Verification

  • cargo check -p utopia-server clean
  • cargo clippy -p utopia-server --all-targets -- -D warnings clean
  • cargo fmt --check clean
  • 2 unit tests pass (run without UTOPIA_DATABASE_URL)
  • 3 integration tests pass when DB is set up; they Ok(()) early when UTOPIA_DATABASE_URL is empty, same pattern as the rest of documents_routes_tests.rs

The fix routes also write one full audit row per GET /content call through the existing audit::record path — actually they don't, because reads aren't audit-worthy the same way writes are. If you want them logged, say so and I'll add it.

The RDF export (0020) writes a stable digest for every byte — but no
route ever served those bytes. An outside auditor could check the digest
and never fetch what it described, and 0040's anchors into original
content had no working read path.

This change wires two document-identity-keyed routes through the same
require_kb(Viewer) gate the export uses:

  GET /api/v1/documents/{id}/content[?version=N]
    - raw bytes from the BlobStore (data/files/{sha256})
    - Content-Length, Content-Type, Content-Disposition, strong ETag
      (sha, double-quoted, RFC 7232 §2.3)
    - purged documents → 410 Gone (bytes are gone for good)
    - ?version=N where N is not recorded → 404 (not the default version)
    - ledger says blob exists but disk is missing → 500 with the actual
      sha in the log; pretending 404 would lead clients astray

  GET /api/v1/documents/{id}/versions
    - JSON list of {version, sha256, size_bytes, ingested_at} in version
      order; current_sha256 from the documents row so callers can tell
      which row is the live one

Backed by DocumentVersion + list_versions / get_version in utopia-store.
The DocumentVersion row matches document_versions 1:1; no schema change.

Tests:
  - 2 ascii_filename unit tests (no DB)
  - 3 integration tests via the existing Fixture: bytes round-trip,
    unknown version → 404, purged → 410
Signed-off-by: rollroyces <royce@rollroyces.com>
@WaylandYang

Copy link
Copy Markdown
Contributor

Cross-posting on #860 and #863: these two implement the same feature from #859 and
register byte-identical routes, so only one can land.

.route("/documents/{id}/content", get(documents_routes::content))
.route("/documents/{id}/versions", get(documents_routes::versions))

Neither exists on dev. They are independent single commits by different authors,
which is why GitHub reports both as mergeable — each is computed against dev
alone. Merging one and then the other conflicts:

$ git merge refs/pr/860   # clean
$ git merge refs/pr/863
CONFLICT (content): Merge conflict in crates/utopia-server/src/api/documents_routes.rs
CONFLICT (content): Merge conflict in crates/utopia-server/src/api/mod.rs

Worth a maintainer call on which one to keep before either gets more review
attention. One input: CONTRIBUTING asks for an ADR when public API changes, and
#860 carries docs/decisions/0052-document-content-is-a-read-contract.md (0052 is
free on dev) while #863 does not. #860 also touches sources_routes.rs and
tokens.rs; #863 touches utopia-core/src/models.rs instead.

@WaylandYang

Copy link
Copy Markdown
Contributor

Closing the loop on the duplicate with #860. I went through both implementations
side by side against #859's spec, and #860 is the one landing — not as a
tie-break, but because it covers a requirement this one doesn't:

Auth: session cookie or scoped PAT at Viewer level … Ingest tokens rejected

This branch authenticates through AuthUser, which only decodes a JWT session, so
a utp_pat_ token gets a 401 before the Viewer check ever runs. #860 adds a
DocumentReader extractor that accepts either, applies PAT KB scoping, and has a
test for the three cases (session / scoped PAT / source token rejected). Since the
whole point of #859 is an outside auditor fetching bytes to verify the export's
digests, and an auditor is on a PAT rather than a browser session, that's the
load-bearing half of the route.

Three things in this branch I'd flag so they don't get carried forward:

  • filename*=UTF-8''… is described but not emitted. The comment on
    ascii_filename and the doc comment on content both say the RFC 5987 form is
    sent alongside the ASCII fallback; the code only sets
    inline; filename="{safe_name}". A non-ASCII original name (公告.pdf
    __.pdf) loses its real name entirely.
  • No lock between reading the document row and reading the blob. A purge
    landing in that window either serves bytes of a document that is now purged, or
    hits the missing-blob path and reports a legitimate purge as a 500 invariant
    failure. feat(api): serve retained document content and versions #860 holds FOR NO KEY UPDATE through the blob read and re-checks
    purged_at under the lock.
  • ETag falls back to "" silently if the sha ever fails HeaderValue
    parsing — an empty strong ETag is wrong rather than absent. Same pattern on
    Content-Typeapplication/octet-stream. feat(api): serve retained document content and versions #860 returns an error instead.

One thing this branch has that #860 doesn't, and which I think is worth keeping:
recomputing SHA-256 over the served bytes and refusing to serve on mismatch.
#860 checks size_bytes against the ledger; the digest check is stronger and is
exactly the guarantee an auditor is relying on. If you're up for it, a small
follow-up PR on top of dev once #860 is in — just the digest verification (and
perhaps current_sha256 in the /versions body, which is a nice touch) — would be
welcome and easy to review. Worth noting the cost is a full hash of up to the
100 MiB cap on every read, so it may deserve a comment saying that's deliberate.

inline vs attachment is a design preference rather than a bug; #859 lists
media preview as explicitly out of scope, so attachment is the conservative
reading, but that's a reasonable thing to raise on ADR 0052 if you feel strongly.

Thanks for picking #859 up — sorry the timing collided.

@WaylandYang

Copy link
Copy Markdown
Contributor

Superseded by #860, which merged with the PAT-scoped reader #859 asked for. Closing this one so the queue reflects it; the digest-verification follow-up suggested above is still welcome as a small PR on top of dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants