JAVA-3292: Add a Codec for converting directly to and from a JSON string #572

bdeleonardis1 · 2020-09-02T22:24:01Z

No description provided.

bdeleonardis1 · 2020-09-02T22:26:22Z

bson/src/main/org/bson/json/JsonString.java

+ *
+ * @since 4.2
+ */
+public class JsonString {


I wasn't sure where to put this file. This folder contains json stuff, but it's not that related to the purpose of this file. I was debating between this folder and its parent folder.

I decided against providing equals and hashcode methods because I thought users shouldn't really keep JsonStrings, they should just use them to access the string. Do you agree with this decision?

I don't have any unit tests for this class. Is this okay for simple wrapper classes like this?

I think this location is fine.

I would add an equals and hashcode for comparison purposes - we don't know how users will use this class and if they will always unbox it directly.

Yes lets add some unit tests.

driver-core/src/main/com/mongodb/MongoClientSettings.java

bdeleonardis1 · 2020-09-03T15:18:09Z

Forgot to run a patch when I first put it up, here it is: https://spruce.mongodb.com/version/5f510b9c7742ae610dba68c5/tasks

rozza

Some comments and nits.

rozza · 2020-09-08T09:24:48Z

bson/src/main/org/bson/codecs/JsonStringCodec.java

+     * Construct a JsonStringCodec with default JsonWriterSettings
+     */
+    public JsonStringCodec() {
+        this(JsonWriterSettings.builder().outputMode(JsonMode.RELAXED).build());


Comment (no action required): Seems odd to explicitly set the output mode when the javadoc notes its the default settings.

I know you said no action required, but I agreed that this seemed weird so I got rid of setting the outputMode explicitly.

rozza · 2020-09-08T09:28:52Z

bson/src/main/org/bson/json/JsonString.java

+ *
+ * @since 4.2
+ */
+public class JsonString {


I think this location is fine.

I would add an equals and hashcode for comparison purposes - we don't know how users will use this class and if they will always unbox it directly.

Yes lets add some unit tests.

rozza · 2020-09-08T09:29:52Z

bson/src/main/org/bson/json/JsonString.java

+package org.bson.json;
+
+/**
+ * A wrapper class that holds a JSON string. This class makes decoding straight


Add a note that it holds a JSON document String. As an array is a valid top level JSON type we should clarify.

rozza · 2020-09-08T09:30:02Z

bson/src/main/org/bson/json/JsonString.java

+     *
+     * @param json the JSON string
+     */
+    public JsonString(final String json) {


Add a not null check here.

What happens if a user tries to do:

MongoCollection<JsonString> collection.insert(new JsonString("['a', 'b', 'c']"));

Do you think there should be any validation on the String?

Added the null check.

I don't think we should verify if the String is valid JSON because that would hurt performance especially for large JSON strings and the whole purpose of this ticket is to improve performance. I decided to do a middle-ground where we check that the first and last characters are curly braces. My motivation is that this helps them catch obvious bugs where they try to pass in an array for example. My concern with this is that our constructor throws exceptions for certain invalid JSON inputs, but not others so this may be unpredictable to clients. What do you think?

Now I'm wondering if we should call this class something different. Since [1, 2, 3] (or even 1?) is valid JSON, should this class be called JsonObjectString instead, if indeed it will forever be restricted to JSON objects (see https://www.json.org/json-en.html)?

Even if we don't change the name, if we are committing to only supporting JSON objects, then this class can and should implement org.bson.conversions.Bson. That way, it can be used for query filters, etc.

Alternatively, we could allow any JSON string, not just JSON objects, though currently BsonReader.pipe currently only supports documents

Done (renamed to JsonObject and implements Bson)

driver-core/src/main/com/mongodb/MongoClientSettings.java

docs/reference/content/bson/extended-json.md

bson/src/main/org/bson/json/JsonString.java

jyemin · 2020-09-08T23:03:39Z

bson/src/main/org/bson/json/JsonString.java

+     *
+     * @param json the JSON string
+     */
+    public JsonString(final String json) {


Now I'm wondering if we should call this class something different. Since [1, 2, 3] (or even 1?) is valid JSON, should this class be called JsonObjectString instead, if indeed it will forever be restricted to JSON objects (see https://www.json.org/json-en.html)?

Even if we don't change the name, if we are committing to only supporting JSON objects, then this class can and should implement org.bson.conversions.Bson. That way, it can be used for query filters, etc.

Alternatively, we could allow any JSON string, not just JSON objects, though currently BsonReader.pipe currently only supports documents

bson/src/main/org/bson/codecs/BsonCodecProvider.java

bson/src/main/org/bson/json/JsonObject.java

bson/src/main/org/bson/codecs/BsonCodecProvider.java

jyemin

Looking good. I just have one more request for additional code coverage of the new whitespace checks.

bson/src/test/unit/org/bson/json/JsonObjectTest.java

rozza · 2020-09-11T09:27:30Z

bson/src/main/org/bson/json/JsonObject.java

+     * @param json the JSON object string
+     */
+    public JsonObject(final String json) {
+        if (json == null) {


We have a notNull helper for this - see org.bson.assertions.Assertions.notNull

bson/src/main/org/bson/json/JsonObject.java

rozza · 2020-09-11T09:48:37Z

bson/src/main/org/bson/json/JsonObject.java

+            }
+
+            // Valid whitespace characters according to the json spec: https://www.json.org/json-en.html
+            if (c != 0x0020 && c != 0x000A && c != 0x000D && c != 0x0009) {


I would use Character.isWhitespace as its what is actually used when parsing json strings and is probably more readable.

I recommended this approach because it matches the JSON spec. Do you think that's not the behavior of most JSON parsers?

I suppose, my thoughts were the only parser that actually matters is our own Json reader. Note we don't follow the strictness of the Json Spec, eg we support missing quotes / single quoted values, so thought Character.isWhitespace was more readable.

Seeing that Character.isWhitespace matches our own JsonReader's behavior, I agree with Ross and I changed it. Happy to change it back if you strongly disagree @jyemin .

I'm ok with this change. You can resolve this.

bson/src/main/org/bson/json/JsonObject.java

docs/reference/content/bson/extended-json.md

rozza

LGTM!

jyemin · 2020-09-11T15:00:00Z

LGTM!

bdeleonardis1 · 2020-09-14T20:18:50Z

Merged on command line.

bdeleonardis1 added 10 commits September 2, 2020 16:05

Add JsonStringCodec

8c5f836

rm changes to DocumentCodecProvider

e187fa7

Undo unnecessary imports

0637c09

Fix indentation for the default registry

c943558

Add tests

d4b2154

Delete unnecessary newlines

1561959

Update java documetation

fc35101

Update scala documentation

5d10c45

Fix documentation

e564704

Add since labels

40c1a80

bdeleonardis1 requested a review from jyemin September 2, 2020 22:24

bdeleonardis1 commented Sep 2, 2020

View reviewed changes

jyemin requested a review from rozza September 3, 2020 13:33

Fix style check problems

2a53c02

Fix codernarc problems

4ae944b

rozza requested changes Sep 8, 2020

View reviewed changes

jyemin requested changes Sep 8, 2020

View reviewed changes

bdeleonardis1 force-pushed the 3292 branch from 65d4d44 to dfa535e Compare September 8, 2020 17:00

Code Review fixes

664a25e

bdeleonardis1 force-pushed the 3292 branch from dfa535e to 664a25e Compare September 8, 2020 17:01

jyemin reviewed Sep 8, 2020

View reviewed changes

Second round of CR fixes

a6a462f

bdeleonardis1 force-pushed the 3292 branch from 866229a to a6a462f Compare September 10, 2020 16:44

bdeleonardis1 commented Sep 10, 2020

View reviewed changes

bson/src/main/org/bson/codecs/BsonCodecProvider.java Outdated Show resolved Hide resolved

Minor test fix

f3d4824

jyemin requested changes Sep 10, 2020

View reviewed changes

bson/src/main/org/bson/json/JsonObject.java Outdated Show resolved Hide resolved

bson/src/main/org/bson/json/JsonObject.java Outdated Show resolved Hide resolved

bson/src/main/org/bson/codecs/BsonCodecProvider.java Outdated Show resolved Hide resolved

Round 3 of code review fixes

c028479

bdeleonardis1 force-pushed the 3292 branch from 6817458 to c028479 Compare September 10, 2020 21:12

Remove trimmed from docs

8be51ed

bdeleonardis1 requested review from jyemin and rozza September 10, 2020 21:16

jyemin requested changes Sep 10, 2020

View reviewed changes

bson/src/test/unit/org/bson/json/JsonObjectTest.java Show resolved Hide resolved

More code review updates

d23b3e8

bdeleonardis1 requested a review from jyemin September 11, 2020 01:10

rozza reviewed Sep 11, 2020

View reviewed changes

jyemin requested changes Sep 11, 2020

View reviewed changes

bson/src/main/org/bson/json/JsonObject.java Outdated Show resolved Hide resolved

docs/reference/content/bson/extended-json.md Outdated Show resolved Hide resolved

Another round of CR fixes

2bcd030

bdeleonardis1 requested review from rozza and jyemin September 11, 2020 14:45

rozza approved these changes Sep 11, 2020

View reviewed changes

jyemin approved these changes Sep 11, 2020

View reviewed changes

bdeleonardis1 closed this Sep 14, 2020

JAVA-3292: Add a Codec for converting directly to and from a JSON string #572

JAVA-3292: Add a Codec for converting directly to and from a JSON string #572

Uh oh!

Conversation

bdeleonardis1 commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdeleonardis1 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rozza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdeleonardis1 Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyemin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rozza left a comment

Choose a reason for hiding this comment

bdeleonardis1 commented Sep 3, 2020 •

edited

Loading

bdeleonardis1 Sep 10, 2020 •

edited

Loading