feat(nodes): migrate neo4j onto the graph base class (graph_neo4j)#1611
Conversation
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughNeo4j was migrated from ChangesNeo4j graph service migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GraphNode
participant IGlobal
participant Neo4jDriver
participant Neo4j
GraphNode->>IGlobal: request query or schema
IGlobal->>Neo4jDriver: open read session
Neo4jDriver->>Neo4j: execute Cypher or reflection procedure
Neo4j-->>Neo4jDriver: return records and counters
Neo4jDriver-->>IGlobal: provide result data
IGlobal-->>GraphNode: return serialized rows or schema
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)nodes/src/nodes/graph_neo4j/services.jsonFile contains syntax errors that prevent linting: Line 2: Expected a property but instead found '//'.; Line 6: End of file expected; Line 6: End of file expected; Line 6: End of file expected; Line 6: End of file expected; Line 11: End of file expected; Line 11: End of file expected; Line 11: End of file expected; Line 11: End of file expected; Line 16: End of file expected; Line 16: End of file expected; Line 16: End of file expected; Line 16: End of file expected; Line 22: End of file expected; Line 22: End of file expected; Line 22: End of file expected; Line 22: End of file expected; Line 28: End of file expected; Line 28: End of file expected; Line 28: End of file expected; Line 28: End of file expected; Line 34: End of file expected; Line 34: End of file expected; Line 34: End of file expected; Line 34: End of file expected; Line 40: End of file expected; Line 40: End of file expected; Line 40: End of file expected; Line 40: End of file expected; Line 45: End of file expected; Li ... [truncated 1795 characters] ... ine 170: End of file expected; Line 177: End of file expected; Line 178: End of file expected; Line 178: End of file expected; Line 178: End of file expected; Line 183: End of file expected; Line 184: End of file expected; Line 184: End of file expected; Line 191: Expected an array, an object, or a literal but instead found '// "graph_neo4j.label",'.; Line 184: End of file expected; Line 191: End of file expected; Line 192: End of file expected; Line 192: End of file expected; Line 193: End of file expected; Line 194: End of file expected; Line 196: End of file expected; Line 196: End of file expected; Line 196: End of file expected; Line 208: End of file expected; Line 214: End of file expected; Line 214: End of file expected; Line 214: End of file expected; Line 221: End of file expected 🔧 ESLint
packages/docs/redirects.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: expand is not a function Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/graph_neo4j/IGlobal.py`:
- Around line 191-200: Update the node_props construction in the session.run
schema loop to use each record’s nodeLabels collection instead of the
nodeType-derived label. Add the property and its type to every individual label
in nodeLabels, preserving the existing fallback type of ANY and skipping records
without usable labels.
- Around line 62-76: The _open_driver method must keep the newly created Neo4j
driver local, close it if connectivity or database verification fails, and
assign it to self.driver only after both checks succeed. In _reflect_nodes, read
the nodeLabels field instead of nodeType so each label is reflected individually
for get_schema.
In `@nodes/test/test_graph_neo4j.py`:
- Around line 161-198: Update _scoped_stubs and _load_node to restore every
dynamically injected canonical module after loading. Add ai.common.graph,
ai.common.config_utils, ai.common.tool_args, and all nodes.graph_neo4j module
names to _STUB_NAMES; capture the graph module before cleanup and return it
alongside the loaded node modules so graph_base does not rely on stale
sys.modules entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0a41f26-8056-4f8d-aa81-10db97523c0d
⛔ Files ignored due to path filters (1)
nodes/src/nodes/graph_neo4j/neo4j.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
docs/README-nodes.mdnodes/src/nodes/db_neo4j/IGlobal.pynodes/src/nodes/db_neo4j/IInstance.pynodes/src/nodes/db_neo4j/utils.pynodes/src/nodes/graph_falkordb/README.mdnodes/src/nodes/graph_neo4j/IGlobal.pynodes/src/nodes/graph_neo4j/IInstance.pynodes/src/nodes/graph_neo4j/README.mdnodes/src/nodes/graph_neo4j/__init__.pynodes/src/nodes/graph_neo4j/requirements.txtnodes/src/nodes/graph_neo4j/services.jsonnodes/test/test_graph_neo4j.pypackages/docs/redirects.ts
💤 Files with no reviewable changes (3)
- nodes/src/nodes/db_neo4j/IInstance.py
- nodes/src/nodes/db_neo4j/utils.py
- nodes/src/nodes/db_neo4j/IGlobal.py
dylan-savage
left a comment
dylan-savage
left a comment
There was a problem hiding this comment.
One thing to fix: Unbounded _run_query — [IGlobal.py:130-131] graph_neo4j/IGlobal.py L128-L133). The limit is None path streams all rows uncapped, violating the GraphGlobalBase contract that the sibling graph_falkordb honors (max_rows cap). The pipeline questions lane always hits this path — memory-blowup risk.
|
Good catch
Fixed in fe208f5, mirroring the sibling: cap = self.read_row_cap if limit is None else min(int(limit), self.read_row_cap) + 1That also clamps an explicit |
fe208f5 to
75f6787
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/graph_neo4j/README.md (1)
118-130: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the documentation generator to update schema keys.
The generated parameters still show the old
neo4jdb.*field keys instead of the updatedgraph_neo4j.*keys. As per path instructions, do not hand-edit this block; please run thenodes:docs-generatescript to refresh this section so it reflects the recent changes made toservices.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/README.md` around lines 118 - 130, Run the nodes:docs-generate script to regenerate the parameter table in the graph_neo4j README, replacing the outdated neo4jdb.* keys with the current graph_neo4j.* keys. Do not manually edit the generated documentation block.Source: Path instructions
♻️ Duplicate comments (2)
nodes/src/nodes/graph_neo4j/IGlobal.py (2)
67-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the temporary driver on failed init.
If connectivity or the database probe raises an exception, the newly created driver stays assigned and open. Keep the driver in a local variable, close it on failure, and assign
self.driveronly after both checks pass to prevent leaking resources or leaving the driver in a broken state.🔧 Proposed fix
- try: - self.driver = neo4j.GraphDatabase.driver(self.uri, auth=self._build_auth(config)) - self.driver.verify_connectivity() - # verify_connectivity() only checks the home database — probe the - # configured one so a wrong name or missing permission fails fast. - with self.driver.session(database=self.database) as session: - session.run('RETURN 1').consume() - except (ServiceUnavailable, Neo4jError) as e: - error(f'Neo4J connection failed: {e}') - raise + try: + driver = neo4j.GraphDatabase.driver(self.uri, auth=self._build_auth(config)) + try: + driver.verify_connectivity() + # verify_connectivity() only checks the home database — probe the + # configured one so a wrong name or missing permission fails fast. + with driver.session(database=self.database) as session: + session.run('RETURN 1').consume() + self.driver = driver + except Exception: + driver.close() + raise + except (ServiceUnavailable, Neo4jError) as e: + error(f'Neo4J connection failed: {e}') + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/IGlobal.py` around lines 67 - 76, Update the initialization flow around self.driver and the connectivity/database probe to create the Neo4j driver in a local variable, close that local driver in the exception path, and assign it to self.driver only after both checks succeed. Preserve the existing error logging and re-raise behavior.
191-200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
nodeLabelswhen buildingnode_props.
nodeTypecollapses multi-label nodes into a combined key likePerson:Employee, which can causeget_schemaqueries on a single label to miss entries. Populate each label individually fromnodeLabelsinstead.🔧 Proposed fix
- for record in session.run('CALL db.schema.nodeTypeProperties()'): - raw_label = record.get('nodeType', '') - label = raw_label.lstrip(':') if raw_label else '' - if not label: - continue - node_props.setdefault(label, []) - prop = record.get('propertyName') or '' - if prop: - types = record.get('propertyTypes') or [] - node_props[label].append((prop, types[0] if types else 'ANY')) + for record in session.run('CALL db.schema.nodeTypeProperties()'): + labels = record.get('nodeLabels') or [] + prop = record.get('propertyName') or '' + types = record.get('propertyTypes') or [] + prop_type = types[0] if types else 'ANY' + for label in labels: + if not label: + continue + node_props.setdefault(label, []) + if prop: + node_props[label].append((prop, prop_type))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/IGlobal.py` around lines 191 - 200, Update the node_props construction in the session.run schema loop to read nodeLabels and add each label independently, rather than deriving a single label from nodeType. Preserve the existing propertyName and propertyTypes handling, associating each property with every label returned in nodeLabels so single-label get_schema queries find the entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/graph_neo4j/__init__.py`:
- Line 31: Update the requirements path construction in the module
initialization code to use os.path.join with the directory derived from __file__
and "requirements.txt", replacing string concatenation while preserving the
resulting file location.
In `@nodes/test/test_graph_neo4j.py`:
- Around line 413-422: Expand test_get_schema_filters_by_label with at least one
relationship unrelated to the Person label, then assert the filtered result
excludes that relationship while retaining the Person-related relationship. Keep
the existing label, node, and database assertions unchanged.
---
Outside diff comments:
In `@nodes/src/nodes/graph_neo4j/README.md`:
- Around line 118-130: Run the nodes:docs-generate script to regenerate the
parameter table in the graph_neo4j README, replacing the outdated neo4jdb.* keys
with the current graph_neo4j.* keys. Do not manually edit the generated
documentation block.
---
Duplicate comments:
In `@nodes/src/nodes/graph_neo4j/IGlobal.py`:
- Around line 67-76: Update the initialization flow around self.driver and the
connectivity/database probe to create the Neo4j driver in a local variable,
close that local driver in the exception path, and assign it to self.driver only
after both checks succeed. Preserve the existing error logging and re-raise
behavior.
- Around line 191-200: Update the node_props construction in the session.run
schema loop to read nodeLabels and add each label independently, rather than
deriving a single label from nodeType. Preserve the existing propertyName and
propertyTypes handling, associating each property with every label returned in
nodeLabels so single-label get_schema queries find the entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec5d3ab4-1c9b-4dbc-abfd-391803f58c3a
⛔ Files ignored due to path filters (1)
nodes/src/nodes/graph_neo4j/neo4j.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
docs/README-nodes.mdnodes/src/nodes/db_neo4j/IGlobal.pynodes/src/nodes/db_neo4j/IInstance.pynodes/src/nodes/db_neo4j/utils.pynodes/src/nodes/graph_falkordb/README.mdnodes/src/nodes/graph_neo4j/IGlobal.pynodes/src/nodes/graph_neo4j/IInstance.pynodes/src/nodes/graph_neo4j/README.mdnodes/src/nodes/graph_neo4j/__init__.pynodes/src/nodes/graph_neo4j/requirements.txtnodes/src/nodes/graph_neo4j/services.jsonnodes/test/test_graph_neo4j.pypackages/docs/redirects.tspackages/server/engine-lib/engLib/store/headers/services.hpppackages/server/engine-lib/engLib/store/pipeline/pipeline_config.cpppackages/server/engine-lib/test/store/pipeline/pipeline_config.cpp
💤 Files with no reviewable changes (3)
- nodes/src/nodes/db_neo4j/utils.py
- nodes/src/nodes/db_neo4j/IInstance.py
- nodes/src/nodes/db_neo4j/IGlobal.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/graph_neo4j/README.md (1)
118-130: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the documentation generator to update schema keys.
The generated parameters still show the old
neo4jdb.*field keys instead of the updatedgraph_neo4j.*keys. As per path instructions, do not hand-edit this block; please run thenodes:docs-generatescript to refresh this section so it reflects the recent changes made toservices.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/README.md` around lines 118 - 130, Run the nodes:docs-generate script to regenerate the parameter table in the graph_neo4j README, replacing the outdated neo4jdb.* keys with the current graph_neo4j.* keys. Do not manually edit the generated documentation block.Source: Path instructions
♻️ Duplicate comments (2)
nodes/src/nodes/graph_neo4j/IGlobal.py (2)
67-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the temporary driver on failed init.
If connectivity or the database probe raises an exception, the newly created driver stays assigned and open. Keep the driver in a local variable, close it on failure, and assign
self.driveronly after both checks pass to prevent leaking resources or leaving the driver in a broken state.🔧 Proposed fix
- try: - self.driver = neo4j.GraphDatabase.driver(self.uri, auth=self._build_auth(config)) - self.driver.verify_connectivity() - # verify_connectivity() only checks the home database — probe the - # configured one so a wrong name or missing permission fails fast. - with self.driver.session(database=self.database) as session: - session.run('RETURN 1').consume() - except (ServiceUnavailable, Neo4jError) as e: - error(f'Neo4J connection failed: {e}') - raise + try: + driver = neo4j.GraphDatabase.driver(self.uri, auth=self._build_auth(config)) + try: + driver.verify_connectivity() + # verify_connectivity() only checks the home database — probe the + # configured one so a wrong name or missing permission fails fast. + with driver.session(database=self.database) as session: + session.run('RETURN 1').consume() + self.driver = driver + except Exception: + driver.close() + raise + except (ServiceUnavailable, Neo4jError) as e: + error(f'Neo4J connection failed: {e}') + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/IGlobal.py` around lines 67 - 76, Update the initialization flow around self.driver and the connectivity/database probe to create the Neo4j driver in a local variable, close that local driver in the exception path, and assign it to self.driver only after both checks succeed. Preserve the existing error logging and re-raise behavior.
191-200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
nodeLabelswhen buildingnode_props.
nodeTypecollapses multi-label nodes into a combined key likePerson:Employee, which can causeget_schemaqueries on a single label to miss entries. Populate each label individually fromnodeLabelsinstead.🔧 Proposed fix
- for record in session.run('CALL db.schema.nodeTypeProperties()'): - raw_label = record.get('nodeType', '') - label = raw_label.lstrip(':') if raw_label else '' - if not label: - continue - node_props.setdefault(label, []) - prop = record.get('propertyName') or '' - if prop: - types = record.get('propertyTypes') or [] - node_props[label].append((prop, types[0] if types else 'ANY')) + for record in session.run('CALL db.schema.nodeTypeProperties()'): + labels = record.get('nodeLabels') or [] + prop = record.get('propertyName') or '' + types = record.get('propertyTypes') or [] + prop_type = types[0] if types else 'ANY' + for label in labels: + if not label: + continue + node_props.setdefault(label, []) + if prop: + node_props[label].append((prop, prop_type))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/IGlobal.py` around lines 191 - 200, Update the node_props construction in the session.run schema loop to read nodeLabels and add each label independently, rather than deriving a single label from nodeType. Preserve the existing propertyName and propertyTypes handling, associating each property with every label returned in nodeLabels so single-label get_schema queries find the entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/graph_neo4j/__init__.py`:
- Line 31: Update the requirements path construction in the module
initialization code to use os.path.join with the directory derived from __file__
and "requirements.txt", replacing string concatenation while preserving the
resulting file location.
In `@nodes/test/test_graph_neo4j.py`:
- Around line 413-422: Expand test_get_schema_filters_by_label with at least one
relationship unrelated to the Person label, then assert the filtered result
excludes that relationship while retaining the Person-related relationship. Keep
the existing label, node, and database assertions unchanged.
---
Outside diff comments:
In `@nodes/src/nodes/graph_neo4j/README.md`:
- Around line 118-130: Run the nodes:docs-generate script to regenerate the
parameter table in the graph_neo4j README, replacing the outdated neo4jdb.* keys
with the current graph_neo4j.* keys. Do not manually edit the generated
documentation block.
---
Duplicate comments:
In `@nodes/src/nodes/graph_neo4j/IGlobal.py`:
- Around line 67-76: Update the initialization flow around self.driver and the
connectivity/database probe to create the Neo4j driver in a local variable,
close that local driver in the exception path, and assign it to self.driver only
after both checks succeed. Preserve the existing error logging and re-raise
behavior.
- Around line 191-200: Update the node_props construction in the session.run
schema loop to read nodeLabels and add each label independently, rather than
deriving a single label from nodeType. Preserve the existing propertyName and
propertyTypes handling, associating each property with every label returned in
nodeLabels so single-label get_schema queries find the entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec5d3ab4-1c9b-4dbc-abfd-391803f58c3a
⛔ Files ignored due to path filters (1)
nodes/src/nodes/graph_neo4j/neo4j.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
docs/README-nodes.mdnodes/src/nodes/db_neo4j/IGlobal.pynodes/src/nodes/db_neo4j/IInstance.pynodes/src/nodes/db_neo4j/utils.pynodes/src/nodes/graph_falkordb/README.mdnodes/src/nodes/graph_neo4j/IGlobal.pynodes/src/nodes/graph_neo4j/IInstance.pynodes/src/nodes/graph_neo4j/README.mdnodes/src/nodes/graph_neo4j/__init__.pynodes/src/nodes/graph_neo4j/requirements.txtnodes/src/nodes/graph_neo4j/services.jsonnodes/test/test_graph_neo4j.pypackages/docs/redirects.tspackages/server/engine-lib/engLib/store/headers/services.hpppackages/server/engine-lib/engLib/store/pipeline/pipeline_config.cpppackages/server/engine-lib/test/store/pipeline/pipeline_config.cpp
💤 Files with no reviewable changes (3)
- nodes/src/nodes/db_neo4j/utils.py
- nodes/src/nodes/db_neo4j/IInstance.py
- nodes/src/nodes/db_neo4j/IGlobal.py
🛑 Comments failed to post (2)
nodes/src/nodes/graph_neo4j/__init__.py (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use
os.path.joinfor path construction.Consider using
os.path.joininstead of string concatenation to build the file path, as it is more robust and idiomatic in Python.♻️ Proposed refactor
-requirements = os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt' +requirements = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'requirements.txt')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.requirements = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'requirements.txt')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/graph_neo4j/__init__.py` at line 31, Update the requirements path construction in the module initialization code to use os.path.join with the directory derived from __file__ and "requirements.txt", replacing string concatenation while preserving the resulting file location.nodes/test/test_graph_neo4j.py (1)
413-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exercise relationship filtering in this test.
The fixture only contains a relationship touching
Person, so returning unrelated relationships would still pass.Proposed test expansion
- 'relationships': [{'type': 'LIVES_IN', 'start': 'Person', 'end': 'City'}], + 'relationships': [ + {'type': 'LIVES_IN', 'start': 'Person', 'end': 'City'}, + {'type': 'CONNECTED_TO', 'start': 'City', 'end': 'City'}, + ], ... assert out['database'] == 'neo4j' + assert out['relationships'] == [ + {'type': 'LIVES_IN', 'start': 'Person', 'end': 'City'} + ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def test_get_schema_filters_by_label(): glb = _FakeGlobal(_FakeDriver()) glb.graph_schema = { 'nodes': {'Person': [('name', 'STRING')], 'City': [('name', 'STRING')]}, 'relationships': [ {'type': 'LIVES_IN', 'start': 'Person', 'end': 'City'}, {'type': 'CONNECTED_TO', 'start': 'City', 'end': 'City'}, ], } out = _instance(glb).get_schema({'label': 'Person'}) assert out['labels'] == ['Person'] assert 'City' not in out['nodes'] assert out['database'] == 'neo4j' assert out['relationships'] == [ {'type': 'LIVES_IN', 'start': 'Person', 'end': 'City'} ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/test/test_graph_neo4j.py` around lines 413 - 422, Expand test_get_schema_filters_by_label with at least one relationship unrelated to the Person label, then assert the filtered result excludes that relationship while retaining the Person-related relationship. Keep the existing label, node, and database assertions unchanged.
joshuadarron
left a comment
joshuadarron
left a comment
There was a problem hiding this comment.
Approving — this is a clean migration and I verified it end-to-end rather than just reading it:
- Ran the new test suite against the real base class (stubs for
rocketlib/neo4jonly, realai.common.graph+ai.common.utilsfrom disk, mirroringtest_graph_falkordb.py): 15/15 pass. Ruff on the three new Python files: clean. - Base-class contract:
_run_query'smin(limit, cap) + 1truncation shape, READ access mode, unconditionalaffected_rows, and theread_row_capfallback all match both theGraphGlobalBasedocstrings and the FalkorDB implementation line-for-line. - Saved-pipe compatibility: the
neo4jdb.→graph_neo4j.field-id rename is safe because dotted field ids flatten to unprefixed root config keys (uri,user, ...), which are unchanged; the provider rename is handled by the newversion < 2upgrade inPipelineConfig::upgradeComponent, which also retrofits thetool_falkordbrename that #1584 shipped without a migration. The version-range validation (1 <= v <= VERSION) and the Catch2 sections line up. - Loose ends checked: repo-wide grep for
db_neo4jfinds only comments/docstrings (below) andconstraints.lock, which CI regenerates when the renamedrequirements.txtpath lands. Docs redirect covers the old URL.
Two non-blocking items, one of which I'd fix before merge:
get_cypheralias returns a different payload than the old tool (inline comment) — the compat claim only holds for the call, not the response.- Stale docstring:
packages/ai/src/ai/common/graph/__init__.py:29still describesdb_neo4jas graph-capable-but-not-migrated — worth updating in this PR since it's the sentence this change makes untrue (same for the passing mention ingraph_falkordb/services.json:202).
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Approving,
one non-blocking change
Renames db_neo4j to graph_neo4j and derives it from GraphGlobalBase / GraphInstanceBase, the base extracted in #1584. The node keeps only what is specific to Neo4J -- the Bolt driver, READ access-mode sessions, and schema reflection -- while the lane handling, the natural-language-to-Cypher loop and the shared tools come from the base. IInstance drops from 431 to 129 lines and the local utils.py copy of the safety helpers is gone. The rename follows the graph_ convention and moves the node out of the Databases category into Graph Databases, so classType, protocol, path and the form field prefix change with it. get_schema is overridden to keep the label filter, and get_cypher stays as a deprecated alias for get_query: it mirrors the statement under the old `cypher` key, since get_query returns it under `query` and the alias would otherwise keep the name while silently changing the payload. Fixes carried in the migration: - _run_query refused unsafe Cypher and opened sessions in READ access mode. Bolt has no server-side read-only mode for a standalone instance, so is_cypher_safe is the primary defence here, not just defence-in-depth. - _run_query honours the base row contract: limit+1 rows for truncation detection, and read_row_cap as the ceiling when the pipeline lane calls with no limit. - affected_rows is counted unconditionally, since a Cypher write commonly also returns rows (CREATE (n) RETURN n). Pipelines are upgraded to services version 2 so saved configs still resolve. This covers both graph renames: db_neo4j -> graph_neo4j, and the earlier tool_falkordb -> graph_falkordb from #1584, which shipped without an upgrade path. Only the provider (and config type) changes -- the stored field names are untouched by either rename, and the new falkordb fields have defaults. Fixes #1600
75f6787 to
97b1c03
Compare
4 participants
Summary
db_neo4j→graph_neo4j(graph DBs use thegraph_prefix, matchinggraph_falkordb) and derive it fromGraphGlobalBase/GraphInstanceBaseinstead of its own copy of the lane handling, NL-to-Cypher loop and tool surface.classTypedatabase → graph; unifiedget_data/get_query/execute/dialecttools.get_cypherkept as a deprecated alias forget_query;get_schemaoverridden to preserve Neo4J'slabelfilter anddatabasekey.is_cypher_safegate (Bolt has no server-side read-only mode for a standalone instance, so the gate is the primary defence)._run_queryhonours the base'slimitcontract (returnslimit+1for honest truncation);_run_query_rawnow countsaffected_rowsunconditionally, fixing the0 if rowsbug (a write that also returns rows was reported as 0).Why
Follow-up to #1584 (the graph base class). @asclearuc's main review point was that
db_neo4jstill shipped its own copy of the extracted logic, so the two graph nodes presented divergent tool surfaces. This unifies them and should land before a third graph node arrives.Type
feature + refactor (node rename)
Testing
nodes/test/test_graph_neo4j.py(13 tests: read-only gate, READ access mode, limit/truncation, unconditionalaffected_rows,get_cypheralias,get_schemalabel filter, envelope unwrap).test_graph_neo4j.py+test_graph_falkordb.py= 51 passed; ruff clean; services.json parses../builder nodes:test— not run locally (the machine OOM-killed the engine build/deps step, SIGKILL 137; unrelated to this change). Relying on CI for the engine-leveltest_contracts/ dynamic loading.Checklist
db_neo4j→graph_neo4j) is breaking for saved pipes, which update when the newservices.jsonships. Neo4J-specific config keys keep working (field prefixneo4jdb.→graph_neo4j.to match the node). Old doc URL redirected.Linked Issue
Fixes #1600
Scoped to Neo4J only. Per the #1600 discussion,
db_arango(multi-model, classTypedatabase) anddb_hydradb(memory store, no query language) are not graph-base candidates, so Neo4J is the last graph-query node to migrate.Summary by CodeRabbit
New Features
graph_neo4j) with tools for schema inspection, querying, and safe execution plus dialect reporting.Documentation
graph_neo4jpath.Tests
Chores