[CALCITE-6509] MongoAdapter throws 'Invalid JSON Number' exception if first character of field name is number #3896
Conversation
… first character of field name is number
dssysolyatin
force-pushed
the
CALCITE-6509
branch
from
cf80bcc
to
1ccd12e
Compare
|
| @@ -114,10 +114,14 @@ static String quote(String s) { | |||
| } | |||
|
|
|||
| private static boolean needsQuote(String s) { | |||
| for (int i = 0, n = s.length(); i < n; i++) { | |||
| if (!s.isEmpty() | |||
There was a problem hiding this comment.
The field name can contain '$' ? The new code doesn't cover when '$' is in the middle of the field like 'te$st'.
There was a problem hiding this comment.
It is valid to use $ in the middle of an identifier. It is invalid only when it is at the beginning of the string.
db.zip.aggregate({ $project: { te$st: "$zip_code" } })
There was a problem hiding this comment.
Yes. Looks good. Do you think we should add this case in a unit test?
There was a problem hiding this comment.
I don't think so, because $ in the middle will work in any case, whether quoted or unquoted. Instead of writing all possible tests for quoting, it would be better to rewrite the Mongo adapter to use MongoDB builders. This way, we can avoid such mistakes.
There was a problem hiding this comment.
The newly modified code and test cases do not cover the original code logic. Testing coverage is not enough for this PR
There was a problem hiding this comment.
Based on your reply, I know it works for me. But only focus on the code, I don't know.
There was a problem hiding this comment.
Probably the best option would be to find a specification for fieldName and add it as a comment. I will try to find it tomorrow. Because usually, people don't write tests to check if letters 'e', 'a', or 'b' are allowed in the middle of a string if the specification states that they are allowed.
… first character of field name is number