feat: Add ODataClient for Apache HttpClient5#1169
Open
Jonas-Isr wants to merge 31 commits into
Open
Conversation
…-- all tests green
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Member
Author
|
Changes after first pair review with Charles:
|
rpanackal
reviewed
May 26, 2026
| return; | ||
| } | ||
|
|
||
| if( request.containsHeader(X_CSRF_TOKEN_HEADER_KEY) ) { |
Member
There was a problem hiding this comment.
(question)
Does this need to be a case insensitive check?
Comment on lines
+61
to
+62
| final HttpHead headRequest = new HttpHead(csrfFetchUri); | ||
| headRequest.addHeader(X_CSRF_TOKEN_HEADER_KEY, X_CSRF_TOKEN_FETCH_VALUE); |
Member
There was a problem hiding this comment.
(major)
Don't we need to load other headers from incoming request eg: Authorization etc?
| final String path = requestUri.getRawPath(); | ||
| // Service root is everything up to and including the trailing slash before the first resource segment. | ||
| // Find the last '/' that is followed by at least one more character (i.e., there is a resource segment). | ||
| final int lastSlash = path.lastIndexOf('/'); |
Member
There was a problem hiding this comment.
(major/question)
Can the request path be like /sap/opu/odata/sap/API_BP/BusinessPartner('123')/to_Address ? If so, we will have a problem in service path resolution.
Comment on lines
+83
to
+90
| catch( final Exception e ) { | ||
| log | ||
| .warn( | ||
| "CSRF token retrieval failed: the HEAD request was not successful. " | ||
| + "The subsequent request may fail if a CSRF token is required.", | ||
| e); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
(question)
Why don't we throw? Am I missing something?
rpanackal
reviewed
May 27, 2026
Comment on lines
+132
to
+141
|
|
||
| @Nonnull | ||
| @Override | ||
| public URI mergeRequestUri( @Nonnull final URI requestUri ) | ||
| { | ||
| final UriPathMerger merger = new UriPathMerger(); | ||
| final URI merged = merger.merge(destination.getUri(), requestUri); | ||
| final String queryString = String.join("&", QueryParamGetter.getQueryParameters(destination)); | ||
| return merger.merge(merged, URI.create("/?" + queryString)); | ||
| } |
Member
There was a problem hiding this comment.
(minor)
I would like to have a test for this
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
3 participants
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:
ODataHttpRequest.requestResource()usesHttpClient.execute(httpRequest). This keeps the connection open.HC5:
ODataHttpRequest.requestResource()usesHttpClient.executeOpen(null, httpRequest, null), which also keeps the connection open. It is explicitly closed inODataRequestResultFactory.WITH_BUFFER.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 cases usingODataRequestResultFactory.WITHOUT_BUFFER(e.g. for streaming/deserialization),executeOpenis the only way to do this in HC5. This is not a change compared to HC4 sinceexecute()in HC4 also did not close the response. So even though the method name changes from execute to executeOpen, the behaviour is the same.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