Replace RestHighLevelClient with OpenSearch Java Client

[reta]

Description

In scope of opensearch-project/OpenSearch#22578, the suggestion is to deprecate the RestHighLevelClient in favor of a single official Java client (OpenSearch Java Client). To make the case that we have no gaps, replacing RestHighLevelClient usage with OpenSearchClient.

Issues Resolved

Part of opensearch-project/OpenSearch#22578

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

Covered by existing tests

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 04e0362.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
build.gradle	823	high	New dependency added: 'org.opensearch.client:opensearch-java:3.9.0'. Per mandatory supply chain policy, all new dependency additions must be flagged regardless of apparent legitimacy. Maintainers should verify the artifact hash and provenance before merging.
build.gradle	533	high	Forced dependency version added: 'jakarta.json:jakarta.json-api:2.1.3'. Per mandatory supply chain policy, version pinning/forcing changes to any dependency must be flagged. Maintainers should verify this version constraint is intentional and the artifact is authentic.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 2 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5d7fffe)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Migrate integration tests to OpenSearch Java Client Relevant files: src/integrationTest/java/org/opensearch/security/SearchOperationTest.java src/integrationTest/java/org/opensearch/security/DlsIntegrationTests.java src/integrationTest/java/org/opensearch/security/PointInTimeOperationTest.java Sub-PR theme: Add new matcher classes for OpenSearch Java Client responses Relevant files: src/integrationTest/java/org/opensearch/test/framework/matcher/client/SuccessfulDeletePitResponseMatcher.java

⚡ Recommended focus areas for review Possible Issue In shouldPerformMultiGetDocuments_positive, the audit log assertion was changed from checking the REST request /_mget to /song_lyrics/_mget, but the request is now built with MgetRequest.of(r -> r.index(SONG_INDEX_NAME).ids(ID_S1, ID_S2)) which does target that index. However, the same change was made in shouldPerformMultiGetDocuments_negative (also to /song_lyrics/_mget) — verify this matches the actual REST path the Java client emits, as a mismatch will cause test failures at runtime. auditLogsRule.assertExactlyOne(userAuthenticated(LIMITED_READ_USER).withRestRequest(POST, "/song_lyrics/_mget")); auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetRequest")); auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetShardRequest")); } @Test public void shouldPerformMultiGetDocuments_negative() throws IOException { try (CloseableOpenSearchClient client = cluster.getClient(DOUBLE_READER_USER)) { MgetRequest request = MgetRequest.of(r -> r.index(SONG_INDEX_NAME).ids(ID_S1)); assertThatThrownBy(() -> client.mget(request, Map.class), statusException(FORBIDDEN)); } auditLogsRule.assertExactlyOne(userAuthenticated(DOUBLE_READER_USER).withRestRequest(POST, "/song_lyrics/_mget")); auditLogsRule.assertExactlyOne(missingPrivilege(DOUBLE_READER_USER, "MultiGetRequest")); Dead Code client.snapshot(); is called without using the result inside shouldDeleteSnapshot_positive. This appears to be a leftover from the migration (previously restHighLevelClient.snapshot() returned a client namespace) and should either be removed or its usage restored, otherwise it does nothing. client.snapshot(); Commented-out Assertion In shouldPerformMultiGetDocuments_partiallyPositive, the assertion assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); was commented out rather than replaced with an equivalent check for the new client API. This weakens the test — only the second failure is now verified while the first item's successful state is no longer asserted. var responses = response.docs(); // assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); assertThat(responses.get(1).failure().error().type(), equalTo("security_exception"));

---

⚠️ Review coverage: The following files were not included in this review because of the token budget:

• src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java
• src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java
• src/integrationTest/java/org/opensearch/security/CrossClusterSearchTests.java
• src/integrationTest/java/org/opensearch/security/http/LdapTlsAuthenticationTest.java
• src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java
• src/integrationTest/java/org/opensearch/security/SecurityIndexSnapshotRestoreTests.java
• src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java
• src/integrationTest/java/org/opensearch/security/SnapshotSteps.java
• src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java
• src/test/java/org/opensearch/security/InitializationIntegrationTests.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SearchResponseMatchers.java
• src/integrationTest/java/org/opensearch/test/framework/client/SearchRequestFactory.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/MultiSearchResponseItemContainsFieldWithValueMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SearchHitContainsFieldWithValueMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SearchHitsContainDocumentsInAnyOrderMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/BulkResponseContainExceptionsAtIndexMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/TransportExceptionMatcher.java
• src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/IndexResponseMatchers.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/BulkResponseContainExceptionsMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/MultiSearchResponseItemContainDocumentWithIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SearchHitsContainDocumentWithIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SearchHitDoesNotContainFieldMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SearchHitDoesContainFieldMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/ContainsAggregationWithNameAndTypeMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/MultiGetResponseItemContainsDocumentWithIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/MultiGetResponseItemDocumentFieldValueMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/NumberOfTotalHitsIsEqualToMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetResultDocumentFieldValueMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetResponseDocumentFieldValueMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/MultiGetResponseItemContainOnlyDocumentIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetResultContainsDocumentWithIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetResponseContainsDocumentWithIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/ContainsFieldWithTypeMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetResultContainOnlyDocumentIdMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetAllPitsContainsExactlyIdsResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/DeletePitsContainsExactlyIdsResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/DeletePitContainsExactlyIdsResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/PitResponseMatchers.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SuccessfulCreateIndexResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SuccessfulResizeResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SuccessfulCloneResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/SuccessfulSplitResponseMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetSettingsResponseContainsIndicesMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/NumberOfHitsInPageIsEqualToMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetMappingsResponseContainsIndicesMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/MultiGetResponseItemDocumentDoesNotContainFieldMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetIndexResponseContainsIndicesMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/GetResultDocumentDoesNotContainFieldMatcher.java
• src/integrationTest/java/org/opensearch/test/framework/matcher/client/ContainsExactlyIndicesMatcher.java
  ... and 29 more

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5d7fffe

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Restore removed partial-failure assertions The assertion checking that the first item is a success and the second is a failure was replaced with a commented-out line, weakening the test. Add an explicit assertion verifying responses.get(0).isFailure() is false and responses.get(1).isFailure() is true before inspecting the failure details, so the partial-success semantics are still enforced. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [866-878] MgetRequest request = MgetRequest.of( r -> r.docs(d -> d.index(SONG_INDEX_NAME).id(ID_S1)).docs(d -> d.index(PROHIBITED_SONG_INDEX_NAME).id(ID_P4)) ); MgetResponse<?> response = client.mget(request, Map.class); assertThat(request, notNullValue()); assertThat(response, not(isSuccessfulMultiGetResponse())); assertThat(response, numberOfGetItemResponsesIsEqualTo(2)); var responses = response.docs(); - // assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); + assertThat(responses.get(0).isFailure(), equalTo(false)); + assertThat(responses.get(1).isFailure(), equalTo(true)); assertThat(responses.get(1).failure().error().type(), equalTo("security_exception")); Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that meaningful assertions were commented out and proposes restoring explicit isFailure() checks, which strengthens the test's verification of partial-success semantics.	Low
	Use POST when sending body to field_caps The workaround sends the field-caps request as GET with a JSON body attached (via Bodies.json(...)). Many HTTP stacks and OpenSearch's _field_caps handler accept a body only on POST; sending a body on GET may be dropped and cause the index_filter to be silently ignored. Use POST (which _field_caps also supports) when attaching a body. src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [153-156] JsonBodyBuilder builder = Requests.builder() .endpoint(indexPrefix + "/_field_caps") - .method("GET") + .method("POST") .query(Map.of("fields", request.fields().stream().collect(Collectors.joining(",")))); Suggestion importance[1-10]: 6 __ Why: Sending a body with a GET request can be problematic in some HTTP stacks and may cause index_filter to be silently ignored. Using POST is a safer choice for field_caps requests with a body, though the impact depends on the runtime behavior.	Low
	Preserve original mget request endpoint The audit log assertions expect requests at /song_lyrics/_mget because the new Java client sends index-scoped mget when .index() is set on the request. This changes the observed URL path from /_mget used previously; verify this behavior is intentional and that the underlying authorization checks still exercise the same code path as before. If not, use the docs-level form (docs(d -> d.index(...).id(...))) without a top-level index to preserve the original /_mget endpoint. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [854-859] - auditLogsRule.assertExactlyOne(userAuthenticated(LIMITED_READ_USER).withRestRequest(POST, "/song_lyrics/_mget")); - auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetRequest")); - auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetShardRequest")); -} - -@Test -public void shouldPerformMultiGetDocuments_negative() throws IOException { try (CloseableOpenSearchClient client = cluster.getClient(DOUBLE_READER_USER)) { - MgetRequest request = MgetRequest.of(r -> r.index(SONG_INDEX_NAME).ids(ID_S1)); + MgetRequest request = MgetRequest.of(r -> r.docs(d -> d.index(SONG_INDEX_NAME).id(ID_S1))); assertThatThrownBy(() -> client.mget(request, Map.class), statusException(FORBIDDEN)); } - auditLogsRule.assertExactlyOne(userAuthenticated(DOUBLE_READER_USER).withRestRequest(POST, "/song_lyrics/_mget")); + auditLogsRule.assertExactlyOne(userAuthenticated(DOUBLE_READER_USER).withRestRequest(POST, "/_mget")); Suggestion importance[1-10]: 5 __ Why: The observation about the endpoint path change from /_mget to /song_lyrics/_mget is valid and the audit log assertion was indeed updated, but since the PR author explicitly updated the assertion to match the new path, this may be intentional.	Low
	Avoid asserting isResult for sourceless doc secondResult corresponds to ID_4, which the setup writes without a document body (via writeDocumentWithoutSource). It is expected to exist but with no source, so containOnlyDocumentId is asserted below. However, isResult() being true asserts a successful result — verify that failing to retrieve source still returns isResult()==true in the new client; if not, the assertion regresses the previous check that only tolerated getFailure()==null. src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java [286-289] MgetRequest request = MgetRequest.of( r -> r.docs(d -> d.index(MARVELOUS_SONGS).id(ID_1)).docs(d -> d.index(MARVELOUS_SONGS).id(ID_4)) ); MgetResponse<?> response = client.mget(request, Map.class); var responses = response.docs(); assertThat(responses, iterableWithSize(2)); MultiGetResponseItem<?> firstResult = responses.get(0); MultiGetResponseItem<?> secondResult = responses.get(1); assertThat(firstResult.isResult(), is(true)); -assertThat(secondResult.isResult(), is(true)); +// ID_4 has no source; only ensure it is not a failure item +assertThat(secondResult.isFailure(), is(false)); Suggestion importance[1-10]: 4 __ Why: The concern is speculative; isResult() in the new client typically indicates a non-failure item regardless of source presence. Without concrete evidence of a regression, this is a minor correctness concern.	Low
	Make snapshot state comparison case-insensitive Comparing the state literal "SUCCESS" as a String is brittle: the java client may return the state in a different case (e.g. "success") or expose it as an enum-like value; the previous code used SnapshotState.SUCCESS.equals(...). If the returned value differs in casing/format, the awaitility loop will spin until timeout even after the snapshot completes. Use case-insensitive comparison or the client's constant. src/integrationTest/java/org/opensearch/security/SnapshotSteps.java [68-73] .until(() -> { count.incrementAndGet(); GetSnapshotResponse snapshotsResponse = snapshotClient.get(getSnapshotsRequest); SnapshotInfo snapshotInfo = snapshotsResponse.snapshots().get(0); - return "SUCCESS".equals(snapshotInfo.state()); + return snapshotInfo.state() != null && "SUCCESS".equalsIgnoreCase(snapshotInfo.state()); }); Suggestion importance[1-10]: 4 __ Why: Case-insensitive comparison adds robustness, but OpenSearch typically returns snapshot state as uppercase "SUCCESS", so the impact is minor and speculative.	Low
	Ensure index template pattern matches indexPatterns(TEMPLATE_INDEX_PREFIX) passes a single-string pattern where the prior code passed List.of(TEMPLATE_INDEX_PREFIX). If TEMPLATE_INDEX_PREFIX is not already a wildcard (e.g. "template_"), the template will not match INDEX_NAME_SONG_TRANSCRIPTION_JAZZ and subsequent assertions on alias-based document lookup will fail. Confirm the pattern still resolves to matching indices, otherwise append a suffix or explicitly pass a wildcard list. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [1533-1537] PutTemplateRequest request = PutTemplateRequest.of( r -> r.name(MUSICAL_INDEX_TEMPLATE) - .indexPatterns(TEMPLATE_INDEX_PREFIX) + .indexPatterns(TEMPLATE_INDEX_PREFIX + "*") .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0001, Alias.builder().build()) .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0002, Alias.builder().build()) ); Suggestion importance[1-10]: 3 __ Why: The concern about indexPatterns is speculative since TEMPLATE_INDEX_PREFIX likely already contains a wildcard as suggested by the tests passing behavior; the suggested change might break things if the constant already includes a wildcard.	Low
Possible issue	Fix varargs index() call on SearchRequest builder The varargs SearchRequest.Builder.index(String, String...) overload does not exist in the OpenSearch Java client; index() accepts either a single String or a List. Callers like searchAll(REMOTE_SONG_INDEX, SONG_INDEX_NAME) will not compile or will only pass one index. Combine the arguments into a single list before calling index(). src/integrationTest/java/org/opensearch/security/CrossClusterSearchTests.java [207-213] private SearchRequest searchAll(String indexName, String... indexNames) { + java.util.List<String> indices = new java.util.ArrayList<>(); + indices.add(indexName); + indices.addAll(java.util.Arrays.asList(indexNames)); return SearchRequest.of( - b -> b.index(indexName, indexNames) + b -> b.index(indices) .ccsMinimizeRoundtrips(ccsMinimizeRoundtrips) .query(QueryBuilders.matchAll().build().toQuery()) ); } Suggestion importance[1-10]: 3 __ Why: The suggestion assumes the varargs index(String, String...) overload doesn't exist, but the OpenSearch Java client builder does provide a varargs index() method. The existing code likely compiles correctly, making this suggestion possibly incorrect.	Low

---

Previous suggestions

Suggestions up to commit f06c6e5

Category	Suggestion	Impact
Possible issue	Support multiple indices in field caps workaround The workaround only handles a single index by placing it in the URL path; when multiple indices are provided the endpoint becomes /_field_caps and the indices list is silently dropped, producing wrong results for multi-index field caps requests. Join all indices into a comma-separated path segment (or pass them via query/body) so multi-index requests are honored. src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [148-156] String indexPrefix = ""; -if (request.index().size() == 1) { - indexPrefix = "/" + request.index().get(0); +if (request.index() != null && !request.index().isEmpty()) { + indexPrefix = "/" + String.join(",", request.index()); } JsonBodyBuilder builder = Requests.builder() .endpoint(indexPrefix + "/_field_caps") .method("GET") .query(Map.of("fields", request.fields().stream().collect(Collectors.joining(",")))); Suggestion importance[1-10]: 8 __ Why: Valid catch: the workaround silently drops multi-index requests by only using the first index when request.index().size() == 1. Joining indices into a comma-separated path segment properly supports multi-index field caps requests, preventing incorrect test behavior.	Medium
General	Restore removed assertion on first response item The test previously asserted that the first item succeeded (failure=null) and only the second failed. That assertion was removed and replaced with a comment, which weakens the test — a regression where both items fail (or the first fails) would no longer be caught. Add an explicit assertion that responses.get(0) is not a failure to preserve the original test intent. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [876-878] MgetResponse<?> response = client.mget(request, Map.class); assertThat(request, notNullValue()); assertThat(response, not(isSuccessfulMultiGetResponse())); assertThat(response, numberOfGetItemResponsesIsEqualTo(2)); var responses = response.docs(); - // assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); + assertThat(responses.get(0).isFailure(), equalTo(false)); + assertThat(responses.get(1).isFailure(), equalTo(true)); assertThat(responses.get(1).failure().error().type(), equalTo("security_exception")); } auditLogsRule.assertExactlyOne(userAuthenticated(LIMITED_READ_USER).withRestRequest(POST, "/_mget")); Suggestion importance[1-10]: 6 __ Why: Restoring the assertion that responses.get(0) is not a failure preserves the original test intent and prevents silent regressions where the first item might also fail. This is a valid improvement to test coverage.	Low
	Assert on correct alias record field AliasesRecord::index returns the underlying index name, but the original test used the alias listing rows (which included alias-to-index pairs like both-indices: marvelous_songs). Using index() here can produce duplicate entries (multiple aliases can point to the same index) and no longer verifies that only alias rows for accessible indices are returned; consider asserting on AliasesRecord::alias (or both alias and index) to preserve the original semantic check. src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java [488-495] try (CloseableOpenSearchClient client = cluster.getClient(LIMITED_USER)) { AliasesResponse getAliasesResponse = client.cat().aliases(); - List<String> aliases = getAliasesResponse.valueBody().stream().map(AliasesRecord::index).sorted().toList(); + List<AliasesRecord> records = getAliasesResponse.valueBody(); + List<String> indices = records.stream().map(AliasesRecord::index).sorted().toList(); - // Does not fail on forbidden, but alias response only contains index which user has access to - assertThat(aliases.size(), equalTo(1)); - assertThat(aliases, hasItem(containsString("marvelous_songs"))); - assertThat(aliases, not(hasItem(containsString("horrible_songs")))); - + assertThat(records.size(), equalTo(1)); + assertThat(indices, hasItem(containsString("marvelous_songs"))); + assertThat(indices, not(hasItem(containsString("horrible_songs")))); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a reasonable concern about semantic differences between the old raw response and AliasesRecord::index, but the existing assertions still validate that the response only contains accessible index entries, so the impact is moderate.	Low
	Verify audit-path assertions match client URL The audit-log assertion path was changed from /_mget to /song_lyrics/_mget because the new client places the index in the URL. However, the shouldPerformMultiGetDocuments_partiallyPositive test below still uses POST /_mget because that request omits a top-level index and uses per-doc indices. Verify these audit path expectations match the actual client behavior on the wire — an incorrect audit-path assertion will silently fail the wrong condition. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [859] auditLogsRule.assertExactlyOne(userAuthenticated(LIMITED_READ_USER).withRestRequest(POST, "/song_lyrics/_mget")); auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetRequest")); auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetShardRequest")); -} -@Test -public void shouldPerformMultiGetDocuments_negative() throws IOException { - try (CloseableOpenSearchClient client = cluster.getClient(DOUBLE_READER_USER)) { - MgetRequest request = MgetRequest.of(r -> r.index(SONG_INDEX_NAME).ids(ID_S1)); - - assertThatThrownBy(() -> client.mget(request, Map.class), statusException(FORBIDDEN)); - } - auditLogsRule.assertExactlyOne(userAuthenticated(DOUBLE_READER_USER).withRestRequest(POST, "/song_lyrics/_mget")); - Suggestion importance[1-10]: 3 __ Why: This is a verification suggestion asking to check that audit paths match actual behavior, without concrete code changes. Verification-only suggestions are of limited impact.	Low
	Verify index pattern list conversion is correct TEMPLATE_INDEX_PREFIX is being passed as a single string to indexPatterns(...), but the original code used List.of(TEMPLATE_INDEX_PREFIX) — check that the new client's indexPatterns varargs overload accepts a single pattern as intended (not the entire string treated as a comma-separated list). If the constant already contains a wildcard like template-*, this is fine; otherwise the template may not match INDEX_NAME_SONG_TRANSCRIPTION_JAZZ and downstream assertions like clusterContainsDocument(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0001, documentId) will fail. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [1532-1537] public void shouldCreateIndexTemplate_positive() throws IOException { try (CloseableOpenSearchClient client = cluster.getClient(LIMITED_WRITE_USER)) { PutTemplateRequest request = PutTemplateRequest.of( r -> r.name(MUSICAL_INDEX_TEMPLATE) - .indexPatterns(TEMPLATE_INDEX_PREFIX) + .indexPatterns(List.of(TEMPLATE_INDEX_PREFIX)) .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0001, Alias.builder().build()) .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0002, Alias.builder().build()) ); Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify the varargs behavior of indexPatterns and the improved_code uses List.of(TEMPLATE_INDEX_PREFIX) which may not compile if the API expects varargs of String. The concern is speculative without confirmation.	Low
	Make PIT cleanup idempotent deleteAllPits will throw if there are no PITs to delete (some server versions return an error for empty deletion), which can cause every test to fail during setup. Guard the cleanup by first listing PITs, or catch the "no PITs" exception, to make the @Before idempotent. src/integrationTest/java/org/opensearch/security/PointInTimeOperationTest.java [133-136] @Before public void cleanUpPits() throws IOException { try (CloseableOpenSearchClient client = cluster.getClient(ADMIN_USER)) { - client.deleteAllPits(); + try { + if (!client.getAllPits().pits().isEmpty()) { + client.deleteAllPits(); + } + } catch (Exception ignore) { + // no PITs to delete + } } } Suggestion importance[1-10]: 3 __ Why: The claim that deleteAllPits throws when no PITs exist is speculative and not clearly supported. The original code used the same pattern with the old client without issue, so this suggestion may be unnecessary defensive code.	Low

Suggestions up to commit bb2a1fa

Category	Suggestion	Impact
General	Restore missing partial-failure assertion The assertion that verified the first item succeeded (no failure) and the second item failed was replaced by a commented-out line, weakening the test. Restore an explicit check that responses.get(0) is not a failure while responses.get(1) is, so the "partiallyPositive" contract is still validated. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [866-878] MgetRequest request = MgetRequest.of( r -> r.docs(d -> d.index(SONG_INDEX_NAME).id(ID_S1)).docs(d -> d.index(PROHIBITED_SONG_INDEX_NAME).id(ID_P4)) ); MgetResponse<?> response = client.mget(request, Map.class); assertThat(request, notNullValue()); assertThat(response, not(isSuccessfulMultiGetResponse())); assertThat(response, numberOfGetItemResponsesIsEqualTo(2)); var responses = response.docs(); - // assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); + assertThat(responses.get(0).isFailure(), equalTo(false)); + assertThat(responses.get(1).isFailure(), equalTo(true)); assertThat(responses.get(1).failure().error().type(), equalTo("security_exception")); Suggestion importance[1-10]: 7 __ Why: Valid observation - the commented-out assertion weakens the test coverage for the partially-positive case. Adding explicit isFailure checks improves test rigor.	Medium
	Align mget audit-log URL assertions The new client sends /song_lyrics/_mget (index-scoped mget) instead of /_mget as previously. The partiallyPositive test below still asserts POST /_mget, which is inconsistent with the other two mget tests that now assert /song_lyrics/_mget. Confirm which URL is actually produced and align the audit-log assertion accordingly, otherwise one of these tests will fail. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [847-859] auditLogsRule.assertExactlyOne(userAuthenticated(LIMITED_READ_USER).withRestRequest(POST, "/song_lyrics/_mget")); - auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetRequest")); - auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetShardRequest")); - } - @Test - public void shouldPerformMultiGetDocuments_negative() throws IOException { - try (CloseableOpenSearchClient client = cluster.getClient(DOUBLE_READER_USER)) { - MgetRequest request = MgetRequest.of(r -> r.index(SONG_INDEX_NAME).ids(ID_S1)); - - assertThatThrownBy(() -> client.mget(request, Map.class), statusException(FORBIDDEN)); - } - auditLogsRule.assertExactlyOne(userAuthenticated(DOUBLE_READER_USER).withRestRequest(POST, "/song_lyrics/_mget")); - Suggestion importance[1-10]: 6 __ Why: The suggestion points out a potential inconsistency in audit log URL assertions between the different mget tests, which could cause test failures if URL patterns don't match the actual requests made.	Low
	Unwrap nested exception causes iteratively The unwrapping of IOException and TransportException only steps one level, but exceptions can be nested more deeply (e.g., IOException wrapping TransportException wrapping OpenSearchException). Use a loop to unwrap causes until a terminal exception type is reached, otherwise valid matches may be missed. src/integrationTest/java/org/opensearch/test/framework/matcher/client/TransportExceptionMatcher.java [36-44] +while ((cause instanceof IOException || cause instanceof TransportException) && cause.getCause() != null && cause.getCause() != cause) { + cause = cause.getCause(); +} + if (cause instanceof OpenSearchException ose) { if (expectedRestStatus.getStatus() != ose.status()) { Suggestion importance[1-10]: 6 __ Why: Correctly identifies that deeply nested wrapping could cause missed matches; iterative unwrapping is more robust than the current fixed two-step approach.	Low
	Guard against empty fields query parameter When request.fields() is null or empty, Map.of("fields", "") will still be sent as a query parameter, which may produce an invalid request. Also, field names should be URL-encoded to safely handle special characters. Guard against empty fields and encode the value. src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [153-156] JsonBodyBuilder builder = Requests.builder() .endpoint(indexPrefix + "/_field_caps") - .method("GET") - .query(Map.of("fields", request.fields().stream().collect(Collectors.joining(",")))); + .method("GET"); +if (request.fields() != null && !request.fields().isEmpty()) { + builder = builder.query(Map.of("fields", String.join(",", request.fields()))); +} Suggestion importance[1-10]: 5 __ Why: Reasonable defensive improvement for the field_caps workaround, though in the current test usage fields is always provided, so impact is limited.	Low
	Ensure template index pattern is a wildcard indexPatterns(TEMPLATE_INDEX_PREFIX) passes a single string; the original test used List.of(TEMPLATE_INDEX_PREFIX) which is a pattern list. If TEMPLATE_INDEX_PREFIX is not a full pattern (e.g. missing ), the template will no longer match INDEX_NAME_SONG_TRANSCRIPTION_JAZZ and subsequent assertions on alias-based document lookup will fail. Verify the value already contains the wildcard, or explicitly append when calling indexPatterns. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [1532-1537] PutTemplateRequest request = PutTemplateRequest.of( r -> r.name(MUSICAL_INDEX_TEMPLATE) - .indexPatterns(TEMPLATE_INDEX_PREFIX) + .indexPatterns(TEMPLATE_INDEX_PREFIX + "*") .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0001, Alias.builder().build()) .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0002, Alias.builder().build()) ); Suggestion importance[1-10]: 4 __ Why: The concern about wildcard is speculative without knowing the value of TEMPLATE_INDEX_PREFIX. The original code used List.of(TEMPLATE_INDEX_PREFIX) similarly, so the prefix likely already contains the wildcard.	Low
	Verify aliases assertion targets correct field Previously the assertion mapped AliasesRecord::alias semantics implicitly via raw text lines; now AliasesRecord::index is used, which returns the underlying index name rather than the alias. Verify that the intended check is on the index (not the alias name) — if the test was intended to check alias names, this changes semantics silently. src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java [488-495] try (CloseableOpenSearchClient client = cluster.getClient(LIMITED_USER)) { AliasesResponse getAliasesResponse = client.cat().aliases(); - List<String> aliases = getAliasesResponse.valueBody().stream().map(AliasesRecord::index).sorted().toList(); + List<String> aliasIndexes = getAliasesResponse.valueBody().stream().map(AliasesRecord::index).sorted().toList(); // Does not fail on forbidden, but alias response only contains index which user has access to - assertThat(aliases.size(), equalTo(1)); - assertThat(aliases, hasItem(containsString("marvelous_songs"))); - assertThat(aliases, not(hasItem(containsString("horrible_songs")))); + assertThat(aliasIndexes.size(), equalTo(1)); + assertThat(aliasIndexes, hasItem(containsString("marvelous_songs"))); + assertThat(aliasIndexes, not(hasItem(containsString("horrible_songs")))); Suggestion importance[1-10]: 4 __ Why: Raises a valid semantic concern about whether the check should be on alias vs index name, but the improved_code only renames a variable without changing behavior, so it mainly asks for verification.	Low

Suggestions up to commit 27530a4

Category	Suggestion	Impact
Possible issue	Fix null assertion on primitive double value The original assertion checked that the average was Double.POSITIVE_INFINITY when the user had no access to the STARS field, but this was changed to expect null. AvgAggregate.value() returns a primitive double in the OpenSearch Java client, which cannot be null — this assertion will fail at runtime or fail to compile depending on autoboxing. Use the appropriate nullable method (e.g., valueAsString() returning null) or assert on Double.POSITIVE_INFINITY/Double.NaN as returned by the aggregation. src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java [491-493] Aggregate actualAggregation = searchResponse.aggregations().get(aggregationName); assertThat(actualAggregation._get(), instanceOf(AvgAggregate.class)); - assertThat(actualAggregation.avg().value(), is(nullValue())); // user cannot see the STARS field + assertThat(Double.isInfinite(actualAggregation.avg().value()) || Double.isNaN(actualAggregation.avg().value()), is(true)); Suggestion importance[1-10]: 7 __ Why: If AvgAggregate.value() returns a primitive double, asserting it is(nullValue()) would fail via autoboxing to a non-null Double. This is a legitimate correctness concern for the test assertion.	Medium
	Guard failure access with isFailure checks The assertion responses.get(1).failure().error().type() assumes ordering of MGet responses matches request order. Additionally, the removed assertion checking that responses[0] has no failure and responses[1] has a failure was replaced with only a commented-out line, weakening the test. Restore an explicit check that responses.get(0).isFailure() is false and responses.get(1).isFailure() is true before accessing failure(), to avoid a NullPointerException if the ordering assumption is wrong. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [876-878] MgetRequest request = MgetRequest.of( r -> r.docs(d -> d.index(SONG_INDEX_NAME).id(ID_S1)).docs(d -> d.index(PROHIBITED_SONG_INDEX_NAME).id(ID_P4)) ); MgetResponse<?> response = client.mget(request, Map.class); assertThat(request, notNullValue()); assertThat(response, not(isSuccessfulMultiGetResponse())); assertThat(response, numberOfGetItemResponsesIsEqualTo(2)); var responses = response.docs(); - // assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); + assertThat(responses.get(0).isFailure(), equalTo(false)); + assertThat(responses.get(1).isFailure(), equalTo(true)); assertThat(responses.get(1).failure().error().type(), equalTo("security_exception")); Suggestion importance[1-10]: 6 __ Why: Adding explicit isFailure() checks before accessing failure() improves test robustness and restores assertions that were removed (only left as a comment). This is a valid but moderate improvement.	Low
	Use POST when sending a request body Using HTTP GET with a body is problematic (many HTTP clients/proxies drop the body on GET). Since the workaround serializes an index_filter payload as the request body, the method should be POST to ensure the body is transmitted; OpenSearch supports POST /_field_caps. Otherwise indexFilter will be silently ignored by callers relying on it. src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [153-156] JsonBodyBuilder builder = Requests.builder() .endpoint(indexPrefix + "/_field_caps") - .method("GET") + .method("POST") .query(Map.of("fields", request.fields().stream().collect(Collectors.joining(",")))); Suggestion importance[1-10]: 6 __ Why: Correct concern that GET with a body is problematic and index_filter may be silently dropped. However, OpenSearch does accept GET with body for _field_caps, so the practical impact depends on the HTTP client behavior.	Low
General	Preserve alias-based assertion semantics AliasesRecord::index returns the underlying index name, not the alias, so the cat().aliases() result is grouped by index. Since two aliases (both-indices and forbidden-index) map to horrible_songs, and one alias (both-indices) maps to marvelous_songs, the previous test-expectation of one line containing "marvelous_songs" and not "horrible_songs" is no longer guaranteed by iterating indices. Consider mapping AliasesRecord::alias (or a composite of alias+index) to preserve the original semantics of the test. src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java [492-497] -List<String> aliases = getAliasesResponse.valueBody().stream().map(AliasesRecord::index).sorted().toList(); +List<String> aliases = getAliasesResponse.valueBody().stream().map(r -> r.alias() + " " + r.index()).sorted().toList(); // Does not fail on forbidden, but alias response only contains index which user has access to assertThat(aliases.size(), equalTo(1)); assertThat(aliases, hasItem(containsString("marvelous_songs"))); assertThat(aliases, not(hasItem(containsString("horrible_songs")))); Suggestion importance[1-10]: 7 __ Why: Valid observation that AliasesRecord::index returns the index name rather than the alias, which changes the test semantics. This could allow the test to pass incorrectly or miss authorization regressions, though the size check still provides some protection.	Medium
	Do not skip security negative test The @Ignore disables a security-relevant negative test verifying that a forbidden index access is properly rejected. Since the fieldCaps method is overridden locally in CloseableOpenSearchClient (and is not actually blocked by the upstream issue #1792), consider removing @Ignore so this important authorization check continues to run. If the test genuinely fails against the new implementation, that is a defect that should be fixed rather than skipped. src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java [394-402] @Test -@Ignore // awaits https://github.com/opensearch-project/opensearch-java/issues/1792 public void shouldGetFieldCapabilities_negative() throws IOException { try (CloseableOpenSearchClient client = cluster.getClient(LIMITED_USER)) { FieldCapsRequest request = FieldCapsRequest.of(r -> r.index(HORRIBLE_SONGS).fields(FIELD_TITLE)); assertThatThrownBy(() -> client.fieldCaps(request), statusException(FORBIDDEN)); } Suggestion importance[1-10]: 5 __ Why: Valid point that disabling a security-relevant negative test reduces coverage. However, the author explicitly linked the ignore to an upstream issue, so removing @Ignore may cause test failures until the upstream issue is resolved.	Low
	Verify audit path expectations across mget tests The audit-log expectations for the multi-get tests were changed from /_mget to /song_lyrics/_mget because the new client places the index in the URL path. However, shouldPerformMultiGetDocuments_partiallyPositive still asserts on /_mget even though its request now goes to /_mget without index; since the new client builds MgetRequest with two docs having separate indices, the URL will indeed be /_mget, but verify all three tests use the correct paths — the inconsistency (/song_lyrics/_mget in two tests, /_mget in the third) reflects an actual behavioral change to confirm rather than a copy-paste error. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [847-859] auditLogsRule.assertExactlyOne(userAuthenticated(LIMITED_READ_USER).withRestRequest(POST, "/song_lyrics/_mget")); auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetRequest")); auditLogsRule.assertExactlyOne(grantedPrivilege(LIMITED_READ_USER, "MultiGetShardRequest")); } -@Test -public void shouldPerformMultiGetDocuments_negative() throws IOException { - try (CloseableOpenSearchClient client = cluster.getClient(DOUBLE_READER_USER)) { - MgetRequest request = MgetRequest.of(r -> r.index(SONG_INDEX_NAME).ids(ID_S1)); - - assertThatThrownBy(() -> client.mget(request, Map.class), statusException(FORBIDDEN)); - } - auditLogsRule.assertExactlyOne(userAuthenticated(DOUBLE_READER_USER).withRestRequest(POST, "/song_lyrics/_mget")); - Suggestion importance[1-10]: 2 __ Why: The suggestion only asks the reviewer to verify the audit path expectations without proposing an actual code change (improved_code equals existing_code), providing minimal value.	Low
	Remove redundant parentheses in assertions There are extra redundant parentheses around the string literal "security_exception" in several equalTo calls (e.g. equalTo(("security_exception"))). While syntactically valid, these look like leftover editing artifacts and should be cleaned up for readability and consistency with other assertions in the same file. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [1283] - assertThat(response, bulkResponseContainExceptions(1, errorType(equalTo(("security_exception"))))); + assertThat(response, bulkResponseContainExceptions(1, errorType(equalTo("security_exception")))); Suggestion importance[1-10]: 2 __ Why: A minor stylistic cleanup with no functional impact.	Low

Suggestions up to commit e02e519

Category	Suggestion	Impact
Possible issue	Fix contradictory assertions on error type The two assertions on responses.get(1).failure().error().type() are contradictory - the same string cannot equal both INTERNAL_SERVER_ERROR (a RestStatus) and "security_exception". The first assertion will always fail because error().type() returns an error type string, not a status. Remove the incorrect status comparison and check status via the appropriate field (e.g., failure().status()). src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [876-879] var responses = response.docs(); -// assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); -assertThat(responses.get(1).failure().error().type(), equalTo(INTERNAL_SERVER_ERROR)); +assertThat(responses.get(0).failure(), nullValue()); +assertThat(responses.get(1).failure(), notNullValue()); assertThat(responses.get(1).failure().error().type(), equalTo("security_exception")); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies contradictory assertions where error().type() is compared to both INTERNAL_SERVER_ERROR (a RestStatus enum) and "security_exception" (a string). The first assertion will always fail, indicating a real bug in the test.	Medium
	Guard against null or empty fields list When request.fields() is null or an undefined list (which ApiTypeHelper.undefinedList() produces in the rebuilt request), calling .stream().collect(...) will either throw NPE or produce an empty string that causes the server to reject the request. Guard against a null/empty fields list, and note that the outer builder passes ApiTypeHelper.undefinedList() to fields(...) but then reads request.fields() from the original request — ensure the original fields are actually forwarded in the query string. src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [148-151] +String fieldsParam = (request.fields() == null || request.fields().isEmpty()) + ? "" + : request.fields().stream().collect(Collectors.joining(",")); JsonBodyBuilder builder = Requests.builder() .endpoint("/_field_caps") .method("GET") - .query(Map.of("fields", request.fields().stream().collect(Collectors.joining(",")))); + .query(Map.of("fields", fieldsParam)); Suggestion importance[1-10]: 5 __ Why: Valid defensive check: if request.fields() were null, the stream call would NPE. However, in the current usage FieldCapsRequest fields are typically provided, and the existing code works for the current test cases. Minor robustness improvement.	Low
General	Restore missing bulk failure assertion The bulk response assertion only verifies the failure at index 1, but the original test also asserted the response as a whole failed and that the first operation on WRITE_SONG_INDEX_NAME succeeded (index deletion). Add an assertion that the response is a partial failure and that the successful delete removed the document, to preserve the original test coverage. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [1104] -assertThat(response, bulkResponseContainExceptions(1, errorType(equalTo(("security_exception"))))); +assertThat(response, bulkResponseContainExceptions(1, errorType(equalTo("security_exception")))); +assertThat(response, failureBulkResponse()); Suggestion importance[1-10]: 4 __ Why: Adding failureBulkResponse() improves test coverage by verifying the response is a partial failure, matching the original test's intent. However, the impact is moderate as the existing assertion still verifies the security exception.	Low
	Walk full cause chain for status match The matcher unwraps only one level of TransportException cause, but real client stacks often nest multiple layers (e.g., IOException wrapping TransportException wrapping another TransportException/ResponseException). Consider walking the full cause chain to find the first OpenSearchException/ResponseException, so status-based assertions do not spuriously fail when the client wraps exceptions more deeply. src/integrationTest/java/org/opensearch/test/framework/matcher/client/TransportExceptionMatcher.java [56-64] +Throwable c = cause; +while (c != null && !(c instanceof OpenSearchException) && !(c instanceof ResponseException) && c.getCause() != c) { + c = c.getCause(); +} +if (c == null) { + mismatchDescription.appendText("no OpenSearchException/ResponseException in cause chain"); + return false; +} +cause = c; if ((cause instanceof ResponseException) == false) { mismatchDescription.appendText("actual exception type is ") .appendValue(cause.getClass().getCanonicalName()) .appendText(", error message ") .appendValue(cause.getMessage()); return false; } ResponseException openSearchException = (ResponseException) cause; if (expectedRestStatus.getStatus() != openSearchException.status()) { Suggestion importance[1-10]: 4 __ Why: Reasonable robustness improvement for the matcher to handle deeply nested exception chains, though the current unwrapping likely handles the common cases in tests. Not a critical issue.	Low
	Verify index pattern list conversion indexPatterns(TEMPLATE_INDEX_PREFIX) passes a single pattern string, but the original code used List.of(TEMPLATE_INDEX_PREFIX). If TEMPLATE_INDEX_PREFIX doesn't already contain a wildcard suffix (e.g., "song-transcription-"), the template will not match indices like INDEX_NAME_SONG_TRANSCRIPTION_JAZZ, breaking subsequent assertions. Verify that TEMPLATE_INDEX_PREFIX is a proper pattern; otherwise append "". src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [1533-1538] PutTemplateRequest request = PutTemplateRequest.of( r -> r.name(MUSICAL_INDEX_TEMPLATE) - .indexPatterns(TEMPLATE_INDEX_PREFIX) + .indexPatterns(List.of(TEMPLATE_INDEX_PREFIX)) .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0001, Alias.builder().build()) .aliases(ALIAS_USED_IN_MUSICAL_INDEX_TEMPLATE_0002, Alias.builder().build()) ); Suggestion importance[1-10]: 3 __ Why: The indexPatterns method likely accepts varargs, so passing a single string should be equivalent to List.of(...). The concern about wildcard suffix is valid but applies equally to the original code, so it's not a regression.	Low
	Do not swallow async close errors The close() submits the transport close to a common pool and returns immediately without awaiting completion; any IOException becomes a swallowed RuntimeException inside the pool thread. In test lifecycles this can lead to resource leaks between tests and hidden failures. Consider at least logging failures, and preferably capturing the returned Future or waiting briefly, so close errors are observable. src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [167-177] @Override public void close() throws IOException { ForkJoinPool.commonPool().submit(() -> { // Do the closing of the restClient asynchronously, as it might cause a 5 second delay try { _transport().close(); } catch (IOException e) { - throw new RuntimeException(e); + LogManager.getLogger(CloseableOpenSearchClient.class).warn("Failed to close transport", e); } }); } Suggestion importance[1-10]: 3 __ Why: Minor improvement for observability in test infrastructure; the original behavior mirrors previous RestHighLevelClient closure pattern and is unlikely to cause significant issues in practice.	Low

Suggestions up to commit db0cfea

Category	Suggestion	Impact
Possible issue	Fix contradictory and incorrect failure assertions The assertion compares the error type string to INTERNAL_SERVER_ERROR (a RestStatus enum), which will never be equal. Additionally, type is asserted twice against different values, so at most one can pass. Compare the HTTP status against failure().status() and check the exception type/reason separately. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [882-885] var responses = response.docs(); -// assertThat(responses, arrayContaining(hasProperty("failure", nullValue()), hasProperty("failure", notNullValue()))); -assertThat(responses.get(1).failure().error().type(), equalTo(INTERNAL_SERVER_ERROR)); -assertThat(responses.get(1).failure().error().type(), equalTo("security_exception")); +assertThat(responses.get(0).isFailure(), equalTo(false)); +assertThat(responses.get(1).isFailure(), equalTo(true)); +assertThat(responses.get(1).failure().status(), equalTo(INTERNAL_SERVER_ERROR.getStatus())); +assertThat(responses.get(1).failure().error().type(), containsString("security_exception")); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a real bug: comparing error().type() (a String) to INTERNAL_SERVER_ERROR (a RestStatus enum) will never be true, and the same field is asserted twice against different values, making at least one assertion always fail.	Medium
	Compare HTTP status against numeric status field Comparing the string field error().type() to the RestStatus.INTERNAL_SERVER_ERROR enum will always fail. Use failure().status() for the HTTP status code comparison, and keep the reason/type string check separate. src/integrationTest/java/org/opensearch/security/SearchOperationTest.java [973-977] var responses = response.responses(); assertThat(responses.get(0).isFailure(), equalTo(false)); assertThat(responses.get(1).isFailure(), equalTo(true)); -assertThat(responses.get(1).failure().error().type(), equalTo(INTERNAL_SERVER_ERROR)); +assertThat(responses.get(1).failure().status(), equalTo(INTERNAL_SERVER_ERROR.getStatus())); assertThat(responses.get(1).failure().error().reason(), containsString("security_exception")); Suggestion importance[1-10]: 8 __ Why: Correctly identifies that error().type() returns a String and comparing it to the RestStatus.INTERNAL_SERVER_ERROR enum will always fail. Using failure().status() for the numeric status is the appropriate fix.	Medium
	Fix nullValue assertion on primitive double AvgAggregate.value() returns a primitive double, so comparing it with nullValue() will not compile or will auto-box to a non-null Double, making the assertion always fail. Assert against a numeric value (e.g., Double.POSITIVE_INFINITY or Double.NaN, matching how the previous ParsedAvg test represented "no visible field") or check isNaN/finite-ness explicitly. src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java [491-493] assertThat(searchResponse, containAggregationWithNameAndType(aggregationName, "avg")); Aggregate actualAggregation = searchResponse.aggregations().get(aggregationName); assertThat(actualAggregation._get(), instanceOf(AvgAggregate.class)); -assertThat(actualAggregation.avg().value(), is(nullValue())); // user cannot see the STARS field +assertThat(Double.isNaN(actualAggregation.avg().value()) || Double.isInfinite(actualAggregation.avg().value()), is(true)); Suggestion importance[1-10]: 7 __ Why: Correctly identifies that AvgAggregate.value() returns a primitive double which auto-boxes and will never be null, making the nullValue() matcher fail. The suggested fix aligns with the original test behavior expecting POSITIVE_INFINITY/NaN.	Medium
	Guard null fields and use POST for body When request.fields() is null or empty, stream() will NPE or produce an empty query parameter. Additionally, sending the request body with HTTP GET can be problematic; switch to POST as the standard _field_caps invocation does, and guard the fields query parameter. [src/integrationTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java [140-151]](https://github.com/opensearch-project/security/pull/6420/files#diff-a4959bf0fdf7b1...

[github-actions]

Persistent review updated to latest commit 6c90a83

[github-actions]

Persistent review updated to latest commit 2f0e8cf

[github-actions]

Persistent review updated to latest commit 74efc55

[github-actions]

Persistent review updated to latest commit 2a88324

[github-actions]

Persistent review updated to latest commit 85d969b

[github-actions]

Persistent review updated to latest commit 2544a96

[github-actions]

Persistent review updated to latest commit 55cc2f8

[github-actions]

Persistent review updated to latest commit b2e6259

[github-actions]

Persistent review updated to latest commit 08c1489

[github-actions]

Persistent review updated to latest commit 807e6bb

[github-actions]

Persistent review updated to latest commit a870004

[github-actions]

Persistent review updated to latest commit 7855f7a

[github-actions]

Persistent review updated to latest commit 3eb18ac

[github-actions]

Persistent review updated to latest commit a819af9

[github-actions]

Persistent review updated to latest commit db0cfea

[github-actions]

Persistent review updated to latest commit e02e519

[github-actions]

Persistent review updated to latest commit 27530a4

[github-actions]

Persistent review updated to latest commit bb2a1fa

[github-actions]

Persistent review updated to latest commit f06c6e5

[github-actions]

Persistent review updated to latest commit 5d7fffe

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.79%. Comparing base (c33b831) to head (5d7fffe).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6420      +/-   ##
==========================================
+ Coverage   75.75%   75.79%   +0.04%     
==========================================
  Files         457      457              
  Lines       30508    30508              
  Branches     4615     4615              
==========================================
+ Hits        23111    23124      +13     
+ Misses       5276     5264      -12     
+ Partials     2121     2120       -1     

see 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[reta]

@cwperks I think we are in good shape here:

• large chunk (if not all) tests where migrated from RHLC to OpenSearchClient
• there is only one pending issue which already has a fix submitted but I was able to workaround it (https://github.com/opensearch-project/security/pull/6420/changes#diff-a4959bf0fdf7b1f66e61f1b40091f069bdaa05a94dd9e896e15e75c26a79488bR138)
• otherwise, no changes outside the test code

I haven't removed the dependency yet (there is usage of the RHLC related classes in the main codebase) but I will be working on this next, didn't want to inflate this pull request even more. Thank you