Skip to content

fix(turtle): a redeclared prefix must change what later names mean - #1565

Open
aaj3f wants to merge 4 commits into
mainfrom
fix/turtle-prefix-redefinition
Open

fix(turtle): a redeclared prefix must change what later names mean#1565
aaj3f wants to merge 4 commits into
mainfrom
fix/turtle-prefix-redefinition

Conversation

@aaj3f

@aaj3f aaj3f commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Two correctness fixes against main

Both found while building the RDF toolkit, both independent of it, both small enough that one review slot beats two.

  1. fix(turtle) — a redeclared prefix silently didn't change what later prefixed names meant. Data corruption, no error.
  2. fix(api) — a literal NUL byte in an admin.rs comment made the whole file binary to every text tool, so plain grep reported "no match" for patterns that occur 23 times in it.

1. A redeclared prefix must change what later names mean

Silent data corruption in the Turtle parser. Turtle allows a prefix to be rebound part-way through a document; the parser ignored the rebinding for any prefixed name it had already seen.

@prefix e: <http://a/> .   e:x <http://p/> "1" .
@prefix e: <http://b/> .   e:x <http://p/> "2" .

Both statements land on <http://a/x>. The second should be <http://b/x>. No error, no warning — the document says one thing and the graph holds another.

Cause

prefixed_term_cache is keyed by span text (e:x). That text identifies an IRI only relative to the bindings in force when the entry was made, and nothing invalidated it when a binding changed.

Two neighbours were not affected, and the PR pins both so the scope is demonstrated rather than asserted:

  • iri_term_cache keys on the resolved IRI, so a different base produces a different key and never collided.
  • @base redeclaration was always correct — a prefix namespace is resolved once, when its directive is read, and prefixed names never consult the base at lookup time.

Fix

On a prefix directive, clear the expansion cache if the prefix rebinds to a different namespace. Only if.

Redeclaring the same binding is the common case rather than a curiosity: chunked import prepends the file's prefix block to every chunk over a pre-seeded prefix map, so the parser sees each prefix declared twice by construction. Every cached expansion stays correct through that, and clearing there would cost hit rate across the entire import path for no correctness gain.

The per-term path is untouched. The diff adds no instruction to resolve_prefixed_term or sink_term_iri — the only executable change is inside the prefix-directive handler. Added work is one hash lookup per prefix directive, of which a document has a handful, against a cache consulted once per prefixed name, of which it has millions. That is a structural property of the diff, not a benchmark result; a wall-clock A/B on this host was noise-dominated (a 4× swing between runs of identical code under concurrent load) and is not offered as evidence.

Clearing is coarse on purpose: a rebind invalidates only that prefix's entries, but finding them means scanning every key, so we drop them all. A per-lookup epoch check would make rebinding O(1) at the cost of a comparison on every cache hit — the wrong trade when the overwhelming majority of documents rebind nothing.

The comparison is on meaning, not spelling, and both directions matter. parse_prefix_directive resolves the namespace before storing it, so prefixes holds resolved IRIs. The same spelling under two different bases (@prefix e: <a/> under <http://x/> then under <http://y/>) names two namespaces and must clear; two spellings that resolve alike (<http://x/a/> and <a/> under <http://x/>) name one and must not. Comparing raw text would get each backwards.

The "only when" needs its own guard

Clearing on a same-binding redeclaration produces identical triples — just colder — because iri_term_cache absorbs the repeated work before it reaches the sink. So a mutation deleting the condition passed every gate in the first version of this PR; confirmed by running it, with all eleven triple-level tests still green.

The cache hit/miss counters are the only place the behaviour is observable, so a test-only parse_reporting_cache exposes them and two tests read them: same-binding redeclaration must leave the cache warm (1 hit, 1 miss), a real rebinding must drop it (0 hits, 2 misses). The same mutation now fails the first.

Blast radius — honest version

This is not serial-only. Bulk import is affected, and this fix is necessary but not sufficient there.

The PrefixAfterData guard lives in compute_chunk_boundaries (the whole-file pre-scan behind TurtleChunkReader). Import does not use that reader. fluree-db-api/src/import.rs uses StreamingTurtleReader exclusively, which has no such guard — verified empirically: a 4000-statement file with a mid-file redefinition is accepted, its extracted prefix block is only the head binding @prefix e: <http://a/> ., and the redefinition is delivered in a chunk like any other data.

So a mid-document redefinition reaching import hits two independent defects, and prefix_redefinition_blast_radius.rs executes the split rather than asserting it. The corpus is 4000 statements of e:s{i} <http://p/> e:tag . with a redefinition half way through, parsed the way import parses it (prelude seeded, prefix block prepended).

The e:tag object is the load-bearing part. It repeats on every line, so it is cached under the first binding and requested again after the rebinding — which is the only shape in which this bug can appear at all. Counting inside the carrier chunk (the one that actually contains the redefinition), post-redefinition triples whose object still resolves to http://a/tag:

  1. Carrier chunk — 379 stale without the fix, 0 with it. This is the cache bug, and this assertion is what distinguishes the fix; verified by reverting the parser to the merge base. Fixed here.
  2. Later chunks — 1621 subjects mis-resolved. The head-extracted prefix block is prepended to each chunk, so they parse with the original binding and never see the redefinition. Not fixed here; that is the §1.4 chunker work (mid-file directive detection → serial fallback), and it needs the splitter, not the parser.

An earlier version of this test used a literal object and asserted only that no subject was mis-resolved in the carrier chunk. That was vacuous — every subject is a distinct span, so no prefixed name was ever looked up twice and the cache was never consulted for a span it held. It passed against a parser with the fix entirely removed. Caught in review; the corpus and the assertion above are the remediation.

The later-chunk defect is asserted live (> 0) rather than #[ignore]d: an ignored test runs nowhere and rots, while this one fails the moment §1.4 lands and tells whoever closed it to come here. The 1621 is a printed diagnostic, not part of the assertion, so ordinary chunking changes cannot make it flap.

Also noted, not addressed: PrefixCheck only matches a directive keyword at line start, so even the guarded pre-scan path would miss … . @prefix e: <…> . written mid-line. Same §1.4 bucket.

Discovery

Found while wiring term validation for H-8 (burn-down cause C), when a probe of the prefix path returned two statements on one subject. Nothing about it is specific to validation — it reproduces on plain parse, on main at 5598ffd6a, with no feature flags.

Tests

Ten, in fluree-graph-turtle/tests/prefix_redefinition.rs. Eight fail without the fix (verified by stashing the parser change and re-running). The two that pass either way are @base and same-binding redeclaration — the pair that shows the fix is scoped correctly rather than accidentally.

Covered: the verbatim repro; SPARQL-style PREFIX; other prefixes left alone; rebinding back and forth; the default (empty) prefix; namespace-only names (e:); same-binding redeclaration as a no-op; the pre-seeded-map shape chunked import uses; @base unaffected; and a 1000-statement case where a partial invalidation would show as a partial failure.

2. Name U+0000 in a comment instead of embedding one

fluree-db-api/src/admin.rs line 283 carried a literal NUL byte inside a comment that meant to name the character — "callers can otherwise sneak a <NUL> past". The lines immediately above spell the same class correctly (U+0000..=U+001F, U+007F); this is the one that got away.

The cost is not cosmetic. A single NUL makes the whole file binary to every text tool:

  • file(1) reported it as data, not source.
  • Search behaviour then splits by grep implementation, and naming which one matters:
    • ugrep 7.5.0 — what grep resolves to in the dev environment this was found in — exits 1 and prints nothing for a pattern occurring 23 times in the file. Not "binary file matches", not a warning: a clean, confident "not found".
    • BSD grep 2.6.0-FreeBSD (/usr/bin/grep, GNU-compatible; what CI and most contributors get) prints Binary file … matches and exits 0. It withholds the matching lines but does not lie about the match.

So the severe reading holds for anyone on ugrep and the milder one for everyone else — both worse than a file that simply works. After: file reports UTF-8 text and both greps find all 23. (An earlier draft of this claim asserted the silent-exit-1 behaviour universally; it was measured in one shell. Corrected on review.)

Comment-only and byte-precise — one line, the NUL replaced by the text U+0000. 670 fluree-db-api lib tests unchanged, full suite re-run at the tip.

Found by the control-byte guard built for the RDF-toolkit work, which flagged this as its second literal NUL; the first was in a Turtle test fixture and is fixed on that branch. Riding here rather than standing alone because a one-byte PR against main is not worth its own review slot.

Gates

Gate Result
nextest turtle + graph-ir + json-ld + format + transact + vocab 720 passed, 0 failed
nextest -p fluree-db-api (full) 2929 passed, 11 skipped
clippy --all-targets --no-deps 0 warnings
cargo fmt --all --check clean
cargo check --all-targets on excluded testsuite-sparql / testsuite-shacl clean

aaj3f added 3 commits July 29, 2026 15:06
@aaj3f
fix(turtle): a redeclared prefix must change what later names mean
6a57641 
Turtle lets a prefix be rebound part-way through a document. The parser
ignored the rebinding for any prefixed name it had already seen, so

    @Prefix e: <http://a/> .   e:x <http://p/> "1" .
    @Prefix e: <http://b/> .   e:x <http://p/> "2" .

put BOTH statements on <http://a/x>. No error, no warning — the document
says one thing and the graph holds another.

Cause: `prefixed_term_cache` is keyed by span text (`e:x`), which identifies
an IRI only relative to the bindings in force when the entry was made, and
nothing invalidated it when a binding changed. `iri_term_cache` was never
affected — it keys on the RESOLVED IRI — and neither was `@base`, since a
namespace is resolved once, when its directive is read. A test pins both, so
the scope of this fix is demonstrated rather than asserted.

Fix: on a prefix directive, clear the expansion cache IF the prefix rebinds to
a different namespace. Only if. Redeclaring the same binding is the common
case, not a curiosity — chunked import prepends the file's prefix block to
every chunk over a pre-seeded map, so the parser sees each prefix declared
twice by construction — and every cached expansion stays correct through it.

The per-term path is untouched: the diff adds no instruction to
`resolve_prefixed_term` or `sink_term_iri`. Added work is one hash lookup per
prefix DIRECTIVE, of which a document has a handful, against a cache consulted
once per prefixed NAME, of which it has millions. Clearing is coarse — a
rebind strands only that prefix's entries, but finding them means scanning
every key — and rare enough that O(cache) once beats a per-lookup epoch check
on the hot path.

Ten tests, eight of which fail without the fix; the two that pass either way
are `@base` and same-binding redeclaration, which is what shows the scope is
right. Found while wiring term validation for H-8; nothing about it is
specific to validation and it reproduces on plain `parse`.
@aaj3f
fix(api): name U+0000 in a comment instead of embedding one
2ba5f02 
admin.rs carried a literal NUL byte at line 283, inside a comment that meant
to NAME the character: "callers can otherwise sneak a `<NUL>` past". The
neighbouring lines spell the same class correctly (`U+0000..=U+001F`,
`U+007F`), so this is the one that got away.

The cost is not cosmetic, but it is grep-dependent, and the honest version
names which grep. A single NUL makes the file binary to text tools —
`file(1)` reported it as `data` rather than source — and the search behaviour
then splits:

  * ugrep 7.5.0 (what `grep` resolves to in this dev environment) exits 1 and
    prints NOTHING for a pattern occurring 23 times in the file. Not "binary
    file matches", not a warning: a clean, confident "not found".
  * BSD grep 2.6.0-FreeBSD at /usr/bin/grep — GNU-compatible, and what CI and
    most contributors get — prints "Binary file ... matches" and exits 0. It
    withholds the matching lines but does not lie about the match.

So the severe reading applies to anyone whose `grep` is ugrep, and the milder
one to everyone else; both are worse than a file that just works. `file` now
reports UTF-8 text and both greps find all 23. The earlier version of this
message claimed the silent-exit-1 behaviour universally, which was true only
for the shell it was measured in — corrected on review.

Comment-only and byte-precise: one line changes, the NUL replaced by the text
`U+0000`. 670 lib tests unchanged.

Found by the control-byte guard from the RDF-toolkit work, which flagged this
as its second literal NUL — the first was in a Turtle test fixture. Riding
here rather than standing alone because a one-byte PR against main is not
worth its own review slot.
@aaj3f
test(turtle): guard the only-when, and adopt the review's probes
56e61de 
Review fold. The fix was right; what it lacked was anything that could tell if
half of it were deleted.

THE GAP: `bind_prefix` clears the expansion cache only when a prefix actually
rebinds, and that "only when" was invisible in parser output. Clearing on a
same-binding redeclaration yields identical triples — just colder — because
`iri_term_cache` absorbs the repeated work before it can reach the sink. So a
mutation dropping the condition passed every gate in the PR. Confirmed by
running it: with the clear made unconditional, all eleven triple-level tests
still pass.

The guard is the review's `parse_reporting_cache` — test-only, returns the
prefixed-name cache hit/miss counters — plus two tests reading them: a
same-binding redeclaration must leave the cache warm (1 hit, 1 miss), a real
rebinding must drop it (0 hits, 2 misses). The same mutation now fails the
first one. That is the whole point: the counters are the only place this
behaviour is observable.

Also documented, from the review: the comparison in `bind_prefix` is on
MEANING, not spelling. `parse_prefix_directive` resolves the namespace before
storing it, so `prefixes` holds resolved IRIs — and both directions of that
matter. Same spelling under two different bases names two namespaces and MUST
clear; two spellings under one base name one namespace and must NOT. Comparing
raw text would get each backwards. It was load-bearing and undocumented.

Two review probe files adopted as permanent regression tests, credited:

- `prefix_redefinition_adversarial.rs` (9 tests) attacks what the fix's own
  tests could not reach, because they never vary the base: the missed-clear and
  spurious-clear cases above, A→B→A epochs with overlapping and disjoint names,
  the coarse whole-cache clear with twenty live prefixes, terms nested in `[ ]`
  and `( )`, escaped local names, a pre-seeded relative binding, and mixed
  directive spellings.
- `prefix_redefinition_blast_radius.rs` (2 tests) executes the PR's
  blast-radius claim instead of asserting it, and now quantifies it: of the
  post-redefinition statements, 0 are mis-resolved inside the chunk carrying
  the redefinition — the fix holds there — and 1438 are mis-resolved in later
  chunks, which is the §1.4 chunker gap this PR explicitly does not close. It
  also confirms the guarded pre-scan path WOULD reject the file, which is what
  makes "import uses the unguarded reader" the operative fact.

That 1438 is an assertion of a live defect, kept live on purpose rather than
`#[ignore]`d: an ignored test runs nowhere and rots, while this one fails the
moment §1.4 lands and tells whoever closed it to come flip the number to 0.

Wording fold: "a rebind invalidates only that prefix's entries, but finding
them means scanning every key, so we drop them all", used consistently.
@aaj3f
aaj3f requested review from bplatz and zonotope July 30, 2026 13:37
@aaj3f
test(turtle): make the carrier-chunk assertion actually distinguish t…
a7f1a39 
…he fix

The blast-radius test's `wrong_in_carrier == 0` proved nothing. Verified
before fixing: with `bind_prefix` deleted outright — the parser reverted to the
merge base, no fix at all — BOTH tests in the file still passed.

The corpus was the problem. Subjects were `e:s{i}`, a distinct span every time,
and predicates were full IRIs, so no prefixed name was ever looked up twice.
The expansion cache was never consulted for a span it already held, which is
the only shape in which a stale entry can be served. The test walked the whole
blast radius and never touched the defect it was named for.

Every line now carries `e:tag` as its object. It is cached under the first
binding and requested again after the rebinding, so a stale cache serves the
old namespace for a span whose meaning has changed. The new assertion counts
post-redefinition triples inside the CARRIER chunk — the one that actually
contains the redefinition — whose object still resolves to `http://a/tag`:

  fix removed (parser at merge base):  379   -> assertion FAILS
  fix present:                           0   -> assertion passes

`wrong_in_carrier` stays 0 in both cases, which is the demonstration that it
was never the discriminating assertion.

Kept unchanged: the subject-index chunk attribution, and the §1.4 tripwire on
later chunks. The tripwire's count moved 1438 -> 1621 with the corpus, which is
why it remains a printed diagnostic and an inequality rather than an equality —
ordinary chunking changes must not make it flap.

Found by an independent adversarial review of #1565. Two doc corrections ride
along: the meaning-vs-spelling cases are pinned in the ADVERSARIAL file (they
need a moving `@base`, which the other file never varies), and the clearing
comment now states the real trade — `retain` scans every key too, so the cost
is the same order; what it would buy is keeping other prefixes warm, and what
it costs is a predicate that has to compare up to the colon, since the obvious
`starts_with(prefix)` evicts `ex:name` when `e` rebinds. Simplicity over
post-rebind hit rate, not cost over cost.
@aaj3f aaj3f mentioned this pull request Jul 30, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bplatz bplatz Awaiting requested review from bplatz
@zonotope zonotope Awaiting requested review from zonotope

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@aaj3f