added filter to both pagination strategies#20616
added filter to both pagination strategies#20616ThyTran1402 wants to merge 4 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
github-actions
bot
added
API
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes introduce authorization filtering to the pagination strategy for indices listing APIs. A new constructor and method signature variations add support for an authorization filter predicate, enabling pagination to respect authorized indices. REST action implementations now extract authorization information from settings responses and pass it to the pagination strategy. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/org/opensearch/action/pagination/IndexPaginationStrategy.java (1)
76-86:⚠️ Potential issue | 🟡 MinorFilter composition is correct but consider null-safety for the
authorizationFilterparameter.The
paginationFilter.and(authorizationFilter)composition at Line 85 correctly applies both filters. However, if a caller passesnullforauthorizationFilter, this will throw aNullPointerExceptionat the.and()call. The 2-arg constructor always passesmetadata -> true, but the public 3-arg constructor at Line 51 doesn't guard against null.🛡️ Proposed fix to add null guard
public IndexPaginationStrategy(PageParams pageParams, ClusterState clusterState, Predicate<IndexMetadata> authorizationFilter) { + Objects.requireNonNull(authorizationFilter, "authorizationFilter should not be null"); IndexStrategyToken requestedToken = Objects.isNull(pageParams.getRequestedToken()) || pageParams.getRequestedToken().isEmpty()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/src/main/java/org/opensearch/action/pagination/IndexPaginationStrategy.javaserver/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.javaserver/src/main/java/org/opensearch/rest/action/list/RestIndicesListAction.javaserver/src/test/java/org/opensearch/action/pagination/IndexPaginationStrategyTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:40:27.167Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.
Applied to files:
server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.javaserver/src/main/java/org/opensearch/rest/action/list/RestIndicesListAction.javaserver/src/main/java/org/opensearch/action/pagination/IndexPaginationStrategy.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code-Diff-Analyzer / Code-Diff-Analyzer
🔇 Additional comments (7)
server/src/test/java/org/opensearch/action/pagination/IndexPaginationStrategyTests.java (3)
357-388: Good test coverage for the filtered pagination in ascending order.The test properly validates that system indices are excluded from pagination results and that the pagination token correctly chains across pages when an authorization filter is applied.
Consider adding a test for the edge case where the authorization filter matches no indices (empty
authorizedIndicesset), to verify the strategy returns an empty list and a null token.
390-410: Descending order test with auth filter looks good.Correctly validates that the auth filter works with descending sort order and that the
.security-indexis excluded.
427-444: Non-deterministic creation timestamps may cause flaky token assertions.
Instant.now()is called per-index creation, so two indices added in the same test can receive differentnow()base values. This is unlikely to cause issues given theplus(creationOrder, ChronoUnit.SECONDS)offset, but if the clock ticks between calls (e.g., across a second boundary), two indices with consecutivecreationOrdervalues could theoretically have their relative ordering affected. The existing tests already used this pattern, so this is not a new concern — just noting it for awareness.server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.java (2)
199-202: LGTM — call site correctly passesgetSettingsResponseto the pagination strategy.
1245-1250: Base class properly accepts the new parameter while maintaining existing behavior.The
getSettingsResponseparameter is unused here (returnsnull) since_cat/indicesis non-paginated, but the signature must include it so thatRestIndicesListActioncan override with authorization-filtered logic. This is a clean approach.server/src/main/java/org/opensearch/action/pagination/IndexPaginationStrategy.java (1)
47-49: Clean delegation pattern for backward compatibility.The existing 2-arg constructor delegates to the new 3-arg constructor with a permissive
metadata -> truefilter, ensuring all existing call sites continue to work without changes.server/src/main/java/org/opensearch/rest/action/list/RestIndicesListAction.java (1)
91-98: Authorization filter correctly derives from security-filteredGetSettingsResponse.The approach of using
getSettingsResponse.getIndexToSettings().keySet()is consistent with the existing pattern documented inRestIndicesAction.sendGetSettingsRequest(Lines 271-277), which explains that Get Settings is used specifically because it respects index-level privileges. The implementation is sound—getIndexToSettings()always returns a non-null Map, and the filter safely checks authorization based on the indices returned by the security-filtered response.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20616 +/- ##
============================================
+ Coverage 73.34% 73.38% +0.03%
- Complexity 72173 72230 +57
============================================
Files 5798 5798
Lines 329753 329755 +2
Branches 47524 47522 -2
============================================
+ Hits 241859 241989 +130
+ Misses 68564 68444 -120
+ Partials 19330 19322 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
|
❌ Gradle check result for 2a002e9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
|
Hi @cwperks Can you review it please? Thank you. |
1 participant
Description
Related Issues
Resolves #20291
Check List
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.