Skip to content

Conversation

@ruai0511
Copy link
Contributor

@ruai0511 ruai0511 commented Jul 10, 2025

Description

This PR contains several bug fix/improvements for the rule-based auto tagging feature.

  • Refines the extraction of attribute parameters from RestRequest in RestGetRuleAction. The previous approach filtered out a few known non-attribute/system params, but it relied on a fixed exclusion list. It could still include other unrelated or unintended params if they weren’t explicitly listed, especially any automatically added query params from the system.
final Set<String> excludedKeys = Set.of(FEATURE_TYPE, ID_STRING, SEARCH_AFTER_STRING, "pretty");
final List<String> requestParams = request.params().keySet().stream().filter(key -> !excludedKeys.contains(key)).toList();

This current change whitelists only keys that are explicitly defined as allowed attributes for the given FeatureType. It makes the logic stricter and safer by ensuring only valid attribute names are parsed and passed for rule filtering.

final List<String> attributeParams = request.params()
    .keySet()
    .stream()
    .filter(key -> featureType.getAllowedAttributesRegistry().containsKey(key))
    .toList();
  • Moved featureValue validation logic into RuleValidator to centralize and standardize rule validation. This ensures consistent validation across different rule APIs and improves maintainability.
  • A force refresh (RefreshPolicy.IMMEDIATE) was added after rule creation and update to ensure the newly written documents are immediately visible for subsequent read operations (e.g., GET or search) in YAML REST tests. Without it, the default refresh interval could delay visibility, causing .yml tests to fail. Note that there seems to be no better way than to add this force refresh, since YAML test doesn't support wait(), and a explicit refresh api performed on system index would result in got unexpected warning header [ 299 OpenSearch-3.2.0-SNAPSHOT-d622ff5e85377efc43d80e9502001fdac552f0c8 "this request accesses system indices: [.wlm_rules], but in a future major version, direct access to system indices will be prevented by default"
  • Added handling to ignore IndexNotFoundException during rule sync in doRun() since this is the expected behavior if the customer has not created any rules yet.

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

@ruai0511 ruai0511 requested a review from a team as a code owner July 10, 2025 22:28
@github-actions
Copy link
Contributor

❌ Gradle check result for 2a0664b: 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?

@ruai0511 ruai0511 force-pushed the bugfix branch 2 times, most recently from f8af0b1 to 341f3b3 Compare July 10, 2025 23:51
@github-actions
Copy link
Contributor

❌ Gradle check result for 341f3b3: 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?

@kaushalmahi12
Copy link
Contributor

Can you run ./gradlew spotlessApply ? There are precommit errors

Signed-off-by: Ruirui Zhang <mariazrr@amazon.com>
@github-actions
Copy link
Contributor

❌ Gradle check result for 5f297a0: 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?

@kaushalmahi12
Copy link
Contributor

@jainankitk Can you review this PR ?

@github-actions
Copy link
Contributor

❌ Gradle check result for 5f297a0: 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?

@jainankitk
Copy link
Contributor

Another flaky test:

[Test Result](https://build.ci.opensearch.org/job/gradle-check/60724/testReport/) (1 failure / ±0)

    [org.opensearch.remotestore.RestoreShallowSnapshotV2IT.testContinuousIndexing {p0={"opensearch.experimental.feature.writable_warm_index.enabled":"true"}}](https://build.ci.opensearch.org/job/gradle-check/60724/testReport/junit/org.opensearch.remotestore/RestoreShallowSnapshotV2IT/testContinuousIndexing__p0___opensearch_experimental_feature_writable_warm_index_enabled___true___/)

@github-actions
Copy link
Contributor

❌ Gradle check result for 5f297a0: 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?

@jainankitk
Copy link
Contributor

Another remote store flaky test:

[Test Result](https://build.ci.opensearch.org/job/gradle-check/60773/testReport/) (1 failure / +1)

    [org.opensearch.remotestore.RestoreShallowSnapshotV2IT.testRestoreShallowSnapshotRepository {p0={"opensearch.experimental.feature.writable_warm_index.enabled":"true"}}](https://build.ci.opensearch.org/job/gradle-check/60773/testReport/junit/org.opensearch.remotestore/RestoreShallowSnapshotV2IT/testRestoreShallowSnapshotRepository__p0___opensearch_experimental_feature_writable_warm_index_enabled___true___/)

@github-actions
Copy link
Contributor

✅ Gradle check result for 5f297a0: SUCCESS

@codecov
Copy link

codecov bot commented Jul 18, 2025

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 72.70%. Comparing base (b609a50) to head (5f297a0).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
...ule/service/IndexStoredRulePersistenceService.java 33.33% 2 Missing ⚠️
.../org/opensearch/rule/action/CreateRuleRequest.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #18726      +/-   ##
============================================
- Coverage     72.80%   72.70%   -0.10%     
+ Complexity    68535    68478      -57     
============================================
  Files          5572     5573       +1     
  Lines        314779   314816      +37     
  Branches      45691    45694       +3     
============================================
- Hits         229166   228879     -287     
- Misses        67014    67339     +325     
+ Partials      18599    18598       -1     

☔ 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.

@jainankitk jainankitk merged commit 89edd4c into opensearch-project:main Jul 18, 2025
34 of 40 checks passed
pranikum pushed a commit to pranikum/OpenSearch that referenced this pull request Jul 21, 2025
rgsriram pushed a commit to rgsriram/OpenSearch that referenced this pull request Jul 22, 2025
tandonks pushed a commit to tandonks/OpenSearch that referenced this pull request Aug 5, 2025
@ruai0511 ruai0511 deleted the bugfix branch October 10, 2025 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants