feat: Add ODataClient for Apache HttpClient5 by Jonas-Isr · Pull Request #1169 · SAP/cloud-sdk-java

Jonas-Isr · 2026-05-08T10:52:31Z

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:

Add new module for HttpClient v5 (by copying over the old v4 module and adapting it to v5)
Tests

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, an HttpRequestInterceptor registered in DefaultApacheHttpClient5Factory. It intercepts mutating requests at the HTTP client level before they are sent. Thus, all calls with ODataRequestGeneric.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 to tryExecute but 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 DefaultApacheHttpClient5Factory to make this opt-in/opt-out.

Notes:

The interceptor sends HEAD to the service root (derived by stripping the last path segment) rather than the full resource URI, matching HC4's behaviour of scoping the token to the service path.
Compared to the HC4 version, if no CSRF token header is returned, there is no CsrfTokenRetrievalException thrown. 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() uses HttpClient.execute(httpRequest). This keeps the connection open.

HC5: ODataHttpRequest.requestResource() uses HttpClient.executeOpen(null, httpRequest, null), which also keeps the connection open. It is explicitly closed in ODataRequestResultFactory.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 using ODataRequestResultFactory.WITHOUT_BUFFER (e.g. for streaming/deserialization), executeOpen is the only way to do this in HC5. This is not a change compared to HC4 since execute() 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 ConnectionPoolTimeoutException from org.apache.http.conn.

HC5: uses ConnectionRequestTimeoutException from org.apache.hc.core5.http.

Why: HC5 renamed and restructured exception types. The exception is caught in ODataHttpRequest to produce a descriptive ODataConnectionException about connection leaks.

Content-Type on JSON Entities

HC4: ContentType.APPLICATION_JSON had no charset and thus getContentType() returned "application/json".

HC5: ContentType.APPLICATION_JSON includes charset=UTF-8 and this getContentType() would return "application/json; charset=UTF-8".

Fix in ODataRequestUpdate: ComparableHttpEntity now uses ContentType.create("application/json") (no charset). ODataRequestCreate sets "application/json" as a literal string. Both avoid the unwanted charset suffix that would appear in batch request bodies.

UriRequestMerger interface

HC4: removeDuplicateQueryParameters was an instance method in ODataRequestResultGeneric that cast httpClient to UriQueryMerger. UriQueryMerger was implemented by HttpClientWrapper and exposed the destination-contributed query parameters (base URL params + URL.queries... properties) via mergeRequestUri(URI.create("")).

HC5: i tried to do it in the same way. I added UriQueryMerger as a public interface in the connectivity-apache-httpclient5 module, implemented by ApacheHttpClient5Wrapper. ODataRequestResultGeneric casts httpClient to UriQueryMerger identically to HC4, strips destination-contributed params that appear verbatim in the next link, and cleans up the resulting query string.

Definition of Done

Functionality scope stated & covered
Tests cover the scope above
- note that there are no e2e tests (yet)
Error handling created / updated & covered by the tests above
Documentation updated
- I will consider documentation after the initial code reviews
Release notes updated

…-- all tests green

…ent-hc5

Jonas-Isr · 2026-05-22T13:20:28Z

Changes after first pair review with Charles:

cleaned up description of the HTTP Execution Method part in the PR comment above
- using executeOpen() in HC5 is mostly equivalent to execute() in HC4, see more details above
introduced additional changes and tests from PR 1138 that were still missing
smaller fixes
- assertThatCode --> assertThatThrownBy
- fixing some outcommented parts of tests
- consistently not setting HttpVersion 1.1
checked that no HC4 modules are used for the new module
- mvn dependency:tree -pl datamodel/odata-client-apache-httpclient5 '-Dincludes=org.apache.httpcomponents:*' gives out nothing

rpanackal · 2026-05-26T16:53:37Z

+            return;
+        }
+
+        if( request.containsHeader(X_CSRF_TOKEN_HEADER_KEY) ) {


(question)

Does this need to be a case insensitive check?

.containsHeader() form org.apache.hc.core5.http.message.HeaderGroup is already case insensitive. Nothing we need to do.

rpanackal · 2026-05-26T17:00:01Z

+        final HttpHead headRequest = new HttpHead(csrfFetchUri);
+        headRequest.addHeader(X_CSRF_TOKEN_HEADER_KEY, X_CSRF_TOKEN_FETCH_VALUE);


(major)

Don't we need to load other headers from incoming request eg: Authorization etc?

All relevant headers (including Authorization) for the CSRF call are taken from the destination object and added to the call via the ApacheHttpClient5Wrapper anyways (see here). So we do not need to add any other headers here.

rpanackal · 2026-05-26T17:17:51Z

+        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('/');


(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.

rpanackal · 2026-05-26T17:24:03Z

+        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);
+        }
+    }


(question)

Why don't we throw? Am I missing something?

This is the downside of the usage of the interceptor pattern for the CSRF call. I noted it already in the PR note above but did not really explain it.

The main problem is that the interceptor runs inside httpClient.executeOpen() of ODataHttpRequest.requestResource(). There is no clean way (that I found) to throw from the CsrfTokenInterceptor.process() method that surfaces through ODataHttpRequest.requestResource() in a way that an SDK user can understand what happened. (CsrfTokenInterceptor.process() is also only allowed to throw HttpException or IOException by the HttpRequestInterceptor interface.)

The next-best option I found was to log the error of the CSRF token call and see if the following requests fail or not. If they fail, then the log message shows why. If they don't fail, we are happy anyways.

rpanackal · 2026-05-27T10:52:37Z

+
+    @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));
+    }


(minor)

I would like to have a test for this

Added some unit tests.

Jonas-Isr added 13 commits March 10, 2026 16:55

add new module (copy of odata-client)

298d9ec

initial change to hc5 classes (excluding tests)

3f5fd26

intermediate test migration

18f9d52

tests compile (but many fail)

aa3e921

tests compile (but many fail)

e5b3ac0

implement fixes

c292c3f

re-implement CSRF token handling

27890e3

codestyle and fix serializable

e3d8059

fix most remaining tests

d8361d7

redo uir query merging (introduce interface) and csrf token handling …

731e7a0

…-- all tests green

small fixes and code style

8eda731

Merge branch 'main' into odata-client-hc5

3edeef7

add javadoc

51930e7

Jonas-Isr self-assigned this May 8, 2026

increase Cloud SDK version by hand

ae93c6e

github-advanced-security AI found potential problems May 8, 2026

View reviewed changes

Comment thread ...lient5/src/main/java/com/sap/cloud/sdk/datamodel/odata/client/ODataResponseDeserializer.java Fixed

Comment thread ...lient5/src/main/java/com/sap/cloud/sdk/datamodel/odata/client/ODataResponseDeserializer.java Fixed

Jonas-Isr added 14 commits May 12, 2026 16:19

improve CSRF token interceptor logic

303b6dd

small improvements and more tests

47c1587

make csrf token handling clearer

22d9911

remove some warnings

77e422c

Merge branch 'main' into odata-client-hc5

b55c15d

formatter

7a8b50c

Merge remote-tracking branch 'origin/odata-client-hc5' into odata-cli…

1b18f40

…ent-hc5

formatter

9ad0c52

PMD

b6a37a6

update @SInCE tag

bd6635b

more pmd

6ab4f8d

spotbugs

4c2950f

exchange asserThatCode with asserThatThrownBy

86df157

remove explcit setting of Http version

0c84b2b

Jonas-Isr added 2 commits May 21, 2026 14:06

clean up some tests

926b028

introduce changes from PR 1138 to new module

dc398a6

improve connection handling

88d6a58

rpanackal reviewed May 26, 2026

View reviewed changes

rpanackal reviewed May 27, 2026

View reviewed changes

Jonas-Isr added 5 commits May 27, 2026 14:33

small changes (review from Charles)

5d289c9

use HttpStatus for codes (review from Charles)

225a8d3

codestyle

ad2e36c

codestyle

dc41fea

add unit tests for mergeRequestUri

d63eef8

Navigation Menu

Uh oh!

feat: Add ODataClient for Apache HttpClient5 - #1169

feat: Add ODataClient for Apache HttpClient5#1169
Jonas-Isr wants to merge 36 commits into
mainfrom
odata-client-hc5

Jonas-Isr commented May 8, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Jonas-Isr commented May 22, 2026

Uh oh!

rpanackal May 26, 2026

Uh oh!

Jonas-Isr Aug 6, 2026

Uh oh!

rpanackal May 26, 2026

Uh oh!

Jonas-Isr Aug 6, 2026

Uh oh!

rpanackal May 26, 2026

Uh oh!

rpanackal May 26, 2026 •

edited

Loading

Uh oh!

Jonas-Isr Aug 6, 2026

Uh oh!

Uh oh!

rpanackal May 27, 2026

Uh oh!

Jonas-Isr Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		final HttpHead headRequest = new HttpHead(csrfFetchUri);
		headRequest.addHeader(X_CSRF_TOKEN_HEADER_KEY, X_CSRF_TOKEN_FETCH_VALUE);

Uh oh!

Conversation

Jonas-Isr commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Comments regarding review

Feature scope:

Design decisions

CSRF Token Handling

HTTP Execution Method

Connection Timeout Exception

Content-Type on JSON Entities

UriRequestMerger interface

Definition of Done

Uh oh!

Uh oh!

Uh oh!

Jonas-Isr commented May 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpanackal May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jonas-Isr commented May 8, 2026 •

edited

Loading

rpanackal May 26, 2026 •

edited

Loading