Escape strings in RemoveClause #334
Conversation
Currently in RemoveClause only symbols are escaped with backticks, meaning that using `.remove` will fail on labels with special characters. This can be encountered with a namespaced label name, such as MyModule::MyThing.
2 similar comments
|
So it seems like this broke a lot of the tests due to it having some expectations that the generated queries not be escaped. I want to make sure that this PR makes sense before I try and fix those, so if I can get a 👍 that this is a reasonable change, I'll go through with it. |
| when String | ||
| "#{key}.#{value}" | ||
| when Symbol | ||
| when String, Symbol |
There was a problem hiding this comment.
this change does not make sense.
"#{key}.#{value}"
is not the same as
"#{key}:`#{value}`"
The former references a property the latter tests a node if it has a label. Use dot notation with escape and that probably will be good although readability of logs may suffer. Maybe there is a way to escape only when necessary.
There was a problem hiding this comment.
Oh I completely missed that there was a different delimiter there, good catch, that is probably also a semi-significant cause of the test errors. It seems like escape handling isn't really standardized, I discovered this particular issue in a chained method that ended in .delete, where it properly escaped everything up until then.
I'm not sure if it makes sense to check if a string needs to be delimited, that is a recipe for injection attacks, and it would incur a slight performance penalty.
This is my first contribution, but it also seems strange to have a distiction between passing a string vs. a symbol to delete. The documentation for delete seems to indicate that there shouldn't be a difference: https://neo4jrb.readthedocs.io/en/v9.4.0/QueryClauseMethods.html#delete. If there should be a distinction, it should be documented.
Currently in RemoveClause only symbols are escaped with backticks, meaning that using
.removewill fail on labels with special characters. This can be encountered with a namespaced label name, such asMyModule::MyThing.This just makes the switch process strings and symbols in the same block.