Skip to content
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

Support jsonValue in JsonTreeWriter #1819

Closed
wants to merge 5 commits into from

Conversation

ZacSweers
Copy link

Resolves #1289 and resolves #1268

Supersedes #1651

This adds support for JsonWriter#jsonValue() in JsonTreeWriter, which currently just throws a nondescript assertion error

Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JsonElement javadoc is now incorrect:

It could either be a {@link JsonObject}, a {@link JsonArray}, a {@link JsonPrimitive}
or a {@link JsonNull}.

Additionally this new JsonElement subclass can break the logic in JsonTreeReader.peek() and MapTypeAdapterFactory.Adapter.keyToString(JsonElement).

I am not sure if adding this class is really a good solution. Maybe this problem cannot be solved in a reasonable way for JsonTreeWriter other than throwning an exception.


Note that I am not a member of this project.

gson/src/main/java/com/google/gson/JsonElement.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonElement.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonElement.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonValue.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonValue.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonValue.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonElement.java Outdated Show resolved Hide resolved
gson/src/main/java/com/google/gson/JsonValue.java Outdated Show resolved Hide resolved
@ZacSweers
Copy link
Author

Can update the map key and doc. I don't expect this would ever appear in a reader, but maybe converting it to a JsonElement directly rather than a raw value could allow seamless interoperability. Will look

@ZacSweers
Copy link
Author

@Marcono1234 I managed to make this work without introducing a new type, which came out a lot simpler and just reuses existing types by eagerly parsing the JSON. It's a little less performant in theory for large blobs, but seems worth the tradeoff and maybe even adds a bit of extra peace of mind for validating the JSON structure is sound.

@ZacSweers
Copy link
Author

Note that a workaround for anyone that needs this and is willing to touch internal APIs - you can convert your string to a JsonElement (i.e. via JsonParser) and then write it directly to the writer with GSON's internal JsonElement adapter (i.e. TypeAdapters.JSON_ELEMENT.write(writer, jsonElement))

Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks! This makes it much more reliable.


then write it directly to the writer with GSON's internal JsonElement adapter (i.e. TypeAdapters.JSON_ELEMENT.write(writer, jsonElement))

Could you please explain which use case you are describing? I assume you are not talking about overriding JsonWriter.jsonValue(...)? For serializing a JsonElement to JSON you would normally use one of the Gson.toJson(JsonElement, ...) methods.

* @since 2.8.7
*/
@Override public JsonWriter jsonValue(String value) throws IOException {
put(JsonParser.parseString(value));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out in the previous review comment, could you please check for null and then performreturn nullValue(); instead; the documentation demands this behavior, currently it would throw a NullPointerException.

.name("string")
.jsonValue("\"hello\"")
.name("null")
.jsonValue("null")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also call jsonValue(null)? E.g.:

Suggested change
.jsonValue("null")
.jsonValue("null")
.name("javaNull")
.jsonValue(null)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants