Skip to content

FC-861 fixes for block & history query validation - #3

Merged
ldw1007 merged 1 commit into
masterfrom
fix/block-prettyPrint
Oct 23, 2020
Merged

ldw1007 merged 1 commit into
masterfrom
fix/block-prettyPrint

Conversation

@ldw1007

@ldw1007 ldw1007 commented Oct 22, 2020

Copy link
Copy Markdown
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
bplatz self-requested a review October 23, 2020 01:08
@ldw1007
ldw1007 merged commit c9a1724 into master Oct 23, 2020
@ldw1007
ldw1007 deleted the fix/block-prettyPrint branch October 23, 2020 12:27
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.
@aaj3f aaj3f mentioned this pull request Apr 30, 2026
5 tasks done
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).
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