From f1c76cbf566a00cb57e67241200321ae66e125cc Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Tue, 6 Aug 2024 17:21:31 +0200 Subject: [PATCH 1/7] Add support for shortcut properties of non-primitive type --- .../_types/query_dsl/FuzzyQuery.java | 2 +- .../_types/query_dsl/MatchQuery.java | 2 +- .../_types/query_dsl/TermQuery.java | 2 +- .../core/rank_eval/RankEvalQuery.java | 2 +- .../core/search/CompletionContext.java | 2 +- .../core/search/SourceFilter.java | 2 +- .../ml/DataframeAnalysisAnalyzedFields.java | 2 +- .../security/RoleTemplateScript.java | 2 +- .../co/elastic/clients/json/JsonpUtils.java | 32 ++++++++++++++ .../clients/json/ObjectDeserializer.java | 42 +++++++++++++++---- .../clients/json/UnionDeserializer.java | 27 +++--------- .../elasticsearch/model/BehaviorsTest.java | 42 ++++++++++++++++++- 12 files changed, 119 insertions(+), 40 deletions(-) diff --git a/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java b/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java index 1569effa9..0933f2bd7 100644 --- a/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java +++ b/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java @@ -389,7 +389,7 @@ protected static void setupFuzzyQueryDeserializer(ObjectDeserializer the type of variant descriptors used by the caller. + * @param variants a map of variant descriptors, keyed by the property name that uniquely identifies the variant. + * @return a pair containing the variant descriptor (or {@code null} if not found), and a parser to be used to read the JSON object. + */ + + public static Map.Entry findVariant( + Map variants, JsonParser parser, JsonpMapper mapper + ) { + if (parser instanceof LookAheadJsonParser) { + return ((LookAheadJsonParser) parser).findVariant(variants); + } else { + // Parse as an object to find matching field names + JsonObject object = parser.getObject(); + + Variant variant = null; + for (String field: object.keySet()) { + variant = variants.get(field); + if (variant != null) { + break; + } + } + + // Traverse the object we have inspected + parser = JsonpUtils.objectParser(object, mapper); + return new AbstractMap.SimpleImmutableEntry<>(variant, parser); + } + } + /** * Create a parser that traverses a JSON object */ diff --git a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java index 8f60b23a8..4d64c33e4 100644 --- a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java +++ b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java @@ -106,7 +106,6 @@ public EnumSet acceptedEvents() { //--------------------------------------------------------------------------------------------- private static final EnumSet EventSetObject = EnumSet.of(Event.START_OBJECT, Event.KEY_NAME); - private static final EnumSet EventSetObjectAndString = EnumSet.of(Event.START_OBJECT, Event.VALUE_STRING, Event.KEY_NAME); private EnumSet acceptedEvents = EventSetObject; // May be changed in `shortcutProperty()` private final Supplier constructor; @@ -115,6 +114,7 @@ public EnumSet acceptedEvents() { private String typeProperty; private String defaultType; private FieldDeserializer shortcutProperty; + private boolean isPrimitiveShortcut; private QuadConsumer unknownFieldHandler; public ObjectDeserializer(Supplier constructor) { @@ -167,11 +167,33 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m event = parser.next(); } - if (shortcutProperty != null && event != Event.START_OBJECT && event != Event.KEY_NAME) { - // This is the shortcut property (should be a value event, this will be checked by its deserializer) - shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); + if (shortcutProperty != null) { + if (isPrimitiveShortcut) { + if (event != Event.START_OBJECT && event != Event.KEY_NAME) { + // This is the shortcut property (should be a value event, this will be checked by its deserializer) + shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); + return value; + } + } else { + // Does the shortcut property exist? If yes, the shortcut is used + Map.Entry shortcut = JsonpUtils.findVariant( + Collections.singletonMap(shortcutProperty.name, Boolean.TRUE /* arbitrary non-null value */), + parser, mapper + ); + + // Parse the buffered events + parser = shortcut.getValue(); + parser.next(); // consume START_OBJECT + + // If shortcut property was not found, this is a shortcut. Otherwise, keep deserializing as usual + if (shortcut.getKey() == null) { + shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); + return value; + } + } + } - } else if (typeProperty == null) { + if (typeProperty == null) { if (event != Event.START_OBJECT && event != Event.KEY_NAME) { // Report we're waiting for a start_object, since this is the most common beginning for object parser JsonpUtils.expectEvent(parser, Event.START_OBJECT, event); @@ -249,14 +271,18 @@ public void ignore(String name) { } public void shortcutProperty(String name) { + shortcutProperty(name, true); + } + + public void shortcutProperty(String name, boolean isPrimitive) { this.shortcutProperty = this.fieldDeserializers.get(name); if (this.shortcutProperty == null) { throw new NoSuchElementException("No deserializer was setup for '" + name + "'"); } - //acceptedEvents = EnumSet.copyOf(acceptedEvents); - //acceptedEvents.addAll(shortcutProperty.acceptedEvents()); - acceptedEvents = EventSetObjectAndString; + acceptedEvents = EnumSet.copyOf(acceptedEvents); + acceptedEvents.addAll(shortcutProperty.acceptedEvents()); + this.isPrimitiveShortcut = isPrimitive; } //----- Object types diff --git a/java-client/src/main/java/co/elastic/clients/json/UnionDeserializer.java b/java-client/src/main/java/co/elastic/clients/json/UnionDeserializer.java index bf51efb33..d6ad6c9b2 100644 --- a/java-client/src/main/java/co/elastic/clients/json/UnionDeserializer.java +++ b/java-client/src/main/java/co/elastic/clients/json/UnionDeserializer.java @@ -20,7 +20,6 @@ package co.elastic.clients.json; import co.elastic.clients.util.ObjectBuilder; -import jakarta.json.JsonObject; import jakarta.json.stream.JsonLocation; import jakarta.json.stream.JsonParser; import jakarta.json.stream.JsonParser.Event; @@ -265,28 +264,12 @@ public Union deserialize(JsonParser parser, JsonpMapper mapper, Event event) { JsonLocation location = parser.getLocation(); if (member == null && event == Event.START_OBJECT && !objectMembers.isEmpty()) { - if (parser instanceof LookAheadJsonParser) { - Map.Entry, JsonParser> memberAndParser = - ((LookAheadJsonParser) parser).findVariant(objectMembers); + Map.Entry, JsonParser> memberAndParser = + JsonpUtils.findVariant(objectMembers, parser, mapper); - member = memberAndParser.getKey(); - // Parse the buffered parser - parser = memberAndParser.getValue(); - - } else { - // Parse as an object to find matching field names - JsonObject object = parser.getObject(); - - for (String field: object.keySet()) { - member = objectMembers.get(field); - if (member != null) { - break; - } - } - - // Traverse the object we have inspected - parser = JsonpUtils.objectParser(object, mapper); - } + member = memberAndParser.getKey(); + // Parse the buffered parser + parser = memberAndParser.getValue(); if (member == null) { member = fallbackObjectMember; diff --git a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java index 6091c9421..298e9d910 100644 --- a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java +++ b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java @@ -20,6 +20,7 @@ package co.elastic.clients.elasticsearch.model; import co.elastic.clients.elasticsearch._types.ErrorCause; +import co.elastic.clients.elasticsearch._types.FieldSort; import co.elastic.clients.elasticsearch._types.FieldValue; import co.elastic.clients.elasticsearch._types.GeoLocation; import co.elastic.clients.elasticsearch._types.GeoShapeRelation; @@ -30,6 +31,7 @@ import co.elastic.clients.elasticsearch._types.query_dsl.ShapeQuery; import co.elastic.clients.elasticsearch._types.query_dsl.TermQuery; import co.elastic.clients.elasticsearch.connector.UpdateIndexNameRequest; +import co.elastic.clients.elasticsearch.core.rank_eval.RankEvalQuery; import co.elastic.clients.json.JsonData; import co.elastic.clients.testkit.ModelTestCase; import co.elastic.clients.util.MapBuilder; @@ -134,7 +136,6 @@ public void testAdditionalPropertyOnContainer() { } } - @Test public void testAdditionalProperties() { // Check that additional property map is initialized even if not set explicitly @@ -161,7 +162,7 @@ public void testAdditionalProperties() { } @Test - public void testShortcutProperty() { + public void testPrimitiveShortcutProperty() { // All-in-one: a variant, wrapping a single-key dictionary with a shortcut property String json = "{\"term\":{\"some-field\":\"some-value\"}}"; @@ -171,6 +172,43 @@ public void testShortcutProperty() { assertEquals("some-value", q.term().value().stringValue()); } + @Test + public void testEnumShortcutProperty() { + + SortOptions so = fromJson("{\"foo\":{\"order\":\"asc\"}}", SortOptions.class); + + assertEquals("foo", so.field().field()); + assertEquals(SortOrder.Asc, so.field().order()); + + so = fromJson("{\"foo\":\"asc\"}", SortOptions.class); + + assertEquals("foo", so.field().field()); + assertEquals(SortOrder.Asc, so.field().order()); + } + + @Test + public void testObjectShortcutProperty() { + + // Standard form + RankEvalQuery req = fromJson("{\"query\":{\"term\":{\"foo\":{\"value\":\"bar\"}}}}", RankEvalQuery.class); + + assertEquals("foo", req.query().term().field()); + assertEquals("bar", req.query().term().value().stringValue()); + + // Shortcut form + req = fromJson("{\"term\":{\"foo\":{\"value\":\"bar\"}}}", RankEvalQuery.class); + + assertEquals("foo", req.query().term().field()); + assertEquals("bar", req.query().term().value().stringValue()); + + // Nested shortcuts + req = fromJson("{\"term\":{\"foo\":\"bar\"}}", RankEvalQuery.class); + + assertEquals("foo", req.query().term().field()); + assertEquals("bar", req.query().term().value().stringValue()); + + } + @Test public void testWithNull() { From 028e592e20ddc5cdf89352ab5cfe1d3d59380575 Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Wed, 7 Aug 2024 19:10:07 +0200 Subject: [PATCH 2/7] Arrays can be handled without look ahead, add tests and fixes for corner cases --- .../_types/query_dsl/FuzzyQuery.java | 2 +- .../_types/query_dsl/MatchQuery.java | 2 +- .../_types/query_dsl/TermQuery.java | 2 +- .../core/rank_eval/RankEvalQuery.java | 2 +- .../core/search/CompletionContext.java | 2 +- .../core/search/SourceFilter.java | 2 +- .../ml/DataframeAnalysisAnalyzedFields.java | 2 +- .../security/RoleTemplateScript.java | 2 +- .../co/elastic/clients/json/JsonpUtils.java | 32 +++++++++----- .../clients/json/ObjectDeserializer.java | 26 ++++++++---- .../elasticsearch/model/BehaviorsTest.java | 42 ++++++++++++++++++- .../json/jackson/JsonValueParserTest.java | 2 +- 12 files changed, 90 insertions(+), 28 deletions(-) diff --git a/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java b/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java index 0933f2bd7..1ad491e4b 100644 --- a/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java +++ b/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FuzzyQuery.java @@ -389,7 +389,7 @@ protected static void setupFuzzyQueryDeserializer(ObjectDeserializer lookAheadFieldValue( throw new JsonpMappingException("Property '" + name + "' not found", location); } - JsonParser newParser = objectParser(object, mapper); + JsonParser newParser = jsonValueParser(object, mapper); // Pin location to the start of the look ahead, as the new parser will return locations in its own buffer newParser = new DelegatingJsonParser(newParser) { @@ -287,33 +287,45 @@ public static Map.Entry findVariant( if (parser instanceof LookAheadJsonParser) { return ((LookAheadJsonParser) parser).findVariant(variants); } else { - // Parse as an object to find matching field names - JsonObject object = parser.getObject(); - + // If it's an object, find matching field names Variant variant = null; - for (String field: object.keySet()) { - variant = variants.get(field); - if (variant != null) { - break; + JsonValue value = parser.getValue(); + + if (value instanceof JsonObject) { + for (String field: value.asJsonObject().keySet()) { + variant = variants.get(field); + if (variant != null) { + break; + } } } // Traverse the object we have inspected - parser = JsonpUtils.objectParser(object, mapper); + parser = JsonpUtils.jsonValueParser(value, mapper); return new AbstractMap.SimpleImmutableEntry<>(variant, parser); } } /** * Create a parser that traverses a JSON object + * + * @deprecated use {@link #jsonValueParser(JsonValue, JsonpMapper)} */ + @Deprecated public static JsonParser objectParser(JsonObject object, JsonpMapper mapper) { + return jsonValueParser(object, mapper); + } + + /** + * Create a parser that traverses a JSON value + */ + public static JsonParser jsonValueParser(JsonValue value, JsonpMapper mapper) { // FIXME: we should have used createParser(object), but this doesn't work as it creates a // org.glassfish.json.JsonStructureParser that doesn't implement the JsonP 1.0.1 features, in particular // parser.getObject(). So deserializing recursive internally-tagged union would fail with UnsupportedOperationException // While glassfish has this issue or until we write our own, we roundtrip through a string. - String strObject = object.toString(); + String strObject = value.toString(); return mapper.jsonProvider().createParser(new StringReader(strObject)); } diff --git a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java index 4d64c33e4..b50ed5cb7 100644 --- a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java +++ b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java @@ -114,7 +114,7 @@ public EnumSet acceptedEvents() { private String typeProperty; private String defaultType; private FieldDeserializer shortcutProperty; - private boolean isPrimitiveShortcut; + private boolean shortcutIsObject; private QuadConsumer unknownFieldHandler; public ObjectDeserializer(Supplier constructor) { @@ -133,6 +133,10 @@ public Set fieldNames() { return this.shortcutProperty == null ? null : this.shortcutProperty.name; } + public boolean shortcutIsObject() { + return this.shortcutIsObject; + } + @Override public EnumSet nativeEvents() { // May also return string if we have a shortcut property. This is needed to identify ambiguous unions. @@ -156,8 +160,9 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m String keyName = null; String fieldName = null; - try { + JsonParser singleKeyParser = null; + try { if (singleKey != null) { // There's a wrapping property whose name is the key value if (event == Event.START_OBJECT) { @@ -165,12 +170,15 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m } singleKey.deserialize(parser, mapper, null, value, event); event = parser.next(); + // Keep the current parser, as we needed to check the wrapping END_OBJECT below, but the variable may + // be overwritten if we have a shortcut property (happens in TermQuery). + singleKeyParser = parser; } if (shortcutProperty != null) { - if (isPrimitiveShortcut) { + if (!shortcutIsObject) { if (event != Event.START_OBJECT && event != Event.KEY_NAME) { - // This is the shortcut property (should be a value event, this will be checked by its deserializer) + // This is the shortcut property (should be a value or array event, this will be checked by its deserializer) shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); return value; } @@ -183,7 +191,7 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m // Parse the buffered events parser = shortcut.getValue(); - parser.next(); // consume START_OBJECT + event = parser.next(); // If shortcut property was not found, this is a shortcut. Otherwise, keep deserializing as usual if (shortcut.getKey() == null) { @@ -233,6 +241,8 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m } if (singleKey != null) { + // Consume the wrapping property's END_OBJECT + parser = singleKeyParser; JsonpUtils.expectNextEvent(parser, Event.END_OBJECT); } } catch (Exception e) { @@ -271,10 +281,10 @@ public void ignore(String name) { } public void shortcutProperty(String name) { - shortcutProperty(name, true); + shortcutProperty(name, false); } - public void shortcutProperty(String name, boolean isPrimitive) { + public void shortcutProperty(String name, boolean isObject) { this.shortcutProperty = this.fieldDeserializers.get(name); if (this.shortcutProperty == null) { throw new NoSuchElementException("No deserializer was setup for '" + name + "'"); @@ -282,7 +292,7 @@ public void shortcutProperty(String name, boolean isPrimitive) { acceptedEvents = EnumSet.copyOf(acceptedEvents); acceptedEvents.addAll(shortcutProperty.acceptedEvents()); - this.isPrimitiveShortcut = isPrimitive; + this.shortcutIsObject = isObject; } //----- Object types diff --git a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java index 298e9d910..24f3344f2 100644 --- a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java +++ b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java @@ -32,7 +32,10 @@ import co.elastic.clients.elasticsearch._types.query_dsl.TermQuery; import co.elastic.clients.elasticsearch.connector.UpdateIndexNameRequest; import co.elastic.clients.elasticsearch.core.rank_eval.RankEvalQuery; +import co.elastic.clients.elasticsearch.core.search.SourceFilter; import co.elastic.clients.json.JsonData; +import co.elastic.clients.json.LazyDeserializer; +import co.elastic.clients.json.ObjectDeserializer; import co.elastic.clients.testkit.ModelTestCase; import co.elastic.clients.util.MapBuilder; import org.junit.jupiter.api.Test; @@ -75,7 +78,6 @@ public void testAdditionalPropertyOnClass() { assertEquals("query-name", q.queryName()); assertTrue(q.ignoreUnmapped()); assertEquals(GeoShapeRelation.Disjoint, q.shape().relation()); - System.out.println(toJson(q)); } @Test @@ -170,11 +172,44 @@ public void testPrimitiveShortcutProperty() { assertEquals("some-field", q.term().field()); assertEquals("some-value", q.term().value().stringValue()); + + } + + @Test + public void testArrayShortcutProperty() { + + // Check that don't look ahead to handle the shortcut + ObjectDeserializer deser = (ObjectDeserializer) LazyDeserializer.unwrap(SourceFilter._DESERIALIZER); + assertEquals("includes", deser.shortcutProperty()); + assertFalse(deser.shortcutIsObject()); + + // Regular form + SourceFilter sf = fromJson("{\"includes\":[\"foo\",\"bar\"]}", SourceFilter.class); + assertEquals(2, sf.includes().size()); + assertEquals("foo", sf.includes().get(0)); + assertEquals("bar", sf.includes().get(1)); + + // Shortcut with an array value + sf = fromJson("[\"foo\",\"bar\"]", SourceFilter.class); + assertEquals(2, sf.includes().size()); + assertEquals("foo", sf.includes().get(0)); + assertEquals("bar", sf.includes().get(1)); + + // Shortcut with a single value (lenient array) + sf = fromJson("\"foo\"]", SourceFilter.class); + assertEquals(1, sf.includes().size()); + assertEquals("foo", sf.includes().get(0)); } @Test public void testEnumShortcutProperty() { + // Check that we don't look ahead to handle the shortcut + ObjectDeserializer deser = (ObjectDeserializer) LazyDeserializer.unwrap(FieldSort._DESERIALIZER); + assertEquals("order", deser.shortcutProperty()); + assertFalse(deser.shortcutIsObject()); + + // We have to test on the enclosing SortOption as FieldSort is used as a single-key dict SortOptions so = fromJson("{\"foo\":{\"order\":\"asc\"}}", SortOptions.class); assertEquals("foo", so.field().field()); @@ -189,6 +224,11 @@ public void testEnumShortcutProperty() { @Test public void testObjectShortcutProperty() { + // Check that we look ahead to handle the shortcut + ObjectDeserializer deser = (ObjectDeserializer) LazyDeserializer.unwrap(RankEvalQuery._DESERIALIZER); + assertEquals("query", deser.shortcutProperty()); + assertTrue(deser.shortcutIsObject()); + // Standard form RankEvalQuery req = fromJson("{\"query\":{\"term\":{\"foo\":{\"value\":\"bar\"}}}}", RankEvalQuery.class); diff --git a/java-client/src/test/java/co/elastic/clients/json/jackson/JsonValueParserTest.java b/java-client/src/test/java/co/elastic/clients/json/jackson/JsonValueParserTest.java index 7398141aa..0f49a6191 100644 --- a/java-client/src/test/java/co/elastic/clients/json/jackson/JsonValueParserTest.java +++ b/java-client/src/test/java/co/elastic/clients/json/jackson/JsonValueParserTest.java @@ -68,7 +68,7 @@ public void testFloatsShouldDeserializeAsFloats() throws Exception { assertEquals(v.hashCode(), v2.hashCode()); assertEquals(v, v2); - parser = JsonpUtils.objectParser(object, mapper); + parser = JsonpUtils.jsonValueParser(object, mapper); Data data = mapper.deserialize(parser, Data.class); Double d = (Double)data.data.get("value"); From ed7c5d53584e8d56513f86f4bc532278f2cb9d0f Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Fri, 9 Aug 2024 16:34:00 +0200 Subject: [PATCH 3/7] Make sure we consume single-key's end_object even if there's a shortcut --- .../clients/json/ObjectDeserializer.java | 61 ++++++++++--------- .../util/WithJsonObjectBuilderBase.java | 3 +- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java index b50ed5cb7..e0af54dc9 100644 --- a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java +++ b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java @@ -149,38 +149,51 @@ public EnumSet acceptedEvents() { } public ObjectType deserialize(JsonParser parser, JsonpMapper mapper, Event event) { - return deserialize(constructor.get(), parser, mapper, event); - } - - public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper mapper, Event event) { if (event == Event.VALUE_NULL) { return null; } - String keyName = null; - String fieldName = null; + ObjectType value = constructor.get(); + deserialize(value, parser, mapper, event); + return value; + } - JsonParser singleKeyParser = null; + public void deserialize(ObjectType value, JsonParser parser, JsonpMapper mapper, Event event) { + // Note: method is public as it's called by `withJson` to augment an already created object - try { - if (singleKey != null) { - // There's a wrapping property whose name is the key value - if (event == Event.START_OBJECT) { - event = JsonpUtils.expectNextEvent(parser, Event.KEY_NAME); - } + if (singleKey == null) { + // Nominal case + deserializeInner(value, parser, mapper, event); + + } else { + // Single key dictionary: there's a wrapping property whose name is the key value + if (event == Event.START_OBJECT) { + event = JsonpUtils.expectNextEvent(parser, Event.KEY_NAME); + } + + String keyName = parser.getString(); + try { singleKey.deserialize(parser, mapper, null, value, event); event = parser.next(); - // Keep the current parser, as we needed to check the wrapping END_OBJECT below, but the variable may - // be overwritten if we have a shortcut property (happens in TermQuery). - singleKeyParser = parser; + deserializeInner(value, parser, mapper, event); + } catch (Exception e) { + throw JsonpMappingException.from(e, value, keyName, parser); } + JsonpUtils.expectNextEvent(parser, Event.END_OBJECT); + } + } + + private void deserializeInner(ObjectType value, JsonParser parser, JsonpMapper mapper, Event event) { + String fieldName = null; + + try { if (shortcutProperty != null) { if (!shortcutIsObject) { if (event != Event.START_OBJECT && event != Event.KEY_NAME) { // This is the shortcut property (should be a value or array event, this will be checked by its deserializer) shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); - return value; + return; } } else { // Does the shortcut property exist? If yes, the shortcut is used @@ -196,7 +209,7 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m // If shortcut property was not found, this is a shortcut. Otherwise, keep deserializing as usual if (shortcut.getKey() == null) { shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); - return value; + return; } } } @@ -239,18 +252,10 @@ public ObjectType deserialize(ObjectType value, JsonParser parser, JsonpMapper m fieldDeserializer.deserialize(innerParser, mapper, variant, value); } } - - if (singleKey != null) { - // Consume the wrapping property's END_OBJECT - parser = singleKeyParser; - JsonpUtils.expectNextEvent(parser, Event.END_OBJECT); - } } catch (Exception e) { - // Add key name (for single key dicts) and field name if present - throw JsonpMappingException.from(e, value, fieldName, parser).prepend(value, keyName); + // Add field name if present + throw JsonpMappingException.from(e, value, fieldName, parser); } - - return value; } protected void parseUnknownField(JsonParser parser, JsonpMapper mapper, String fieldName, ObjectType object) { diff --git a/java-client/src/main/java/co/elastic/clients/util/WithJsonObjectBuilderBase.java b/java-client/src/main/java/co/elastic/clients/util/WithJsonObjectBuilderBase.java index fc40dc3dc..811f65fc1 100644 --- a/java-client/src/main/java/co/elastic/clients/util/WithJsonObjectBuilderBase.java +++ b/java-client/src/main/java/co/elastic/clients/util/WithJsonObjectBuilderBase.java @@ -51,7 +51,8 @@ public B withJson(JsonParser parser, JsonpMapper mapper) { @SuppressWarnings("unchecked") ObjectDeserializer builderDeser = (ObjectDeserializer) DelegatingDeserializer.unwrap(classDeser); - return builderDeser.deserialize(self(), parser, mapper, parser.next()); + builderDeser.deserialize(self(), parser, mapper, parser.next()); + return self(); } private static class WithJsonMapper extends DelegatingJsonpMapper { From a382e604befa49463bbdbe3f14d1d6af1afeead4 Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Fri, 9 Aug 2024 16:55:53 +0200 Subject: [PATCH 4/7] Fix and test FunctionScoreQuery --- .../_types/query_dsl/FunctionScoreQuery.java | 2 + .../elasticsearch/model/BehaviorsTest.java | 44 ++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FunctionScoreQuery.java b/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FunctionScoreQuery.java index 28640b77f..d48f7bb0e 100644 --- a/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FunctionScoreQuery.java +++ b/java-client/src/main/java/co/elastic/clients/elasticsearch/_types/query_dsl/FunctionScoreQuery.java @@ -359,6 +359,8 @@ protected static void setupFunctionScoreQueryDeserializer(ObjectDeserializer Date: Fri, 9 Aug 2024 19:02:16 +0200 Subject: [PATCH 5/7] Fix Jackson look-ahead, refactor shortcut handling to improve readability --- .../clients/json/ObjectDeserializer.java | 69 ++++++++++++------- .../json/jackson/JacksonJsonpParser.java | 47 +++++++------ .../elasticsearch/model/BehaviorsTest.java | 6 +- .../elasticsearch/model/UnionTests.java | 4 ++ 4 files changed, 80 insertions(+), 46 deletions(-) diff --git a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java index e0af54dc9..f1eaba697 100644 --- a/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java +++ b/java-client/src/main/java/co/elastic/clients/json/ObjectDeserializer.java @@ -188,30 +188,9 @@ private void deserializeInner(ObjectType value, JsonParser parser, JsonpMapper m String fieldName = null; try { - if (shortcutProperty != null) { - if (!shortcutIsObject) { - if (event != Event.START_OBJECT && event != Event.KEY_NAME) { - // This is the shortcut property (should be a value or array event, this will be checked by its deserializer) - shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); - return; - } - } else { - // Does the shortcut property exist? If yes, the shortcut is used - Map.Entry shortcut = JsonpUtils.findVariant( - Collections.singletonMap(shortcutProperty.name, Boolean.TRUE /* arbitrary non-null value */), - parser, mapper - ); - - // Parse the buffered events - parser = shortcut.getValue(); - event = parser.next(); - - // If shortcut property was not found, this is a shortcut. Otherwise, keep deserializing as usual - if (shortcut.getKey() == null) { - shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); - return; - } - } + if ((parser = deserializeWithShortcut(value, parser, mapper, event)) == null) { + // We found the shortcut form + return; } if (typeProperty == null) { @@ -258,6 +237,48 @@ private void deserializeInner(ObjectType value, JsonParser parser, JsonpMapper m } } + /** + * Try to deserialize the value with its shortcut property, if any. + * + * @return {@code null} if the shortcut form was found, and otherwise a parser that should be used to parse the + * non-shortcut form. It may be different from the orginal parser if look-ahead was needed. + */ + private JsonParser deserializeWithShortcut(ObjectType value, JsonParser parser, JsonpMapper mapper, Event event) { + if (shortcutProperty != null) { + if (!shortcutIsObject) { + if (event != Event.START_OBJECT && event != Event.KEY_NAME) { + // This is the shortcut property (should be a value or array event, this will be checked by its deserializer) + shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); + return null; + } + } else { + // Fast path: we don't need to look ahead if the current event isn't an object + if (event != Event.START_OBJECT) { + shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); + return null; + } + + // Look ahead: does the shortcut property exist? If yes, the shortcut is used + Map.Entry shortcut = JsonpUtils.findVariant( + Collections.singletonMap(shortcutProperty.name, Boolean.TRUE /* arbitrary non-null value */), + parser, mapper + ); + + // Parse the buffered events + parser = shortcut.getValue(); + event = parser.next(); + + // If shortcut property was not found, this is a shortcut. Otherwise, keep deserializing as usual + if (shortcut.getKey() == null) { + shortcutProperty.deserialize(parser, mapper, shortcutProperty.name, value, event); + return null; + } + } + } + + return parser; + } + protected void parseUnknownField(JsonParser parser, JsonpMapper mapper, String fieldName, ObjectType object) { if (this.unknownFieldHandler != null) { this.unknownFieldHandler.accept(object, fieldName, parser, mapper); diff --git a/java-client/src/main/java/co/elastic/clients/json/jackson/JacksonJsonpParser.java b/java-client/src/main/java/co/elastic/clients/json/jackson/JacksonJsonpParser.java index 22c841a26..4a7676ebb 100644 --- a/java-client/src/main/java/co/elastic/clients/json/jackson/JacksonJsonpParser.java +++ b/java-client/src/main/java/co/elastic/clients/json/jackson/JacksonJsonpParser.java @@ -372,29 +372,34 @@ public Map.Entry findVariant(Map TokenBuffer tb = new TokenBuffer(parser, null); try { - // The resulting parser must contain the full object, including START_EVENT - tb.copyCurrentEvent(parser); - while (parser.nextToken() != JsonToken.END_OBJECT) { - - expectEvent(JsonToken.FIELD_NAME); - String fieldName = parser.getCurrentName(); - - Variant variant = variants.get(fieldName); - if (variant != null) { - tb.copyCurrentEvent(parser); - return new AbstractMap.SimpleImmutableEntry<>( - variant, - new JacksonJsonpParser( - JsonParserSequence.createFlattened(false, tb.asParser(), parser), - mapper - ) - ); - } else { - tb.copyCurrentStructure(parser); + if (parser.currentToken() != JsonToken.START_OBJECT) { + // Primitive value or array + tb.copyCurrentStructure(parser); + } else { + // The resulting parser must contain the full object, including START_EVENT + tb.copyCurrentEvent(parser); + while (parser.nextToken() != JsonToken.END_OBJECT) { + + expectEvent(JsonToken.FIELD_NAME); + String fieldName = parser.getCurrentName(); + + Variant variant = variants.get(fieldName); + if (variant != null) { + tb.copyCurrentEvent(parser); + return new AbstractMap.SimpleImmutableEntry<>( + variant, + new JacksonJsonpParser( + JsonParserSequence.createFlattened(false, tb.asParser(), parser), + mapper + ) + ); + } else { + tb.copyCurrentStructure(parser); + } } + // Copy ending END_OBJECT + tb.copyCurrentEvent(parser); } - // Copy ending END_OBJECT - tb.copyCurrentEvent(parser); } catch (IOException e) { throw JacksonUtils.convertException(e); } diff --git a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java index b44f2ab2d..54128c4b5 100644 --- a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java +++ b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java @@ -44,6 +44,10 @@ public class BehaviorsTest extends ModelTestCase { +// public BehaviorsTest() { +// super(JsonImpl.Jackson); +// } + /** * Test for SingleKeyDictionary transformed to a behavior. For regular fields, see NamedValue tests in {@link ClassStructureTest} */ @@ -286,7 +290,7 @@ public void testFunctionScoreQuery() { fsq = fromJson(full, FunctionScoreQuery.class); assertEquals(MultiValueMode.Avg, fsq.functions().get(0).gauss().untyped().multiValueMode()); - + fsq = fromJson(shortcut, FunctionScoreQuery.class); assertEquals(MultiValueMode.Avg, fsq.functions().get(0).gauss().untyped().multiValueMode()); } diff --git a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java index 8ea89306f..16650def4 100644 --- a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java +++ b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java @@ -41,6 +41,10 @@ public class UnionTests extends ModelTestCase { +// public UnionTests() { +// super(JsonImpl.Simple); +// } + @Test public void testScriptDeserializer() { // A union discriminated by its field names (source -> inline, id -> stored) From 40df147b533f57eeedbc2abbc791893498523e9c Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Thu, 22 Aug 2024 11:12:37 +0200 Subject: [PATCH 6/7] Update java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java Co-authored-by: Laura Trotta <153528055+l-trotta@users.noreply.github.com> --- .../co/elastic/clients/elasticsearch/model/BehaviorsTest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java index 54128c4b5..a67dd7df6 100644 --- a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java +++ b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/BehaviorsTest.java @@ -44,10 +44,6 @@ public class BehaviorsTest extends ModelTestCase { -// public BehaviorsTest() { -// super(JsonImpl.Jackson); -// } - /** * Test for SingleKeyDictionary transformed to a behavior. For regular fields, see NamedValue tests in {@link ClassStructureTest} */ From e6b2d2653726a3a9bb44fa256451173d437c74c9 Mon Sep 17 00:00:00 2001 From: Sylvain Wallez Date: Thu, 22 Aug 2024 11:12:43 +0200 Subject: [PATCH 7/7] Update java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java Co-authored-by: Laura Trotta <153528055+l-trotta@users.noreply.github.com> --- .../co/elastic/clients/elasticsearch/model/UnionTests.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java index 16650def4..8ea89306f 100644 --- a/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java +++ b/java-client/src/test/java/co/elastic/clients/elasticsearch/model/UnionTests.java @@ -41,10 +41,6 @@ public class UnionTests extends ModelTestCase { -// public UnionTests() { -// super(JsonImpl.Simple); -// } - @Test public void testScriptDeserializer() { // A union discriminated by its field names (source -> inline, id -> stored)