feat(http): switch default http transport to jdk & enhance jdk http transport#445
feat(http): switch default http transport to jdk & enhance jdk http transport#445AlbumenJ merged 3 commits intoagentscope-ai:mainfrom
Conversation
…ransport Change-Id: I502d8ecd96642f41057b1d92e4b2995cca26ed54
There was a problem hiding this comment.
Pull request overview
This PR switches the default HTTP transport implementation from OkHttp to JDK's built-in HttpClient, eliminating external dependencies and improving maintainability. The changes include enhanced error handling for streaming responses, SSL certificate verification bypass support for testing, and improved resource management using Reactor's Flux.using pattern.
Key Changes
- Changed default HttpTransport from OkHttpTransport to JdkHttpTransport in HttpTransportFactory
- Added SSL certificate verification bypass capability with
ignoreSslconfiguration option - Refactored streaming error handling to read error response bodies immediately before stream closure
- Reduced constructor visibility to package-private, encouraging use of builder pattern
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
agentscope-distribution/agentscope-all/pom.xml |
Reordered dependencies for better organization (no functional changes) |
agentscope-core/src/main/java/io/agentscope/core/model/transport/HttpTransportFactory.java |
Updated default transport from OkHttpTransport to JdkHttpTransport with corresponding documentation |
agentscope-core/src/main/java/io/agentscope/core/model/transport/JdkHttpTransport.java |
Added SSL bypass support, refactored streaming with better error handling, changed constructor visibility, made buildClient static |
agentscope-core/src/test/java/io/agentscope/core/model/transport/JdkHttpTransportTest.java |
Added comprehensive tests for SSL ignore feature and streaming error body handling across different HTTP status codes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static HttpClient buildClient(HttpTransportConfig config) { | ||
| HttpClient.Builder builder = | ||
| HttpClient.newBuilder() | ||
| .version(Version.HTTP_2) | ||
| .followRedirects(Redirect.NORMAL) | ||
| .connectTimeout(config.getConnectTimeout()); | ||
|
|
||
| if (config.isIgnoreSsl()) { | ||
| try { | ||
| SSLContext sslContext = SSLContext.getInstance("TLS"); | ||
| sslContext.init( | ||
| null, new TrustManager[] {new TrustAllManager()}, new SecureRandom()); | ||
| builder.sslContext(sslContext); | ||
| log.warn( | ||
| "SSL certificate verification is disabled. " | ||
| + "This should only be used for testing."); | ||
| } catch (NoSuchAlgorithmException | KeyManagementException e) { | ||
| throw new HttpTransportException("Failed to create insecure SSL context", e); | ||
| } | ||
| } | ||
|
|
||
| return builder.build(); | ||
| } |
There was a problem hiding this comment.
The buildClient method has been changed from instance method to static method. While this is generally a good practice since the method doesn't use instance state, ensure this doesn't break any potential mocking or testing scenarios that relied on the instance method behavior. The change appears safe since the method only depends on the config parameter.
| String errorBody = readInputStream(response.body()); | ||
| log.warn( | ||
| "HTTP request failed. URL: {} | Status: {} | Error:" | ||
| + " {}", | ||
| request.getUrl(), | ||
| statusCode, | ||
| errorBody); | ||
| throw new CompletionException( | ||
| new HttpTransportException( | ||
| "HTTP request failed with status " | ||
| + statusCode | ||
| + " | " | ||
| + errorBody, | ||
| statusCode, | ||
| errorBody)); |
There was a problem hiding this comment.
In the stream error handling, when an IOException occurs while reading the error body in readInputStream (line 323), the method catches the exception and returns null. This means if the error body cannot be read, the HttpTransportException will contain null as the error body. Consider whether it would be better to include a placeholder message like "(error body could not be read)" or rethrow the exception to properly surface the I/O error.
| private static class TrustAllManager implements X509TrustManager { | ||
| @Override | ||
| public void checkClientTrusted(X509Certificate[] chain, String authType) { | ||
| // Trust all client certificates | ||
| } | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(X509Certificate[] chain, String authType) { | ||
| // Trust all server certificates | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getAcceptedIssuers() { | ||
| return new X509Certificate[0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
The TrustAllManager implementation disables SSL certificate verification. While the Javadoc includes a warning, this is a significant security risk. Consider adding a runtime warning log when this is used, and ensure that the documentation clearly explains when this should be used (e.g., only in development/testing environments with trusted self-signed certificates). The warning log on line 117-119 is good, but it would be helpful to also include the URL being accessed in the log message for debugging purposes.
| synchronized (lock) { | ||
| if (defaultTransport == null) { | ||
| defaultTransport = new OkHttpTransport(); | ||
| defaultTransport = new JdkHttpTransport(); |
There was a problem hiding this comment.
Potential race condition. This assignment to defaultTransport is visible to other threads before the subsequent statements are executed.
…ransport Change-Id: I16c5458efee05d411be87179804fcfe1f0b9b0e0
…ransport Change-Id: I62798a7bda442da1bca704e312e4a3baba98de00
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Change-Id: I502d8ecd96642f41057b1d92e4b2995cca26ed54
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.5, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)