[BUG] 0.2.1 branch breaks completion suggesters

[apatrida]

What is the bug?

The required fields for completion suggesters are incorrect. Some fields with default values are marked as required fields. In completion suggester, this includes transpositions and unicodeAware and therefore breaks code that doesn't set these.

Also, the response objects for completion suggestion do not work. Completion suggestion results are coming back as isTerm and therefore blowing up since they do not have score or some other values in the correct places, and all of their other fields are therefore missing.

@imRishN

[apatrida]

The bug is in union deserializer.

It removes all fields as you add members, for example two members have text and so it is removed from both sides, and then a third member comes in with text and it is not removed, therefore a common field ends up in the list associated with one union member that should not.

The field removal can happen in the build() phase while remembering all previously removed items. Or accumulate in the builder. A test case is probably missing with more than 2 items in the union with common fields.

[apatrida]

Fix might be:

remove all of the field removal from addMember changing this line to just add a copy of all fields in this one object:

  UnionDeserializer.SingleMemberHandler<Union, Kind, Member> member = 
        new SingleMemberHandler<>(tag, deserializer, new HashSet<>(od.fieldNames()));

then at top of build() find fields that are duped across any two or more members, and remove them from all members.

Map<String, Long> fieldFrequencies = objectMembers.stream().flatMap(m -> m.fields.stream())
        .collect( Collectors.groupingBy(Function.identity(), Collectors.counting()));
Set<String> duplicateFields = fieldFrequencies.entrySet().stream()
        .filter(entry -> entry.getValue() > 1)
        .map(Map.Entry::getKey)
        .collect(Collectors.toSet());
for (UnionDeserializer.SingleMemberHandler<Union, Kind, Member> member: objectMembers) {
    member.fields.removeAll(duplicateFields);
}

And you end up with correct unique fields.

[apatrida]

Verified this code change solves the problem for the use case that started this bug.

[dblock]

@apatrida Care to PR a fix?

[iamdanhart]

Hi @dblock, I'm actively running into an issue trying to do a completion suggestion search due to it being parsed as a term suggestion. I've never done a PR before and I'm not trying to claim @apatrida 's work as my own. What is the right thing to do here?

[dblock]

@iamdanhart I think the best place to start is trying to write a failing unit test in this project for this problem. I would check out the code and build it first (https://github.com/opensearch-project/opensearch-java/blob/main/DEVELOPER_GUIDE.md has lots of helpful things), then try to implement a failing test?

[iamdanhart]

@dblock Here's the failing test and stacktrace. I added this to AbstractRequestIT and ran RequestIT. Is posting it here sufficient or should I make a PR?

@Test
public void testCompletionSuggester() throws IOException {

        String index = "test-completion-suggester";

        Property intValueProp = new Property.Builder()
                .long_(v -> v)
                .build();
        Property msgCompletionProp = new Property.Builder()
                .completion(c -> c)
                .build();
        javaClient().indices().create(c -> c
                .index(index)
                .mappings(m -> m
                        .properties("intValue", intValueProp)
                        .properties("msg", msgCompletionProp)));

        AppData appData = new AppData();
        appData.setIntValue(1337);
        appData.setMsg("foo");

        javaClient().index(b -> b
                .index(index)
                .id("1")
                .document(appData)
                .refresh(Refresh.True));

        appData.setIntValue(1338);
        appData.setMsg("bar");

        javaClient().index(b -> b
                .index(index)
                .id("2")
                .document(appData)
                .refresh(Refresh.True));

        CompletionSuggester completionSuggester = FieldSuggesterBuilders.completion()
                .field("msg")
                .size(1)
                .build();
        FieldSuggester fieldSuggester = new FieldSuggester.Builder()
                .completion(completionSuggester)
                .build();
        Suggester suggester = new Suggester.Builder()
                .suggesters(Collections.singletonMap("msgSuggester", fieldSuggester))
                .text("foo")
                .build();
        SearchRequest searchRequest = new SearchRequest.Builder()
                .index(index)
                .suggest(suggester)
                .build();

        SearchResponse<AppData> response = javaClient().search(searchRequest, AppData.class);
        assertTrue(response.suggest().size() > 0);
    }

Missing required property 'TermSuggestOption.score'
org.opensearch.client.util.MissingRequiredPropertyException: Missing required property 'TermSuggestOption.score'
at __randomizedtesting.SeedInfo.seed([527808F4969F6080:5E0DD2DBA7CD6FF]:0)
at app//org.opensearch.client.util.ApiTypeHelper.requireNonNull(ApiTypeHelper.java:88)
at app//org.opensearch.client.opensearch.core.search.TermSuggestOption.(TermSuggestOption.java:69)
at app//org.opensearch.client.opensearch.core.search.TermSuggestOption$Builder.build(TermSuggestOption.java:170)
at app//org.opensearch.client.opensearch.core.search.TermSuggestOption$Builder.build(TermSuggestOption.java:129)
at app//org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:99)
at app//org.opensearch.client.json.DelegatingDeserializer$SameType.deserialize(DelegatingDeserializer.java:61)
at app//org.opensearch.client.json.UnionDeserializer$SingleMemberHandler.deserialize(UnionDeserializer.java:88)
at app//org.opensearch.client.json.UnionDeserializer.deserialize(UnionDeserializer.java:305)
at app//org.opensearch.client.json.JsonpDeserializerBase$ArrayDeserializer.deserialize(JsonpDeserializerBase.java:353)
at app//org.opensearch.client.json.JsonpDeserializerBase$ArrayDeserializer.deserialize(JsonpDeserializerBase.java:322)
at app//org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:88)
at app//org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:85)
at app//org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:189)
at app//org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:150)
at app//org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:98)
at app//org.opensearch.client.json.JsonpDeserializerBase$ArrayDeserializer.deserialize(JsonpDeserializerBase.java:353)
at app//org.opensearch.client.json.JsonpDeserializerBase$ArrayDeserializer.deserialize(JsonpDeserializerBase.java:322)
at app//org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:88)
at app//org.opensearch.client.json.JsonpDeserializerBase$StringMapDeserializer.deserialize(JsonpDeserializerBase.java:378)
at app//org.opensearch.client.json.JsonpDeserializerBase$StringMapDeserializer.deserialize(JsonpDeserializerBase.java:364)
at app//org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:88)
at app//org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:85)
at app//org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:189)
at app//org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:150)
at app//org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:88)
at app//org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:92)
at app//org.opensearch.client.json.DelegatingDeserializer$SameType.deserialize(DelegatingDeserializer.java:56)
at app//org.opensearch.client.transport.endpoints.EndpointWithResponseMapperAttr$1.deserialize(EndpointWithResponseMapperAttr.java:69)
at app//org.opensearch.client.transport.rest_client.RestClientTransport.decodeResponse(RestClientTransport.java:321)
at app//org.opensearch.client.transport.rest_client.RestClientTransport.getHighLevelResponse(RestClientTransport.java:287)
at app//org.opensearch.client.transport.rest_client.RestClientTransport.performRequest(RestClientTransport.java:144)
at app//org.opensearch.client.opensearch.OpenSearchClient.search(OpenSearchClient.java:1219)
at app//org.opensearch.client.opensearch.integTest.AbstractRequestIT.testCompletionSuggester(AbstractRequestIT.java:495)
at [email protected]/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at [email protected]/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at [email protected]/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at [email protected]/java.lang.reflect.Method.invoke(Method.java:566)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)
at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at app//org.junit.rules.RunRules.evaluate(RunRules.java:20)
at app//org.apache.lucene.tests.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:48)
at app//org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
at app//org.apache.lucene.tests.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
at app//org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
at app//org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
at app//org.junit.rules.RunRules.evaluate(RunRules.java:20)
at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at app//com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
at app//com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817)
at app//com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:947)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:832)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:883)
at app//com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:894)
at app//org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at app//org.apache.lucene.tests.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:38)
at app//com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
at app//com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at app//org.apache.lucene.tests.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
at app//org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
at app//org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
at app//org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
at app//org.apache.lucene.tests.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:47)
at app//org.junit.rules.RunRules.evaluate(RunRules.java:20)
at app//com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at app//com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
at [email protected]/java.lang.Thread.run(Thread.java:829)

[dblock]

Make a PR! If you have time try to fix it. We’ll help you.

[iamdanhart]

@dblock Thank you for your guidance in this process! So that I can set expectations for my stakeholders when they have questions about my integrating with OpenSearch, is there a rough estimate for when the next release would get cut?

[wbeckler]

The java client has recently implemented a faster release process, and a release went out this week. If you have an urgent need for your fix to get into maven, make a request in a separate issue. I don't see why it would take more than a week or two to get a release out after your PR gets merged, and it could be under a week if it's been a while and there are other merged PRs waiting to get out.

[iamdanhart]

Thanks for that explanation. That sounds like a reasonable time frame.

[VachaShah]

This will be released in v2.3.0 (#380).