Repository navigation
FC-861 fixes for block & history query validation - #3
Merged
Merged
Conversation
ldw1007
commented
Oct 22, 2020
Contributor
- fix block query to accept :prettyPrint, remove warnings for pretty-print
- remove pretty-print & show-auth warnings from history query
* fix block query to accept :prettyPrint, remove warnings for pretty-print * remove pretty-print & show-auth warnings from history query
bplatz
self-requested a review
October 23, 2020 01:08
bplatz
approved these changes
Oct 23, 2020
flyingmachine
pushed a commit
that referenced
this pull request
Feb 22, 2023
Add websocket handler & echo service at /ws
dpetran
added a commit
that referenced
this pull request
May 17, 2024
Three approaches considered: 1) just document that :values needs to appear first 2) discard solutions that don't match the given solution 3) during the parse step, sort the patterns so :values is first I don't like #1, though that's the most expedient. #2 is a bit wasteful as it forces us to generate useless solutions. #3 is doable, and may be a minimal requirement in basic query planning. This implements solution #2.
dpetran
added a commit
that referenced
this pull request
May 31, 2024
Three approaches considered: 1) just document that :values needs to appear first 2) discard solutions that don't match the given solution 3) during the parse step, sort the patterns so :values is first I don't like #1, though that's the most expedient. #2 is a bit wasteful as it forces us to generate useless solutions. #3 is doable, and may be a minimal requirement in basic query planning. This implements solution #2.
dpetran
added a commit
that referenced
this pull request
Jun 12, 2024
Three approaches considered: 1) just document that :values needs to appear first 2) discard solutions that don't match the given solution 3) during the parse step, sort the patterns so :values is first I don't like #1, though that's the most expedient. #2 is a bit wasteful as it forces us to generate useless solutions. #3 is doable, and may be a minimal requirement in basic query planning. This implements solution #2.
JaceRockman
pushed a commit
that referenced
this pull request
Jul 2, 2024
Three approaches considered: 1) just document that :values needs to appear first 2) discard solutions that don't match the given solution 3) during the parse step, sort the patterns so :values is first I don't like #1, though that's the most expedient. #2 is a bit wasteful as it forces us to generate useless solutions. #3 is doable, and may be a minimal requirement in basic query planning. This implements solution #2.
5 tasks done
5 tasks
bplatz
added a commit
that referenced
this pull request
May 17, 2026
… diagnostics, nits Selected items from the additional review feedback. Each call explained. **#1 — Retry-fragility comment on `take(opts.shapes)`.** No retry exists on the staging path today, but `take()` moves the inline SHACL JSON off the txn, so a future retry would silently skip inline-shapes validation on the second attempt. Hard invariant comment added at the move site; if retry is ever added, defer the take until after `stage_txn` returns successfully. **#3 — `UnsupportedFeature` errors no longer carry an empty `ledger_id`.** `resolve_graph_ref` rejected `f:atT` / `f:trustPolicy` / `f:rollbackGuard` *before* verifying that the ref was actually cross-ledger, so a misrouted same-ledger ref hitting this path would have produced a confusing error with `ledger_id: ""`. Reordered: the same-ledger-ref guard runs first, binding `raw_ledger_ref` once; the unsupported-feature checks now carry the real ledger id in every diagnostic. **#4 — `f:atT` rejection breadth: audited, no gap.** All five `f:GraphRef`-shaped predicates already reject `f:atT` at their same-ledger entry points: - `policy_builder::resolve_policy_source_g_ids` - `tx::resolve_shapes_source_g_ids` (SHACL) - `ontology_imports::resolve_schema_bundle` - `tx::resolve_constraint_source_g_ids_for` - `view::fluree_ext::resolve_local_rules_source_g_id` Plus the cross-ledger `resolver` (now with non-empty ledger_id per #3). Audit-only change; no code modification beyond the doc note. **#2 — Cross-ledger policy target IRI loss is now logged.** `wire_to_restrictions` previously `filter_map`'d unresolvable target IRIs and `for_classes` IRIs silently. A cross-ledger policy could quietly narrow its scope on D if M referenced IRIs D had never registered. Two `tracing::warn!` calls now surface both cases with `restriction_id`, requested count, and resolved count — operators can spot the silent narrowing in logs. Kept the restriction even when targets become empty (rather than dropping the whole restriction): same-ledger via `load_policy_restriction` also produces empty target sets for the same IRI-not-in-snapshot scenario, and dropping the restriction would diverge cross-ledger from same-ledger semantics. The warn log is the parity-preserving signal. `fluree-db-policy` gains a `tracing` dependency for this. **Nits taken:** - `TAtUnavailable` doc-commented as Phase 3 reserved (unreachable today because `UnsupportedFeature { feature: "f:atT" }` fires first). - Empty `uniqueProperties: []` over HTTP is now documented as an intentional "no inline constraints" treatment, not a silent bug. - Cache read/write race in `resolver` documented as benign: losers materialize the same value structurally; single-flight is a future optimization, not a correctness need. - `f:AccessPolicy` IRI promoted from local `const` / duplicated-literal to `fluree_vocab::policy_iris::ACCESS_POLICY`. Two call sites updated; nothing else used the local consts. **Nits skipped per "don't make changes that don't seem needed":** - HTTP-level negative test for malformed `opts.shapes` / `uniqueProperties` (#5) — unit-level coverage exists; HTTP harness setup cost not justified. - Same-ledger `rules_source_g_id` end-to-end test (#6) — already covered by `it_rules_source::rules_source_in_named_graph_is_honored`, which runs an actual query against the named-graph rule. - DynamoDB hard-drop failure handling (#7) — storage layer, unrelated to this PR. - Duplicate pattern match in `shapes_materializer` — cosmetic. - `snapshot.ledger_id` redundancy in inline-shapes / inline- ontology `txn_id` — cosmetic. - `is_last_live_branch` WARN log — unrelated path.
bplatz
added a commit
that referenced
this pull request
Jun 27, 2026
The segment-aware overlay path concatenated K already-sorted per-segment runs and re-sorted them with sort_unstable (review High #3). Add sort_overlay_ops_stable (stable, run-adaptive) and use it for the merge: Rust's stable sort detects the K runs and merges them in ~O(n log k) instead of re-sorting. The remaining O(n) copy is inherent; true O(new-segment) needs an incremental/persistent merge, deferred until the (now integer-only) merge is shown to dominate the cached per-segment translation. Also key GlobalTranslationKey on store_id instead of store_max_t, closing the same latent per-view-vs-live divergence the SegmentOpsKey fix addressed (a same-index_t store rebuild re-ranks dict ids at an unchanged store_max_t).
aaj3f
added a commit
that referenced
this pull request
Jul 6, 2026
Address the review's findings — all verified real; all make the harness's green trustworthy rather than changing what it covers: - (review #1) the 'no unexpected named graph' guard was dead code: the engine binds GRAPH ?g as a plain literal, so the Iri-only filter in list_named_graphs always produced []. Accept literal and IRI bindings (excluding the alias-named default graph) so the guard survives the eventual engine fix. Verified live: an update leaving a stray graph with no expected graphs now fails with 'Unexpected non-empty named graph'. - (review #2) bail when a manifest yields zero tests — a submodule restructure or manifest-parser regression must not report green. - (review #3) a registered test that dies by timeout/subprocess crash now hard-fails: the register excuses a known wrong answer, not an infra death masking a new hang or panic. - (review #4) UpdateEvaluationTest now rejects an mf:result blank node exposing none of ut:data/ut:graphData/ut:result, instead of degrading to a trivially satisfiable 'expected empty store'. - (review #5) register entries matching no discovered test now fail the suite, so dead entries (typos, upstream renames) cannot accumulate. - notes: tightened the TSV bare-integer heuristic to a single optional leading sign; documented the line-based directive-hoisting assumption; fixed the stale 'CI runs make ci' Makefile comment. Full suite remains green: 36 suites, ~1420 tests, 0 failed, 0 ignored.
aaj3f
added a commit
that referenced
this pull request
Jul 12, 2026
Four additions, one per open review item: - Multi-op atomicity on mid-request failure (review blocking #4): op 1 INSERT DATA stages cleanly, op 2 fails at staging while evaluating over op 1's data (BIND(STRDT(?m, <bad:datatype>)) — proving op 2 ran against the sequential state); the request errors as a unit, 't' is unchanged, and op 1 leaves no trace. - UPDATE BASE resolution at the seam (review blocking #2): the same BASE-carrying document stores and finds identical absolute IRIs on the update and query surfaces, GRAPH names included. - DELETE WHERE label independence at the seam (review blocking #5): label reuse across DELETE WHERE ops executes, each op deleting its own matches. - Indexed EXISTS-with-GRAPH-?g (review blocking #3): the GraphVarCorrelated strategy driven end-to-end on a binary-indexed ledger — including the string-literal back-compat — which no memory-backed suite (W3C harness included) ever exercised. On the #1443 EncodedSid/EncodedLit extraction arms specifically, the audit went one step past the review's finding: no current plan shape delivers a still-encoded binding to either extraction site (probed: plain join, OPTIONAL, FILTER EXISTS, UNION, string variant — upstream operators materialize batches first). The arms stay as defense-in-depth with their comment rewritten to say exactly that (this also retires the stale 'never on a scan hot path' wording the review flagged).
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
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.