KSQL-15000 | Address misc items for ksql - #11057
Open
Vedarth Sharma (VedarthConfluent) wants to merge 6 commits into
Open
KSQL-15000 | Address misc items for ksql#11057Vedarth Sharma (VedarthConfluent) wants to merge 6 commits into
Vedarth Sharma (VedarthConfluent) wants to merge 6 commits into
Conversation
…er.fail when login service start fails JaasAuthProvider.getUser caught exceptions from login.start() and failed the promise, but did not return. Execution fell through to login.login(...) on an unstarted JAASLoginService — which can throw further (often NPE on internal state) or, depending on Jetty's behavior, authenticate against stale or default state, after the promise has already been marked as failed. Add an explicit return inside the catch so the request is rejected cleanly without touching the unstarted service. Add a regression test that mocks loginService.start() to throw and asserts that: 1. the user handler receives the expected failure message, and 2. loginService.login(...) is never invoked. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…Exceptionally in ClientImpl.makeRequest The httpClient.request callback called cf.completeExceptionally(ar.cause()) when ar.failed(), but did not return. Execution then ran ar.result() — which is null when ar.failed() — producing an NPE on the next line's request.response(...) call. The same anti-pattern exists at the response.failed() branch a few lines below: cf.completeExceptionally without return, then responseHandler.handle(response.result()) on null. In both branches the NPE escapes the lambda into Vert.x's event loop and is logged as an "unhandled exception" while the original cause is silently discarded — callers receive the NPE, not the real network error, masking transient connection issues during debugging. Add explicit return after each completeExceptionally so the lambda short-circuits without touching the null result. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…text.fail on unsupported HTTP version
QueryStreamHandler.handle rejected requests on HTTP versions other
than 1.1 and 2 with routingContext.fail(BAD_REQUEST, ...), but did
not return. Execution then fell through to getRequest(routingContext)
and endpoints.createQueryPublisher(...) — meaning a query plan was
allocated and started asynchronously on the worker pool against a
routing context whose failure had already been dispatched. The
eventual response would attempt to write into a context that had
already been failed, surfacing as IllegalStateException("Response
head already written") in the worker logs, while the started query
was orphaned (no subscriber, no close path).
Add explicit return after the fail() so the handler stops processing
the rejected request immediately.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
…asAuthProviderTest setUp The regression test added in the previous commit (shouldNotAttemptLoginWhenLoginServiceStartThrows) verifies that the fix to JaasAuthProvider.getUser correctly returns before loginService.login(...) is invoked. Under MockitoJUnitRunner's default STRICT_STUBS policy, the login/getSubject stubs configured in @before setUp are then reported as UnnecessaryStubbing for that specific test - failing the build even though every other test in the class exercises them. Wrap those two stubs with lenient() so the regression test no longer trips the strict-stub check while the rest of the suite continues to verify them implicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
… tests for missing-return fixes ClientImplTest.shouldCompleteExceptionallyWithoutNpeWhenTransportFails - Drives streamQuery against a mocked HttpClient whose request callback yields a failed AsyncResult. Before the fix, the callback called cf.completeExceptionally(ar.cause()) then fell through to ar.result() (null), NPE-ing on the next request.response(...) invocation. The caller received an unhelpful NPE while the real transport error was logged as "unhandled exception" by Vert.x. The test verifies the future is now completed exceptionally with the original cause - not the masking NPE. QueryStreamHandlerTest.shouldNotCreatePublisherOnUnsupportedHttpVersion - Drives handle() with HttpVersion.HTTP_1_0 (neither HTTP/1.1 nor 2) and asserts that endpoints.createQueryPublisher is never invoked and publisher.subscribe is never called. Before the fix, handle() failed the routing context but fell through into createQueryPublisher, starting a query against an already-failed context that later tripped "Response head already written" while the started query was orphaned. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…re regression test that hung CI The shouldCompleteExceptionallyWithoutNpeWhenTransportFails test added in the previous commit drove client.streamQuery() with a mocked HttpClient whose request callback yielded a failed AsyncResult, then awaited future.get() to verify the future completed with the transport cause. The hidden problem: streamQuery() returns the OUTER CompletableFuture<StreamedQueryResult>, while the makeRequest fix operates on an INNER CompletableFuture created inside makeQueryRequest(). When the transport fails, the inner CF completes exceptionally but the outer CF is not driven on that path - so future.get() blocked indefinitely and Semaphore killed the job at the 4-hour execution-time limit. Drop the test and leave a comment so a future contributor doesn't reintroduce the same shape. The fix itself is correct - a proper regression test needs to call the private makeRequest directly, which requires a test-visibility refactor that is out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What behavior do you want to change, why, how does your patch achieve the changes?
Address misc items for ksql
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist