Skip to content

Range Validation#20518

Open
urmichm wants to merge 25 commits intoopensearch-project:mainfrom
urmichm:20497-range-validation
Open

Range Validation#20518
urmichm wants to merge 25 commits intoopensearch-project:mainfrom
urmichm:20497-range-validation

Conversation

@urmichm
Copy link

@urmichm urmichm commented Feb 1, 2026

Description

Added range validation for the query builder and the field mapper.

Related Issues

Resolves #20497

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.

@github-actions github-actions bot added bug Something isn't working Indexing Indexing, Bulk Indexing and anything related to indexing labels Feb 1, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 1, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

  • 🔍 Trigger a full review
📝 Walkthrough

Walkthrough

Adds validation for range bounds in both indexing (RangeFieldMapper) and query parsing (RangeQueryBuilder); refactors range parsing into a dedicated method, enforces single-assignment and inclusion semantics, handles CIDR IP input, and adds tests and a changelog entry. No public API signature changes.

Changes

Cohort / File(s) Summary
Range mapper implementation
server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java
Refactors parsing into a new private parseRange(ParseContext) method; validates bounds (single-assignment, inclusion flags), special-cases CIDR IP strings, handles ignore_malformed, fills missing bounds with type min/max, and centralizes error handling. Adjusts post-parse field-names creation logic.
Range query parsing
server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java
Replaces primitive inclusion flags with Boolean wrappers to track explicit presence; centralizes parsing with DeprecationHandler, validates conflicting/duplicate lower/upper bounds, and applies include flags only when explicitly provided. Adds new error messages for invalid bounds.
Mapper & Query tests
server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java, server/src/test/java/org/opensearch/index/query/RangeQueryBuilderTests.java
Adds tests asserting parsing errors for invalid/conflicting lower and upper bound configurations for both mapper and query parsing.
Changelog
CHANGELOG.md
Adds an Unreleased 3.x entry noting "Added range validation for query builder and field mapper" linking issue #20497.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client as Client
participant Parser as ParseContext / XContentParser
participant Mapper as RangeFieldMapper
participant Type as RangeType
rect rgba(220,220,255,0.5)
Client->>Parser: send JSON range input
Parser->>Mapper: parseRange(context)
Mapper->>Parser: read tokens (gt/gte/lt/lte, cidr, nulls)
alt CIDR string detected
Mapper->>Type: parseIpRangeFromCidr(...)
Type-->>Mapper: Range or error
else Object bounds
Mapper->>Type: rangeType.parseFrom/parseTo for bounds
Type-->>Mapper: parsed bound values
end
Mapper->>Mapper: validate single-assignment & inclusivity
alt malformed and ignore_malformed
Mapper->>Mapper: record ignored field, return null
else valid
Mapper->>Type: rangeType.createFields(...)
Type-->>Mapper: Lucene fields
Mapper->>Parser: optionally create _field_names
end
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Range Validation' is directly related to the main change—adding range validation for the query builder and field mapper—and clearly summarizes the primary objective.
Description check ✅ Passed The PR description covers the key section headings from the template (Description, Related Issues, Check List) and provides adequate detail about the validation changes and test coverage.
Linked Issues check ✅ Passed The PR successfully addresses issue #20497 by implementing range validation in RangeQueryBuilder and RangeFieldMapper to reject conflicting bound specifications, with comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly related to the range validation objective: modifications to RangeQueryBuilder, RangeFieldMapper, corresponding tests, and CHANGELOG update are all in scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2026

❕ Gradle check result for 9a9b3a9: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@codecov
Copy link

codecov bot commented Feb 1, 2026

Codecov Report

❌ Patch coverage is 88.46154% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.30%. Comparing base (3ba2f37) to head (ec92add).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../org/opensearch/index/mapper/RangeFieldMapper.java 83.05% 5 Missing and 5 partials ⚠️
.../org/opensearch/index/query/RangeQueryBuilder.java 95.55% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #20518      +/-   ##
============================================
+ Coverage     73.25%   73.30%   +0.04%     
- Complexity    72103    72172      +69     
============================================
  Files          5798     5798              
  Lines        329732   329747      +15     
  Branches      47519    47526       +7     
============================================
+ Hits         241554   241716     +162     
+ Misses        68805    68716      -89     
+ Partials      19373    19315      -58     

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

@urmichm
Copy link
Author

urmichm commented Feb 1, 2026

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 1, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2026

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

@urmichm
Copy link
Author

urmichm commented Feb 1, 2026

Regression
org.opensearch.action.admin.cluster.filecache.PruneFileCacheIT.testPruneCacheWithRealData

Error Message

java.lang.AssertionError: Should have pruned bytes

Stacktrace

java.lang.AssertionError: Should have pruned bytes
	at __randomizedtesting.SeedInfo.seed([CDE0C412E58D517C:374F261BFA2814E2]:0)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
.....

@urmichm
Copy link
Author

urmichm commented Feb 2, 2026

org.opensearch.action.admin.cluster.filecache.PruneFileCacheIT.testPruneCacheWithRealData is a known flaky test -> #19724

simplification of range set-up

Signed-off-by: Michael <urmich.m@gmail.com>
null check moved to simplify nested if-blocks

Signed-off-by: Michael <urmich.m@gmail.com>
Added a null check, to avoid uncontrolled NullPointerException

Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
while-loop simplified, if-else changed to if and continue

Signed-off-by: Michael <urmich.m@gmail.com>
DeprecationHandler as a variable

Signed-off-by: Michael <urmich.m@gmail.com>
DeprecationHandler as a shorter variable

Signed-off-by: Michael <urmich.m@gmail.com>
spotless

Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
Signed-off-by: Michael <urmich.m@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2026

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

@urmichm
Copy link
Author

urmichm commented Feb 4, 2026

Failed tests: https://build.ci.opensearch.org/job/gradle-check/70933/

    org.opensearch.cluster.settings.ClusterSettingsIT.testWithMultipleIndexCreationAndVerifySettingRegisteredOnce
    org.opensearch.cluster.settings.ClusterSettingsIT.testClusterNonExistingSettingsUpdate
    org.opensearch.cluster.settings.ClusterSettingsIT.testUpdateSettings
    org.opensearch.cluster.settings.ClusterSettingsIT.testThreadPoolSettings
    org.opensearch.cluster.settings.ClusterSettingsIT.testUserMetadata
    org.opensearch.cluster.settings.ClusterSettingsIT.testClusterSettingsUpdateResponse
    org.opensearch.cluster.settings.ClusterSettingsIT.testCanUpdateTracerSettings

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2026

❌ Gradle check result for c51c126: null

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?

@urmichm
Copy link
Author

urmichm commented Feb 5, 2026

Hi @sandeshkr419
unfortunately, one job on the pipeline failed due to auth access 403 Forbidden
could you please have a look?

@urmichm urmichm closed this Feb 6, 2026
@urmichm urmichm reopened this Feb 6, 2026
Signed-off-by: Mikhail Urmich <32458509+urmichm@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2026

❕ Gradle check result for ec92add: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@urmichm
Copy link
Author

urmichm commented Feb 7, 2026

Hello, all tests passed, please review my PR
@msfroh @epugh @sandeshkr419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Indexing Indexing, Bulk Indexing and anything related to indexing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Range fields validation

1 participant