feat: Add ODataClient for Apache HttpClient5#1169
Open
Jonas-Isr wants to merge 26 commits into
Open
Conversation
…-- all tests green
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
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.
Context
SAP/cloud-sdk-java-backlog#504
This PR adds a new module to the Cloud SDK:
datamodel/odata-client-apache-httpclient5. This aims to enable users to use Apaches HttpClient v5 in conjunction with our OData functionalities. I tried to stick as closely to the v4 implementation as possible while respecting the design choices of the new HttpClient v5.This PR does not consider the high-level OData v2 and v4 modules. This will be handled in the future.
Comments regarding review
The first commit of this branch only features the copying of the v4 version of the module. If you want to see the changes that are made compared to our v4 version, look at the changes between the current state of the branch compared to the state after the first commit.
Currently, nothing is marked beta. It might be reasonable to mark (parts of) the new code as such. I am happy to discuss this in the review.
I am also happy to do pair reviews.
Feature scope:
Design decisions
CSRF Token Handling
HC4:
ODataRequestGeneric.tryExecuteWithCsrfToken()issued a HEAD request to the service path before every mutating request, using DefaultCsrfTokenRetriever.HC5: Moved to
CsrfTokenInterceptor, anHttpRequestInterceptorregistered inDefaultApacheHttpClient5Factory. It intercepts mutating requests at the HTTP client level before they are sent. Thus, all calls withODataRequestGeneric.tryExecute()use it.tryExecuteWithCsrfToken()therefor does not exist. (This is a breaking change for users that update to HC5. There is of course also the option to keep the method and simply forward this totryExecutebut I felt this is even more confusing since then it looks like the two methods should do different things. Happy to discuss this.)Why: HC5's HttpClient interface no longer exposes an easy way to issue a pre-request side-effect at the OData layer. The interceptor mechanism is the idiomatic HC5 way to inject cross-cutting behavior. It also means CSRF handling applies to all HTTP clients built by the factory, not just OData requests. If this is not wanted, we can add a flag to the builder of the
DefaultApacheHttpClient5Factoryto make this opt-in/opt-out.Notes:
CsrfTokenRetrievalExceptionthrown. Instead, the problem is only logged (at WARN level). This is a necessary evil since we use HC5's interceptor pattern for CSRF tokens and throwing the exception there is not possible.HTTP Execution Method
HC4: In
httpClient.execute(HttpUriRequest)the client owns the response lifecycle.HC5: In
httpClient.executeOpen(null, httpRequest, null)the caller owns the response lifecycle and is responsible for closing it.Why: The HC5
execute(request, responseHandler)pattern closes the response after the handler returns. Since the OData layer needs to keep the response open for streaming/deserialization,executeOpenis the only way to do this that I saw.Connection Timeout Exception
HC4 uses
ConnectionPoolTimeoutExceptionfrom org.apache.http.conn.HC5: uses
ConnectionRequestTimeoutExceptionfrom org.apache.hc.core5.http.Why: HC5 renamed and restructured exception types. The exception is caught in
ODataHttpRequestto produce a descriptiveODataConnectionExceptionabout connection leaks.Content-Type on JSON Entities
HC4:
ContentType.APPLICATION_JSONhad no charset and thusgetContentType()returned "application/json".HC5:
ContentType.APPLICATION_JSONincludescharset=UTF-8and thisgetContentType()would return "application/json; charset=UTF-8".Fix in
ODataRequestUpdate:ComparableHttpEntitynow usesContentType.create("application/json")(no charset).ODataRequestCreatesets "application/json" as a literal string. Both avoid the unwanted charset suffix that would appear in batch request bodies.UriRequestMerger interface
HC4:
removeDuplicateQueryParameterswas an instance method inODataRequestResultGenericthat casthttpClienttoUriQueryMerger.UriQueryMergerwas implemented byHttpClientWrapperand exposed the destination-contributed query parameters (base URL params + URL.queries... properties) viamergeRequestUri(URI.create("")).HC5: i tried to do it in the same way. I added
UriQueryMergeras a public interface in theconnectivity-apache-httpclient5module, implemented byApacheHttpClient5Wrapper.ODataRequestResultGenericcastshttpClienttoUriQueryMergeridentically to HC4, strips destination-contributed params that appear verbatim in the next link, and cleans up the resulting query string.Definition of Done