Repository navigation
feat(api): branch from a point in time with at: "time:<ISO>" - #1969
Conversation
create_branch reported an unborn source as an internal error, and the server's status mapping had no arm for InvalidBranch, so every InvalidBranch reaching it through a direct API call was a 500 as well. That also covers merge-preview's documented 400s (previewing a root branch, merging a branch into itself).
datetime_to_t and recorded_to_t reported a time before the first commit as ApiError::Internal, so `@time:` / `@recorded:` there reached HTTP clients as a 500 err:system/InternalError and the CLI as "Internal error: ...". It is the caller asking for data before the ledger existed. Report it as an invalid query, the class the same resolver already uses for a malformed timestamp. The message is unchanged. Query `from`, SPARQL `FROM` and export all resolve through here; the test pins the status on both query surfaces.
Branch creation parsed `at` with CommitRef::parse, which knows t:, commit:, bare integers and CIDs and treats anything else as a commit prefix. `at: "time:2026-01-01T00:00:00Z"` therefore reached the prefix scan and came back 404, while queries and export accept that spelling. A branch point is the source as of some point, which is what TimeSpec describes, so `at` now uses that grammar and its resolver rather than a second parser kept in step by hand: - Fluree::create_branch takes Option<TimeSpec> in place of Option<CommitRef>. Everything but a commit spelling resolves through time_resolve::resolve_time_spec, the resolver queries and export use, and then to the commit at that t, so a branch at time:X is headed by exactly the commit a query at @time:X reads. Commit spellings still resolve directly, and a CID stays exact, so every value accepted before resolves as before. - POST /branch and local `fluree branch create --at` parse with TimeSpec::parse_at, the function behind export's `at` and query's `--at`. Remote CLI mode sends the string through to the same parse, so both modes accept the same spellings with the same error text. Accepted: t:<N> and bare integers, time:/iso: and bare ISO-8601, recorded:, commit:<prefix> and bare hex prefixes, full CIDs, and latest / t:latest (the source head). A time before the source's first commit, a malformed timestamp, and snapshot:<id> (a graph-source table snapshot, never a commit on a ledger) are InvalidBranch, a 400. A bare prefix under six characters is now refused at parse time (400) instead of by the resolver (500). CommitRef::parse is unchanged. Its remaining callers (branch revert and its preview, show) name a commit to act on, where a timestamp would select a commit nobody identified.
List time:/iso:, recorded:, latest and the bare timestamp form for `fluree branch create --at` and POST /branch `at`, with the 400 cases, and add a point-in-time example to the CLI reference, the endpoint reference and the branching cookbook. The CLI-to-server contract now documents the `at` field the CLI already sends. Drop the claim that t: and hex-prefix resolution need an indexed source: both resolvers scan novelty, and the HTTP suite branches at t:2 with indexing disabled. `show` and `branch revert` now say they accept the commit spellings of `branch create --at`, since they take no timestamps. The Rust create_branch example gains its fourth argument.
bplatz
added
enhancement
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
This is really nice, @bplatz — deleting the second parser rather than teaching it new words is exactly the right move, and the CommitRef::parse-stays-put reasoning (its remaining callers name a commit to act on, where a timestamp would select a commit nobody identified) is the part I'd have worried about and you'd already settled it. I spent most of my time trying to break the time semantics and couldn't: parse_time_travel_iso is parse_from_rfc3339, so an offset-less time:2026-01-01T00:00:00 is refused rather than silently read as UTC; the boundary is inclusive because probe_timestamp_axis uses an exclusive lower bound and then steps back one, and the parity test pins that instant explicitly; a future time collapses to the head exactly as @time: does on a query, index copy and all; and resolved_t = 0 is unreachable because event time is enforced monotonically non-decreasing at commit (fluree-db-transact/src/commit.rs:487-505), so the .max(0) stays defensive rather than load-bearing. The docs commit's quiet correction — dropping the "requires an indexed source" claim for t:/prefix resolution, because both resolvers pass novelty to range_with_overlay — is the kind of stale-doc fix that usually never happens.
On the security question I went looking for a time-travel policy escape and there isn't one here: /branch is inside v1_admin_protected_writes behind require_admin_token (routes/mod.rs:136), the view branch_point_commit resolves against reads only commit-metadata flakes and returns an id rather than data, and branching at a historical point was already reachable via at: "t:5" — this PR widens spellings, not privileges.
Two optional notes, both of which you already named in "Found along the way": /v1/fluree/multi-query's asOf still reports a pre-genesis time as a 500, because From<EnvelopeSnapshotError> for ApiError flattens everything to internal — and that's the one non-admin route where a caller typo still reads as a server fault, which also makes the new comment at time_resolve.rs:108 ("a 400 on every surface that resolves one") slightly ahead of the code. And at: "t:0" / "0" / "-3" on POST /branch is still a 500 out of resolve_t_to_commit_id. Both look like one-constructor fixes in the same direction as the second commit, so I'd rather see them here than filed; if you'd rather keep the commit tight on the second one, that's fine, but I'd at least narrow that comment so it doesn't read as settled.
Two smaller things for the record rather than for changing: the body says all three paths "report the same error text" — the TimeSpec::parse_at message is identical, but the wrappers differ (invalid --at value: … in the CLI vs invalid 'at' time spec: … from routes/export.rs:210), which is fine, just not literally the same string. And on merge order — 119068e1 is #1945's commit riding along in this diff, so approving this is also approving that server InvalidBranch → 400 mapping. I checked its blast radius: all ~25 InvalidBranch producers across merge / merge_preview / rebase / revert / revert_preview / loading are caller-input validation, and ApiError::status_code() had already said 400 for it (fluree-db-api/src/error.rs:618) while the server's catch-all was quietly turning it into a 500 — so that arm is aligning two tables that had diverged, not loosening anything. Whichever of #1945 / #1969 lands first, the other needs a retarget.
fwiw, for the pin: fluree/solo sits on fluree-db-api rev ffeeec554 (this PR's merge-base) and its only create_branch call is handler.rs:3368 passing None, so the Option<CommitRef> → Option<TimeSpec> break is a no-op for us when the pin next moves.
Adherence to repo commitments:
- Patterns/abstractions: ✔ Collapses branch creation onto
TimeSpec::parse_at+time_resolve::resolve_time_spec— the existing shared grammar and resolver — instead of widening a second parser; the one deliberate exception (AtCommitresolving directly) is documented and I verified the reasoning holds. - Performance (speed first, memory second): ✔ No hot path touched. Resolving by time is one or two bounded POST probes at
flake_limit(1)plus one atflake_limit(16)— no scan that grows with history; the linearcollect_first_parent_cidswalk inverify_ancestoris unchanged and already ran for--at t:N. Theto_ledger_state()→from_state()round trip isArcrefcount traffic, not a deep copy. No performance-degradation risk. - Testing: ✔ Nine new/changed tests, all confirmed running by name in the green
testjob onf9f6c2946(run 36274812888). Wiring checked against theautotests = falsetrap:it_branch.rs→grp_ledger:4-5,it_query_time_travel.rs→grp_query_history:16-17, serverintegration.rs→grp_http:18-19;fluree-db-clihas noautotests = false. Nothing orphaned. The parity test asserts against the resolver rather than hard-coded values, so it can't drift. - Conventions: ✔ Four self-describing subjects with substantial multi-line bodies, no AI-attribution trailers, fmt/clippy green on the exact head, and the docs actually track the behavior change across six files — including the
show/branch revertcross-references that no longer point atbranch create --at.
Verified locally at branch HEAD f9f6c2946: traced only — no local compile (the workspace's --all-features build is broken on my machine via vector → usearch → cxx, and CI already ran the full --all-features nextest on this exact SHA with every new test present by name, so a narrower local run would have proved less). I pulled the CI log and grepped each new test name, traced the timezone / boundary / future-time / pre-genesis / t=0 paths through time_resolve.rs and ledger_view.rs, enumerated every Fluree::create_branch call site and every InvalidBranch producer, and read the /multi-query error path end to end.
Approving so you can merge when ready — but if you agree on the multi-query one, I'd rather see it in this PR than tracked and forgotten.
|
|
||
| // Check if target is before earliest commit | ||
| // A time before the ledger existed is the caller's mistake, not a fault: | ||
| // a 400 on every surface that resolves one (query, export, branch). |
There was a problem hiding this comment.
fluree-db-api/src/time_resolve.rs:108 — optional. This is more of a question than a suggestion, but I think the comment's "every surface" is one surface short, and the gap is on a route ordinary users can reach.
/v1/fluree/multi-query's asOf resolves through this exact function — query/multi/snapshot.rs:161 calls datetime_to_t directly — but impl From<EnvelopeSnapshotError> for ApiError at query/multi/snapshot.rs:63-67 flattens every variant to ApiError::internal(err.to_string()), and fluree-db-server/src/routes/query.rs:4434 hands that straight back as ServerError::Api. So the new invalid_query class is re-typed to Internal one frame later and the caller still gets a 500 err:system/InternalError. /multi-query sits in the plain v1 router (routes/mod.rs:343), not the admin bracket — unlike /branch and /export — so it's the one data-token surface where a caller typo is still reported as a server fault.
POST /v1/fluree/multi-query with {"asOf": "1999-01-01T00:00:00Z", "queries": {...}} on any ledger → 500 err:system/InternalError, where the same instant on POST /v1/fluree/query now correctly returns 400. EnvelopeSnapshotError::InvalidIso gets the same treatment, so a straightforwardly malformed asOf timestamp is a 500 too.
The fix looks like it stays inside that one From:
impl From<EnvelopeSnapshotError> for ApiError {
fn from(err: EnvelopeSnapshotError) -> Self {
match err {
// The caller named an instant we can't use — same class the
// per-surface resolvers now use.
e @ EnvelopeSnapshotError::InvalidIso { .. } => ApiError::invalid_query(e.to_string()),
EnvelopeSnapshotError::PerLedgerResolve { ledger, source } => match source {
ApiError::Query(_) => ApiError::invalid_query(format!(
"failed to resolve asOf for ledger '{ledger}': {source}"
)),
other => ApiError::internal(format!(
"failed to resolve asOf for ledger '{ledger}': {other}"
)),
},
other => ApiError::internal(other.to_string()),
}
}
}I recognize this is adjacent scope and you flagged it deliberately — but it's the same defect the second commit exists to fix, on the one surface where a normal user hits it, and the comment on this line is currently making a promise the code doesn't quite keep. If you agree it's right, I'd rather see it folded in here than lost in the backlog. If you'd rather keep the commit tight, then minimally we may want to narrow the comment to name the surfaces it actually covers, so the next person doesn't read it as settled. Commenting here because fluree-db-api/src/query/multi/snapshot.rs is not in this diff.
There was a problem hiding this comment.
Agreed, and fixed in ad4edee before the merge. The From now keeps the class of the caller's mistake: an asOf that is malformed, or that falls before a referenced ledger's first commit, is a 400 err:db/InvalidQuery naming the ledger. I matched QueryError::InvalidQuery rather than any Query(_), since Query also carries cancellations and storage denials.
Writing the test turned up a third case on the same route. A ledger that doesn't exist was also a 500, with or without asOf, because LedgerLoad was flattened the same way. It's now passed through as the 404 it already was. Other load and resolve failures stay 500.
The new tests multi_query_asof_naming_no_data_returns_400 and multi_query_unknown_ledger_returns_404 each failed with their arm reverted. The comment at time_resolve.rs:108 now names multi-query asOf, and the status table in docs/api/multi-query.md lists the 400 and 404 cases.
| other => other, | ||
| })?; | ||
| LedgerView::from_state(&state) | ||
| .resolve_commit(CommitRef::T(t)) |
There was a problem hiding this comment.
fluree-db-api/src/ledger/loading.rs:460 — optional. The other half of the same story: at: "t:0", at: "0" and at: "-3" still come back as 500s from this hop.
parse_time_travel_spec puts no lower bound on t: (fluree-db-core/src/ledger_id.rs:143-152), resolve_time_spec's AtT arm returns the value verbatim, and then resolve_t_to_commit_id raises ApiError::query("Transaction number must be >= 1, got 0") at fluree-db-api/src/ledger_view.rs:463-467 — and ApiError::query builds an ApiError::Internal (error.rs:560-562), so it lands as 500 err:system/InternalError. Because this resolve_commit call sits outside the map_err above it, the InvalidBranch relabel doesn't reach it either.
POST /v1/fluree/branch {"ledger":"mydb","branch":"x","at":"t:0"} → 500, on the same endpoint where {"at":"time:1999-01-01T00:00:00Z"} now correctly returns a 400 err:api/BadRequest. Same for a bare "0" or "-3", since parse_at reads a bare integer as a t.
I don't think this is more than a one-constructor swap — ApiError::query → ApiError::invalid_query at ledger_view.rs:464 — and it would improve @t:0 on the query surfaces in exactly the same direction as the second commit. Worth noting it lands as err:db/InvalidQuery rather than InvalidBranch on the branch path, since it's outside the mapper; if you'd rather it read as a branch error, moving the resolve_commit(CommitRef::T(t)) call inside the same map_err would do it. You did call this one out in the PR body, so this is genuinely "if you agree, now rather than later" — not a disagreement with the scoping.
There was a problem hiding this comment.
Fixed in 7673457, both halves:
resolve_t_to_commit_idnow usesinvalid_queryfor atbelow 1.- The branch path's commit lookup moved inside the same relabel.
t:0,0and-3now read asInvalidBranch/err:api/BadRequest, like the other points that name no commit.
branch revert and its preview resolve commits the same way, so they get the 400 too. The API and HTTP bad-request tests now include all three values, and reverting either half failed them.
| async fn branch_point_commit(view: LedgerView, source_id: &str, at: TimeSpec) -> Result<CommitId> { | ||
| if let TimeSpec::AtCommit(spelling) = at { | ||
| // Resolved directly rather than through its `t`: a CID stays exact, so | ||
| // an off-line commit reaches `verify_ancestor`'s explanation. |
There was a problem hiding this comment.
fluree-db-api/src/ledger/loading.rs:443 — praise. This early return is the subtle call in the whole change and the two-line comment undersells it. I traced the counterfactual: if AtCommit went through resolve_time_spec, it'd hit time_resolve::commit_to_t, which for a commit that isn't on the source's line returns ApiError::query("No commit found with prefix: …") — an ApiError::Internal, so a 500 — and create_branch_at_merged_in_commit_fails would have lost the "a commit that arrived through a merge cannot be branched at…" explanation that makes that error useful. Keeping the CID exact is what routes it into verify_ancestor instead. Please leave the comment in; it's the kind of thing that gets "simplified" away in six months.
There was a problem hiding this comment.
Thanks. The comment stays, and this thread records the counterfactual in case anyone is tempted to route AtCommit through resolve_time_spec later.
| /// A branch at `time:X` is the source as a query at `@time:X` reads it: the | ||
| /// same `t` and the same rows, at every position relative to the commits. | ||
| #[tokio::test] | ||
| async fn create_branch_at_time_matches_query_at_time() { |
There was a problem hiding this comment.
fluree-db-api/tests/it_branch.rs:532 — praise. Asserting the branch's t against db_at(source, spec).t and its rows against a @time: query — rather than against hard-coded expectations — is the right shape for a parity claim: it can't drift if the resolver changes, which is the whole point of collapsing onto one resolver. The four instants cover the two cases I'd have gone looking for (exactly on a commit's event time, and past the head), and create_branch_at_recorded_uses_the_recorded_axis pinning t=2 on the recorded axis against t=3 on the event axis for the same instant is a genuinely good way to prove the axes aren't silently aliased.
There was a problem hiding this comment.
Thanks. That's why the test asserts against db_at and a query rather than fixed numbers: if the resolver changes, the test follows it instead of going stale.
# Conflicts: # docs/api/endpoints.md # docs/cli/server-integration.md # fluree-db-api/src/ledger/loading.rs # fluree-db-server/src/error.rs # fluree-db-server/src/routes/ledger.rs
Resolving `t:0`, `0` or `-3` to a commit raised `ApiError::query`, an internal error, so `POST /branch` with such an `at` answered 500 `err:system/InternalError`. The check now returns `invalid_query`, which also gives `branch revert` and its preview a 400 for the same input. Branch creation's commit lookup now runs inside the same relabel as the time resolver, so a `t` below 1 reads as `InvalidBranch` like the other points that name no commit on the source.
`From<EnvelopeSnapshotError> for ApiError` turned every snapshot failure into an internal error, so `/multi-query` answered 500 where `/query` answers 400 or 404: - an ISO `asOf` before a referenced ledger's first commit, or a malformed one, is now a 400 `err:db/InvalidQuery`, naming the ledger; - a referenced ledger that does not exist is now a 404, with or without `asOf`. Other load and resolve failures stay 500. The HTTP status table in the multi-query docs lists the new 400 and 404 cases.
|
Thanks for the thorough review. For the record, here's what changed after it; the PR is now merged:
|
2 participants
Problem
POST /branchwithat: "time:2026-01-01T00:00:00Z"returned 404. Branch creation parsedatwithCommitRef::parse, which recognises only:t:commit:Anything else fell through to a commit-prefix lookup, so the literal string
time:2026-…was searched as a hash prefix. Queries and export already accepttime:, which is how people think about a branch point ("the data as of last quarter").What this does
POST /branch,Fluree::create_branchandfluree branch create --at(local and remote) now accept the sameatspellings as queries and export. A branch attime:Xis headed by exactly the commit a query at@time:Xreads.TimeSpecdescribes, soatuses that grammar and its resolver. There is no second parser kept in step by hand.commit:, bare hex prefix, CID) still resolve directly. A CID stays exact, so a commit that isn't on the source's line of commits still getsverify_ancestor's explanation.time_resolve::resolve_time_spec, the resolver queries and export use, and then to the commit at thatt.POST /branchreuses export'sparse_time_spec, a thin wrapper aroundTimeSpec::parse_at. Local CLI mode reusesquery --at's parser. Remote CLI mode passes the string through to the server, so all three paths accept the same spellings. They report the same underlying message, inside different wrappers:invalid --at value: …in the CLI,invalid 'at' time spec: …from the server.CommitRef::parseis unchanged. Its remaining callers (branch revertand its preview,show, and the server's revert routes) name a commit to act on. There, a timestamp would select a commit nobody identified.Rust API change
Fluree::create_branch(ledger, branch, source, at: Option<TimeSpec>)replacessource_commit: Option<CommitRef>.Nonecompile unchanged.TimeSpec::at_commit(cid.to_string()).Accepted forms and statuses
Accepted:
t:Nand bare integerstime:/iso:and bare ISO-8601recorded:commit:<prefix>and bare hex prefixes of 6 or more characters, with or withoutsha256:/fluree:commit:latest/t:latestEvery value accepted before resolves to the same commit as before.
time:between two commitstime:after the headat, and the index is copied)recorded:InvalidBranchInvalidBranchsnapshot:<id>InvalidBranch(a graph-source table snapshot, never a commit on a ledger)t:0,0, a negativetInvalidBranch(was a 500)time:with no value,t:abcAlso changes queries: a time before the first commit is a 400
datetime_to_tandrecorded_to_treported a time before the first commit asApiError::Internal. So@time:/@recorded:there reached HTTP clients as a 500err:system/InternalError. Branching must use the same resolver and must return a 400, so both now returninvalid_query, the class the resolver already uses for a malformed timestamp. The message is unchanged. This also reaches JSON-LDfrom, SPARQLFROMand export. It is a separate commit.Also: other time and
trefusals that were 500sAdded after review, one commit each:
A
tbelow 1. Resolvingt:0,0or-3to a commit raisedApiError::query, an internal error. It now returnsinvalid_query. Branch creation's commit lookup also moved inside theInvalidBranchrelabel, so it answers like the other points that name no commit.branch revertand its preview resolve commits the same way, so they now return a 400 for the same input.Multi-query
asOf.From<EnvelopeSnapshotError> for ApiErrormade every snapshot failure internal. Now:asOfbefore a referenced ledger's first commit, or a malformed one, is a 400err:db/InvalidQuerynaming the ledger;asOf. It was a 500 even with noasOf.Other load and resolve failures stay 500. The status table in
docs/api/multi-query.mdlists the new cases.Docs
docs/cli/branch.md,docs/api/endpoints.md(POST /branch) anddocs/guides/cookbook-branching.mdlist the accepted forms and show a point-in-time example. The CLI help text and theCreateBranchRequestdoc match.docs/cli/show.mdand the revert paragraph ofbranch.mdnow say they take commit spellings only. Before, they cross-referenced "the same forms asbranch create --at".docs/cli/server-integration.mdlists theatfield the CLI already sends.docs/concepts/ledgers-and-nameservice.mdgains its fourth argument.t:and hex-prefix resolution need an indexed source. Both resolvers scan novelty, and the HTTP suite branches att:2with indexing disabled.Tests
For each change below, the test(s) listed after it were run with that change reverted and failed, then the change was restored.
it_branch):create_branch_at_time_matches_query_at_timecovers four instants. For each it checks:tand head;tequalsdb_at(source, spec).t;@time:.Event and recorded times are pinned through
CommitOpts.create_branch_at_iso_and_latestcreate_branch_at_recorded_uses_the_recorded_axis: the same instant gives t=2 on the recorded axis and t=3 on the event axis.create_branch_at_a_point_with_no_commit_is_a_bad_requestcovers a time before the first commit on both axes, a malformed timestamp,t:0,0,-3andsnapshot:. Each returnsInvalidBranch, and no branch record is created.time_travel_iso_too_early_errorsasserts 400 on JSON-LDfromand on a new SPARQLFROMtwin.create_branch_at_time:time:gives t=1 andiso:gives t=2. A time before the first commit, a malformed timestamp,t:0,0,-3,snapshot:7and a baretime:each return 400 with@typeerr:api/BadRequest.multi_query_asof_naming_no_data_returns_400: anasOfbefore the first commit and a malformed one.multi_query_unknown_ledger_returns_404: with and withoutasOf.branch_create_at_time_matches_query_at_time: the branch reads likequery --at, and a time before the first commit fails.help_text_lists_the_accepted_spellingsnow requires the time spellings forbranch create.time:resolved on the recorded axist - 1, with the test's hard-codedtand head checks disableddb_at().tcheck alone; with that also disabled, the row checkinternalagainerr:system/InternalError)InvalidQuery→InvalidBranchrelabel disabled@typecheck (err:db/InvalidQuery)atas a raw commit404 No commit found with prefix: time:…) and the existingcreate_branch_at_historical_tatas a raw commit, with the old help textbranch_create_at_and_query_at_accept_the_same_spellingstbelow 1 isApiError::queryagainerr:system/InternalError)@typecheck (err:db/InvalidQuery)asOfinternal againasOftest's malformed caseasOfinternal againasOftest's pre-genesis caseAdjacent paths, checked and left alone: revert, revert preview,
showand the server'sparse_commit_refstill useCommitRef::parse. Merge, rebase and merge-preview take no commit refs.Not verified
fluree-db-api:grp_ledger,grp_query_historyand--lib.fluree-db-server:grp_http, plusgrp_queryafter the review fixes.fluree-db-cli:integration,time_travel_specanddocs_coverage. Not re-run after the review fixes, which change no CLI code.fluree-db-api,-server,-cliand-consensus.fluree-db-consensustests were not run; the crate compiles under clippy--all-targets.create_branch_at_merged_in_commit_failscovers the behavior it protects.Found along the way, not changed
ApiError::query: "Ambiguous t=N", a too-short prefix passed directly to the Rust API, an abbreviated CID, and an ambiguous prefix. They are part of theApiError::queryaudit.Follow-up: ApiError::query returns 500 for caller errors: audit its callers onto invalid_query #1908
t:Nandtime:Xambiguous. That did not reproduce. Probing it found a different bug: a rebase can leave the branch on the index built from its pre-rebase chain, so after a reload the branch silently loses the source's commits.Follow-up: Rebase leaves the branch's own index in place when the source's is older or absent, so the rebased branch reads pre-rebase data #1985
reindexfails on any branch whose history reaches back before the fork. It reads commits through the flat store instead of the branch-aware one.Follow-up:
reindexfails on any branch: it reads commits through the flat store and misses the ones before the fork #1984