Use GetMappingsRequest instead of GetIndexRequest in QueryPlanningTool

[owaiskazi19]

Description

QueryPlanningTool uses GetIndexRequest to retrieve index mappings for query planning, but it only accesses .mappings() from the response. GetIndexRequest requires the indices:admin/get permission, which also exposes index settings, aliases, and defaults — none of which QPT uses.

This PR switches to GetMappingsRequest / GetMappingsResponse, which only requires indices:admin/mappings/get. This:

• Follows the principle of least privilege
• Aligns with how knn_full_access and query_assistant_access roles already handle mapping retrieval
• Enables the ml_full_access role to support Agentic Search (QueryPlanningTool) with the narrower indices:admin/mappings/get permission instead of the broader indices:admin/get

Related Issue

Resolves #4775

Changes

QueryPlanningTool.java:

• GetIndexRequest → GetMappingsRequest
• GetIndexResponse → GetMappingsResponse
• client.admin().indices().getIndex() → client.admin().indices().getMappings()
• Response handling unchanged — .mappings() returns the same Map<String, MappingMetadata>

QueryPlanningToolTests.java:

• All mocks updated to use GetMappingsResponse
• All indicesAdminClient.getIndex() → indicesAdminClient.getMappings()

Check List

• New functionality includes testing
• New functionality has been documented
  • N/A (internal API change, no user-facing doc change)
• Commits are signed off as per the DCO using --signoff

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 33441c5)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Stale Comment The comment on line 1512 still says "Mock getIndex to return mappings keyed by multiple concrete indices" but the method is now getMappings. Similarly, the comment on line 159 still references "getIndex operation". These stale comments may cause confusion. // Mock getIndex to return mappings keyed by multiple concrete indices (simulating alias resolution) ArgumentCaptor<ActionListener<GetMappingsResponse>> captor = ArgumentCaptor.forClass(ActionListener.class); doNothing().when(indicesAdminClient).getMappings(any(), captor.capture()); Variable Naming The field getIndexResponse (line 109) is typed as GetMappingsResponse but retains the old name getIndexResponse. This is misleading and could cause confusion during future maintenance. It should be renamed to getMappingsResponse or similar. private GetMappingsResponse getIndexResponse;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 33441c5

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove unsupported method call on new request type GetMappingsRequest does not have a local(boolean) method — that method belongs to GetIndexRequest. Calling a non-existent method will cause a compilation error. Remove the .local(false) call since GetMappingsRequest uses a different API for controlling local reads (if needed, use masterNodeTimeout or similar). ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/QueryPlanningTool.java [366-370] GetMappingsRequest getMappingsRequest = new GetMappingsRequest() .indices(indexName) .indicesOptions(IndicesOptions.strictExpand()) - .local(false) .clusterManagerNodeTimeout(DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT); Suggestion importance[1-10]: 7 __ Why: GetMappingsRequest may not have a .local(false) method like GetIndexRequest does, which could cause a compilation error. However, this depends on the OpenSearch version being used, as some versions of GetMappingsRequest do support a local method. The suggestion is potentially valid but needs verification against the actual API.	Medium

---

Previous suggestions

Suggestions up to commit 33441c5

Category	Suggestion	Impact
Possible issue	Remove unsupported method calls on new request type GetMappingsRequest does not have a local() method or a clusterManagerNodeTimeout() method in the same way GetIndexRequest does. These method calls may not exist on GetMappingsRequest and could cause a compilation error. Verify that these methods are available on GetMappingsRequest and remove or replace any unsupported method calls. ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/QueryPlanningTool.java [366-370] GetMappingsRequest getMappingsRequest = new GetMappingsRequest() .indices(indexName) - .indicesOptions(IndicesOptions.strictExpand()) - .local(false) - .clusterManagerNodeTimeout(DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT); + .indicesOptions(IndicesOptions.strictExpand()); Suggestion importance[1-10]: 7 __ Why: GetMappingsRequest may not support .local() and .clusterManagerNodeTimeout() methods unlike GetIndexRequest, which could cause a compilation error. This is a valid concern worth verifying, though the actual API availability depends on the OpenSearch version being used.	Medium

Suggestions up to commit 00b97d2

Category	Suggestion	Impact
Possible issue	Add missing mappings stub on mocked response After removing the when(getIndexResponse.indices()).thenReturn(...) stub, the mock for getIndexResponse.mappings() is never set up in mockGetIndexMapping(). Tests that call mockGetIndexMapping() and then invoke actionListenerCaptor.getValue().onResponse(getIndexResponse) will receive null from getMappingsResponse.mappings(), causing unexpected IllegalStateException failures instead of exercising the happy path. Add a when(getIndexResponse.mappings()).thenReturn(...) stub with the expected mapping data. ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/QueryPlanningToolTests.java [165-169] this.getIndexResponse = mock(GetMappingsResponse.class); // Create real MappingMetadata with actual source String mappingSource = "{\"properties\":{\"title\":{\"type\":\"text\"}}}"; this.mapping = new MappingMetadata("testIndex", XContentHelper.convertToMap(JsonXContent.jsonXContent, mappingSource, true)); + Map<String, MappingMetadata> mappingsMap = new HashMap<>(); + mappingsMap.put("testIndex", this.mapping); + when(getIndexResponse.mappings()).thenReturn(mappingsMap); Suggestion importance[1-10]: 8 __ Why: The old code had when(getIndexResponse.indices()).thenReturn(new String[] { "testIndex" }) which was removed, but no when(getIndexResponse.mappings()).thenReturn(...) stub was added in mockGetIndexMapping(). Without this stub, tests using mockGetIndexMapping() that call onResponse(getIndexResponse) will get null from getMappingsResponse.mappings(), causing unexpected IllegalStateException failures in happy-path tests.	Medium

Suggestions up to commit 702c3c0

Category	Suggestion	Impact
Possible issue	Remove unsupported method call on new request type GetMappingsRequest does not have a local() method — that method belongs to GetIndexRequest. Calling a non-existent method will cause a compilation error. Remove the .local(false) call since GetMappingsRequest does not support it. ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/QueryPlanningTool.java [366-370] GetMappingsRequest getMappingsRequest = new GetMappingsRequest() .indices(indexName) .indicesOptions(IndicesOptions.strictExpand()) - .local(false) .clusterManagerNodeTimeout(DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT); Suggestion importance[1-10]: 8 __ Why: GetMappingsRequest does not have a .local(false) method (that belongs to GetIndexRequest), so this would cause a compilation error. Removing it is necessary for the code to compile correctly.	Medium

Suggestions up to commit 16120a8

Category	Suggestion	Impact
Possible issue	Remove unsupported method call on new request type GetMappingsRequest does not have a local(boolean) method — this method exists on GetIndexRequest but not on GetMappingsRequest. This will cause a compilation error. Remove the .local(false) call since it is not supported by GetMappingsRequest. ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/QueryPlanningTool.java [366-370] GetMappingsRequest getMappingsRequest = new GetMappingsRequest() .indices(indexName) .indicesOptions(IndicesOptions.strictExpand()) - .local(false) .clusterManagerNodeTimeout(DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT); Suggestion importance[1-10]: 8 __ Why: GetMappingsRequest does not have a .local(boolean) method like GetIndexRequest does, so this would cause a compilation error. Removing the unsupported .local(false) call is necessary for the code to compile correctly.	Medium

Suggestions up to commit adda5de

Category	Suggestion	Impact
Possible issue	Remove unsupported method calls on new request type GetMappingsRequest does not have a local() method or a clusterManagerNodeTimeout() method in the same way GetIndexRequest does. These methods may not exist on GetMappingsRequest, which would cause a compilation error. Verify that these methods are available on GetMappingsRequest and remove or replace any unsupported method calls. ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/QueryPlanningTool.java [366-369] GetMappingsRequest getMappingsRequest = new GetMappingsRequest().indices(indexName) - .indicesOptions(IndicesOptions.strictExpand()) - .local(false) - .clusterManagerNodeTimeout(DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT); + .indicesOptions(IndicesOptions.strictExpand()); Suggestion importance[1-10]: 7 __ Why: GetMappingsRequest may not support .local() and .clusterManagerNodeTimeout() methods unlike GetIndexRequest, which could cause a compilation error. This is a potentially valid concern about API compatibility that warrants verification.	Medium

[github-actions]

Persistent review updated to latest commit 16120a8

[github-actions]

Persistent review updated to latest commit 702c3c0

[rithinpullela]

Thanks for the PR Owais!

[github-actions]

Persistent review updated to latest commit 00b97d2

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.42%. Comparing base (ed3dd10) to head (33441c5).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4785      +/-   ##
============================================
+ Coverage     77.40%   77.42%   +0.01%     
- Complexity    11898    11903       +5     
============================================
  Files           963      963              
  Lines         53309    53310       +1     
  Branches       6500     6500              
============================================
+ Hits          41265    41274       +9     
+ Misses         9293     9286       -7     
+ Partials       2751     2750       -1     

Flag	Coverage Δ
ml-commons	77.42% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mingshl]

the change make sense, only the mapping is used for query planning. The only thing that comes out of my mind is to double check if there is any conflicts with indexInsightTool? I think people could use indexInsightTool with queryPlanningTool.

I think it should be fine, if LLM call indexInsightTool first, which generate index insight, then use queryPlanningTool to generate a suitable query. This is my theory, but have you test this case?

[owaiskazi19]

I think it should be fine, if LLM call indexInsightTool first, which generate index insight, then use queryPlanningTool to generate a suitable query. This is my theory, but have you test this case?

Index Insight's StatisticalDataTask already uses GetMappingsRequest independently. Both the tools are independent of each other.

[mingshl]

I think it should be fine, if LLM call indexInsightTool first, which generate index insight, then use queryPlanningTool to generate a suitable query. This is my theory, but have you test this case?

Index Insight's StatisticalDataTask already uses GetMappingsRequest independently. Both the tools are independent of each other.

make sense. I took a peak at the IndexInsightTool and it seems there are similar operations, like get index mapping, search few sample documents, make a model inference should think about extracting the similar operations into a utility class when more and more repetitive codes are merged.

[github-actions]

Persistent review updated to latest commit 33441c5

[github-actions]

Persistent review updated to latest commit 33441c5