RFC 3986 compatibility as optional for HttpApplicationSettings

docs/config-app.md

@@ -346,6 +346,7 @@ For HTTP data source available next options:
 - `settings.http.amp-endpoint` - the url to fetch AMP stored requests.
 - `settings.http.video-endpoint` - the url to fetch video stored requests.
 - `settings.http.category-endpoint` - the url to fetch categories for long form video.
+- `settings.http.rfc3986-compatible` - if equals to `true` the url will be build according to RFC 3986, `false` by default
 
 For account processing rules available next options:
 - `settings.enforce-valid-account` - if equals to `true` then request without account id will be rejected with 401.

extra/pom.xml

@@ -39,6 +39,7 @@
         <commons.collections.version>4.4</commons.collections.version>
         <commons.compress.version>1.27.1</commons.compress.version>
         <commons-math3.version>3.6.1</commons-math3.version>
+        <commons-validator.version>1.10.0</commons-validator.version>
         <scram.version>2.1</scram.version>
         <httpclient.version>4.5.14</httpclient.version>
         <ipaddress.version>5.5.1</ipaddress.version>
@@ -135,6 +136,11 @@
                 <artifactId>commons-math3</artifactId>
                 <version>${commons-math3.version}</version>
             </dependency>
+            <dependency>
+                <groupId>commons-validator</groupId>
+                <artifactId>commons-validator</artifactId>
+                <version>${commons-validator.version}</version>
+            </dependency>
             <!-- TODO: refactor code to replace URIBuilder with something else so that this dep can be removed -->
             <dependency>
                 <groupId>org.apache.httpcomponents</groupId>

pom.xml

@@ -114,6 +114,10 @@
             <groupId>org.apache.httpcomponents</groupId>
             <artifactId>httpclient</artifactId>
         </dependency>
+        <dependency>
+            <groupId>commons-validator</groupId>
+            <artifactId>commons-validator</artifactId>
+        </dependency>
         <dependency>
             <groupId>com.github.seancfoley</groupId>
             <artifactId>ipaddress</artifactId>

src/main/java/org/prebid/server/settings/HttpApplicationSettings.java

@@ -8,6 +8,7 @@
 import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.http.client.utils.URIBuilder;
 import org.prebid.server.exception.PreBidException;
 import org.prebid.server.execution.timeout.Timeout;
 import org.prebid.server.json.DecodeException;
@@ -25,6 +26,7 @@
 import org.prebid.server.vertx.httpclient.HttpClient;
 import org.prebid.server.vertx.httpclient.model.HttpClientResponse;
 
+import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -44,10 +46,14 @@
  * In order to enable caching and reduce latency for read operations {@link HttpApplicationSettings}
  * can be decorated by {@link CachingApplicationSettings}.
  * <p>
- * Expected the endpoint to satisfy the following API:
+ * Expected the endpoint to satisfy the following API (URL is encoded):
  * <p>
  * GET {endpoint}?request-ids=["req1","req2"]&imp-ids=["imp1","imp2","imp3"]
  * <p>
+ * or settings.http.rfc3986-compatible is set to true
+ * <p>
+ *  * GET {endpoint}?request-id=req1&request-id=req2&imp-id=imp1&imp-id=imp2&imp-id=imp3
+ *  * <p>
  * This endpoint should return a payload like:
  * <pre>
  * {
@@ -76,20 +82,27 @@ public class HttpApplicationSettings implements ApplicationSettings {
     private final String categoryEndpoint;
     private final HttpClient httpClient;
     private final JacksonMapper mapper;
+    private final boolean isRfc3986Compatible;
+
+    public HttpApplicationSettings(HttpClient httpClient,
+                                   JacksonMapper mapper,
+                                   String endpoint,
+                                   String ampEndpoint,
+                                   String videoEndpoint,
+                                   String categoryEndpoint,
+                                   boolean isRfc3986Compatible) {
 
-    public HttpApplicationSettings(HttpClient httpClient, JacksonMapper mapper, String endpoint, String ampEndpoint,
-                                   String videoEndpoint, String categoryEndpoint) {
         this.httpClient = Objects.requireNonNull(httpClient);
         this.mapper = Objects.requireNonNull(mapper);
-        this.endpoint = HttpUtil.validateUrl(Objects.requireNonNull(endpoint));
-        this.ampEndpoint = HttpUtil.validateUrl(Objects.requireNonNull(ampEndpoint));
-        this.videoEndpoint = HttpUtil.validateUrl(Objects.requireNonNull(videoEndpoint));
-        this.categoryEndpoint = HttpUtil.validateUrl(Objects.requireNonNull(categoryEndpoint));
+        this.endpoint = HttpUtil.validateUrlSyntax(Objects.requireNonNull(endpoint));
+        this.ampEndpoint = HttpUtil.validateUrlSyntax(Objects.requireNonNull(ampEndpoint));
+        this.videoEndpoint = HttpUtil.validateUrlSyntax(Objects.requireNonNull(videoEndpoint));
+        this.categoryEndpoint = HttpUtil.validateUrlSyntax(Objects.requireNonNull(categoryEndpoint));
+        this.isRfc3986Compatible = isRfc3986Compatible;
     }
 
     @Override
     public Future<Account> getAccountById(String accountId, Timeout timeout) {
-
         return fetchAccountsByIds(Collections.singleton(accountId), timeout)
                 .map(accounts -> accounts.stream()
                         .findFirst()
@@ -111,15 +124,20 @@ private Future<Set<Account>> fetchAccountsByIds(Set<String> accountIds, Timeout
                 .recover(Future::failedFuture);
     }
 
-    private static String accountsRequestUrlFrom(String endpoint, Set<String> accountIds) {
-        final StringBuilder url = new StringBuilder(endpoint);
-        url.append(endpoint.contains("?") ? "&" : "?");
-
-        if (!accountIds.isEmpty()) {
-            url.append("account-ids=[\"").append(joinIds(accountIds)).append("\"]");
+    private String accountsRequestUrlFrom(String endpoint, Set<String> accountIds) {
+        try {
+            final URIBuilder uriBuilder = new URIBuilder(endpoint);
+            if (!accountIds.isEmpty()) {
+                if (isRfc3986Compatible) {
+                    accountIds.forEach(accountId -> uriBuilder.addParameter("account-id", accountId));
+                } else {
+                    uriBuilder.addParameter("account-ids", "[\"%s\"]".formatted(joinIds(accountIds)));
+                }
+            }
+            return uriBuilder.build().toString();
+        } catch (URISyntaxException e) {
+            throw new PreBidException("URL %s has bad syntax".formatted(endpoint));
         }
-
-        return url.toString();
     }
 
     private Future<Set<Account>> processAccountsResponse(HttpClientResponse response, Set<String> accountIds) {
@@ -165,9 +183,6 @@ public Future<StoredDataResult> getAmpStoredData(String accountId, Set<String> r
         return fetchStoredData(ampEndpoint, requestIds, Collections.emptySet(), timeout);
     }
 
-    /**
-     * Not supported and returns failed result.
-     */
     @Override
     public Future<StoredDataResult> getVideoStoredData(String accountId, Set<String> requestIds, Set<String> impIds,
                                                        Timeout timeout) {
@@ -240,22 +255,27 @@ private Future<StoredDataResult> fetchStoredData(String endpoint, Set<String> re
                 .recover(exception -> failStoredDataResponse(exception, requestIds, impIds));
     }
 
-    private static String storeRequestUrlFrom(String endpoint, Set<String> requestIds, Set<String> impIds) {
-        final StringBuilder url = new StringBuilder(endpoint);
-        url.append(endpoint.contains("?") ? "&" : "?");
-
-        if (!requestIds.isEmpty()) {
-            url.append("request-ids=[\"").append(joinIds(requestIds)).append("\"]");
-        }
-
-        if (!impIds.isEmpty()) {
+    private String storeRequestUrlFrom(String endpoint, Set<String> requestIds, Set<String> impIds) {
+        try {
+            final URIBuilder uriBuilder = new URIBuilder(endpoint);
             if (!requestIds.isEmpty()) {
-                url.append("&");
+                if (isRfc3986Compatible) {
+                    requestIds.forEach(requestId -> uriBuilder.addParameter("request-id", requestId));
+                } else {
+                    uriBuilder.addParameter("request-ids", "[\"%s\"]".formatted(joinIds(requestIds)));
+                }
+            }
+            if (!impIds.isEmpty()) {
+                if (isRfc3986Compatible) {
+                    impIds.forEach(impId -> uriBuilder.addParameter("imp-id", impId));
+                } else {
+                    uriBuilder.addParameter("imp-ids", "[\"%s\"]".formatted(joinIds(impIds)));
+                }
             }
-            url.append("imp-ids=[\"").append(joinIds(impIds)).append("\"]");
+            return uriBuilder.build().toString();
+        } catch (URISyntaxException e) {
+            throw new PreBidException("URL %s has bad syntax".formatted(endpoint));
         }
-
-        return url.toString();
     }
 
     private static String joinIds(Set<String> ids) {

src/main/java/org/prebid/server/spring/config/SettingsConfiguration.java

@@ -122,10 +122,17 @@ HttpApplicationSettings httpApplicationSettings(
                 @Value("${settings.http.endpoint}") String endpoint,
                 @Value("${settings.http.amp-endpoint}") String ampEndpoint,
                 @Value("${settings.http.video-endpoint}") String videoEndpoint,
-                @Value("${settings.http.category-endpoint}") String categoryEndpoint) {
+                @Value("${settings.http.category-endpoint}") String categoryEndpoint,
+                @Value("${settings.http.rfc3986-compatible:false}") boolean isRfc3986Compatible) {
 
-            return new HttpApplicationSettings(httpClient, mapper, endpoint, ampEndpoint, videoEndpoint,
-                    categoryEndpoint);
+            return new HttpApplicationSettings(
+                    httpClient,
+                    mapper,
+                    endpoint,
+                    ampEndpoint,
+                    videoEndpoint,
+                    categoryEndpoint,
+                    isRfc3986Compatible);
         }
     }
 

src/main/java/org/prebid/server/util/HttpUtil.java

@@ -8,6 +8,7 @@
 import io.vertx.core.http.HttpServerResponse;
 import io.vertx.ext.web.RoutingContext;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.validator.routines.UrlValidator;
 import org.prebid.server.log.ConditionalLogger;
 import org.prebid.server.log.Logger;
 import org.prebid.server.log.LoggerFactory;
@@ -78,12 +79,15 @@ public final class HttpUtil {
     public static final String MACROS_OPEN = "{{";
     public static final String MACROS_CLOSE = "}}";
 
+    private static final UrlValidator URL_VALIDAROR = UrlValidator.getInstance();
+
     private HttpUtil() {
     }
 
     /**
      * Checks the input string for using as URL.
      */
+    @Deprecated
     public static String validateUrl(String url) {
         if (containsMacrosses(url)) {
             return url;
@@ -96,6 +100,14 @@ public static String validateUrl(String url) {
         }
     }
 
+    public static String validateUrlSyntax(String url) {
+        if (containsMacrosses(url) || URL_VALIDAROR.isValid(url)) {
+            return url;
+        }
+
+        throw new IllegalArgumentException("URL supplied is not valid: " + url);
+    }
+
     // TODO: We need our own way to work with url macrosses
     private static boolean containsMacrosses(String url) {
         return StringUtils.contains(url, MACROS_OPEN) && StringUtils.contains(url, MACROS_CLOSE);

src/test/groovy/org/prebid/server/functional/testcontainers/container/NetworkServiceContainer.groovy

@@ -8,6 +8,9 @@ class NetworkServiceContainer extends MockServerContainer {
 
     NetworkServiceContainer(String version) {
         super(DockerImageName.parse("mockserver/mockserver:mockserver-$version"))
+        def aliasWithTopLevelDomain = "${getNetworkAliases().first()}.com".toString()
+        withCreateContainerCmdModifier { it.withHostName(aliasWithTopLevelDomain) }
+        setNetworkAliases([aliasWithTopLevelDomain])
     }
 
     String getHostAndPort() {

src/test/groovy/org/prebid/server/functional/testcontainers/scaffolding/HttpSettings.groovy

@@ -1,13 +1,21 @@
 package org.prebid.server.functional.testcontainers.scaffolding
 
+import org.mockserver.matchers.Times
+import org.mockserver.model.Header
 import org.mockserver.model.HttpRequest
+import org.mockserver.model.HttpStatusCode
+import org.prebid.server.functional.model.ResponseModel
 import org.testcontainers.containers.MockServerContainer
 
 import static org.mockserver.model.HttpRequest.request
+import static org.mockserver.model.HttpResponse.response
+import static org.mockserver.model.HttpStatusCode.OK_200
+import static org.mockserver.model.MediaType.APPLICATION_JSON
 
 class HttpSettings extends NetworkScaffolding {
 
     private static final String ENDPOINT = "/stored-requests"
+    private static final String RFC_ENDPOINT = "/stored-requests-rfc"
     private static final String AMP_ENDPOINT = "/amp-stored-requests"
 
     HttpSettings(MockServerContainer mockServerContainer) {
@@ -27,12 +35,47 @@ class HttpSettings extends NetworkScaffolding {
 
     @Override
     void setResponse() {
+    }
+
+    protected HttpRequest getRfcRequest(String accountId) {
+        request().withPath(RFC_ENDPOINT)
+                .withQueryStringParameter("account-id", accountId)
+    }
+
+
+    void setRfcResponse(String value,
+                     ResponseModel responseModel,
+                     HttpStatusCode statusCode = OK_200,
+                     Map<String, String> headers = [:]) {
+        def responseHeaders = headers.collect { new Header(it.key, it.value) }
+        def mockResponse = encode(responseModel)
+        mockServerClient.when(getRfcRequest(value), Times.unlimited())
+                .respond(response().withStatusCode(statusCode.code())
+                        .withBody(mockResponse, APPLICATION_JSON)
+                        .withHeaders(responseHeaders))
+    }
 
+    int getRfcRequestCount(String value) {
+        mockServerClient.retrieveRecordedRequests(getRfcRequest(value))
+                .size()
     }
 
     @Override
     void reset() {
         super.reset(ENDPOINT)
+        super.reset(RFC_ENDPOINT)
         super.reset(AMP_ENDPOINT)
     }
+
+    static String getEndpoint() {
+        return ENDPOINT
+    }
+
+    static String getAmpEndpoint() {
+        return AMP_ENDPOINT
+    }
+
+    static String getRfcEndpoint() {
+        return RFC_ENDPOINT
+    }
 }