Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing partial success results for msearch template #709

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Fixing partial success results for msearch template #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac4e71d
Fixing partial success results for msearch template
VachaShah Nov 5, 2023
63c16cf
Adding changelog
VachaShah Nov 5, 2023
4afc10a
Spotless apply
VachaShah Nov 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ This section is for maintaining a changelog for all breaking changes for the cli
### Removed

### Fixed
- Fix partial success results for msearch_template ([#709](https://github.com/opensearch-project/opensearch-java/pull/709))

### Security

Expand Down
12 changes: 7 additions & 5 deletions java-client/src/main/java/org/opensearch/client/opensearch/_types/ErrorResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ private enum Kind {

private final ErrorCause error;

private final int status;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VachaShah hm I believe status should be always present, we may have an issue on the server side?

Copy link
Collaborator Author

@VachaShah VachaShah Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta Yeah I dug through the server side and the server does not return status for msearch template requests. (see additional context section in #708).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server does not set a status for MultiSearchTemplateResponse (https://github.com/opensearch-project/OpenSearch/blob/main/modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MultiSearchTemplateResponse.java#L169) whereas for MultiSearchResponse, it does https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/search/MultiSearchResponse.java#L195.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this is something that should be fixed on the server side? I am not sure if there is any history behind not setting status for mutli search templates 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for digging it in @VachaShah !

Do you think this is something that should be fixed on the server side?

I believe so, the status field should always be set by the server.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you @reta! I think we should still merge this fix though so that the users can use it with the current versions of OpenSearch, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's right, no objection

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I will raise an issue on OpenSearch to add the status field for multi search templates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta I opened an issue on the server for this: opensearch-project/OpenSearch#11133

private final Integer status;

// ---------------------------------------------------------------------------------------------

private ErrorResponse(Builder builder) {

this.error = ApiTypeHelper.requireNonNull(builder.error, this, "error");
this.status = ApiTypeHelper.requireNonNull(builder.status, this, "status");
this.status = builder.status;

}

Expand All @@ -87,7 +87,7 @@ public final ErrorCause error() {
/**
* Required - API name: {@code status}
*/
public final int status() {
public final Integer status() {
return this.status;
}

Expand All @@ -105,8 +105,10 @@ protected void serializeInternal(JsonGenerator generator, JsonpMapper mapper) {
generator.writeKey("error");
this.error.serialize(generator, mapper);

generator.writeKey("status");
generator.write(this.status);
if (this.status != null) {
generator.writeKey("status");
generator.write(this.status);
}

}

Expand Down
2 changes: 1 addition & 1 deletion ...ent/src/test/java/org/opensearch/client/opensearch/integTest/AbstractClusterClientIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void testClusterUpdateSettingNonExistent() throws IOException {
fail();
} catch (OpenSearchException e) {
assertNotNull(e);
assertEquals(e.response().status(), 400);
assertEquals(e.response().status().intValue(), 400);
assertTrue(e.getMessage().contains("transient setting [no_idea_what_you_are_talking_about], not recognized"));
}
}
Expand Down
2 changes: 1 addition & 1 deletion java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractRequestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ public void testDataIngestion() throws Exception {
assertTrue(msearch.responses().get(0).isResult());
assertEquals(1, msearch.responses().get(0).result().hits().hits().size());
assertTrue(msearch.responses().get(1).isFailure());
assertEquals(404, msearch.responses().get(1).failure().status());
assertEquals(404, msearch.responses().get(1).failure().status().intValue());
}

@Test
Expand Down
38 changes: 28 additions & 10 deletions ...test/java/org/opensearch/client/opensearch/integTest/AbstractSearchTemplateRequestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
package org.opensearch.client.opensearch.integTest;

import java.io.IOException;
import java.util.List;
import java.util.Map;
import org.junit.Test;
import org.opensearch.client.json.JsonData;
import org.opensearch.client.opensearch._types.Refresh;
import org.opensearch.client.opensearch._types.mapping.Property;
import org.opensearch.client.opensearch.core.PutScriptRequest;
import org.opensearch.client.opensearch.core.SearchTemplateResponse;
import org.opensearch.client.opensearch.core.msearch_template.RequestItem;

public abstract class AbstractSearchTemplateRequestIT extends OpenSearchJavaClientTestCase {

Expand Down Expand Up @@ -87,27 +89,43 @@ public void testTemplateSearchAggregations() throws Exception {

@Test
public void testMultiSearchTemplate() throws Exception {
System.out.println("Multi search template test");
var index = "test-msearch-template";
createDocuments(index);

RequestItem requestItem = RequestItem.of(
r -> r.header(h -> h.index(index))
.body(
t -> t.id(TEST_SEARCH_TEMPLATE)
.params("title", JsonData.of("Document"))
.params("suggs", JsonData.of(false))
.params("aggs", JsonData.of(false))
)
);
// adding a request to non existing template to test partial results
RequestItem requestItem2 = RequestItem.of(
r -> r.header(h -> h.index(index))
.body(
t -> t.id("my-other-search-template")
.params("title", JsonData.of("Document"))
.params("suggs", JsonData.of(false))
.params("aggs", JsonData.of(false))
)
);

var searchResponse = javaClient().msearchTemplate(
request -> request.searchTemplates(
r -> r.header(h -> h.index(index))
.body(
t -> t.id(TEST_SEARCH_TEMPLATE)
.params("title", JsonData.of("Document"))
.params("suggs", JsonData.of(false))
.params("aggs", JsonData.of(false))
)
),
request -> request.searchTemplates(List.of(requestItem, requestItem2)),
SimpleDoc.class
);

assertEquals(1, searchResponse.responses().size());
assertEquals(2, searchResponse.responses().size());
var response = searchResponse.responses().get(0);
assertTrue(response.isResult());
assertNull(response.result().status());
assertEquals(4, response.result().hits().hits().size());
var failureResponse = searchResponse.responses().get(1);
assertTrue(failureResponse.isFailure());
assertNull(failureResponse.failure().status());
}

private SearchTemplateResponse<SimpleDoc> sendTemplateRequest(String index, String title, boolean suggs, boolean aggs)
Expand Down
2 changes: 1 addition & 1 deletion ...t/src/test/java/org/opensearch/client/opensearch/json/jackson/JacksonJsonpParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public void testMultiSearchResponse() {
MsearchResponse<Foo> response = fromJson(json, MsearchResponse.class, mapper);

assertEquals(2, response.responses().size());
assertEquals(404, response.responses().get(0).failure().status());
assertEquals(404, response.responses().get(0).failure().status().intValue());
assertEquals((Integer) 200, response.responses().get(1).result().status());
}

Expand Down