-
Notifications
You must be signed in to change notification settings - Fork 62
Fix: Escape special characters in entity descriptions #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
parmar-abhinav
wants to merge
1
commit into
FalkorDB:main
Choose a base branch
from
parmar-abhinav:develop
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Good fix for the immediate problem, but consider additional special characters and consistency.
The escaping approach is correct for apostrophes, and the order of replacements (backslash first, then quotes) properly prevents double-escaping. However, there are several areas that need verification:
Other special characters: Newlines (
\n), tabs (\t), carriage returns (\r), and other control characters can also break Cypher syntax if present in descriptions. Consider escaping these as well.Consistency with attributes: Lines 141 and 144 convert attributes using
str(attr). If the Attribute class doesn't perform similar escaping, attributes containing special characters will cause the same syntax errors.More robust approach: Parameterized queries would handle all escaping automatically and be more secure. If FalkorDB supports this, it would be preferable.
Verify that the escaping is sufficient for Cypher by checking the FalkorDB documentation:
Run this script to check if the Attribute class handles escaping:
Consider adding a test to verify special character handling:
🏁 Script executed:
Length of output: 1183
🌐 Web query:
💡 Result:
Short answer: FalkorDB follows OpenCypher conventions — use backslashes inside string literals to escape special characters (", ', \ , \n, \t, \uXXXX for Unicode), and use backticks (
...) to escape identifiers (labels, property names, variables) that contain spaces, punctuation or reserved words. Examples:Customer-Account) RETURN n.first-nameSources: FalkorDB Cypher docs and Cypher parsing / string-escape reference. [1][2]
References
[1] FalkorDB — Cypher docs.
[2] Cypher parsing / string-literal escaping (Neo4j Cypher manual).
Add missing escape sequences and fix attribute name escaping.
The verification confirms the original review concerns are valid. FalkorDB requires escaping for
\n,\t,\r, and other control characters per OpenCypher conventions, but the current implementation only handles\\,', and".Additionally, the Attribute class
__str__method (lines 161-170 in graphrag_sdk/attribute.py) returns a string without escaping the attribute name, which means attribute names containing special characters will cause syntax errors.Required fixes:
Line 148 (graphrag_sdk/entity.py): Expand escaping to include:
Lines 141, 144 (graphrag_sdk/entity.py): Attribute names must be escaped before embedding in the query. Either:
__str__method to escape the name field🤖 Prompt for AI Agents