Migrate from Apache HttpClient to JDK 11 HttpClient #257
Conversation
- Removed Apache HttpClient dependencies from pom.xml - Rewrote ProxyServlet to use java.net.http.HttpClient - Updated URITemplateProxyServlet to remove Apache HTTP dependencies - Updated OSGI manifest imports - Code compiles successfully Co-authored-by: dsmiley <[email protected]>
- Replaced LocalTestServer with Jetty Server in ProxyServletTest - Created RequestInfoServlet to replace RequestInfoHandler - Updated test methods to use JDK HttpClient instead of Apache HttpClient - Fixed ChunkedTransferTest to use JDK HttpClient - Fixed AcceptEncodingTest to use JDK HttpClient - Fixed ModifyHeadersProxyServletTest for new API - Fixed URITemplateProxyServletTest references Note: Some ProxyServletTest methods still need refactoring for dynamic servlet registration - they currently don't compile. Co-authored-by: dsmiley <[email protected]>
- Added replaceTargetServlet() helper method for test customization - Fixed testHopByHopHeadersOnSource - Fixed testWithExistingXForwardedFor - Fixed testEnabledXForwardedFor Remaining work: ~11 more tests need similar conversion Co-authored-by: dsmiley <[email protected]>
- Fixed all remaining localTestServer.register calls - Converted all tests to use replaceTargetServlet() or inline servlets - Fixed all Apache HttpClient API references (HttpStatus, HttpHeaders, etc.) - Tests now compile successfully - 72 tests run: 56 pass, 9 fail, 7 errors, 2 skipped Most tests passing! Remaining failures likely need minor tweaks to handle differences between Apache HttpClient and JDK HttpClient behavior. Co-authored-by: dsmiley <[email protected]>
- Removed references to Apache HttpClient dependency - Updated documentation to state proxy is now dependency-free - Kept reference to previous Apache HttpClient versions for historical context Co-authored-by: dsmiley <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Perhaps this should be accomplished in two phases (commits/PRs): first make the tests free of Apache HttpClient infrastructure -- only update tests & dependencies of such. Then, do the rest. This way, it should be more apparent at the final step what the test impact is. On the other hand... I guess who cares. |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
@copilot Review the public static final String constants of ProxyServlet that start with "P_". If any cannot be implemented anymore, then remove it. Double-check that we interrpet these equivalently with JDK HttpClient as we did Apache HttpClient.
| if (proxyResponse instanceof Closeable) { | ||
| ((Closeable) proxyResponse).close(); | ||
| protected void handleRequestException(HttpRequest proxyRequest, HttpResponse<InputStream> proxyResponse, Exception e) throws ServletException, IOException { | ||
| // Close the response if present |
| String method = servletRequest.getMethod(); | ||
| String proxyRequestUri = rewriteUrlFromRequest(servletRequest); | ||
| HttpRequest proxyRequest; | ||
| //spec: RFC 2616, sec 4.3: either of these two headers signal that there is a message body. |
| "TE", "Trailers", "Transfer-Encoding", "Upgrade" }; | ||
| for (String header : headers) { | ||
| hopByHopHeaders.addHeader(new BasicHeader(header, null)); | ||
| hopByHopHeaders.add(header.toLowerCase()); |
| int read; | ||
| while ((read = is.read(buffer)) != -1) { | ||
| os.write(buffer, 0, read); | ||
| /*- |
There was a problem hiding this comment.
@copilot this comment was useful; keep it. In general, if there's compression, we don't trust is.available().
There was a problem hiding this comment.
Fixed in 9627bc5. Restored the detailed comment explaining the issue with GzipInputStream's available() implementation and why we flush for compressed streams.
- Updated destroy() to check for AutoCloseable and close HttpClient (Java 21+) - Restored valuable comments about protocol version and chunked responses - Restored detailed comment about compression and InputStream.available() - Changed hopByHopHeaders to use Set.of() for immutability - Used Locale.ROOT for all toLowerCase() calls - Optimized copyResponseHeaders to iterate map entries directly - Removed unimplementable P_ constants (P_CONNECTIONREQUESTTIMEOUT, P_MAXCONNECTIONS, P_USESYSTEMPROPERTIES) - Marked related tests as @ignore with explanations - Removed redundant close in handleRequestException (already in finally block) Co-authored-by: dsmiley <[email protected]>
Fixed in 9627bc5. Reviewed all P_ constants. Removed P_CONNECTIONREQUESTTIMEOUT, P_MAXCONNECTIONS, and P_USESYSTEMPROPERTIES as they cannot be implemented with JDK HttpClient. All other constants are properly implemented with equivalent JDK HttpClient functionality. Tests using removed constants have been marked as @ignore with explanations. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
src/main/java/org/mitre/dsmiley/httpproxy/URITemplateProxyServlet.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mitre/dsmiley/httpproxy/URITemplateProxyServlet.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mitre/dsmiley/httpproxy/URITemplateProxyServlet.java
Outdated
Show resolved
Hide resolved
| byte[] body = EntityUtils.toByteArray(entity); | ||
| baos.write(body); | ||
| // Copy request body | ||
| InputStream is = request.getInputStream(); |
src/test/java/org/mitre/dsmiley/httpproxy/ProxyServletTest.java
Outdated
Show resolved
Hide resolved
| response.setHeader(COOKIE_SET_HEADER,"JSESSIONID=1234; path=/;"); | ||
| response.setStatusCode(HttpStatus.SC_MOVED_TEMPORARILY); | ||
|
|
||
| // Stop the target server and restart with redirect servlet |
There was a problem hiding this comment.
@copilot This was made more complex. Can't we use the replaceTargetServlet method you added?
There was a problem hiding this comment.
Fixed in 7fb5af4. Simplified testRedirect to use the replaceTargetServlet helper method, removing the manual server stop/start code.
- Extracted query parsing logic to parseQueryParams method in URITemplateProxyServlet - Changed extractHost from private to protected in ProxyServlet to expose for subclasses - Inlined extractHost call in URITemplateProxyServlet (removed intermediate variable) - Simplified RequestInfoServlet to use transferTo() instead of manual buffer copying - Simplified testRedirect to use replaceTargetServlet helper method - Improved replaceTargetServlet to use delegating servlet pattern (no server restart needed) Co-authored-by: dsmiley <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@girgen Please check out this variant of the servlet. This'll be a new major version. |
Migration Plan: Apache HttpClient to JDK 11 HttpClient
Converting this project to use JDK 11's HttpClient instead of Apache HttpClient to make it dependency-free (except servlet API).
Checklist:
Code Review Changes (Round 1):
Code Review Changes (Round 2):
Status:
✅ All code review feedback addressed
✅ Code compiles successfully
✅ Tests run (57/72 passing, 9 failures, 3 errors, 6 skipped)
✅ Test infrastructure improved with no-restart servlet replacement
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.