-
Notifications
You must be signed in to change notification settings - Fork 207
Fix direct serialization of primitive types in DEFAULT_JSONP_MAPPER
#1524
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
base: main
Are you sure you want to change the base?
Fix direct serialization of primitive types in DEFAULT_JSONP_MAPPER
#1524
Conversation
9bbb2ec
to
68718dd
Compare
@@ -78,11 +80,31 @@ public JsonProvider jsonProvider() { | |||
public <T> void serialize(T value, JsonGenerator generator) { | |||
if (value instanceof JsonpSerializable) { | |||
((JsonpSerializable) value).serialize(generator, this); | |||
return; | |||
} else if (value instanceof JsonValue) { |
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.
@Xtansia the DEFAULT_JSONP_MAPPER was not designed to be generic JSON mapper, it shouldn't be used with primitive types. Back to the issue: what is expectation of sending the index request (which is just a string) to OpenSearch? I would expect the document
to a class instance of JSON type.
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.
Have switched to a more sensible example. ExtendedBounds
is generic and uses JsonpUtils.serialize
, Which directly passes through to mapper.serialize
if no explicit serializer is passed or type isn't JsonpSerializable
. Which is the same mechanism that IndexRequest
uses, just ExtendedBounds<Double>
is an actively in-use type within the API types.
I don't think it's unreasonable to directly handle the primitives that map directly to JsonGenerator
calls. The alternative is moving this logic into JsonpUtils.serialize
.
Hi, any feedback on this PR? |
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
68718dd
to
6d43be5
Compare
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
Description
Fix direct serialization of primitive types in
DEFAULT_JSONP_MAPPER
as used bytoJsonString()
.Issues Resolved
Closes #1503
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.