Conversation
| .collect(Collectors.joining("|")); | ||
| String key = StreamSupport.stream(keys.spliterator(), false) | ||
| .map(s -> String.format(PROPERTY_QUOTING_FORMAT, "n", s)) | ||
| .map(s -> String.format(PROPERTY_QUOTING_FORMAT, "n", s.replace("`", "``"))) |
There was a problem hiding this comment.
Is there a particular reason this uses a hand crafted escape? Same with the one further down.
There was a problem hiding this comment.
Yeah true, should probably use the sanitize function :) will update
| } | ||
|
|
||
| public static String quoteIdentifierSafely(String value) { | ||
| if (SourceVersion.isIdentifier(value) && !value.contains("$")) { |
There was a problem hiding this comment.
Unrelated to your change but this !var.contains("$") I guess is some kind of parameter quick escape? E.g. you don't want to quote the param $parameter. If that's the case - won't it behave weirdly with input that has $ in the variable name? var$name?
There was a problem hiding this comment.
So, the SourceVersion.isIdentifier doesn't include dollar signs, so it checks if the string contains that and then quotes it. So var$name would always be quoted which is correct behaviour
| } | ||
|
|
||
| // This is in common, so I suspect this is used by extended, leaving as is, please use quoteIdentifierSafely | ||
| // instead. |
There was a problem hiding this comment.
is there a way to warn us when invoking this method? perhaps an annotation saying that its deprecated with a pointer to the quoteIdentifierSafely method. it might not be possible since the quote method seem to be used elsewhere, and is not in fact considered deprecated
There was a problem hiding this comment.
I can add a deprecated, the reason it stays is because the common module is shared by the APOC extended repo, so if I remove it they may get failures there all of a sudden :)
There was a problem hiding this comment.
i think that makes sense, its not a blocker for me so you can decide. just wanted to point out that there might a chance that its overlooked ( :
There was a problem hiding this comment.
I have already added it :P would you like to approve the PR if you're happy?
There was a problem hiding this comment.
oh my bad, it was my intention to write this comment & approve PR at the same time!
I followed Joels comment on my last PR and decided to update everything to use the safer way of sanitizing, but APOC extended uses the old way, so I left it for them with a comment.
Linear: fixes: SURF-1405, SURF-1436