Skip to content

Conversation

@karenyrx
Copy link
Contributor

@karenyrx karenyrx commented Dec 4, 2025

Description

Request-side fields that exist in the protobufs but are not yet implemented in the server should throw exception instead of silently ignoring the provided field.

Related Issues

=#19526

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.

Summary by CodeRabbit

  • New Features

    • Added support for the store field in TermsLookup for gRPC requests.
  • Bug Fixes

    • Added validation to throw exceptions when unsupported gRPC parameters are used: type and global_params in bulk requests; typed_keys and global_params in search requests; aggregations and suggest in search source.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This PR adds runtime validation guards to reject currently unsupported GRPC request fields by throwing UnsupportedOperationException for type, global_params, aggregations, suggest, and typed_keys fields. It also extends TermsLookup deserialization to include the store field.

Changes

Cohort / File(s) Summary
CHANGELOG documentation
CHANGELOG.md
Added entry under Unreleased 3.x → Added section documenting the new exception-throwing behavior for unsupported GRPC fields.
Bulk request validation
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestProtoUtils.java
Introduced runtime guards in prepareRequest to throw UnsupportedOperationException for deprecated type field and unsupported global_params field.
Search request validation
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchRequestProtoUtils.java
Added validation checks in parseSearchRequest to throw UnsupportedOperationException for unsupported typed_keys and global_params fields.
Search source builder validation
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java
Introduced guards in parseNonQueryFields to reject aggregations (when count > 0) and suggest fields by throwing UnsupportedOperationException; modified previous suggest handling to throw exception instead of constructing builder.
TermsLookup deserialization
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtils.java
Extended parseTermsLookup to populate the store field from protobuf when present.
Bulk request validation tests
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestProtoUtilsTests.java
Added two test methods: testPrepareRequestWithTypeThrowsUnsupportedOperationException and testPrepareRequestWithGlobalParamsThrowsUnsupportedOperationException to verify exception handling.
Search request validation tests
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchRequestProtoUtilsTests.java
Added two test methods: testParseSearchRequestWithTypedKeysThrowsUnsupportedOperationException and testParseSearchRequestWithGlobalParamsThrowsUnsupportedOperationException to validate unsupported field rejection.
Search source builder validation tests
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtilsTests.java
Added testParseProtoWithAggregationsThrowsUnsupportedOperationException test; modified testParseProtoWithSuggest to expect UnsupportedOperationException instead of successful parsing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Notable considerations:
    • Multiple validation guards added across different proto utility files following a consistent pattern (defensive checks throwing exceptions)
    • Test coverage mirrors the implementation changes with repetitive test structure
    • One enhancement (TermsLookup.store support) differs from the main validation theme and requires separate reasoning
    • The modifications to SearchSourceBuilderProtoUtils change existing behavior for suggest (from construction to rejection), which may warrant extra attention

Poem

🐰 Hop, skip, and throw we go,
Unsupported fields? Non! Non! Non!
With guards so swift and guards so true,
We catch the fields that won't pass through!
TermsLookup now stores what it knew—
Exceptions reign, our code stays new!

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

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00cf7c1 and b485652.

📒 Files selected for processing (8)
  • CHANGELOG.md (1 hunks)
  • modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestProtoUtils.java (1 hunks)
  • modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchRequestProtoUtils.java (1 hunks)
  • modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java (1 hunks)
  • modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtils.java (1 hunks)
  • modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestProtoUtilsTests.java (1 hunks)
  • modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchRequestProtoUtilsTests.java (1 hunks)
  • modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtilsTests.java (2 hunks)

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

❌ Gradle check result for 3ee08b3: 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: Karen X <karenxyr@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

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

@karenyrx karenyrx closed this Dec 5, 2025
@karenyrx karenyrx reopened this Dec 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

✅ Gradle check result for b485652: SUCCESS

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.23%. Comparing base (d47931e) to head (b485652).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #20162      +/-   ##
============================================
- Coverage     73.30%   73.23%   -0.07%     
+ Complexity    71732    71713      -19     
============================================
  Files          5793     5793              
  Lines        328056   328088      +32     
  Branches      47245    47252       +7     
============================================
- Hits         240476   240277     -199     
- Misses        68264    68541     +277     
+ Partials      19316    19270      -46     

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

@karenyrx karenyrx marked this pull request as ready for review December 5, 2025 07:09
@karenyrx karenyrx requested a review from a team as a code owner December 5, 2025 07:09
@karenyrx karenyrx merged commit 0449ce8 into opensearch-project:main Dec 5, 2025
66 of 70 checks passed
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