-
Notifications
You must be signed in to change notification settings - Fork 1.5k
JAVA-2589 : Quoting the Keys within extended JSON are now not compulsory anymore #463
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
Conversation
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.
Thanks for the pull request. It needs some work but you're on the right track.
@@ -88,6 +89,15 @@ public void toJsonShouldTakeACustomDocumentCodec() { | |||
assertEquals("{ \"database\" : { \"name\" : \"MongoDB\" } }", customDocument.toJson(customDocumentCodec)); | |||
} | |||
|
|||
/* |
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.
A test like this in JsonReaderTest would be more to the point:
@Test
public void testTimestampWithUnquotedKeys() {
String json = "{$timestamp : { t : 1234, i : 1 } }";
bsonReader = new JsonReader(json);
assertEquals(BsonType.TIMESTAMP, bsonReader.readBsonType());
assertEquals(new BsonTimestamp(1234, 1), bsonReader.readTimestamp());
assertEquals(AbstractBsonReader.State.DONE, bsonReader.getState());
}
@@ -1147,7 +1147,7 @@ private BsonRegularExpression visitRegularExpressionExtendedJson(final String fi | |||
|
|||
private String readStringFromExtendedJson() { | |||
JsonToken patternToken = popToken(); | |||
if (patternToken.getType() != JsonTokenType.STRING) { | |||
if (patternToken.getType() != JsonTokenType.STRING && patternToken.getType() != JsonTokenType.UNQUOTED_STRING) { |
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.
This change casts too wide a net, as this method is called from both methods implementing "Extended JSON" and for methods implementing "Shell" mode, e.g. JsonReader#visitObjectIdConstructor, and it's not correct in the shell to do something like:
{ _id : ObjectId(5aaff8caa33024673db4cddf) }
It's also called to read values in some places, not just keys.
To address this, I think you will need to leave the current method alone and add a new method that is used only to read extended JSON keys, e.g. in JsonReader#visitTimestampExtendedJson
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.
Thank you for the tips, however I wonder what is the best way to identify methods implementing "Shell" mode? Is it by looking for methods name built like this : visitXxxxConstructor? e.g "JsonReader#visitTimestampConstructor"
Hello @jyemin, sorry for my late reply and thank you for your previous advices, I think now that I have a better understanding of the Json reader. Can you please review my new commit? I've updated the Pull Request based on what you told me in your code review. |
Merged on the command line: fa2b65c Thanks for the contribution and apologies for the long delay. |
@jyemin : Hi, this is my fix for the JIRA ticket for the issue JAVA-2589.
I hope I won't get checkstyle error again. :-)
The ticket on JIRA will be updated in order to point to this actual pull request.