[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 <[email protected]> * Add PR number Signed-off-by: Thomas Farr <[email protected]> * Add guide instructions Signed-off-by: Thomas Farr <[email protected]> * Handle existing attribute value case Signed-off-by: Thomas Farr <[email protected]> --------- Signed-off-by: Thomas Farr <[email protected]> (cherry picked from commit 3792e39) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Fix test Signed-off-by: Thomas Farr <[email protected]> --------- Signed-off-by: Thomas Farr <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Thomas Farr <[email protected]>
opensearch-project · Nov 18, 2024 · 810f4bc · 810f4bc
1 parent f35d224
commit 810f4bc
Show file tree

Hide file tree

Showing 11 changed files with 135 additions and 53 deletions.
diff --git 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
@@ -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
@@ -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
@@ -188,10 +188,17 @@ public void deserializeEntry(String key, JsonParser parser, JsonpMapper mapper,
         JsonGenerator generator,
         JsonpMapper mapper
     ) {
-        for (Map.Entry<String, T> 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<String, T> entry : map.entrySet()) {
+                T value = entry.getValue();
+                generator.writeKey(value._kind().jsonValue() + "#" + entry.getKey());
+                value.serialize(generator, mapper);
+            }
+        } else {
+            for (Map.Entry<String, T> 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
@@ -88,8 +88,12 @@ default <T> 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 <T> 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
@@ -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
@@ -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<String, Object> 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> 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<String, Object> 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 <T> JsonpDeserializer<T> getDefaultDeserializer(Class<T> 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
@@ -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 <T> 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
@@ -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 <T> JsonpMapper withAttribute(String name, T value) {
+        return new JsonbJsonpMapper(this).addAttribute(name, value);
+    }
+
     @Override
     protected <T> JsonpDeserializer<T> getDefaultDeserializer(Class<T> clazz) {
         return new Deserializer<>(clazz);

diff --git a/...t/src/test/java/org/opensearch/client/opensearch/json/jackson/JacksonJsonpParserTest.java b/...t/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<String, Object> attributes;
-
-        public AttributedJacksonJsonpMapper() {
-            super();
-        }
-
-        public AttributedJacksonJsonpMapper(ObjectMapper objectMapper) {
-            super(objectMapper);
-        }
-
-        @Nullable
-        @Override
-        @SuppressWarnings("unchecked")
-        public <T> 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<String, Object> newAttrs = new HashMap<>(attributes.size() + 1);
-                newAttrs.putAll(attributes);
-                newAttrs.put(name, value);
-                this.attributes = newAttrs;
-            }
-            return this;
-        }
-
-        @Override
-        public <T> 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
@@ -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<ObjectNode> resp = new SearchResponse.Builder<ObjectNode>().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)));
+    }
 }