jp-fake-auth-fix #115
jp-fake-auth-fix #115
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes HTTP client port handling issues in Vert.x 4 when making requests to URLs with non-standard ports. The change addresses a problem where HttpClient.request(method, url) doesn't properly parse port numbers from URLs, causing connection failures to services running on non-standard ports.
Changes:
- Updated
FakeAuthenticatorandOidcKeycloakAuthenticatorto useRequestOptionswithsetAbsoluteURI()instead of passing URL strings directly toHttpClient.request() - Added comprehensive test suite demonstrating the port parsing issue and validating the fix
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| FakeAuthenticator.java | Modified HTTP client request to use RequestOptions with setAbsoluteURI for proper port parsing |
| OidcKeycloakAuthenticator.java | Modified HTTP client request to use RequestOptions with setAbsoluteURI for proper port parsing |
| FakeAuthenticatorHttpClientTest.java | Added new test class with two tests validating HTTP client port handling with non-standard ports |
| private static final Logger log = LoggerFactory.getLogger(FakeAuthenticatorHttpClientTest.class); | ||
| private Vertx vertx; | ||
| private HttpClient client; | ||
| private int port = 8899; // Non-standard port to test port handling |
There was a problem hiding this comment.
The hardcoded port number may cause test failures if port 8899 is already in use. Consider using a dynamic port assignment (port 0) and retrieving the actual port from the server after it starts, which is a common pattern in Vert.x testing.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
src/test/java/com/github/susom/vertx/base/test/FakeAuthenticatorHttpClientTest.java
Outdated
Show resolved
Hide resolved
| } else if (response2.statusCode() == 302 || response2.statusCode() == 200) { | ||
| log.info("Test passed: Token endpoint was successfully reached"); | ||
| async.complete(); | ||
| } else { | ||
| // Some other error, but not a port issue | ||
| log.info("Got response code {} which indicates the port was reached", | ||
| response2.statusCode()); | ||
| async.complete(); | ||
| } |
There was a problem hiding this comment.
The test completes successfully for any response code other than a connection refused error, including error codes that might indicate actual problems (e.g., 400, 404). The test should have more specific assertions about what constitutes a successful outcome rather than accepting any response that reaches the server.
…orHttpClientTest.java Co-authored-by: Copilot <[email protected]>
Change in handling of URIs by HttpClient.