Cleanup HttpServerTransport.Dispatcher in Netty tests #20160
Conversation
WalkthroughAdds TestDispatcherBuilder utilities for netty4 and reactor-netty4 tests, replaces many inline anonymous HttpServerTransport.Dispatcher implementations with the builder, removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (1)
75-75: Consider adding PR reference for consistencyNearby changelog entries all link the corresponding PR/issue; adding
([#20160](...))here would keep the section consistent and traceable, but is optional.modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
28-30: Optionally mark the builder classfinalSince this is a focused test utility not intended for extension, you could make the class
finalto prevent accidental subclassing in tests:-public class TestDispatcherBuilder { +public final class TestDispatcherBuilder {Purely optional, but it better communicates intent and avoids surprising inheritance chains in test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java(6 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java(11 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java(3 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java(8 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
⏰ 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). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Mend Security Check
🔇 Additional comments (45)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (3)
73-78: Non-throwingsetupis fine
setup()does not throw checked exceptions, so droppingthrows Exceptionmatches the implementation and keeps the signature clean.
80-83: Non-throwingshutdownis appropriate
shutdown()body cannot throw checked exceptions; removingthrows Exceptionis correct and consistent with other tests.
85-96: Dispatcher builder usage preserves bad‑request behaviorUsing
TestDispatcherBuilder.withDefaults()with a failingdispatchRequestand customdispatchBadRequestmirrors the old anonymous Dispatcher while centralizing the common wiring; looks good.plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (3)
50-55:setupsignature cleanup is correctNo checked exceptions are thrown here, so removing
throws Exceptionis safe and simplifies the test.
57-60:shutdownsignature cleanup is correctDropping
throws Exceptionmatches the implementation and other tests.
62-73: Refactored dispatcher matches previous bad‑request handlingThe TestDispatcherBuilder configuration (failing normal requests, custom
dispatchBadRequest) keeps the original test intent while reducing boilerplate.plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8)
32-32: ImportingTestDispatcherBuilderaligns secure tests with shared helperPulling in the builder here is consistent with other Netty/Reactor tests and centralizes Dispatcher setup.
174-183:shutdownsignature change is benignThe method only does null checks and shutdowns; removing
throws Exceptionis appropriate.
185-205: Javadoc formatting onlyOnly spacing/blank-line adjustments in Javadoc; behavior is unaffected.
221-224: Expect‑header dispatcher correctly uses builderUsing the builder with a simple OK
BytesRestResponse("done")preserves the original expect/continue behavior while leveraging shared defaults.
318-328: Bad‑request dispatcher mirrors non‑secure variantThe custom
withDispatchBadRequestthat recordscauseand sends a BAD_REQUEST response matches the non-secure Netty test and keeps the assertion that aTooLongFrameExceptionoccurred.
381-390: Large compressed response dispatcher preserved via builderThe lambda guarding on
urland returning a large OK body is unchanged in logic; using the builder just removes anonymous class noise.
425-425: CORS tests still rely on pre‑dispatch handlingUsing
TestDispatcherBuilder.withDefaults().build()here keeps the “no application handler should be hit” invariant similar toNullDispatcher; CORS logic remains in the transport layer.
479-481: Read‑timeout test now uses shared dispatcher helperUsing the default builder dispatcher is consistent with other timeout tests and keeps all actual behavior in the transport.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (7)
115-121:setupno longer declaringthrows Exceptionis appropriateMethod body doesn’t throw checked exceptions; signature cleanup is correct.
123-132:shutdowncleanup is safeExplicit shutdown and nulling of fields don’t require a checked exception; updated signature is fine.
173-179: Builder‑based dispatcher for expect‑header tests looks goodThe dispatcher sending
"done"viaBytesRestResponsematches prior behavior while using the shared TestDispatcherBuilder.
269-280: Bad‑request dispatcher correctly focuses ondispatchBadRequestOverriding only
withDispatchBadRequestand using the builder defaults elsewhere clearly expresses the test’s intent and keeps the cause capture semantics.
332-341: Large compressed response dispatcher refactor is behavior‑preservingThe
urlguard and large OK body logic are unchanged; moving to the builder simply standardizes Dispatcher construction.
391-393: CORS tests leveraging default dispatcher is consistentUsing
withDefaults().build()here mirrors priorNullDispatcherbehavior: CORS handling occurs before any app dispatch.
445-447: Read‑timeout test now uses shared dispatcher helperDefault dispatcher from TestDispatcherBuilder is sufficient since the test only cares about connection closure; change is straightforward.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (9)
31-31: Shared dispatcher helper imported for secure Netty testsAdding
TestDispatcherBuilderhere brings the secure tests in line with the rest of the Netty test suite.
145-163:setupsignature simplification is correctInitialization does not throw checked exceptions; dropping
throws Exceptionmatches the implementation.
165-174:shutdowncleanup is appropriateThe teardown logic is unchanged; removing
throws Exceptionis safe.
176-207: Javadoc spacing onlyOnly blank-line formatting in the Javadocs changed; no behavioral impact.
218-224: Expect‑header dispatcher correctly refactored to builderThe dispatcher’s behavior (sending
"done"on success) stays the same while using the central TestDispatcherBuilder.
318-328: Bad‑request dispatcher logic preservedUsing
withDispatchBadRequestto send a BAD_REQUEST with a custom message and trackcauseReferencematches the previous anonymous implementation.
381-390: Large compressed response handler via builder is equivalentURL check plus OK response are unchanged; builder usage simply standardizes construction across tests.
425-425: CORS dispatcher viawithDefaults()is consistent with other testsRelying on transport-level CORS logic with a default dispatcher keeps app-level handlers out of the path as before.
479-481: Read‑timeout test uses shared dispatcher patternDefault dispatcher from the builder is sufficient here; behavior remains focused on timeout-induced close.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (9)
109-115:setupsignature cleaned up correctlyNo checked exceptions thrown; removing
throws Exceptionis appropriate.
117-126:shutdownsignature cleaned up correctlyTeardown is unchanged functionally; non-throwing signature is fine.
170-176: Expect‑header dispatcher via builder mirrors Netty variantThe dispatcher responding with
"done"now uses TestDispatcherBuilder, matching the non-Reactor tests and keeping semantics the same.
257-258: Bad‑request test using default dispatcher is intentionalUsing
withDefaults().build()ensures any unexpected dispatch would fail the test, while URI length errors are handled at the transport layer as before.
301-305: Dispatch‑failed test refactor preserves 500 behaviorThe custom
withDispatchRequestthrowingRuntimeExceptionstill drives the INTERNAL_SERVER_ERROR expectations; builder just removes boilerplate.
341-350: Large compressed response dispatcher refactor is behavior‑preservingURL guard and body content checks remain identical; dispatcher is now built via the shared helper.
401-410: Connections‑closed test dispatcher refactor looks goodThe dispatcher still serves only the expected URL and asserts on others; the looped request/response and stats checks remain unchanged.
446-448: CORS test using default dispatcher keeps focus on transport CORS logic
withDefaults().build()continues to ensure no app handler is needed; behavior matches other CORS tests.
500-502: Connect‑timeout test uses shared dispatcher helper correctlyDefault dispatcher suffices; the test still validates timeout via channel closure.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (4)
63-68: Lambda simplification is fineChanging
(str) ->tostr ->is a pure style tweak with no behavioral impact; XCONTENT_CONVERTER remains equivalent.
75-81:setupsignature cleanup is appropriateInitialization logic doesn’t throw checked exceptions; non-throwing signature is correct.
83-92:shutdownsignature cleanup is appropriateTeardown logic is unchanged; removing
throws Exceptionis safe and consistent with other tests.
177-216: Streaming dispatcher refactor preserves behavior and clarifies responsibilitiesUsing TestDispatcherBuilder to:
- advertise a streaming
RestHandlerviawithDispatchHandler(supportsStreaming=true,handleRequestguarded to fail if ever called), and- implement the actual streaming pipeline in
withDispatchRequest(assertingStreamingRestChannel, waiting for writability, emitting chunks on the WRITE executor, and closingbytesOutputifReleasable)keeps the original streaming semantics while centralizing Dispatcher construction. The URL guard and chunking logic remain unchanged.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
24-86: Well-factored test dispatcher helper; defaults and delegation look correctThe builder defaults (logging +
AssertionError) and thebuild()implementation cleanly delegate to the configured lambdas and should make dispatcher setup in tests much clearer. No issues from a correctness or test-behavior perspective.modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
24-86: Dispatcher builder implementation is correct and consistent with reactor-netty4 versionThe builder’s defaults and
build()delegation are sound and match the reactor-netty4 counterpart, giving consistent test behavior across modules. No functional issues spotted.
|
❌ Gradle check result for e980258: 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? |
|
Seems like a flaky test failure |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20160 +/- ##
============================================
+ Coverage 72.82% 73.24% +0.41%
- Complexity 71315 71768 +453
============================================
Files 5795 5795
Lines 328297 328302 +5
Branches 47282 47283 +1
============================================
+ Hits 239089 240459 +1370
+ Misses 69893 68492 -1401
- Partials 19315 19351 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
❌ Gradle check result for 7e8b64f: 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? |
|
❌ Gradle check result for e896324: 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? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
🪛 LanguageTool
CHANGELOG.md
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ontroller with org.opensearch.secure_sm.AccessController in discovery plugins ([#19802](https://...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~31-~31: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....Stats and RemoteSegmentTransferTracker.Stats class to use the Builder pattern instea...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
- GitHub Check: Mend Security Check
|
@reta would you be able to take a look whenever time permits? This change is a tiny bit of polishing of netty-related testing code. Hope it makes it easier to maintain and add more testing further. |
fdesu
force-pushed
the
netty-http-dispatcher-tests-cleanup
branch
3 times, most recently
from
d04ade5 to
fcf389d
Compare
Signed-off-by: Sergei Ustimenko <[email protected]>
Signed-off-by: Sergei Ustimenko <[email protected]>
fdesu
force-pushed
the
netty-http-dispatcher-tests-cleanup
branch
from
fcf389d to
3f4e68b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
49-62: Fail fast on null callbacks (and enforce non-nullOptionalresult).
PassingnulltowithDispatch*will currently fail later with a less actionable NPE; also consider guarding against anullreturn fromDispatchHandler.+import java.util.Objects; @@ public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) { - this.dispatchRequest = dispatchRequest; + this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest"); return this; } @@ public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) { - this.dispatchBadRequest = dispatchBadRequest; + this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest"); return this; } @@ public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) { - this.dispatchHandler = dispatchHandler; + this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler"); return this; } @@ ) { - return dispatchHandler.dispatchHandler(uri, rawPath, method, params); + return Objects.requireNonNull( + dispatchHandler.dispatchHandler(uri, rawPath, method, params), + "dispatchHandler returned null" + ); }Also applies to: 98-101
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (2)
49-62: Fail fast on null callbacks (and enforce non-nullOptionalresult).
Same recommendation as the reactor-netty4 variant.+import java.util.Objects; @@ public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) { - this.dispatchRequest = dispatchRequest; + this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest"); return this; } @@ public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) { - this.dispatchBadRequest = dispatchBadRequest; + this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest"); return this; } @@ public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) { - this.dispatchHandler = dispatchHandler; + this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler"); return this; } @@ ) { - return dispatchHandler.dispatchHandler(uri, rawPath, method, params); + return Objects.requireNonNull( + dispatchHandler.dispatchHandler(uri, rawPath, method, params), + "dispatchHandler returned null" + ); }Also applies to: 98-101
24-103: Two near-identical builders: consider a shared test-fixture to prevent drift.
If these must stay duplicated due to module boundaries, it may still be worth a short comment in both files pointing to the “paired” implementation to keep changes synchronized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java(6 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java(11 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java(3 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java(8 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
📚 Learning: 2025-12-12T18:40:01.303Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
📚 Learning: 2025-12-12T13:31:41.214Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
🧬 Code graph analysis (6)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~14-~14: Use a hyphen to join words.
Context: .../pull/19539)) - Add a mapper for context aware segments grouping criteria ([#1923...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~16-~16: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...568)) - Add support for repository with Server side encryption enabled and client side encr...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...erver side encryption enabled and client side encryption as well based on a flag....
(QB_NEW_EN_HYPHEN)
[uncategorized] ~33-~33: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...enSearch/pull/19878)) - Add support for context aware segments ([#19098](https://github.com/o...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ontroller with org.opensearch.secure_sm.AccessController in discovery plugins ([#19802](https://...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~67-~67: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....Stats and RemoteSegmentTransferTracker.Stats class to use the Builder pattern instea...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~116-~116: The official name of this software platform is spelled with a capital “H”.
Context: ...-project/OpenSearch/pull/19692)) - Bump com.github.spotbugs:spotbugs-annotations from 4.9...
(GITHUB)
[uncategorized] ~119-~119: The official name of this software platform is spelled with a capital “H”.
Context: ...-project/OpenSearch/pull/19763)) - Bump github/codeql-action from 3 to 4 ([#19785](ht...
(GITHUB)
⏰ 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). (17)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
🔇 Additional comments (17)
CHANGELOG.md (1)
81-81: LGTM - Test infrastructure improvement appropriately documented.The CHANGELOG entry accurately describes the test cleanup. While a previous review questioned whether test-only changes should appear in the main CHANGELOG, documenting test infrastructure improvements can help maintainers understand the evolution of the test suite.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)
74-83: LGTM - Lifecycle methods cleaned up.Removing
throws Exceptionfrom setup() and shutdown() is a good cleanup since these methods don't throw checked exceptions.
86-96: LGTM - Excellent refactoring with TestDispatcherBuilder.The migration from anonymous dispatcher to the builder pattern improves maintainability and provides consistent default behaviors across tests. The lambda-based dispatch configuration is cleaner and more expressive than the previous anonymous class approach.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (2)
51-60: LGTM - Lifecycle methods cleaned up.Consistent with the changes in the netty4 module, removing unnecessary exception declarations.
63-73: LGTM - Builder pattern applied consistently.The TestDispatcherBuilder is applied consistently across both netty4 and reactor-netty4 test suites, promoting uniformity in test infrastructure.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (3)
146-174: LGTM - Lifecycle cleanup in secure transport tests.Consistent removal of exception declarations from setup() and shutdown() methods.
218-224: LGTM - Builder pattern improves test readability.The multiline lambda format in
withDispatchRequestmakes the test intent clearer compared to nested anonymous classes. The dispatcher immediately shows it will send a simple "done" response.
318-328: LGTM - Consistent builder usage across test methods.All test methods now use the same TestDispatcherBuilder pattern, making the test suite more uniform and easier to maintain.
Also applies to: 381-390
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)
175-183: LGTM - Appropriate exception handling in lifecycle methods.Note that setup() correctly retains
throws Exception(line 109) because KeyStoreUtils.createServerKeyStore() can throw checked exceptions. Only shutdown() has the exception declaration removed, which is correct.
223-229: LGTM - Builder pattern applied consistently across reactor-netty4 secure tests.The TestDispatcherBuilder integration is consistent across all test methods, with appropriate customization per test scenario (basic dispatching, bad requests, exception handling, streaming).
Also applies to: 307-307, 353-357
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (2)
116-132: LGTM - Clean lifecycle methods.Consistent with other test files in removing unnecessary exception declarations.
173-179: LGTM - Builder pattern enhances test suite uniformity.The TestDispatcherBuilder creates a consistent testing pattern across the entire netty4 test suite. Tests are now easier to understand and maintain, with clear separation between default behaviors and test-specific customizations.
Also applies to: 270-280, 332-341
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (2)
110-126: LGTM - Lifecycle cleanup in reactor transport tests.Consistent application of the lifecycle improvements.
170-176: LGTM - Comprehensive builder adoption across reactor-netty4 tests.All test scenarios (expect headers, bad requests, dispatch failures, large responses, CORS, timeouts) now use the TestDispatcherBuilder, creating a unified test infrastructure across both netty4 and reactor-netty4 modules.
Also applies to: 257-257, 301-305, 341-350
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (2)
76-92: LGTM - Lifecycle cleanup in streaming tests.Consistent with all other test files.
178-216: LGTM - Builder pattern handles complex streaming scenarios elegantly.The TestDispatcherBuilder successfully accommodates the more complex streaming test scenario by allowing customization of both
dispatchHandler(to return a streaming-capable RestHandler) anddispatchRequest(to handle the actual streaming logic). This demonstrates the flexibility of the builder approach while maintaining clear test structure.The streaming logic itself (Flux concatenation, channel writability checks, thread pool usage) remains unchanged and correct.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
24-103: Clean, consistent test helper; defaults are sensible.
Centralizing “unexpected request” logging + assertion failure should make test intent and failures clearer.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
Show resolved
Hide resolved
...t-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
CHANGELOG.md (1)
13-48: Fix small changelog text issues + reconsider test-only entry placement.
- Line 13: missing space before the PR link:
sub aggregation([#19573]...)→sub aggregation ([#19573]...).- Line 17: “s3” → “S3” for consistency with common acronym casing.
- Line 42:
IndexPressutreStatslooks like a typo (likelyIndexPressureStats); please confirm and correct.- Lines 23 / 26: trailing periods are inconsistent with most bullets—worth normalizing in this block.
- Line 48: since this is a test-only cleanup, please double-check whether it belongs in the top-level “Changed” release notes per project convention, or should be omitted/moved to a testing/dev-notes section (if you have one).
- - Combining filter rewrite and skip list to optimize sub aggregation([#19573](https://github.com/opensearch-project/OpenSearch/pull/19573)) + - Combining filter rewrite and skip list to optimize sub aggregation ([#19573](https://github.com/opensearch-project/OpenSearch/pull/19573)) ... - - Onboarding new maven snapshots publishing to s3 ([#19619](https://github.com/opensearch-project/OpenSearch/pull/19619)) + - Onboarding new maven snapshots publishing to S3 ([#19619](https://github.com/opensearch-project/OpenSearch/pull/19619)) ... - - Refactor the IndexPressutreStats, DeviceStats and TransportStats class to use the Builder pattern instead of constructors ([#19991](https://github.com/opensearch-project/OpenSearch/pull/19991)) + - Refactor the IndexPressureStats, DeviceStats and TransportStats class to use the Builder pattern instead of constructors ([#19991](https://github.com/opensearch-project/OpenSearch/pull/19991))plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
64-86: Snapshot builder state inbuild()to prevent cross-test mutation.The anonymous dispatcher references the builder's mutable fields directly. If a builder is reused or mutated after
build(), previously built dispatchers will change behavior, potentially causing flaky tests.Apply this fix to capture the current state:
public HttpServerTransport.Dispatcher build() { + final DispatchRequest dispatchRequest = this.dispatchRequest; + final DispatchBadRequest dispatchBadRequest = this.dispatchBadRequest; + final DispatchHandler dispatchHandler = this.dispatchHandler; return new HttpServerTransport.Dispatcher() { @Override public Optional<RestHandler> dispatchHandler( String uri, String rawPath, RestRequest.Method method, Map<String, String> params ) { return dispatchHandler.dispatchHandler(uri, rawPath, method, params); } @Override public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) { dispatchRequest.dispatchRequest(request, channel, threadContext); } @Override public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) { dispatchBadRequest.dispatchBadRequest(channel, threadContext, cause); } }; }Note: This same issue exists in the Netty4 variant at
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(lines 64-86) and should be fixed there as well.modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
32-85: Snapshot builder state inbuild()to avoid surprising cross-test mutation/flakes.The returned
Dispatchercloses over mutable builder fields (dispatchHandler,dispatchRequest,dispatchBadRequest). If a test reuses the builder and mutates it afterbuild(), previously built dispatchers can silently change behavior.public HttpServerTransport.Dispatcher build() { + final DispatchRequest dispatchRequest = this.dispatchRequest; + final DispatchBadRequest dispatchBadRequest = this.dispatchBadRequest; + final DispatchHandler dispatchHandler = this.dispatchHandler; return new HttpServerTransport.Dispatcher() { @Override public Optional<RestHandler> dispatchHandler( String uri, String rawPath, RestRequest.Method method, Map<String, String> params ) { return dispatchHandler.dispatchHandler(uri, rawPath, method, params); } @Override public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) { dispatchRequest.dispatchRequest(request, channel, threadContext); } @Override public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) { dispatchBadRequest.dispatchBadRequest(channel, threadContext, cause); } }; }
🧹 Nitpick comments (2)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)
63-68: LGTM! Minor lambda syntax cleanup.Removing unnecessary parentheses around single lambda parameter improves consistency with Java conventions.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
49-62: Fail fast on null callbacks inwithDispatch*setters.Today
withDispatchRequest(null)(etc.) will defer failure to dispatch time via NPE, which is harder to debug in tests.+import java.util.Objects; ... public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) { - this.dispatchRequest = dispatchRequest; + this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest"); return this; } public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) { - this.dispatchBadRequest = dispatchBadRequest; + this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest"); return this; } public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) { - this.dispatchHandler = dispatchHandler; + this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler"); return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java(6 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(1 hunks)modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java(11 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java(3 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java(8 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
📚 Learning: 2025-12-12T18:40:01.303Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
📚 Learning: 2025-12-12T13:31:41.214Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
🧬 Code graph analysis (7)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-103)
🪛 LanguageTool
CHANGELOG.md
[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ontroller with org.opensearch.secure_sm.AccessController in discovery plugins ([#19802](https://...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~33-~33: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....Stats and RemoteSegmentTransferTracker.Stats class to use the Builder pattern instea...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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). (19)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
🔇 Additional comments (17)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)
74-83: LGTM! Lifecycle method signatures simplified.Removing
throws Exceptionfrom setup() and shutdown() is appropriate since neither method throws checked exceptions.
86-96: LGTM! Builder pattern improves test readability.The refactor from anonymous dispatcher to TestDispatcherBuilder makes the test setup clearer and more maintainable while preserving the original test behavior.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (2)
51-60: LGTM! Consistent lifecycle method cleanup.The removal of
throws Exceptionis consistent with the pattern applied across all test files in this PR.
63-73: LGTM! Builder pattern applied consistently.The TestDispatcherBuilder usage mirrors the implementation in the Netty4 module, maintaining consistency across the codebase.
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (3)
175-183: LGTM! Lifecycle cleanup.Removing
throwsclause is safe and appropriate.
221-229: LGTM! Dispatcher construction improved.The builder pattern consolidates dispatcher setup, making the test helper method cleaner and easier to understand.
307-307: LGTM! Consistent builder adoption across test methods.All test methods now use TestDispatcherBuilder consistently, improving maintainability while preserving test coverage.
Also applies to: 353-357, 394-403, 453-453, 508-508
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (2)
116-132: LGTM! Lifecycle method cleanup.Consistent with the pattern applied across all test files.
173-179: LGTM! Comprehensive builder adoption.All test methods in this class now consistently use the TestDispatcherBuilder pattern, improving code quality without affecting test coverage.
Also applies to: 270-280, 332-341, 392-392, 446-446
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (2)
146-174: LGTM! Lifecycle and dispatcher construction improvements.The changes are consistent with the pattern applied across all other test files, improving maintainability.
Also applies to: 218-224
318-328: LGTM! Builder pattern applied consistently.All test methods now use TestDispatcherBuilder, maintaining consistency across the secure Netty4 transport tests.
Also applies to: 381-390, 425-425, 480-480
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (2)
110-126: LGTM! Lifecycle method cleanup.Consistent with the refactoring pattern across all test files.
170-176: LGTM! Comprehensive builder adoption across all test methods.The TestDispatcherBuilder is now used consistently throughout this test class, improving readability and maintainability.
Also applies to: 257-257, 301-305, 341-350, 401-410, 447-447, 501-501
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (2)
76-92: LGTM! Lifecycle method cleanup.Consistent with the pattern applied across all test files.
178-216: LGTM! Builder pattern applied to streaming dispatcher.The refactor successfully preserves the complex streaming logic while adopting the builder pattern for consistency. The streaming-specific handler is properly configured via
withDispatchHandler()andwithDispatchRequest().plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (2)
28-62: LGTM! Clean builder API with sensible test defaults.The fluent API provides an intuitive way to configure dispatchers for tests. The default implementations (logging + AssertionError) are appropriate for catching unexpected behavior in tests.
88-101: LGTM! Clean functional interfaces.The three interfaces provide clear contracts for customizing dispatcher behavior. Using
@FunctionalInterfaceis appropriate.
Signed-off-by: Sergei Ustimenko <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
📚 Learning: 2025-12-12T18:40:01.303Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
📚 Learning: 2025-12-12T13:31:41.214Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.javamodules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
🧬 Code graph analysis (2)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-106)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-106)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
🔇 Additional comments (4)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (4)
1-22: LGTM!License header, package declaration, and imports are correct and properly organized.
24-43: Well-designed fail-fast defaults for test utilities.The default behaviors appropriately surface unexpected dispatch calls with detailed logging and immediate assertion failures, which is ideal for catching test setup issues early.
45-62: LGTM!Clean fluent builder API that follows standard patterns.
91-106: LGTM!Clean functional interface definitions that enable concise lambda-based test configurations.
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
Outdated
Show resolved
Hide resolved
| public HttpServerTransport.Dispatcher build() { | ||
| DispatchRequest builderDispatchRequest = this.dispatchRequest; | ||
| DispatchBadRequest buidlerDispatchBadRequest = this.dispatchBadRequest; | ||
| DispatchHandler builderDispatcherHandler = this.dispatchHandler; | ||
| return new HttpServerTransport.Dispatcher() { |
There was a problem hiding this comment.
Typo in variable name: buidlerDispatchBadRequest
Minor typo: buidlerDispatchBadRequest should be builderDispatchBadRequest. Additionally, builderDispatcherHandler is inconsistent with the other variables (has extra "er").
Note: The same typos exist in modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java. Consider fixing both for consistency.
public HttpServerTransport.Dispatcher build() {
DispatchRequest builderDispatchRequest = this.dispatchRequest;
- DispatchBadRequest buidlerDispatchBadRequest = this.dispatchBadRequest;
- DispatchHandler builderDispatcherHandler = this.dispatchHandler;
+ DispatchBadRequest builderDispatchBadRequest = this.dispatchBadRequest;
+ DispatchHandler builderDispatchHandler = this.dispatchHandler;
return new HttpServerTransport.Dispatcher() {Also update the usages at lines 76 and 86:
- return builderDispatcherHandler.dispatchHandler(uri, rawPath, method, params);
+ return builderDispatchHandler.dispatchHandler(uri, rawPath, method, params);- buidlerDispatchBadRequest.dispatchBadRequest(channel, threadContext, cause);
+ builderDispatchBadRequest.dispatchBadRequest(channel, threadContext, cause);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
around lines 64 to 68, there are inconsistent variable names: change
"buidlerDispatchBadRequest" to "builderDispatchBadRequest" and rename
"builderDispatcherHandler" to "builderDispatchHandler" to match the pattern used
for other locals; update all subsequent usages at lines 76 and 86 accordingly.
Also apply the same fixes in
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
to keep both files consistent.
|
❌ Gradle check result for 084ab2f: 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? |
Thanks @fdesu , quick question, beside just refactoring (or polishing as you said), any particular problem(s) you run into and this change helps you to address? Yes, the test cases are relying on quite a few duplicated pieces, but I would generally lean towards "we had/have a problem, refactoring the test case would help us the not have this problem anymore". Thank you. |
|
Seems like a failure in spotless in unrelated module, I guess rerunning CI should do the trick |
|
@reta fair question, there was no issue with running tests or failures of any sorts. While adding additional testing in another PR, I was going through existing test suites to see how it’s done and had just a tiny bit of trouble to distinguish the setup code/preconditions from actual code under test. So, my hope here was - with clear naming, concise lambdas and centralized DSL to highlight it very clearly - this is setup code. Also I wanted to remove duplications and refactor a bit the visual noise to help focus on code under test. So, this PR is really about reducing mental complexity and helping to focus on the code we’re currently testing. |
Signed-off-by: Sergei Ustimenko <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
64-88: Make captured localsfinal+ fixbuilderDispatcherHandlernaming.
The snapshot approach is great;finalimproves intent. The handler local name typo has been flagged before.public HttpServerTransport.Dispatcher build() { - DispatchRequest builderDispatchRequest = this.dispatchRequest; - DispatchBadRequest builderDispatchBadRequest = this.dispatchBadRequest; - DispatchHandler builderDispatcherHandler = this.dispatchHandler; + final DispatchRequest builderDispatchRequest = this.dispatchRequest; + final DispatchBadRequest builderDispatchBadRequest = this.dispatchBadRequest; + final DispatchHandler builderDispatchHandler = this.dispatchHandler; return new HttpServerTransport.Dispatcher() { @@ ) { - return builderDispatcherHandler.dispatchHandler(uri, rawPath, method, params); + return builderDispatchHandler.dispatchHandler(uri, rawPath, method, params); }plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
67-67: Fix typo:builderDispatcherHandler→builderDispatchHandler.The variable name
builderDispatcherHandleron line 67 contains an extra "er" and is inconsistent with the naming pattern used forbuilderDispatchRequestandbuilderDispatchBadRequest. This was flagged in a previous review but remains unfixed.Apply this diff to correct the variable name:
- DispatchHandler builderDispatcherHandler = this.dispatchHandler; + DispatchHandler builderDispatchHandler = this.dispatchHandler;Also update the usage at line 76:
- return builderDispatcherHandler.dispatchHandler(uri, rawPath, method, params); + return builderDispatchHandler.dispatchHandler(uri, rawPath, method, params);Note: The same typo exists in
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaand should be fixed there as well for consistency.
🧹 Nitpick comments (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
49-62: Fail fast on null callbacks inwithDispatch*(clearer test failures).+import java.util.Objects; @@ public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) { - this.dispatchRequest = dispatchRequest; + this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest"); return this; } @@ public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) { - this.dispatchBadRequest = dispatchBadRequest; + this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest"); return this; } @@ public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) { - this.dispatchHandler = dispatchHandler; + this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler"); return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java(1 hunks)plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
📚 Learning: 2025-12-12T13:31:41.214Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
📚 Learning: 2025-12-12T18:40:01.303Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.javaplugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
🧬 Code graph analysis (2)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-106)
plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
TestDispatcherBuilder(28-106)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)
91-104: Functional interfaces look good and appropriately minimal for tests.
| private DispatchBadRequest dispatchBadRequest = (channel, threadContext, cause) -> { | ||
| logger.error( | ||
| new ParameterizedMessage("--> Unexpected bad request [{}]", FakeRestRequest.requestToString(channel.request())), | ||
| cause | ||
| ); | ||
| throw new AssertionError(cause); | ||
| }; |
There was a problem hiding this comment.
Guard against possible null channel.request() in default dispatchBadRequest logging.
If RestChannel.request() can be null on “bad request” paths, the logger line will NPE and hide the original failure.
private DispatchBadRequest dispatchBadRequest = (channel, threadContext, cause) -> {
+ final var req = channel.request();
logger.error(
- new ParameterizedMessage("--> Unexpected bad request [{}]", FakeRestRequest.requestToString(channel.request())),
+ new ParameterizedMessage(
+ "--> Unexpected bad request [{}]",
+ req == null ? "<null request>" : FakeRestRequest.requestToString(req)
+ ),
cause
);
throw new AssertionError(cause);
};🤖 Prompt for AI Agents
In
modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
around lines 36 to 42, the default dispatchBadRequest logger calls
FakeRestRequest.requestToString(channel.request()) which can NPE if
channel.request() is null; update the logging to guard against a null request
(e.g., compute a safeRequest string by checking channel.request() != null or
using a null-safe toString default) and pass that safeRequest into the
ParameterizedMessage so the logger cannot throw an NPE while preserving the
original cause and still throwing the AssertionError(cause).
Thanks @fdesu Have no intention to discourage improvements, @andrross @cwperks do you folks have an opinion? For me personally the cognitive load has increased actually: instead of looking what dispatcher does in every single test, I have to go to the builder and figure out what does dispatcher do, separate class, far from the test case context. |
|
@reta Thanks! Sure, no worries at all, this is very valuable feedback! I was thinking sensible defaults + a lambda is simpler but that's just me so I'm absolutely fine if consensus is to avoid such change. |
2 participants
Description
This PR introduces helper builders
TestDispatcherBuilderin modules/Netty & plugins/Reactor Netty that simplify the way instances ofHttpTransport.Disaptcherget created.It also includes some minor cleanups like remove stale
throwsclause from some of setup/teardown method and such.Related Issues
This is a cleanup PR, no additional issue was created.
Check List
[ ] 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
Refactor
Performance & Compatibility
Tests
Chore
✏️ Tip: You can customize this high-level summary in your review settings.