Fix reasoning defaults - #1763
Merged
Merged
Conversation
Config-graph defaults were applied at each site that built a view, so a view
built any other way never received them. `GraphDb::from_ledger_state` is the
sync public constructor and cannot resolve config, which is async. It is what
the ledger-scoped server routes build from, and it is also the quickstart in
this crate's own module docs, so a query through it dropped the ledger's
`f:reasoningDefaults` with no diagnostic. The reporter of 1577 worked around it
by adding `# PRAGMA reasoning: rdfs` to every query.
Apply the defaults once, at the choke point the single-view and dataset paths
already share. `complete_config_defaults` resolves the config graph when the
view arrives without one and then applies the defaults;
`apply_reasoning_to_executable` calls it. The five per-site
`apply_config_defaults` calls come out.
That choke point sees more than the five sites it replaces, and two kinds of
view reaching it must stay exempt.
A caller's own reasoning is one. `with_reasoning_precedence` assigns `reasoning`
outright, so applying config last would discard whatever the caller had
attached, `Force` included; `effective_reasoning` arbitrates between the wrapper
and the query's modes, never against a wrapper already thrown away. Config
supplies defaults, so it skips the mode wrapper when the view carries one. The
materialization budget still applies either way, because a ledger's cap governs
whichever modes win.
Cypher write-decision probes are the other. `MERGE` asks whether its pattern
already exists and `DETACH DELETE` asks whether the target still has
relationships, both over a bare `GraphDb::from_ledger_state`. Staging does not
reason, so a probe that did would let a match existing only by entailment send
the write down `ON MATCH`, which then finds nothing asserted to update: no node
created, no property set, the statement silently a no-op.
`resolve_conditional_cypher` covers the `MERGE` probe and the delete guard for
all three of its callers, and the sequential driver gets `seq_decision_view` for
its own `MERGE` guard and the re-bind after it. `seq_probe_view` itself stays
reasoning-enabled: it also backs the statement's read clauses and trailing
RETURN, which are ordinary reads and should see entailments like any other
query, so only the two clauses that decide what gets written moved.
Views that arrive without a resolved config now re-resolve per query.
`resolve_config_sid` already guards the common case: a ledger whose config graph
holds no flakes in either the novelty overlay or the base index returns before
the rdf:type scan, so a ledger with no config pays a stats check rather than a
read.
Tests cover both layers, because the fix lives in the api crate while the
reported symptom was over HTTP. `ledger_route_config_defaults` drives the real
`POST /v1/fluree/query/{ledger}` route on both front-ends; `it_config_graph`
pins the same gap against a view built by `GraphDb::from_ledger_state`, and
widens the config surface to an uppercase `f:RDFS` mode IRI and an
`f:schemaSource` GraphRef. `it_cypher_probe_reasoning` pins the write probes.
Every pin is paired with a control in the opposite direction, so neither
applying config nor withholding it can pass on its own: the `# PRAGMA` queries
entail without config, an explicit `rdfs()` survives a configured `f:none`,
config still governs a view the caller left alone, and the Cypher fixture
asserts its own entailment before asserting anything about writes. With the
source changes reverted, every pin fails and every control still passes.
The gap had been papered over once already: `it_rules_source` carries a comment
routing around `GraphDb::from_ledger_state` so that `f:rulesSource` takes
effect. That workaround is no longer needed, confirmed by running the test
through the bare constructor on both sides of this change.
Closes #1577
Query preparation now completes the ledger's config defaults, so a fault in the
stored config graph reaches `build_executable_for_view` on every query. The
tracked read path reported that failure as 400, telling the caller their request
was bad. It was not: the config graph is operator data, no request can cause a
fault in it, and no change to a request can clear one. A reader following the
status looks in the only place the answer cannot be.
Two things produced the 400. The tracked path hardcoded it for any error out of
`build_executable_for_view`, and `ApiError::Config` is itself a 400 in both
`ApiError::status_code` and the server's mapping.
`ApiError::Config` cannot simply move. It has 112 raisers, most of them
genuinely about caller or deployment input ("ledger_info requires a managed
storage backend"), for which 400 is right. Add `ApiError::LedgerConfig` for
faults in a ledger's *stored* config instead, mapping to 500, and raise it from
the four sites in `resolve_local_rules_source_g_id` that reject an unsupported
or unresolvable `f:rulesSource`.
The error code stays `err:system/ConfigError` on purpose, so the code still
names the problem while the status names who can fix it.
The tracked path now reports each error's own status rather than a blanket 400.
That corrects a second case on the same path: `attach_cross_ledger_rules` raises
`ApiError::CrossLedger`, which is a 502 everywhere else and which the resolver's
own doc comment describes as a 502, but which this path flattened to 400 along
with everything else.
Pinned over the real route. Before this change the response to a valid query
against a ledger whose `f:rulesSource` carries `f:atT` was:
{"error":"Invalid configuration: f:rulesSource with f:atT (temporal pinning) is not yet supported",
"status":400,
"@type":"err:system/ConfigError"}
The test asserts both halves of the fix, that the status is not 400 and that
it is 500, so neither a stray remap nor a swallowed error can satisfy it.
bplatz
approved these changes
Sep 3, 2026
bplatz
left a comment
bplatz
left a comment
Contributor
There was a problem hiding this comment.
Approving — the choke-point design holds up. I checked the parts it depends on: db.reasoning() is read in exactly one place, so dropping the per-source apply_config_defaults from the dataset builder loses nothing; everything the completed view carries is copied into executable inside the function, so the throwaway clone strands no datalog config; the budget really does still apply after the new early-return arm; and the guard's key is safe, since the only internal with_reasoning sites are the two deliberate none() probes. The empty-config short-circuit and the best-effort/propagating split behind the 500 both check out too.
One ask before merge: the caching pattern for the follow-up already exists — policy_view::resolve_ledger_config_cached — and I'd port it here rather than defer it. The write path hit this exact problem and solved it, with a doc comment that reads almost word for word like the situation this PR creates for reads. It's keyed on Novelty::config_write_t, fail-safe by construction, and is a private fn one pub(crate) away. Details inline.
The rest is inline as well. Two that have no diff line to hang on:
- The dataset tracked path still flattens to 400 (
dataset_query.rs:418and:538). Config completion now runs there too viadataset_query.rs:618, so the same fault reports as a client error on dataset queries — including theCrossLedger502 flattening the description says is corrected. One-line twin of thequery.rschange, and the server test only covers the single-view route. - The
it_rules_sourceworkaround wasn't actually removed.it_rules_source.rs:109still carries the comment and still routes throughfluree.db(...). Flipping it back toGraphDb::from_ledger_statewould turn that comment into a regression pin for this fix; otherwise the sentence in the description should go.
The single-view tracked path stopped flattening every preparation error to 400, but its dataset twin did not. Config defaults are completed on both, so the same fault reported as a client error on a dataset query, including the `CrossLedger` 502 the earlier change said it corrected. Pinning this turned up two gaps in the existing test. It sent no `fluree-track-*` header, so it never took a tracked path at all and pinned only the `LedgerConfig` variant, not the status passthrough. And on the dataset path a config-graph fault is the wrong probe: views load through `db()`, so the fault fires at view load and never reaches `build_executable_for_dataset`. A cross-ledger `f:rulesSource` naming a missing model ledger does reach it, because same-ledger pre-resolution skips it and the cross-ledger resolver runs at preparation. The route tests now cover the untracked config fault, the tracked single-view config fault, and the cross-ledger fault on both tracked paths. Each of the three status changes has a test that fails when only that change is reverted.
The argument for moving a stored-config fault off 400 was that the status names who can fix it while the code still names what it is. The route test asserted only the status, so the code half of that claim was unpinned: the server could have let `LedgerConfig` fall through to its `INTERNAL` catch-all and the test would not have noticed. Confirmed by removing the variant from the config-code arm, which fails the new assertion with `err:system/InternalError`. The assertion lives on the untracked test only. `TrackedErrorResponse` carries a status and a message but no error kind, so the tracked path always reports `err:system/InternalError` regardless of the fault; that is a pre-existing property of the tracked envelope, not something these tests should enshrine.
`query_grandparent` routed through `fluree.db(...)` because a view from `GraphDb::from_ledger_state` carried no resolved config, so `f:rulesSource` never took effect on it. That is the gap 1577 closed, and the comment saying so was still there, still steering the test around the very path the fix made work. Build the view with the bare constructor instead. The comment now says what the choice pins: config defaults reaching a view that arrives at query preparation without any. Removing the `complete_config_defaults` call fails `rules_source_in_named_graph_is_honored` and nothing else, which is the right shape. The two fail-loudly cases call `fluree.db(...)` directly and assert the misconfiguration at load time, so they do not go through the helper and are independent of the choke point; the no-config control passes either way. The `ledger_id` parameter existed only to feed `db()`, so it goes.
`resolve_conditional_cypher` reads asserted data only for both of its probes, but only the MERGE half was tested. The DELETE half is the guard Cypher requires on a bare `DELETE`: refuse while the node still has relationships, so the caller has to say `DETACH DELETE` to strand nothing. A reasoning probe would refuse a legal delete over an edge the ledger never held, since an entailed edge is not stranded by anything; it merely stops being derivable once its subject is gone. The fixture needs OWL2-RL rather than RDFS. Every asserted edge touches both endpoints and the guard probes both directions, so `subPropertyOf` cannot produce a node whose only relationship is entailed. `owl:hasValue` can: a `Person` carrying nothing but a literal property gets `likes Pizza` from its type alone. The control asserts both halves, that the relationship is entailed for reads and that it is not asserted, because a fixture that asserted it would make the delete fail for a legitimate reason and the test would pass for the wrong one. Removing the probe's `ReasoningModes::none()` wrapper fails the new test with the guard's own refusal, and fails `merge_on_match_set_does_not_silently_vanish` with it, since the `MergeSet` arm shares that probe view. The bare-MERGE and sequential cases take other paths and keep passing, as they should. Also corrects the module doc, which named `DETACH DELETE` as the probing form. It is the bare `DELETE` that probes; `DETACH DELETE` is what skips it.
`complete_config_defaults` is now the one place a server-verified identity reaches the config merges, and it hard-coded `None`. Take it as a parameter and pass it through, and let `apply_reasoning_to_executable` carry it to the two preparation entry points. No behavior changes: both entry points still pass `None`, so `f:IdentityRestricted` still denies every override, the same as `f:OverrideNone`. What changes is the shape of the fix for that. Wiring the identity from the request boundary is now an edit to two call sites rather than a re-plumb through query preparation, and the doc on `complete_config_defaults` says so, in place of the earlier note that claimed the identity was already respected.
Completing config defaults at query preparation made `resolve_and_attach_config` run for every view that arrives without a resolved config. On the ledger-scoped server routes that is every request, since they build a fresh view from the loaded `LedgerState`, and it also covers each Cypher write probe and each sampled subject in R2RML twin verification. A configured ledger paid the full config-graph read each time: the `f:LedgerConfig` lookup plus eight setting groups, each of which fans out into per-field reads with no `join!`, so the latencies add. The write path solved this already. `policy_view::resolve_ledger_config_cached` memoizes the resolved config on the loaded ledger handle, keyed by `Novelty::config_write_t`, which advances iff a commit writes the config graph. It serves a value only on an exact key match at head, so a stale read is impossible; any deviation, time travel, a non-`Novelty` overlay, no loaded handle, resolves fresh. It also memoizes the no-config result, so unconfigured ledgers skip the scan after their first miss. Make it `pub(crate)` and route the read path through it. The read path keeps its best-effort handling: a resolve error still logs and falls back to system defaults. `view.novelty()` is passed as the explicit stats handle. A read view's overlay can be a composed reasoning overlay rather than a bare `Novelty`, and the resolver's fallback downcast would then find no marker and silently stop caching for exactly the views this is meant to help. A unit test in `fluree_ext` pins that a read over a config-less view populates the cache, for both the resolved-config and the no-config memo, and fails with the read path put back on the direct resolver; it is a unit test because `config_cache_get` is crate-private. An integration test pins the invalidation contract from the outside: a config-graph commit must be visible to the next read. That one passes with or without the cache and exists to catch a wrong key, not the cache's absence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A ledger configured with
f:reasoningDefaultsserved non-entailed results. The reporter had to add# PRAGMA reasoning: rdfsto every query to get the behavior the ledger was already configured for.Config-graph defaults were applied at each site that built a view, so a view built any other way never received them.
GraphDb::from_ledger_stateis the sync public constructor and cannot resolve config, which is async. It is what the ledger-scoped server routes build from, and it is also the quickstart in the api crate's own module docs, so a query through it dropped the ledger's configured defaults with no diagnostic.The gap had been papered over once already:
it_rules_sourcecarried a comment routing aroundGraphDb::from_ledger_stateso thatf:rulesSourcewould take effect. That workaround is no longer needed.The fix
Apply config defaults once, at the choke point the single-view and dataset paths already share.
complete_config_defaultsresolves the config graph when the view arrives without one, then applies the defaults;apply_reasoning_to_executablecalls it. The five per-siteapply_config_defaultscalls come out.That choke point sees more than the five sites it replaces, and two kinds of view reaching it stay exempt:
with_reasoning_precedenceassignsreasoningoutright, so applying config last would discard whatever the caller attached,Forceincluded. Config supplies defaults, so it skips the mode wrapper when the view already carries one. The materialization budget still applies either way.MERGEasks whether its pattern already exists andDETACH DELETEasks whether the target still has relationships. Staging does not reason, so a probe that did would let an entailment-only match takeON MATCHand then find nothing asserted to update: no node created, no property set, the statement silently a no-op.resolve_conditional_cypherand the sequential driver'sseq_decision_viewboth read asserted data only now.seq_probe_viewitself stays reasoning-enabled, because it also backs the statement's read clauses and trailing RETURN, which are ordinary reads.Status codes
Because query preparation now completes config defaults, a fault in the stored config graph reaches
build_executable_for_viewon every query, and the tracked read path reported it as 400. The config graph is operator data: no request can cause a fault in it and no change to a request can clear one.ApiError::Configcould not simply move, since it has 112 raisers, most genuinely about caller or deployment input. NewApiError::LedgerConfigmaps to 500 and is raised from the four sites inresolve_local_rules_source_g_id. The error code stayserr:system/ConfigError, so the code names the problem while the status names who can fix it.The tracked path now reports each error's own status rather than a blanket 400. That also corrects
attach_cross_ledger_rules, whoseCrossLedgererrors are 502 everywhere else but were flattened to 400 on this path.Tests
Coverage sits at both layers, because the fix lives in the api crate while the reported symptom was over HTTP.
fluree-db-server/tests/ledger_route_config_defaults.rsdrives the realPOST /v1/fluree/query/{ledger}route on both front-ends, plus the status code for a malformed config graph.it_config_graphpins the same gap against a view built byGraphDb::from_ledger_state, and widens the config surface to an uppercasef:RDFSmode IRI and anf:schemaSourceGraphRef.it_cypher_probe_reasoning(new target) pins the write probes.Every pin is paired with a control in the opposite direction, so neither applying config nor withholding it can pass on its own: the
# PRAGMAqueries entail without config, an explicitrdfs()survives a configuredf:none, config still governs a view the caller left alone, and the Cypher fixture asserts its own entailment before asserting anything about writes.Every regression test was verified to fail with the source changes reverted, and every control to pass either way.
Follow-ups, not in this PR
Two issues found while reviewing this work, written up but deliberately left out of these commits. Both are marked unverified and carry the repro to run before either is acted on.
policy_view::resolve_ledger_config_cachedalready memoizes exactly this on the ledger handle. Sharing that cache is the fix.f:IdentityRestrictedoverride control is never enforced. Every production call site passesserver_identity: None, so the mode behaves asf:OverrideNonefor policy, reasoning, and datalog alike, while the docs describe it as working. Predates this branch.Fixes #1577