From 56ff7b21c91e5ae664102d7e76cceb431a0f93f1 Mon Sep 17 00:00:00 2001
From: Alexandre Dutra <adutra@users.noreply.github.com>
Date: Fri, 13 Dec 2024 14:41:24 +0100
Subject: [PATCH] review

---
 .../org/apache/iceberg/rest/HTTPHeaders.java  |   4 +-
 .../apache/iceberg/rest/TestHTTPHeaders.java  |  17 +--
 .../apache/iceberg/rest/TestHTTPRequest.java  | 115 +++++++++++++-----
 3 files changed, 93 insertions(+), 43 deletions(-)

diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
index 9563266b28a4..e7807a8cb071 100644
--- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
+++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
@@ -59,7 +59,7 @@ default boolean contains(String name) {
    * Returns a new instance with the added header, or the current instance if the header is already
    * present.
    */
-  default HTTPHeaders addIfAbsent(HTTPHeader header) {
+  default HTTPHeaders withHeaderIfAbsent(HTTPHeader header) {
     Preconditions.checkNotNull(header, "header");
     return contains(header.name())
         ? this
@@ -71,7 +71,7 @@ default HTTPHeaders addIfAbsent(HTTPHeader header) {
    * Returns a new instance with the added headers, or the current instance if all headers are
    * already present.
    */
-  default HTTPHeaders addIfAbsent(HTTPHeaders headers) {
+  default HTTPHeaders withHeaderIfAbsent(HTTPHeaders headers) {
     Preconditions.checkNotNull(headers, "headers");
     List<HTTPHeader> newHeaders =
         headers.entries().stream().filter(e -> !contains(e.name())).collect(Collectors.toList());
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java
index bff1f6121c57..e8fc493ad393 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java
@@ -57,11 +57,11 @@ void contains() {
   }
 
   @Test
-  void addIfAbsentHTTPHeader() {
-    HTTPHeaders actual = headers.addIfAbsent(HTTPHeader.of("Header1", "value1c"));
+  void withHeaderIfAbsentHTTPHeader() {
+    HTTPHeaders actual = headers.withHeaderIfAbsent(HTTPHeader.of("Header1", "value1c"));
     assertThat(actual).isSameAs(headers);
 
-    actual = headers.addIfAbsent(HTTPHeader.of("header3", "value3"));
+    actual = headers.withHeaderIfAbsent(HTTPHeader.of("header3", "value3"));
     assertThat(actual.entries())
         .containsExactly(
             HTTPHeader.of("header1", "value1a"),
@@ -69,18 +69,19 @@ void addIfAbsentHTTPHeader() {
             HTTPHeader.of("header2", "value2"),
             HTTPHeader.of("header3", "value3"));
 
-    assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeader) null))
+    assertThatThrownBy(() -> headers.withHeaderIfAbsent((HTTPHeader) null))
         .isInstanceOf(NullPointerException.class)
         .hasMessage("header");
   }
 
   @Test
-  void addIfAbsentHTTPHeaders() {
-    HTTPHeaders actual = headers.addIfAbsent(HTTPHeaders.of(HTTPHeader.of("Header1", "value1c")));
+  void withHeaderIfAbsentHTTPHeaders() {
+    HTTPHeaders actual =
+        headers.withHeaderIfAbsent(HTTPHeaders.of(HTTPHeader.of("Header1", "value1c")));
     assertThat(actual).isSameAs(headers);
 
     actual =
-        headers.addIfAbsent(
+        headers.withHeaderIfAbsent(
             ImmutableHTTPHeaders.builder()
                 .addEntry(HTTPHeader.of("Header1", "value1c"))
                 .addEntry(HTTPHeader.of("header3", "value3"))
@@ -95,7 +96,7 @@ void addIfAbsentHTTPHeaders() {
                     HTTPHeader.of("header3", "value3"))
                 .build());
 
-    assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null))
+    assertThatThrownBy(() -> headers.withHeaderIfAbsent((HTTPHeaders) null))
         .isInstanceOf(NullPointerException.class)
         .hasMessage("headers");
   }
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java
index 3cc6d520b591..84e1b0830c9b 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java
@@ -21,16 +21,22 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import java.net.URI;
+import java.util.Map;
 import java.util.stream.Stream;
 import org.apache.iceberg.catalog.Namespace;
 import org.apache.iceberg.exceptions.RESTException;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
 import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mockito;
 
 class TestHTTPRequest {
 
@@ -96,43 +102,86 @@ public void invalidPath() {
             "Failed to create request URI from base http://localhost/ not a valid path, params {}");
   }
 
-  @ParameterizedTest
-  @MethodSource("validRequestBodies")
-  public void encodedBody(HTTPRequest request, String expected) {
-    assertThat(request.encodedBody()).isEqualTo(expected);
+  @Test
+  public void encodedBodyJSON() {
+    HTTPRequest request =
+        ImmutableHTTPRequest.builder()
+            .baseUri(URI.create("http://localhost"))
+            .method(HTTPRequest.HTTPMethod.POST)
+            .path("v1/namespaces/ns")
+            .body(
+                CreateNamespaceRequest.builder()
+                    .withNamespace(Namespace.of("ns"))
+                    .setProperties(ImmutableMap.of("prop1", "value1"))
+                    .build())
+            .build();
+    assertThat(request.encodedBody())
+        .isEqualTo("{\"namespace\":[\"ns\"],\"properties\":{\"prop1\":\"value1\"}}");
   }
 
-  public static Stream<Arguments> validRequestBodies() {
-    return Stream.of(
-        // form data
-        Arguments.of(
-            ImmutableHTTPRequest.builder()
-                .baseUri(URI.create("http://localhost"))
-                .method(HTTPRequest.HTTPMethod.POST)
-                .path("token")
-                .body(
-                    ImmutableMap.of(
-                        "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
-                        "subject_token", "token",
-                        "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
-                        "scope", "catalog"))
-                .build(),
+  @Test
+  public void encodedBodyJSONInvalid() throws JsonProcessingException {
+    ObjectMapper mapper = Mockito.mock(ObjectMapper.class);
+    Mockito.when(mapper.writeValueAsString(Mockito.any()))
+        .thenThrow(new JsonMappingException(null, "invalid"));
+    HTTPRequest request =
+        ImmutableHTTPRequest.builder()
+            .baseUri(URI.create("http://localhost"))
+            .method(HTTPRequest.HTTPMethod.POST)
+            .path("token")
+            .body("invalid")
+            .mapper(mapper)
+            .build();
+    assertThatThrownBy(request::encodedBody)
+        .isInstanceOf(RESTException.class)
+        .hasMessage("Failed to encode request body: invalid");
+  }
+
+  @Test
+  public void encodedBodyFormData() {
+    HTTPRequest request =
+        ImmutableHTTPRequest.builder()
+            .baseUri(URI.create("http://localhost"))
+            .method(HTTPRequest.HTTPMethod.POST)
+            .path("token")
+            .body(
+                ImmutableMap.of(
+                    "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
+                    "subject_token", "token",
+                    "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
+                    "scope", "catalog"))
+            .build();
+    assertThat(request.encodedBody())
+        .isEqualTo(
             "grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&"
                 + "subject_token=token&"
                 + "subject_token_type=urn%3Aietf%3Aparams%3Aoauth%3Atoken-type%3Aaccess_token&"
-                + "scope=catalog"),
-        // JSON
-        Arguments.of(
-            ImmutableHTTPRequest.builder()
-                .baseUri(URI.create("http://localhost"))
-                .method(HTTPRequest.HTTPMethod.POST)
-                .path("v1/namespaces/ns") // trailing slash should be removed
-                .body(
-                    CreateNamespaceRequest.builder()
-                        .withNamespace(Namespace.of("ns"))
-                        .setProperties(ImmutableMap.of("prop1", "value1"))
-                        .build())
-                .build(),
-            "{\"namespace\":[\"ns\"],\"properties\":{\"prop1\":\"value1\"}}"));
+                + "scope=catalog");
+  }
+
+  @Test
+  public void encodedBodyFormDataNullKeysAndValues() {
+    Map<String, String> body = Maps.newHashMap();
+    body.put(null, "token");
+    body.put("scope", null);
+    HTTPRequest request =
+        ImmutableHTTPRequest.builder()
+            .baseUri(URI.create("http://localhost"))
+            .method(HTTPRequest.HTTPMethod.POST)
+            .path("token")
+            .body(body)
+            .build();
+    assertThat(request.encodedBody()).isEqualTo("null=token&scope=null");
+  }
+
+  @Test
+  public void encodedBodyNull() {
+    HTTPRequest request =
+        ImmutableHTTPRequest.builder()
+            .baseUri(URI.create("http://localhost"))
+            .method(HTTPRequest.HTTPMethod.POST)
+            .path("token")
+            .build();
+    assertThat(request.encodedBody()).isNull();
   }
 }