From 810f4bc094f518d6b555192e326b05aec00dd2fd Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Tue, 19 Nov 2024 09:36:21 +1300 Subject: [PATCH] [Backport 2.x] Add support for disabling typed keys serialization (#1310) * Add support for disabling typed keys serialization (#1296) * Add support for disabling typed_keys serialization Signed-off-by: Thomas Farr * Add PR number Signed-off-by: Thomas Farr * Add guide instructions Signed-off-by: Thomas Farr * Handle existing attribute value case Signed-off-by: Thomas Farr --------- Signed-off-by: Thomas Farr (cherry picked from commit 3792e39ef2e9490d250127894e3f0f17c3a20b97) Signed-off-by: github-actions[bot] * Fix test Signed-off-by: Thomas Farr --------- Signed-off-by: Thomas Farr Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] Co-authored-by: Thomas Farr --- CHANGELOG.md | 1 + guides/json.md | 16 ++++++ .../client/json/AttributedJsonpMapper.java | 1 + .../client/json/ExternallyTaggedUnion.java | 15 ++++-- .../opensearch/client/json/JsonpMapper.java | 4 ++ .../client/json/JsonpMapperAttributes.java | 15 ++++++ .../client/json/JsonpMapperBase.java | 49 ++++++++++++++++++- .../json/jackson/JacksonJsonpMapper.java | 12 ++++- .../client/json/jsonb/JsonbJsonpMapper.java | 11 +++++ .../json/jackson/JacksonJsonpParserTest.java | 48 +----------------- .../opensearch/model/TypedKeysTest.java | 16 ++++++ 11 files changed, 135 insertions(+), 53 deletions(-) create mode 100644 java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 4125179fd6..895b5f4130 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ## [Unreleased 2.x] ### Added +- Added support for disabling typed keys serialization ([#1296](https://github.com/opensearch-project/opensearch-java/pull/1296)) ### Dependencies 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/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..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 @@ -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..c0540edc1b --- /dev/null +++ b/java-client/src/main/java/org/opensearch/client/json/JsonpMapperAttributes.java @@ -0,0 +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() {} + + 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..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 @@ -36,9 +36,57 @@ 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 java.util.Objects; 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 (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; + } /** Get a serializer when none of the builtin ones are applicable */ protected abstract JsonpDeserializer getDefaultDeserializer(Class clazz); @@ -103,5 +151,4 @@ public void serialize(JsonValue value, JsonGenerator generator, JsonpMapper mapp protected static final JsonpSerializer INSTANCE = new JsonpValueSerializer(); } - } 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 df69bc98a7..5f22c76b5f 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,12 @@ package org.opensearch.client.opensearch.model; +import com.fasterxml.jackson.databind.node.ObjectNode; +import java.util.ArrayList; import java.util.Collections; 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 +116,17 @@ 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.0,\"successful\":1.0,\"total\":1.0},\"hits\":{\"hits\":[]},\"aggregations\":{\"aggKey\":{\"buckets\":[],\"sum_other_doc_count\":0}}}"; + + assertEquals(json, toJson(resp, mapper.withAttribute(JsonpMapperAttributes.SERIALIZE_TYPED_KEYS, false))); + } }