Fix #570: Bug: encode_node mutates its argument#641
Fix #570: Bug: encode_node mutates its argument#641JiwaniZakir wants to merge 1 commit intoFalkorDB:stagingfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
tests/test_entity_encoder.py (1)
2-2: Remove unusedsysimport.The
sysmodule is imported but never used in this file.Proposed fix
import importlib.util -import sys import os🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_entity_encoder.py` at line 2, Remove the unused import of the sys module in tests/test_entity_encoder.py: delete the line importing `sys` (the unused symbol `sys`) so the file no longer contains an unused import.api/entities/entity_encoder.py (1)
5-5: Rename ambiguous variableltolabel.Ruff E741 flags
las ambiguous because it can be confused with1orIin certain fonts.Proposed fix
- result['labels'] = [l for l in n.labels if l != 'Searchable'] + result['labels'] = [label for label in n.labels if label != 'Searchable']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/entities/entity_encoder.py` at line 5, Replace the ambiguous loop variable "l" in the list comprehension that builds result['labels'] from n.labels with a clearer name like "label": update the expression that reads result['labels'] = [l for l in n.labels if l != 'Searchable'] to use "label" (e.g., result['labels'] = [label for label in n.labels if label != 'Searchable']) so the comprehension and filter reference n.labels and the string 'Searchable' unchanged but no longer use the ambiguous identifier "l".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_entity_encoder.py`:
- Around line 7-14: Replace the dynamic import block that loads entity_encoder
via importlib with a normal import: use from api.entities.entity_encoder import
encode_node; update references that currently use encode_node from the _mod
variable to use the direct import, and ensure entity_encoder.py (and its
dependencies Node/Edge/Path from falkordb) continue to import normally; you can
ignore api/__init__.py and api/info.py side effects because their logging/Redis
logic does not run on import.
---
Nitpick comments:
In `@api/entities/entity_encoder.py`:
- Line 5: Replace the ambiguous loop variable "l" in the list comprehension that
builds result['labels'] from n.labels with a clearer name like "label": update
the expression that reads result['labels'] = [l for l in n.labels if l !=
'Searchable'] to use "label" (e.g., result['labels'] = [label for label in
n.labels if label != 'Searchable']) so the comprehension and filter reference
n.labels and the string 'Searchable' unchanged but no longer use the ambiguous
identifier "l".
In `@tests/test_entity_encoder.py`:
- Line 2: Remove the unused import of the sys module in
tests/test_entity_encoder.py: delete the line importing `sys` (the unused symbol
`sys`) so the file no longer contains an unused import.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8839a9cc-a893-44b1-b4de-8ec3cad9bc90
📒 Files selected for processing (2)
api/entities/entity_encoder.pytests/test_entity_encoder.py
| # Import entity_encoder directly without triggering api/__init__.py | ||
| _spec = importlib.util.spec_from_file_location( | ||
| "entity_encoder", | ||
| os.path.join(os.path.dirname(__file__), "..", "api", "entities", "entity_encoder.py"), | ||
| ) | ||
| _mod = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(_mod) | ||
| encode_node = _mod.encode_node |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what api/__init__.py does that would cause issues when importing entity_encoder
cat api/__init__.py 2>/dev/null || echo "File not found"
# Check if other test files also use this pattern
rg -l "importlib.util.spec_from_file_location" tests/Repository: FalkorDB/code-graph
Length of output: 336
🏁 Script executed:
# Check what api/entities/__init__.py does
cat api/entities/__init__.py 2>/dev/null || echo "File not found"
# Check the test file itself to understand the context
head -30 tests/test_entity_encoder.py
# Check if there are any initialization or side effects in the modules imported by api/__init__.py
ls -la api/*.pyRepository: FalkorDB/code-graph
Length of output: 1641
🏁 Script executed:
# Check entity_encoder.py to see what it actually imports
head -30 api/entities/entity_encoder.py
# Check if graph.py or llm.py have problematic initialization
grep -n "^import\|^from" api/graph.py | head -20
grep -n "^import\|^from" api/llm.py | head -20
# Check api/info.py as it's imported first
head -20 api/info.pyRepository: FalkorDB/code-graph
Length of output: 1614
🏁 Script executed:
# Check if api/info.py has module-level code that executes connections
cat api/info.py
# Also check if any other modules in api/ might have initialization code
grep -n "redis\|Redis\|connection\|connect" api/info.pyRepository: FalkorDB/code-graph
Length of output: 5453
Remove the importlib workaround and use standard imports instead.
The dynamic import pattern is unnecessary. entity_encoder.py only depends on falkordb (imports Node, Edge, Path), and api/__init__.py has no problematic side effects at import time—it merely imports modules and calls logging.basicConfig(). The Redis connection logic in api/info.py only executes when functions are called, not during module import.
Use: from api.entities.entity_encoder import encode_node
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_entity_encoder.py` around lines 7 - 14, Replace the dynamic import
block that loads entity_encoder via importlib with a normal import: use from
api.entities.entity_encoder import encode_node; update references that currently
use encode_node from the _mod variable to use the direct import, and ensure
entity_encoder.py (and its dependencies Node/Edge/Path from falkordb) continue
to import normally; you can ignore api/__init__.py and api/info.py side effects
because their logging/Redis logic does not run on import.
1 participant
Closes #570
Fix
encode_nodeinapi/entities/entity_encoder.pyto return a copy of the node's data rather than mutating the original object.Changes
api/entities/entity_encoder.py—encode_noden.labels.remove('Searchable')+return vars(n)with a shallow copy viavars(n).copy(), then filters'Searchable'out ofresult['labels']using a list comprehension.Nodeobject is no longer touched.tests/test_entity_encoder.py(new file)encode_nodecovering the corrected behavior.Motivation
The original implementation called
n.labels.remove('Searchable')directly on the node object passed in, permanently stripping the label from the caller's data. Two concrete failure modes resulted:Nodewould observe the label gone after a single call toencode_node.ValueErroron second encode — callingencode_nodeon the same node twice raisedValueError: list.remove(x): x not in listbecause'Searchable'no longer existed inn.labels.The fix builds the output dict from a copy, leaving the source node intact.
Testing
Four pytest cases in
tests/test_entity_encoder.pyverify the fix directly:test_encode_node_removes_searchable— result dict excludes'Searchable'and retains other labels (e.g.'File').test_encode_node_does_not_mutate_original—n.labelsstill contains'Searchable'after encoding.test_encode_node_twice_does_not_raise— second call completes withoutValueErrorand returns a clean result.test_encode_node_without_searchable— nodes with no'Searchable'label pass through unchanged.All four tests fail against the original implementation and pass with the fix applied.
This PR was created with AI assistance (Claude). The changes were reviewed by quality gates and a critic model before submission.
Summary by CodeRabbit
Bug Fixes
Tests