13776: allow adding query parameters to RequestOptions (#13778)

- Fix bug highlighted by unit testing - Address code review comments - Run spotlessApply to satisfy formatting rules - Fix more queryParams->parameters cases - Apply code review feedback Signed-off-by: Oliver Lockwood <[email protected]>
opensearch-project · Jun 5, 2024 · 0a26d5a · 0a26d5a
1 parent f76e745
commit 0a26d5a
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Remote Store] Upload translog checkpoint as object metadata to translog.tlog([#13637](https://github.com/opensearch-project/OpenSearch/pull/13637))
 - [Remote Store] Add dynamic cluster settings to set timeout for segments upload to Remote Store ([#13679](https://github.com/opensearch-project/OpenSearch/pull/13679))
 - Add getMetadataFields to MapperService ([#13819](https://github.com/opensearch-project/OpenSearch/pull/13819))
+- Allow setting query parameters on requests ([#13776](https://github.com/opensearch-project/OpenSearch/issues/13776))
 
 ### Dependencies
 - Bump `com.github.spullara.mustache.java:compiler` from 0.9.10 to 0.9.13 ([#13329](https://github.com/opensearch-project/OpenSearch/pull/13329), [#13559](https://github.com/opensearch-project/OpenSearch/pull/13559))

diff --git a/client/rest/src/main/java/org/opensearch/client/Request.java b/client/rest/src/main/java/org/opensearch/client/Request.java
@@ -110,7 +110,13 @@ public void addParameters(Map<String, String> paramSource) {
      * will change it.
      */
     public Map<String, String> getParameters() {
-        return unmodifiableMap(parameters);
+        if (options.getParameters().isEmpty()) {
+            return unmodifiableMap(parameters);
+        } else {
+            Map<String, String> combinedParameters = new HashMap<>(parameters);
+            combinedParameters.putAll(options.getParameters());
+            return unmodifiableMap(combinedParameters);
+        }
     }
 
     /**

diff --git a/client/rest/src/main/java/org/opensearch/client/RequestOptions.java b/client/rest/src/main/java/org/opensearch/client/RequestOptions.java
@@ -40,8 +40,11 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
+import java.util.stream.Collectors;
 
 /**
  * The portion of an HTTP request to OpenSearch that can be
@@ -53,18 +56,21 @@ public final class RequestOptions {
      */
     public static final RequestOptions DEFAULT = new Builder(
         Collections.emptyList(),
+        Collections.emptyMap(),
         HeapBufferedResponseConsumerFactory.DEFAULT,
         null,
         null
     ).build();
 
     private final List<Header> headers;
+    private final Map<String, String> parameters;
     private final HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory;
     private final WarningsHandler warningsHandler;
     private final RequestConfig requestConfig;
 
     private RequestOptions(Builder builder) {
         this.headers = Collections.unmodifiableList(new ArrayList<>(builder.headers));
+        this.parameters = Collections.unmodifiableMap(new HashMap<>(builder.parameters));
         this.httpAsyncResponseConsumerFactory = builder.httpAsyncResponseConsumerFactory;
         this.warningsHandler = builder.warningsHandler;
         this.requestConfig = builder.requestConfig;
@@ -74,7 +80,7 @@ private RequestOptions(Builder builder) {
      * Create a builder that contains these options but can be modified.
      */
     public Builder toBuilder() {
-        return new Builder(headers, httpAsyncResponseConsumerFactory, warningsHandler, requestConfig);
+        return new Builder(headers, parameters, httpAsyncResponseConsumerFactory, warningsHandler, requestConfig);
     }
 
     /**
@@ -84,6 +90,14 @@ public List<Header> getHeaders() {
         return headers;
     }
 
+    /**
+     * Query parameters to attach to the request. Any parameters present here
+     * will override matching parameters in the {@link Request}, if they exist.
+     */
+    public Map<String, String> getParameters() {
+        return parameters;
+    }
+
     /**
      * The {@link HttpAsyncResponseConsumerFactory} used to create one
      * {@link HttpAsyncResponseConsumer} callback per retry. Controls how the
@@ -139,6 +153,12 @@ public String toString() {
                 b.append(headers.get(h).toString());
             }
         }
+        if (parameters.size() > 0) {
+            if (comma) b.append(", ");
+            comma = true;
+            b.append("parameters=");
+            b.append(parameters.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining(",")));
+        }
         if (httpAsyncResponseConsumerFactory != HttpAsyncResponseConsumerFactory.DEFAULT) {
             if (comma) b.append(", ");
             comma = true;
@@ -163,13 +183,14 @@ public boolean equals(Object obj) {
 
         RequestOptions other = (RequestOptions) obj;
         return headers.equals(other.headers)
+            && parameters.equals(other.parameters)
             && httpAsyncResponseConsumerFactory.equals(other.httpAsyncResponseConsumerFactory)
             && Objects.equals(warningsHandler, other.warningsHandler);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(headers, httpAsyncResponseConsumerFactory, warningsHandler);
+        return Objects.hash(headers, parameters, httpAsyncResponseConsumerFactory, warningsHandler);
     }
 
     /**
@@ -179,17 +200,20 @@ public int hashCode() {
      */
     public static class Builder {
         private final List<Header> headers;
+        private final Map<String, String> parameters;
         private HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory;
         private WarningsHandler warningsHandler;
         private RequestConfig requestConfig;
 
         private Builder(
             List<Header> headers,
+            Map<String, String> parameters,
             HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
             WarningsHandler warningsHandler,
             RequestConfig requestConfig
         ) {
             this.headers = new ArrayList<>(headers);
+            this.parameters = new HashMap<>(parameters);
             this.httpAsyncResponseConsumerFactory = httpAsyncResponseConsumerFactory;
             this.warningsHandler = warningsHandler;
             this.requestConfig = requestConfig;
@@ -216,6 +240,21 @@ public Builder addHeader(String name, String value) {
             return this;
         }
 
+        /**
+         * Add the provided query parameter to the request. Any parameters added here
+         * will override matching parameters in the {@link Request}, if they exist.
+         *
+         * @param name  the query parameter name
+         * @param value the query parameter value
+         * @throws NullPointerException if {@code name} or {@code value} is null.
+         */
+        public Builder addParameter(String name, String value) {
+            Objects.requireNonNull(name, "query parameter name cannot be null");
+            Objects.requireNonNull(value, "query parameter value cannot be null");
+            this.parameters.put(name, value);
+            return this;
+        }
+
         /**
          * Set the {@link HttpAsyncResponseConsumerFactory} used to create one
          * {@link HttpAsyncResponseConsumer} callback per retry. Controls how the

diff --git a/client/rest/src/test/java/org/opensearch/client/RequestOptionsTests.java b/client/rest/src/test/java/org/opensearch/client/RequestOptionsTests.java
@@ -38,12 +38,15 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
@@ -89,6 +92,39 @@ public void testAddHeader() {
         }
     }
 
+    public void testAddParameter() {
+        assertThrows(
+            "query parameter name cannot be null",
+            NullPointerException.class,
+            () -> randomBuilder().addParameter(null, randomAsciiLettersOfLengthBetween(3, 10))
+        );
+
+        assertThrows(
+            "query parameter value cannot be null",
+            NullPointerException.class,
+            () -> randomBuilder().addParameter(randomAsciiLettersOfLengthBetween(3, 10), null)
+        );
+
+        RequestOptions.Builder builder = RequestOptions.DEFAULT.toBuilder();
+        int numParameters = between(0, 5);
+        Map<String, String> parameters = new HashMap<>();
+        for (int i = 0; i < numParameters; i++) {
+            String name = randomAsciiAlphanumOfLengthBetween(5, 10);
+            String value = randomAsciiAlphanumOfLength(3);
+            parameters.put(name, value);
+            builder.addParameter(name, value);
+        }
+        RequestOptions options = builder.build();
+        assertEquals(parameters, options.getParameters());
+
+        try {
+            options.getParameters().put(randomAsciiAlphanumOfLengthBetween(5, 10), randomAsciiAlphanumOfLength(3));
+            fail("expected failure");
+        } catch (UnsupportedOperationException e) {
+            assertNull(e.getMessage());
+        }
+    }
+
     public void testSetHttpAsyncResponseConsumerFactory() {
         try {
             RequestOptions.DEFAULT.toBuilder().setHttpAsyncResponseConsumerFactory(null);
@@ -144,6 +180,13 @@ static RequestOptions.Builder randomBuilder() {
             }
         }
 
+        if (randomBoolean()) {
+            int queryParamCount = between(1, 5);
+            for (int i = 0; i < queryParamCount; i++) {
+                builder.addParameter(randomAsciiAlphanumOfLength(3), randomAsciiAlphanumOfLength(3));
+            }
+        }
+
         if (randomBoolean()) {
             builder.setHttpAsyncResponseConsumerFactory(new HeapBufferedResponseConsumerFactory(1));
         }