fix: [OData] Remove duplicate query parameters from nextLink references (#813)

Author: newtork

cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java

@@ -31,7 +31,7 @@
  * and it will append the url configured in the destination.
  */
 @Slf4j
-class HttpClientWrapper extends CloseableHttpClient
+class HttpClientWrapper extends CloseableHttpClient implements UriQueryMerger
 {
     private final CloseableHttpClient httpClient;
     @Getter( AccessLevel.PACKAGE )
@@ -107,15 +107,20 @@ HttpClientWrapper withDestination( final HttpDestinationProperties destination )
         return new HttpClientWrapper(httpClient, destination);
     }
 
-    HttpUriRequest wrapRequest( final HttpUriRequest request )
+    @Nonnull
+    public URI mergeRequestUri( @Nonnull final URI requestUri )
     {
         final UriPathMerger merger = new UriPathMerger();
-        URI requestUri = merger.merge(destination.getUri(), request.getURI());
+        final URI mergedUri = merger.merge(destination.getUri(), requestUri);
 
         final String queryString = Joiner.on("&").join(QueryParamGetter.getQueryParameters(destination));
-        requestUri = merger.merge(requestUri, URI.create("/?" + queryString));
+        return merger.merge(mergedUri, URI.create("/?" + queryString));
+    }
 
+    HttpUriRequest wrapRequest( final HttpUriRequest request )
+    {
         final RequestBuilder requestBuilder = RequestBuilder.copy(request);
+        final URI requestUri = mergeRequestUri(request.getURI());
         requestBuilder.setUri(requestUri);
 
         for( final Header header : destination.getHeaders(requestUri) ) {

cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/UriQueryMerger.java

@@ -0,0 +1,22 @@
+package com.sap.cloud.sdk.cloudplatform.connectivity;
+
+import java.net.URI;
+
+import javax.annotation.Nonnull;
+
+/**
+ * Interface to resolve the request URI for a given request. This is used to determine the full request URI.
+ */
+@FunctionalInterface
+public interface UriQueryMerger
+{
+    /**
+     * Returns the request URI for the given request. This method is used to resolve the request URI
+     *
+     * @param requestUri
+     *            the request uri to merge.
+     * @return the request URI.
+     */
+    @Nonnull
+    URI mergeRequestUri( @Nonnull final URI requestUri );
+}

datamodel/odata-client/src/main/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataRequestResultGeneric.java

@@ -34,6 +34,7 @@
 import com.google.gson.JsonElement;
 import com.google.gson.JsonParser;
 import com.google.gson.stream.JsonToken;
+import com.sap.cloud.sdk.cloudplatform.connectivity.UriQueryMerger;
 import com.sap.cloud.sdk.datamodel.odata.client.JsonPath;
 import com.sap.cloud.sdk.datamodel.odata.client.ODataProtocol;
 import com.sap.cloud.sdk.datamodel.odata.client.ODataResponseDeserializer;
@@ -554,10 +555,10 @@ public Option<String> getNextLink()
         for( final JsonPath path : getODataRequest().getProtocol().getPathToNextLink().getPaths() ) {
             final ResultElement resultElement = getResultElement(path);
             if( resultElement != null ) {
-                return Option
-                    .of(resultElement)
-                    .map(ResultElement::asString)
-                    .peek(link -> log.debug("Found reference to next page: {}", link));
+                String nextLink = resultElement.asString();
+                log.debug("Found reference to next page: {}", nextLink);
+                nextLink = removeDuplicateQueryParameters(nextLink);
+                return Option.of(nextLink);
             }
         }
         log.debug("Result does not reference any further pages.");
@@ -751,4 +752,31 @@ private void assertResultTypeIsNotVoid( @Nonnull final Class<?> cls )
         }
 
     }
+
+    @Nonnull
+    private String removeDuplicateQueryParameters( @Nonnull final String nextLink )
+    {
+        if( !(httpClient instanceof UriQueryMerger) ) {
+            return nextLink;
+        }
+        final String query = ((UriQueryMerger) httpClient).mergeRequestUri(URI.create("")).getRawQuery();
+        if( query == null ) {
+            return nextLink;
+        }
+        final String[] segments = nextLink.split("\\?", 2);
+        final String[] queryArguments = query.split("&");
+        for( final String argument : queryArguments ) {
+            if( segments[1].contains(argument) ) {
+                segments[1] = segments[1].replace(argument, "");
+            }
+        }
+        if( nextLink.length() + 1 == segments[0].length() + segments[1].length() ) {
+            return nextLink;
+        }
+        // after removal of arguments clean-up query: fix "?foo=bar&&&one=1", fix "?&one=1", fix "?foo=bar&"
+        segments[1] = segments[1].replaceAll("&&+", "&").replace("?&", "?").replaceAll("&$", "");
+        final String updatedLink = segments[0] + "?" + segments[1];
+        log.debug("Updated reference to next page: {}", updatedLink);
+        return updatedLink;
+    }
 }

datamodel/odata-client/src/test/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataNextLinkTest.java

@@ -6,11 +6,15 @@
 
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpVersion;
+import org.apache.http.client.HttpClient;
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.StringEntity;
 import org.apache.http.message.BasicHttpResponse;
 import org.junit.jupiter.api.Test;
 
+import com.sap.cloud.sdk.cloudplatform.connectivity.DefaultHttpDestination;
+import com.sap.cloud.sdk.cloudplatform.connectivity.Destination;
+import com.sap.cloud.sdk.cloudplatform.connectivity.HttpClientAccessor;
 import com.sap.cloud.sdk.datamodel.odata.client.ODataProtocol;
 
 class ODataNextLinkTest
@@ -24,6 +28,30 @@ class ODataNextLinkTest
         }
         """;
 
+    @Test
+    void testRemoveDuplicateQueryArguments()
+    {
+        final ODataRequestGeneric request =
+            new ODataRequestRead("/v1/foo/bar/", "endpoint", "blub=42", ODataProtocol.V2);
+
+        final HttpResponse httpResponse = new BasicHttpResponse(HttpVersion.HTTP_1_1, 200, "Ok");
+        httpResponse.setEntity(new StringEntity(PAYLOAD_NEXT_LINK, ContentType.APPLICATION_JSON));
+
+        final String baseUrl = "http://blub/?high=five";
+
+        // case 1: query parameters are EQUAL in destination and in nextLink -> remove redundant query parameter
+        final Destination dest1 = DefaultHttpDestination.builder(baseUrl).property("URL.queries.foo", "bar").build();
+        final HttpClient client1 = HttpClientAccessor.getHttpClient(dest1);
+        final ODataRequestResultGeneric result1 = new ODataRequestResultGeneric(request, httpResponse, client1);
+        assertThat(result1.getNextLink()).contains("/v1/foo/bar/endpoint?$skiptoken=s3cReT-t0k3n");
+
+        // case 2: query parameters are NOT EQUAL in destination and in nextLink -> retain query parameter
+        final Destination dest2 = DefaultHttpDestination.builder(baseUrl).property("URL.queries.foo", "baz").build();
+        final HttpClient client2 = HttpClientAccessor.getHttpClient(dest2);
+        final ODataRequestResultGeneric result2 = new ODataRequestResultGeneric(request, httpResponse, client2);
+        assertThat(result2.getNextLink()).contains("/v1/foo/bar/endpoint?$skiptoken=s3cReT-t0k3n&foo=bar");
+    }
+
     @Test
     void testNotParsedNextLinkV4()
     {

release_notes.md

@@ -20,4 +20,5 @@
 
 ### 🐛 Fixed Issues
 
-- Fixed OData V2 error: Disable Validation in the absense of DOCTYPE
+- Fix OData V2 error: Disable Validation in the absense of DOCTYPE
+- Fix OData V2/V4 error: Navigating `nextLink` in paginated result-set no longer results in duplicate query parameters.