fix: 6 bugs found running the ArcadeDB driver end-to-end (cross-tenant leak, MVCC retry, DDL syntax, edge save, temporal filter)#1
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
@agc-63 All six of your findings are now resolved upstream in ArcadeDB - four fixed and merged, two kept as genuine graphiti-side fixes. Thank you again; the repros and fail-before/pass-after tests made every one easy to act on, and you're credited on each issue. Fixed upstream (merged to
So on a build with these, four workarounds can go:
Keep the two genuine driver-side fixes:
One heads-up: ArcadeDB stores a datetime as an instant (zone normalized to UTC) and reads sub-millisecond precision back as a zone-less value - the instant is preserved and comparisons stay correct, just noting the offset isn't round-tripped. Non-issue for graphiti's semantics. Could you trim the PR to the two keepers? Happy to do that pass and push to your branch if you'd prefer. Really appreciate you driving this. |
database_ was set inside the Cypher query params dict instead of passed as a real kwarg to the underlying neo4j driver call, so per-tenant database routing was silently ignored -- a real cross-tenant data leak (verified: querying tenant B's driver returned tenant A's data). Verified: 30 tenants x 15 rounds concurrent isolation check (450 verifications, 0 leaks).
…ported fulltext DDL get_range_indices()/get_fulltext_indices() generated ArcadeDB SQL DDL (CREATE VERTEX TYPE ... IF NOT EXISTS, CREATE INDEX ... NOTUNIQUE, FULL_TEXT ENGINE LUCENE) but sent it over the Cypher/Bolt channel, which rejects it entirely -- verified all 28 statements failed with 'mismatched input TYPE/ON' syntax errors. - Vertex/edge type declarations are unnecessary: ArcadeDB creates types implicitly on first MERGE/CREATE (schemaless behaviour, verified throughout this driver). - Range indexes rewritten with standard Cypher syntax (CREATE INDEX <name> IF NOT EXISTS FOR (n:Label) ON (n.prop)) -- ArcadeDB does support standard/RANGE indexes via Bolt, just not FULLTEXT. - Fulltext indexes dropped entirely (return []): not supported by ArcadeDB via Bolt regardless of syntax, and graphiti-core already runs without BM25/fulltext search enabled. Verified: build_indices_and_constraints() completes with zero syntax errors (previously all 28 statements failed).
- test_execute_query_success / test_execute_query_with_params: updated to assert the correct behaviour (database_ passed as a kwarg, not inside parameters_) -- the previous versions asserted the buggy behaviour. - test_execute_query_routes_to_cloned_database: with_database() must route execute_query() to the cloned database. - test_range_indices_use_valid_cypher_syntax / test_fulltext_indices_empty_for_arcadedb: DDL generation.
e1d48cb to
b430440
Compare
|
Thanks for the fast, thorough review — and for tracking down and fixing those 4 bugs upstream so quickly. Really appreciate it. Trimmed the PR down to the two driver-side keepers, per your review. Removed (verified end-to-end against a live
Kept: |
|
@agc-63 perfect thanks! |
Follow-up to getzep#1310 — two driver-side bug fixes found while running this driver end-to-end against a real ArcadeDB instance.
Based on
feat/arcadedb-backend@e9e4b47, targeting that branch (notmain) so it merges cleanly into getzep#1310.What's fixed
execute_query()'s database routing —database_was set inside the Cypher query params dict instead of passed as a real kwarg to the underlyingneo4jdriver call, so per-tenant database routing was silently ignored.build_indices_and_constraints()DDL syntax invalid over the Cypher/Bolt channel —get_range_indices()/get_fulltext_indices()generated ArcadeDB SQL DDL sent over the Cypher/Bolt channel, which rejects it entirely. Rewritten with standard CypherCREATE INDEXsyntax; fulltext indexes dropped (not supported by ArcadeDB via Bolt).What changed since the original 6-bug version
This PR originally included 4 more fixes (MVCC retry, single edge-save
UNWINDworkaround,datetime(...)wrap inretrieve_episodes(), plus a documenteddehydrate_datetimeapp-level workaround). @lvca confirmed all 4 are now fixed upstream in ArcadeDB itself (arcadedb#4906, #4910, #4921, #4923 — see the comment on #1310), so those workarounds were reverted from this PR — no longer needed on the driver side. Verified end-to-end against a livearcadedata/arcadedb:26.8.1-SNAPSHOTcontainer that the 2 remaining fixes are sufficient without them.Testing
tests/driver/test_arcadedb_driver.py), trimmed to cover only the 2 fixes above.unit-testsjob).ruff checkandruff format --checkclean.build_indices_and_constraints()completing with zero syntax errors.