-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Add ODataClient for Apache HttpClient5 #1169
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: main
Are you sure you want to change the base?
Changes from all commits
298d9ec
3f5fd26
18f9d52
aa3e921
e5b3ac0
c292c3f
27890e3
e3d8059
d8361d7
731e7a0
8eda731
3edeef7
51930e7
ae93c6e
303b6dd
47c1587
22d9911
77e422c
b55c15d
7a8b50c
1b18f40
9ad0c52
b6a37a6
bd6635b
6ab4f8d
4c2950f
86df157
0c84b2b
926b028
dc398a6
88d6a58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package com.sap.cloud.sdk.cloudplatform.connectivity; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.URI; | ||
| import java.util.Set; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| import javax.annotation.Nonnull; | ||
|
|
||
| import org.apache.hc.client5.http.classic.HttpClient; | ||
| import org.apache.hc.client5.http.classic.methods.HttpHead; | ||
| import org.apache.hc.core5.http.EntityDetails; | ||
| import org.apache.hc.core5.http.Header; | ||
| import org.apache.hc.core5.http.HttpException; | ||
| import org.apache.hc.core5.http.HttpRequest; | ||
| import org.apache.hc.core5.http.HttpRequestInterceptor; | ||
| import org.apache.hc.core5.http.protocol.HttpContext; | ||
|
|
||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| @Slf4j | ||
| @RequiredArgsConstructor | ||
| class CsrfTokenInterceptor implements HttpRequestInterceptor | ||
| { | ||
| static final String X_CSRF_TOKEN_HEADER_KEY = "x-csrf-token"; | ||
| private static final String X_CSRF_TOKEN_FETCH_VALUE = "fetch"; | ||
|
|
||
| private static final Set<String> MUTATING_METHODS = Set.of("POST", "PUT", "PATCH", "DELETE"); | ||
| private static final Pattern NON_PRINTABLE_CHARS = Pattern.compile("[^ -~]"); | ||
|
|
||
| @Nonnull | ||
| private final HttpClient httpClient; | ||
|
|
||
| @Override | ||
| public | ||
| void | ||
| process( @Nonnull final HttpRequest request, final EntityDetails entityDetails, final HttpContext context ) | ||
| throws HttpException, | ||
| IOException | ||
| { | ||
| if( !MUTATING_METHODS.contains(request.getMethod().toUpperCase()) ) { | ||
| return; | ||
| } | ||
|
|
||
| if( request.containsHeader(X_CSRF_TOKEN_HEADER_KEY) ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (question) Does this need to be a case insensitive check? |
||
| log.debug("CSRF token already present in request, skipping retrieval."); | ||
| return; | ||
| } | ||
|
|
||
| final URI requestUri; | ||
| try { | ||
| requestUri = request.getUri(); | ||
| } | ||
| catch( final Exception e ) { | ||
| log.debug("Failed to determine request URI for CSRF token fetch, skipping.", e); | ||
| return; | ||
| } | ||
|
|
||
| final URI csrfFetchUri = deriveServiceRootUri(requestUri); | ||
| final HttpHead headRequest = new HttpHead(csrfFetchUri); | ||
| headRequest.addHeader(X_CSRF_TOKEN_HEADER_KEY, X_CSRF_TOKEN_FETCH_VALUE); | ||
|
Comment on lines
+61
to
+62
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (major) Don't we need to load other headers from incoming request eg: |
||
|
|
||
| try { | ||
| final String token = httpClient.execute(headRequest, response -> { | ||
| final Header header = response.getFirstHeader(X_CSRF_TOKEN_HEADER_KEY); | ||
| if( header == null || header.getValue() == null ) { | ||
| log | ||
| .warn( | ||
| "Target system did not respond with a {} header. " | ||
| + "The subsequent request may fail if a CSRF token is required.", | ||
| X_CSRF_TOKEN_HEADER_KEY); | ||
| return null; | ||
| } | ||
| return NON_PRINTABLE_CHARS.matcher(header.getValue()).replaceAll(""); | ||
| }); | ||
|
|
||
| if( token != null ) { | ||
| log.debug("Successfully retrieved CSRF token, adding to request."); | ||
| request.addHeader(X_CSRF_TOKEN_HEADER_KEY, token); | ||
| } | ||
| } | ||
| 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); | ||
| } | ||
| } | ||
|
Comment on lines
+83
to
+90
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (question) Why don't we throw? Am I missing something? |
||
|
|
||
| /** | ||
| * Derives the service root URI from the full request URI by truncating the path at the first OData resource | ||
| * segment. This matches the HC4 behavior where the CSRF token HEAD request was always sent to the service path root | ||
| * rather than the specific resource path. | ||
| * <p> | ||
| * The service root is identified as the path up to and including the trailing slash before the first resource | ||
| * segment. Example: {@code http://host/service/$batch} → {@code http://host/service/}, | ||
| * {@code http://host/service/Entity} → {@code http://host/service/} | ||
| */ | ||
| @Nonnull | ||
| static URI deriveServiceRootUri( @Nonnull final URI requestUri ) | ||
| { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (major/question) Can the request path be like |
||
| // If the path ends with '/' already (e.g. "/service/"), use it as-is. | ||
| // Otherwise, strip the last segment (e.g. "/service/Entity" -> "/service/", "/service/$batch" -> "/service/"). | ||
| final String servicePath = | ||
| (lastSlash >= 0 && lastSlash < path.length() - 1) ? path.substring(0, lastSlash + 1) : path; | ||
| try { | ||
| return new URI(requestUri.getScheme(), requestUri.getAuthority(), servicePath, null, null); | ||
| } | ||
| catch( final Exception e ) { | ||
| log.debug("Failed to derive service root URI, falling back to full request URI.", e); | ||
| return requestUri; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package com.sap.cloud.sdk.cloudplatform.connectivity; | ||
|
rpanackal marked this conversation as resolved.
|
||
|
|
||
| import java.net.URI; | ||
|
|
||
| import javax.annotation.Nonnull; | ||
|
|
||
| /** | ||
| * Interface to resolve the request URI for a given request. Used to determine the destination-contributed query | ||
| * parameters so that next-link pagination can strip duplicate parameters. | ||
| */ | ||
| @FunctionalInterface | ||
| public interface UriQueryMerger | ||
| { | ||
| /** | ||
| * Returns the fully-merged request URI for the given relative request URI. Merges the destination base URL, | ||
| * destination URL query parameters, and destination property query parameters into the URI. | ||
| * | ||
| * @param requestUri | ||
| * The relative request URI to merge. | ||
| * @return The merged request URI. | ||
| */ | ||
| @Nonnull | ||
| URI mergeRequestUri( @Nonnull URI requestUri ); | ||
| } | ||
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.
(minor)
I would like to have a test for this