From bdafb4c42da433170b46d0227bbb6a8e3dcd089f Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Fri, 15 Nov 2024 13:58:23 +1300 Subject: [PATCH 1/4] Add support for disabling typed_keys serialization Signed-off-by: Thomas Farr --- CHANGELOG.md | 1 + .../client/json/AttributedJsonpMapper.java | 1 + .../client/json/ExternallyTaggedUnion.java | 15 ++++-- .../opensearch/client/json/JsonpMapper.java | 4 ++ .../client/json/JsonpMapperAttributes.java | 8 ++++ .../client/json/JsonpMapperBase.java | 37 ++++++++++++++ .../json/jackson/JacksonJsonpMapper.java | 12 ++++- .../client/json/jsonb/JsonbJsonpMapper.java | 11 +++++ .../json/jackson/JacksonJsonpParserTest.java | 48 +------------------ .../opensearch/model/TypedKeysTest.java | 19 ++++++++ 10 files changed, 104 insertions(+), 52 deletions(-) create mode 100644 java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java diff --git a/CHANGELOG.md b/CHANGELOG.md index f780e125c7..a4c92cfb79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ This section is for maintaining a changelog for all breaking changes for the cli ## [Unreleased 2.x] ### Added +- Added support for disabling `typed_keys` serialization ([#]()) ### Dependencies diff --git a/java-client/src/main/java/org/opensearch/client/json/AttributedJsonpMapper.java b/java-client/src/main/java/org/opensearch/client/json/AttributedJsonpMapper.java index baf834324a..5739e77566 100644 --- a/java-client/src/main/java/org/opensearch/client/json/AttributedJsonpMapper.java +++ b/java-client/src/main/java/org/opensearch/client/json/AttributedJsonpMapper.java @@ -36,6 +36,7 @@ import jakarta.json.stream.JsonGenerator; import jakarta.json.stream.JsonParser; +@Deprecated class AttributedJsonpMapper implements JsonpMapper { private final JsonpMapper mapper; diff --git a/java-client/src/main/java/org/opensearch/client/json/ExternallyTaggedUnion.java b/java-client/src/main/java/org/opensearch/client/json/ExternallyTaggedUnion.java index 4d5b954539..ec1adb4982 100644 --- a/java-client/src/main/java/org/opensearch/client/json/ExternallyTaggedUnion.java +++ b/java-client/src/main/java/org/opensearch/client/json/ExternallyTaggedUnion.java @@ -188,10 +188,17 @@ public void deserializeEntry(String key, JsonParser parser, JsonpMapper mapper, JsonGenerator generator, JsonpMapper mapper ) { - for (Map.Entry entry : map.entrySet()) { - T value = entry.getValue(); - generator.writeKey(value._kind().jsonValue() + "#" + entry.getKey()); - value.serialize(generator, mapper); + if (mapper.attribute(JsonpMapperAttributes.SERIALIZE_TYPED_KEYS, true)) { + for (Map.Entry entry : map.entrySet()) { + T value = entry.getValue(); + generator.writeKey(value._kind().jsonValue() + "#" + entry.getKey()); + value.serialize(generator, mapper); + } + } else { + for (Map.Entry entry : map.entrySet()) { + generator.writeKey(entry.getKey()); + entry.getValue().serialize(generator, mapper); + } } } } diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java index 410c97cfd9..9be3caad98 100644 --- a/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java @@ -88,8 +88,12 @@ default T attribute(String name, T defaultValue) { /** * Create a new mapper with a named attribute that delegates to this one. + * + * @implNote The default implementation will be removed in a future release, and inheritors will be required to implement it themselves. + * See {@link org.opensearch.client.json.jsonb.JsonbJsonpMapper#withAttribute(String, Object)} and {@link org.opensearch.client.json.jackson.JacksonJsonpMapper#withAttribute(String, Object)} for examples. */ default JsonpMapper withAttribute(String name, T value) { + //noinspection deprecation return new AttributedJsonpMapper(this, name, value); } } diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java new file mode 100644 index 0000000000..ea8d3ac40b --- /dev/null +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java @@ -0,0 +1,8 @@ +package org.opensearch.client.json; + +public final class JsonpMapperAttributes { + private JsonpMapperAttributes() { + } + + public static final String SERIALIZE_TYPED_KEYS = JsonpMapperAttributes.class.getName() + ":SERIALIZE_TYPED_KEYS"; +} diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java index 9e3295f984..9e205a5d0b 100644 --- a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java @@ -36,9 +36,46 @@ import jakarta.json.stream.JsonGenerator; import jakarta.json.stream.JsonParser; import java.lang.reflect.Field; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; public abstract class JsonpMapperBase implements JsonpMapper { + @Nullable + private Map attributes; + + protected JsonpMapperBase() { + } + + protected JsonpMapperBase(JsonpMapperBase o) { + this.attributes = o.attributes; // We always copy in `setAttribute` so no need to copy here. + } + + @Override + public T attribute(String name) { + //noinspection unchecked + return attributes == null ? null : (T) attributes.get(name); + } + + /** + * Updates attributes to a copy of the current ones with an additional key/value pair. + * Mutates the current mapper, intended to be used in implementations of {@link #withAttribute(String, Object)} + */ + protected JsonpMapperBase addAttribute(String name, Object value) { + if (attributes == null) { + this.attributes = Collections.singletonMap(name, value); + } else { + // Copy the map to avoid modifying the original in case it was shared. + // We're generally only ever called from implementations' `withAttribute` methods which are intended + // to construct new instances rather than modify existing ones. + Map attributes = new HashMap<>(this.attributes.size() + 1); + attributes.putAll(this.attributes); + attributes.put(name, value); + this.attributes = attributes; + } + return this; + } /** Get a serializer when none of the builtin ones are applicable */ protected abstract JsonpDeserializer getDefaultDeserializer(Class clazz); diff --git a/java-client/src/main/java/org/opensearch/client/json/jackson/JacksonJsonpMapper.java b/java-client/src/main/java/org/opensearch/client/json/jackson/JacksonJsonpMapper.java index 186018a0d2..c10379abc2 100644 --- a/java-client/src/main/java/org/opensearch/client/json/jackson/JacksonJsonpMapper.java +++ b/java-client/src/main/java/org/opensearch/client/json/jackson/JacksonJsonpMapper.java @@ -48,7 +48,6 @@ import org.opensearch.client.json.JsonpSerializer; public class JacksonJsonpMapper extends JsonpMapperBase { - private final JacksonJsonProvider provider; private final ObjectMapper objectMapper; @@ -68,6 +67,17 @@ public JacksonJsonpMapper() { ); } + private JacksonJsonpMapper(JacksonJsonpMapper o) { + super(o); + this.provider = o.provider; + this.objectMapper = o.objectMapper; + } + + @Override + public JsonpMapper withAttribute(String name, T value) { + return new JacksonJsonpMapper(this).addAttribute(name, value); + } + /** * Returns the underlying Jackson mapper. */ diff --git a/java-client/src/main/java/org/opensearch/client/json/jsonb/JsonbJsonpMapper.java b/java-client/src/main/java/org/opensearch/client/json/jsonb/JsonbJsonpMapper.java index 0a7c98e2da..d0df6de4eb 100644 --- a/java-client/src/main/java/org/opensearch/client/json/jsonb/JsonbJsonpMapper.java +++ b/java-client/src/main/java/org/opensearch/client/json/jsonb/JsonbJsonpMapper.java @@ -65,6 +65,17 @@ public JsonbJsonpMapper() { this(JsonProvider.provider(), JsonbProvider.provider()); } + private JsonbJsonpMapper(JsonbJsonpMapper o) { + super(o); + this.jsonProvider = o.jsonProvider; + this.jsonb = o.jsonb; + } + + @Override + public JsonpMapper withAttribute(String name, T value) { + return new JsonbJsonpMapper(this).addAttribute(name, value); + } + @Override protected JsonpDeserializer getDefaultDeserializer(Class clazz) { return new Deserializer<>(clazz); diff --git a/java-client/src/test/java/org/opensearch/client/opensearch/json/jackson/JacksonJsonpParserTest.java b/java-client/src/test/java/org/opensearch/client/opensearch/json/jackson/JacksonJsonpParserTest.java index dc0d04b1b1..faea1c9709 100644 --- a/java-client/src/test/java/org/opensearch/client/opensearch/json/jackson/JacksonJsonpParserTest.java +++ b/java-client/src/test/java/org/opensearch/client/opensearch/json/jackson/JacksonJsonpParserTest.java @@ -32,18 +32,12 @@ package org.opensearch.client.opensearch.json.jackson; -import com.fasterxml.jackson.databind.ObjectMapper; import jakarta.json.stream.JsonParser; import jakarta.json.stream.JsonParser.Event; import java.io.StringReader; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import javax.annotation.Nullable; import org.junit.Test; import org.opensearch.client.json.JsonpDeserializer; import org.opensearch.client.json.JsonpMapper; -import org.opensearch.client.json.JsonpMapperBase; import org.opensearch.client.json.jackson.JacksonJsonProvider; import org.opensearch.client.json.jackson.JacksonJsonpMapper; import org.opensearch.client.opensearch.core.MsearchResponse; @@ -176,7 +170,7 @@ public void testMultiSearchResponse() { + " ]\n" + "}\n"; - JsonpMapper mapper = new AttributedJacksonJsonpMapper().withAttribute( + JsonpMapper mapper = new JacksonJsonpMapper().withAttribute( "org.opensearch.client:Deserializer:_global.msearch.TDocument", JsonpDeserializer.of(Foo.class) ); @@ -189,46 +183,6 @@ public void testMultiSearchResponse() { assertEquals((Integer) 200, response.responses().get(1).result().status()); } - public static class AttributedJacksonJsonpMapper extends JacksonJsonpMapper { - private Map attributes; - - public AttributedJacksonJsonpMapper() { - super(); - } - - public AttributedJacksonJsonpMapper(ObjectMapper objectMapper) { - super(objectMapper); - } - - @Nullable - @Override - @SuppressWarnings("unchecked") - public T attribute(String name) { - return attributes == null ? null : (T) attributes.get(name); - } - - /** - * Updates attributes to a copy of the current ones with an additional key/value pair. - * Mutates the current mapper, intended to be used in implementations of {@link #withAttribute(String, Object)} - */ - protected JsonpMapperBase addAttribute(String name, Object value) { - if (attributes == null) { - this.attributes = Collections.singletonMap(name, value); - } else { - Map newAttrs = new HashMap<>(attributes.size() + 1); - newAttrs.putAll(attributes); - newAttrs.put(name, value); - this.attributes = newAttrs; - } - return this; - } - - @Override - public JsonpMapper withAttribute(String name, T value) { - return new AttributedJacksonJsonpMapper(objectMapper()).addAttribute(name, value); - } - } - public static class Foo { private int x; private boolean y; diff --git a/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java b/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java index 3472db587c..f91cd10c62 100644 --- a/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java +++ b/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java @@ -32,9 +32,13 @@ package org.opensearch.client.opensearch.model; +import java.util.ArrayList; import java.util.Collections; + +import com.fasterxml.jackson.databind.node.ObjectNode; import org.junit.Test; import org.opensearch.client.json.JsonpDeserializer; +import org.opensearch.client.json.JsonpMapperAttributes; import org.opensearch.client.opensearch._types.aggregations.Aggregate; import org.opensearch.client.opensearch._types.aggregations.AvgAggregate; import org.opensearch.client.opensearch._types.aggregations.StringTermsAggregate; @@ -113,4 +117,19 @@ public void testAdditionalProperties() { assertEquals("key_2", foo.buckets().array().get(1).key()); assertEquals(2.0, foo.buckets().array().get(1).aggregations().get("bar").avg().value(), 0.01); } + + @Test + public void testDisablingSerializeTypedKeys() { + SearchResponse resp = new SearchResponse.Builder() + .aggregations("aggKey", v -> v.lterms(lv -> lv.buckets(b -> b.array(new ArrayList<>())).sumOtherDocCount(0))) + .took(0) + .timedOut(false) + .shards(s -> s.failed(0).successful(1).total(1)) + .hits(h -> h.hits(new ArrayList<>())) + .build(); + + String json = "{\"took\":0,\"timed_out\":false,\"_shards\":{\"failed\":0,\"successful\":1,\"total\":1},\"hits\":{\"hits\":[]},\"aggregations\":{\"aggKey\":{\"buckets\":[],\"sum_other_doc_count\":0}}}"; + + assertEquals(json, toJson(resp, mapper.withAttribute(JsonpMapperAttributes.SERIALIZE_TYPED_KEYS, false))); + } } From d8e13c547a0e3045e8aaf29177bdb4805bcfbf97 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Fri, 15 Nov 2024 14:11:56 +1300 Subject: [PATCH 2/4] Add PR number Signed-off-by: Thomas Farr --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4c92cfb79..5a54ced1cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,7 @@ This section is for maintaining a changelog for all breaking changes for the cli ## [Unreleased 2.x] ### Added -- Added support for disabling `typed_keys` serialization ([#]()) +- Added support for disabling typed keys serialization ([#1296](https://github.com/opensearch-project/opensearch-java/pull/1296)) ### Dependencies From 7589ec838234ccf4379bdb28c2f4ad2ec1075a75 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Fri, 15 Nov 2024 14:27:13 +1300 Subject: [PATCH 3/4] Add guide instructions Signed-off-by: Thomas Farr --- guides/json.md | 16 ++++++++++++++++ .../opensearch/client/json/JsonpMapper.java | 2 +- .../client/json/JsonpMapperAttributes.java | 11 +++++++++-- .../client/json/JsonpMapperBase.java | 5 ++--- .../opensearch/model/TypedKeysTest.java | 19 ++++++++----------- 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/guides/json.md b/guides/json.md index 74cbf826be..86e7f15fb9 100644 --- a/guides/json.md +++ b/guides/json.md @@ -55,6 +55,22 @@ private String toJson(JsonpSerializable object) { } ``` +### Disabling Typed Keys Serialization +By default, the JSON serialization of the OpenSearch Java client uses typed keys for certain types, notably Aggregations. +This is done for the benefit of unambiguous deserialization, but may result in JSON output that is incompatible with use-cases expecting OpenSearch's default untyped keys. +You may disable this behavior by setting the `JsonpMapperAttributes.SERIALIZE_TYPED_KEYS` attribute to `false` on a JsonpMapper instance. +For example, the following code demonstrates how to serialize a SearchResponse without typed keys: +```java +private String withoutTypedKeys(OpenSearchClient client, SearchResponse response) { + JsonpMapper mapper = client._transport().jsonpMapper().withAttribute(JsonpMapperAttributes.SERIALIZE_TYPED_KEYS, false); + StringWriter writer = new StringWriter(); + try (JsonGenerator generator = mapper.jsonProvider().createGenerator(writer)) { + response.serialize(generator, mapper); + } + return writer.toString(); +} +``` + ## Deserialization For demonstration let's consider an IndexTemplateMapping JSON String. diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java index 9be3caad98..ae3a1d1538 100644 --- a/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapper.java @@ -93,7 +93,7 @@ default T attribute(String name, T defaultValue) { * See {@link org.opensearch.client.json.jsonb.JsonbJsonpMapper#withAttribute(String, Object)} and {@link org.opensearch.client.json.jackson.JacksonJsonpMapper#withAttribute(String, Object)} for examples. */ default JsonpMapper withAttribute(String name, T value) { - //noinspection deprecation + // noinspection deprecation return new AttributedJsonpMapper(this, name, value); } } diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java index ea8d3ac40b..c0540edc1b 100644 --- a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java @@ -1,8 +1,15 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + package org.opensearch.client.json; public final class JsonpMapperAttributes { - private JsonpMapperAttributes() { - } + private JsonpMapperAttributes() {} public static final String SERIALIZE_TYPED_KEYS = JsonpMapperAttributes.class.getName() + ":SERIALIZE_TYPED_KEYS"; } diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java index 9e205a5d0b..2536d14199 100644 --- a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java @@ -45,8 +45,7 @@ public abstract class JsonpMapperBase implements JsonpMapper { @Nullable private Map attributes; - protected JsonpMapperBase() { - } + protected JsonpMapperBase() {} protected JsonpMapperBase(JsonpMapperBase o) { this.attributes = o.attributes; // We always copy in `setAttribute` so no need to copy here. @@ -54,7 +53,7 @@ protected JsonpMapperBase(JsonpMapperBase o) { @Override public T attribute(String name) { - //noinspection unchecked + // noinspection unchecked return attributes == null ? null : (T) attributes.get(name); } diff --git a/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java b/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java index f91cd10c62..7309014690 100644 --- a/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java +++ b/java-client/src/test/java/org/opensearch/client/opensearch/model/TypedKeysTest.java @@ -32,10 +32,9 @@ package org.opensearch.client.opensearch.model; +import com.fasterxml.jackson.databind.node.ObjectNode; import java.util.ArrayList; import java.util.Collections; - -import com.fasterxml.jackson.databind.node.ObjectNode; import org.junit.Test; import org.opensearch.client.json.JsonpDeserializer; import org.opensearch.client.json.JsonpMapperAttributes; @@ -120,15 +119,13 @@ public void testAdditionalProperties() { @Test public void testDisablingSerializeTypedKeys() { - SearchResponse resp = new SearchResponse.Builder() - .aggregations("aggKey", v -> v.lterms(lv -> lv.buckets(b -> b.array(new ArrayList<>())).sumOtherDocCount(0))) - .took(0) - .timedOut(false) - .shards(s -> s.failed(0).successful(1).total(1)) - .hits(h -> h.hits(new ArrayList<>())) - .build(); - - String json = "{\"took\":0,\"timed_out\":false,\"_shards\":{\"failed\":0,\"successful\":1,\"total\":1},\"hits\":{\"hits\":[]},\"aggregations\":{\"aggKey\":{\"buckets\":[],\"sum_other_doc_count\":0}}}"; + SearchResponse resp = new SearchResponse.Builder().aggregations( + "aggKey", + v -> v.lterms(lv -> lv.buckets(b -> b.array(new ArrayList<>())).sumOtherDocCount(0)) + ).took(0).timedOut(false).shards(s -> s.failed(0).successful(1).total(1)).hits(h -> h.hits(new ArrayList<>())).build(); + + String json = + "{\"took\":0,\"timed_out\":false,\"_shards\":{\"failed\":0,\"successful\":1,\"total\":1},\"hits\":{\"hits\":[]},\"aggregations\":{\"aggKey\":{\"buckets\":[],\"sum_other_doc_count\":0}}}"; assertEquals(json, toJson(resp, mapper.withAttribute(JsonpMapperAttributes.SERIALIZE_TYPED_KEYS, false))); } From b3740fec1e1454871305a98df3f9942743552b13 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 18 Nov 2024 12:15:38 +1300 Subject: [PATCH 4/4] Handle existing attribute value case Signed-off-by: Thomas Farr --- .../client/json/JsonpMapperBase.java | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java index 2536d14199..83e210fc31 100644 --- a/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperBase.java @@ -39,6 +39,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.Objects; import javax.annotation.Nullable; public abstract class JsonpMapperBase implements JsonpMapper { @@ -62,17 +63,28 @@ public T attribute(String name) { * Mutates the current mapper, intended to be used in implementations of {@link #withAttribute(String, Object)} */ protected JsonpMapperBase addAttribute(String name, Object value) { - if (attributes == null) { - this.attributes = Collections.singletonMap(name, value); - } else { - // Copy the map to avoid modifying the original in case it was shared. - // We're generally only ever called from implementations' `withAttribute` methods which are intended - // to construct new instances rather than modify existing ones. - Map attributes = new HashMap<>(this.attributes.size() + 1); - attributes.putAll(this.attributes); - attributes.put(name, value); - this.attributes = attributes; + if (this.attributes == null) { + // Don't bother creating a map if the value is null, as we don't distinguish between explicit null and missing on retrieval. + if (value != null) { + this.attributes = Collections.singletonMap(name, value); + } + return this; + } + + Object existingValue = this.attributes.get(name); + + if (Objects.equals(existingValue, value)) { + return this; } + + // Copy the map to avoid modifying the original in case it was shared. + // We're generally only ever called from implementations' `withAttribute` methods which are intended + // to construct new instances rather than modify existing ones. + Map attributes = new HashMap<>(this.attributes.size() + (!this.attributes.containsKey(name) ? 1 : 0)); + attributes.putAll(this.attributes); + attributes.put(name, value); + this.attributes = attributes; + return this; } @@ -139,5 +151,4 @@ public void serialize(JsonValue value, JsonGenerator generator, JsonpMapper mapp protected static final JsonpSerializer INSTANCE = new JsonpValueSerializer(); } - }