-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add HTTP response headers access to OtpHttpClient #6971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev-2.x
Are you sure you want to change the base?
Add HTTP response headers access to OtpHttpClient #6971
Conversation
Enable access to HTTP response headers through the ResponseMapper interface by introducing OtpHttpResponse wrapper class that encapsulates both headers and body content. Key Changes: - Created OtpHttpResponse class with case-insensitive header access - Updated ResponseMapper interface signature from InputStream to OtpHttpResponse - Modified all ResponseMapper implementations to use response.body() - Added comprehensive unit tests for OtpHttpResponse functionality - Enhanced integration tests to verify header access works correctly Benefits: - Enables HTTP caching with ETag and Last-Modified headers - Supports rate limit monitoring via response headers - Allows content type inspection and validation - Facilitates debugging with header access Implementation Details: - OtpHttpResponse provides immutable, case-insensitive header access - Supports multi-value headers (e.g., Set-Cookie) - InputStream lifecycle remains managed by OtpHttpClient - Breaking change: All ResponseMapper implementations require update Files Modified: - 13 source files updated to use response.body() - 2 test files updated - 1 integration test enhanced with header access tests All existing tests pass. No functional changes to HTTP request behavior. Only adds capability to access response headers when needed.
1b1af02
to
ebe3585
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6971 +/- ##
=============================================
+ Coverage 72.13% 72.17% +0.03%
- Complexity 19673 19853 +180
=============================================
Files 2127 2156 +29
Lines 79562 80090 +528
Branches 8041 8089 +48
=============================================
+ Hits 57396 57804 +408
- Misses 19332 19438 +106
- Partials 2834 2848 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a984c7b
to
51a14da
Compare
application/src/main/java/org/opentripplanner/framework/io/OtpHttpClient.java
Outdated
Show resolved
Hide resolved
21114c2
to
e9d6910
Compare
e9d6910
to
38f4063
Compare
If you add the label "Integration Test", then they are run for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a more general strategy for handling response codes. We now have special cases for 204 and 304. I don't think there is a one-size-fits-all solution here and we will need to have different handling of response codes for different situations.
This would be my suggestion:
- By default we fail on anything other than 200
- We expose another api
executeAndMapWithoutValidation()
(or some better name) where you don't get response code validation and then yourResponseMapper
can do whatever it wants depending on the response code. That way we don't need special logic in the OtpHttpClient for 204/304/whatever. This should be feasible now that the mapper gets the whole response.
} | ||
|
||
// Handle NOT_MODIFIED response. | ||
if (response.getEntity() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: you could change this to if (response.getCode() == SC_NOT_MODIFIED)
and move it to before the previous if statement. I think that would make the logic more straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
private static boolean isFailedRequest(org.apache.hc.core5.http.HttpResponse response) { | ||
return ( | ||
(response.getCode() < SC_OK || response.getCode() >= SC_REDIRECTION) && | ||
response.getCode() != SC_NOT_MODIFIED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit inconsistent to add an exception for 304. I think the correct thing in that case would be to treat all 3XX as non-failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that OTP cannot process correctly 3XX responses, apart from 304.
I don't think we have support for redirection for example.
response.getHeaders(), | ||
response.getCode() | ||
); | ||
return contentMapper.apply(httpResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think contentMapper
should be renamed responseMapper
now. For all places in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
We can discuss this in the developer meeting. |
Summary
Enable access to HTTP response headers through the ResponseMapper interface by introducing OtpHttpResponse wrapper class that encapsulates the body content, headers and status code.
This is required for example to implement support for e-tag.
Note: The integration tests use
com.sun.net.httpserver.HttpServer
for emulating a simple HTTP server.Key Changes:
Benefits:
Implementation Details:
Issue
No
Unit tests
Added unit tests
Documentation
No