feat(validate): carry the validated ledger t on ValidateReport - #1757
Conversation
The full-ledger validator already pins the snapshot it evaluates (`to_t = view.t` at the top of validate_view_inner), but the report it returned did not say which t that was. A caller that pairs the results with other reads (record counts for a violation rate, for instance) had to read the handle's t before and after the call and bracket the two, because a commit landing between validate_ledger's internal snapshot and the caller's own read silently breaks the pin. Add `ValidateReport.t`, set from the same `to_t` at both construction sites. The serde summary picks it up directly; the W3C renderings carry it on the report node as `f:t` (sh:ValidationReport has no slot for a ledger time), and the server JSON envelope and CLI table line print it. Integration tests assert report.t equals the ledger handle's t through validate_ledger, that it advances with the head, and that the cross-ledger shapes path pins the data ledger's t rather than the model's.
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz this is good and makes sense and I also have some context for understanding its value/utility given @jakep36's comments on fluree/solo#1088. So this is an approve with just some small Claude-found items which I'll leave verbatim below.
This is the right fix in the right place, and I verified the one question that decides whether it's real or cosmetic: the reported t is not a handle read taken around the validate — it's the t of the same immutable LedgerView the validation walks. LedgerView::from_state captures t: state.t() in the same &LedgerState borrow (under the state read lock) as the snapshot/novelty Arcs, and that single to_t variable bounds every GraphDbRef the validator builds and lands on the report at both construction sites. A commit landing mid-validate changes neither what the validator reads nor what the report says — structurally closed, and strictly stronger than the two-handle-read bracket downstream callers (solo#1097) can now delete: the bracket had to refuse when a commit landed mid-validate; report.t stays exact through it.
What I ran and probed, on a warm target with the changed files touched:
- All 5 new/extended fluree-db-api tests pass by name (
--features shacl— worth knowing that a default-featuresnextestrun silently skips the entire validate module; CI's--all-featurescovers it). Server envelope test and both CLI file-mode tests pass. - Mutation check:
t: to_t→t: 0at the final construction site turns all three integration tests red — the coverage is real, not decorative. - The cross-ledger assertion genuinely discriminates: the fixture leaves the model ledger at t=1 and the data ledger at t=2, so a model-t leak would fail, not coast.
- Blast radius claim verified: the only
ValidateReportstruct-literal constructions are invalidate.rs; wire changes are additive on all three renderings;f:treuses the existingns.flur.ee/db#tpredicate rather than minting a term; the@t:addressing the docs teach is real (dataset.rs). - testsuite-shacl's pre-existing break confirmed at
runner.rs:58— this PR adds no new one (the runner buildsValidateOptionswith nomax_fuel/cancellationand no..Default::default(), so the excluded crate already fails to compile on main;compare.rsonly readsValidateReportfields). Worth its own small issue so it doesn't rot further — reviving a harness CI never builds is a separate decision (does the W3C SHACL suite still pass?), so leaving it alone here is defensible.
The one thing to fix before merge is small: the new CLI doc says file-mode t is "always 1", but it's 2 (the staging-SHACL-disable config commit precedes the data commit) — proven by pinning the test assertion, which currently matches any t value. One word in the doc, one character in the test.
Adherence checklist:
- Patterns / abstractions ✔ — extends
ValidateReportin place, reuses the existingf:tvocab predicate and the view's ownto_t; no parallel construct, no engine change. - Performance ✔ — neutral, verified: no new ledger read on the validate path (
to_talready existed), one copiedi64per report, O(1) render additions; no hot crate touched. - Testing ✔ — unit + API + HTTP + CLI integration tests, all wired, all run under CI's
--all-features, and mutation-verified red on a wrong t. The CLI file-mode assertion is the one soft spot (unpinned — see nit). - Conventions ✔ — self-describing subject, thorough body with an honest blast-radius and testsuite-shacl disclosure; clippy/fmt green on the exact head; docs updated on both surfaces (one constant to correct).
| `jsonld` and `turtle` emit a W3C-shaped `sh:ValidationReport` with | ||
| `sh:focusNode`, `sh:resultPath` (single-predicate paths only — complex paths | ||
| are omitted rather than misrepresented), `sh:resultSeverity`, | ||
| `sh:sourceShape`, `sh:sourceConstraintComponent`, `sh:resultMessage`, and |
There was a problem hiding this comment.
Blocking-adjacent nit (one-word fix — fold in before merge): file-mode t is 2, not "always 1".
The doc says "In file mode t is the ephemeral ledger's commit (always 1)." But validate_file (fluree-db-cli/src/commands/validate.rs:84-125) commits twice before validating — disable_staging_shacl upserts the config graph (commit 1), then the data insert lands as commit 2 — so the ephemeral ledger's head is t=2 when validate_ledger snapshots it. Verified empirically, not just by trace: pinning the CLI integration assertion to checked at t=1 fails with actual output …(1 shape(s) checked at t=2).
Fix: change the doc to 2 (or, better, say why: the loader's staging-SHACL-disable commit plus your data commit), and pin the assertion at integration.rs:3080 to checked at t=2.
| .success() | ||
| .stdout(predicate::str::contains("Conforms: true")); | ||
| .stdout(predicate::str::contains("Conforms: true")) | ||
| .stdout(predicate::str::contains("checked at t=")); |
There was a problem hiding this comment.
Optional (folds into the docs nit above) — predicate::str::contains("checked at t=") asserts the field exists but not its value. File mode is the one caller with a deterministic t (=2), so pinning it to checked at t=2 makes the docs claim machine-checked — the unpinned matcher is exactly how the wrong constant got past the test.
The CLI doc said file-mode `t` is "always 1". It is 2: `validate_file` commits the staging-SHACL-disable config graph before the data insert, so the ephemeral ledger's head is already at 2 when `validate_ledger` snapshots it. The assertion that should have caught this matched `checked at t=` without the value, so the wrong constant went unchallenged. Pin it to `t=2` and say in the doc why the number is what it is.
Summary
The full-ledger SHACL validator already pins the snapshot it evaluates (
to_t = view.tat the top ofvalidate_view_inner), but the report it returned never said whichtthat was. A caller that pairs the results with other reads, such as record counts for a violation rate, had to read the handle'stbefore and aftervalidate_ledgerand only trust the pair when they agreed, because a commit landing between the validator's internal snapshot and the caller's own read silently breaks the pin.This puts the
ton the report so consumers stop reconstructing it from a second read.Changes
ValidateReportgainspub t: i64, set from the sameto_tat both construction sites (the empty-shapes early return and the final sorted report).f:t(f=https://ns.flur.ee/db#), sincesh:ValidationReporthas no slot for a ledger time."t". CLI table summary line now ends withchecked at t=N.docs/cli/validate.mdand the/validatesection ofdocs/api/endpoints.mdshow the field and explain reading any paired measurement at the samet.Tests
report.tequals the ledger handle'stthroughvalidate_ledger, advances with the head after another commit, and on the cross-ledger shapes path pins the data ledger'strather than the model's.Blast radius
Adding a public field is source-breaking only for struct-literal constructions, and all three are in
validate.rs. Nothing removed or renamed on the wire; the summary envelope and W3C documents gain one term each.Note: the excluded
testsuite-shaclcrate does not compile onmaintoday for an unrelated reason (its runner predates thecancellation/max_fuelfields onValidateOptions, and CI never builds it). Left alone here — reviving a harness no CI job builds is its own decision.Follow-up: #1779
Review follow-ups
docs/cli/validate.mdsaid file-modetis "always1"; it is2—validate_filecommits the staging-SHACL-disable config before the data insert. Corrected to say2and why, so the number is derivable rather than magic.checked at t=without pinning the value, which is how the wrong constant reached the docs unchallenged. Pinned tochecked at t=2, making the docs claim machine-checked. Verified non-vacuous: the actual output is…(1 shape(s) checked at t=2), and the pinned assertion goes red againstt=1.