fix(neo4j): write content_hash, so the incremental push actually diffs - #138
Merged
Conversation
bolt.ts reads each module's stored `content_hash` to find what changed, but nothing wrote it: finalizeAnalysis stripped the field from the wire envelope, and the Neo4j projection -- built from that envelope -- omitted it, with a comment in project.ts noting the strip as the reason. So the diff compared against NULL for every module and concluded everything had changed. Every push was a full re-upsert. Three parts of the system disagreed: schema.ts DECLARED `content_hash` on :TSModule, bolt.ts READ it, project.ts never WROTE it. The contract needed no change -- the property was already declared, only the writer was missing. codeanalyzer-python is the reference and does this correctly: content_hash is wire payload (schema/py_schema.py:450), projected (neo4j/project.py:689), and read back (neo4j/bolt.py:105). Verified on a live Neo4j against superset-frontend: push 1 1841 modules (1841 changed) 55s push 2, unchanged 1841 modules ( 0 changed) 25s content_hash stored 1841 / 1841 (was 0) The regression test asserts every projected :TSModule carries a non-null hash, and is break-checked. That shape matters here: the existing bolt test seeded `content_hash:'stale'` into a fixture node by hand, so it exercised the diff against data the projection could not produce, which is how this survived. Note this enables a path that has never run in anger. A permanent full re-upsert was masking any latent bug in the incremental route; the superset double-push above is the evidence that it holds at scale. Closes #118.
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.
Closes #118. Replaces #119, which GitHub auto-closed when its base branch was deleted on merging #117 — same commit, cherry-picked onto
main.The bug
bolt.tsreads each module's storedcontent_hashto decide which modules changed:Nothing wrote it.
finalizeAnalysisstripped the field from the wire envelope, and the Neo4jprojection — built from that envelope — omitted it, with
project.tsnaming the strip as thereason. The diff compared against
NULLfor every module and concluded everything had changed, soevery push was a full re-upsert.
Three parts of the system disagreed:
schema.tsdeclaredcontent_hashon:TSModule,bolt.tsread it,project.tsnever wrote it. The contract needed no change — only thewriter was missing, so
schema.neo4j.jsonis untouched.Parity
codeanalyzer-python is the reference and does this correctly:
content_hashis wire payload(
schema/py_schema.py:450), projected (neo4j/project.py:689), read back (neo4j/bolt.py:105).Verification
Live Neo4j, superset-frontend (1,841 modules):
1841 modules (1841 changed)— 55s1841 modules (0 changed)— 25scontent_hashstoredAlso confirmed on
sample-appthat editing exactly one file yields(1 changed).Suite: 240 pass, 0 fail.
About the test
The regression test asserts every projected
:TSModulecarries a non-nullcontent_hash, and isbreak-checked — reverting the projection line fails it.
That shape is deliberate. The existing bolt test hand-seeded
content_hash:'stale'into a fixturenode, exercising the diff against data the real projection could never produce. A test that supplies
the value it is verifying cannot catch this class of bug.
Risk worth naming
This enables a code path that has never run in production — the permanent full re-upsert was
masking anything latent in the incremental route. The superset double-push is evidence it holds at
real scale, but it is a first execution.