Changes utilities.py:to_cypher_value to utilize double quotes when …

[james-geiger]

…creating cypher values.

Description

This PR alters the to_cypher_values function to utilize double quotes when serializing into a cypher query.

Pull request type

Please delete options that are not relevant.

• Bugfix

Related issues

Delete section if this PR doesn't resolve any issues.

Closes:

#318
#249

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

######################################

Reviewer checklist (the reviewer checks this part)

• Core feature implementation
• Tests
• Code documentation
• Documentation on gqlalchemy/docs

######################################

[antejavor]

It looks good, since it improves consistent in string quotes

[katarinasupe]

Hi @james-geiger, I see there are other tests that should be fixed accordingly. Also, let me see if I understood correctly:

The problem you're resolving here is the case of having a single apostrophe in a string. A current approach in GQLAlchemy uses a single apostrophe around the string in the Cypher query. That is not a good approach when a string has a single apostrophe. A solution to this issue is to have double quotes around a string in Cypher that allows a single apostrophe when needed. This PR doesn't include any changes related to the \n issue mentioned.

[CLAassistant]

All committers have signed the CLA.

[james-geiger]

@katarinasupe Changes in a727fbb to utilize json.dumps() addresses both issues, and added tests in the same commit highlight some of the issues with the current approach.

In the current release, gqlalchemy utilizes a f-string with single quotes to wrap string property values being generated as part of a cypher query. Because python allows for string instantiation with either single or double quotes, developers who utilize double quotes to instantiate a string containing a single quote, e.g.:

Node(property="This is the node's best friend.")

will encounter a cypher Invalid Query error because the generated cypher command would result in a broken property value, e.g.:

MERGE (n:Node {property:'This is the node's best friend.'}

Similar issues are encountered in the OGM when newlines are utilized alongside single and double-quotes. The escape_value() function leveraged a similar approach, first evaluating if a string .isprintable() and then either returning the repr or a single-quoted f-string. This would work, except for strings that contain a single quote and evaluate .isprintable() == False:

cases = [
    ('C1', 'This text is very plain and dry.'),
    ('C2', 'This text has a\nsemantic newline.'),
    ('C3', "This text both a possessive's and a\nsemantic newline."),
    ('C4', 'This text has "quotes" around something specific.'),
    ('C5', 'This text has an escaped possessive\'s, a\nsemantic newline, and a "quote" from the author.')
]

for title, text in cases:
    print(text.isprintable(),'|', repr(text) if text.isprintable() else rf"'{text}'")

>>> True | 'This text is very plain and dry.'
>>> False | 'This text has a
>>> semantic newline.'
>>> False | 'This text both a possessive's and a
>>> semantic newline.'
>>> True | 'This text has "quotes" around something specific.'
>>> False | 'This text has an escaped possessive's, a
>>> semantic newline, and a "quote" from the author.'

json.dumps() solves these issues and allows flexibility in how the developer instantiates their strings and whether they chose to escape their strings ahead of sending them through to cypher or not.

[katarinasupe]

Hi @james-geiger, thank you for this contribution! Can you just make sure you run all tests and fix them so they work well with your fix? You can see where they're failing in our checks.

[james-geiger]

@katarinasupe Do I need to have an active Neo4j instance for all the tests to run? I think I'm missing some because I only currently have Memgraph set up.

[katarinasupe]

No worries about Neo4j tests, you can ignore those @james-geiger (I think they will pass here).

[james-geiger]

@katarinasupe All tests passing on my local runner for python 3.11.

[katarinasupe]

@james-geiger it seems that all tests are passing now! Thank you again for your contribution! 🙏
I would love to thank you with Memgraph swag, can you reach out to me or send me your contact information so I can arrange this (I need shipping address and T-shirt size)? 😄

[katarinasupe]

Hi @james-geiger, can you remove Enum commit from this PR?

[james-geiger]

Hi @james-geiger, can you remove Enum commit from this PR?

@katarinasupe Reverted, sorry!