Auth Manager API part 1: HTTPRequest, HTTPHeader

core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java

@@ -161,6 +161,13 @@ interface HTTPHeader {
     @Value.Redacted
     String value();
 
+    @Value.Check
+    default void check() {
+      if (name().isEmpty()) {
+        throw new IllegalArgumentException("Header name cannot be empty");
+      }
+    }
+
     static HTTPHeader of(String name, String value) {
       return ImmutableHTTPHeader.builder().name(name).value(value).build();
     }

core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java

@@ -18,10 +18,14 @@
  */
 package org.apache.iceberg.rest;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.Map;
 import javax.annotation.Nullable;
+import org.apache.hc.core5.net.URIBuilder;
+import org.apache.iceberg.exceptions.RESTException;
 import org.immutables.value.Value;
 
 /** Represents an HTTP request. */
@@ -49,7 +53,19 @@ enum HTTPMethod {
    */
   @Value.Lazy
   default URI requestUri() {
-    return RESTUtil.buildRequestUri(this);
+    // if full path is provided, use the input path as path
+    String fullPath =
+        (path().startsWith("https://") || path().startsWith("http://"))
+            ? path()
+            : String.format("%s/%s", baseUri(), path());
+    try {
+      URIBuilder builder = new URIBuilder(RESTUtil.stripTrailingSlash(fullPath));
+      queryParameters().forEach(builder::addParameter);
+      return builder.build();
+    } catch (URISyntaxException e) {
+      throw new RESTException(
+          "Failed to create request URI from base %s, params %s", fullPath, queryParameters());
+    }
   }
 
   /** Returns the HTTP method of this request. */
@@ -77,7 +93,17 @@ default HTTPHeaders headers() {
   @Nullable
   @Value.Redacted
   default String encodedBody() {
-    return RESTUtil.encodeRequestBody(this);
+    Object body = body();
+    if (body instanceof Map) {
+      return RESTUtil.encodeFormData((Map<?, ?>) body);
+    } else if (body != null) {
+      try {
+        return mapper().writeValueAsString(body);
+      } catch (JsonProcessingException e) {
+        throw new RESTException(e, "Failed to encode request body: %s", body);
+      }
+    }
+    return null;
   }
 
   /**
@@ -88,4 +114,13 @@ default String encodedBody() {
   default ObjectMapper mapper() {
     return RESTObjectMapper.mapper();
   }
+
+  @Value.Check
+  default void check() {
+    if (path().startsWith("/")) {
+      throw new RESTException(
+          "Received a malformed path for a REST request: %s. Paths should not start with /",
+          path());
+    }
+  }
 }

core/src/main/java/org/apache/iceberg/rest/RESTUtil.java

@@ -18,18 +18,13 @@
  */
 package org.apache.iceberg.rest;
 
-import com.fasterxml.jackson.core.JsonProcessingException;
 import java.io.UncheckedIOException;
 import java.io.UnsupportedEncodingException;
-import java.net.URI;
-import java.net.URISyntaxException;
 import java.net.URLDecoder;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
-import org.apache.hc.core5.net.URIBuilder;
 import org.apache.iceberg.catalog.Namespace;
-import org.apache.iceberg.exceptions.RESTException;
 import org.apache.iceberg.relocated.com.google.common.base.Joiner;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.relocated.com.google.common.base.Splitter;
@@ -220,45 +215,4 @@ public static Namespace decodeNamespace(String encodedNs) {
 
     return Namespace.of(levels);
   }
-
-  /** Builds a request URI from a base URI and an {@link HTTPRequest}. */
-  public static URI buildRequestUri(HTTPRequest request) {
-    // if full path is provided, use the input path as path
-    String path = request.path();
-    if (path.startsWith("/")) {
-      throw new RESTException(
-          "Received a malformed path for a REST request: %s. Paths should not start with /", path);
-    }
-    String fullPath =
-        (path.startsWith("https://") || path.startsWith("http://"))
-            ? path
-            : String.format("%s/%s", request.baseUri(), path);
-    try {
-      URIBuilder builder = new URIBuilder(stripTrailingSlash(fullPath));
-      request.queryParameters().forEach(builder::addParameter);
-      return builder.build();
-    } catch (URISyntaxException e) {
-      throw new RESTException(
-          "Failed to create request URI from base %s, params %s",
-          fullPath, request.queryParameters());
-    }
-  }
-
-  /**
-   * Encodes the body of an HTTP request as a String. By convention, maps are encoded as form data
-   * and other objects are encoded as JSON.
-   */
-  public static String encodeRequestBody(HTTPRequest request) {
-    Object body = request.body();
-    if (body instanceof Map) {
-      return encodeFormData((Map<?, ?>) body);
-    } else if (body != null) {
-      try {
-        return request.mapper().writeValueAsString(body);
-      } catch (JsonProcessingException e) {
-        throw new RESTException(e, "Failed to encode request body: %s", body);
-      }
-    }
-    return null;
-  }
 }

core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java

@@ -19,17 +19,20 @@
 package org.apache.iceberg.rest;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.List;
 import java.util.Map;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableListMultimap;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
 import org.apache.iceberg.rest.HTTPHeaders.HTTPHeader;
 import org.junit.jupiter.api.Test;
 
 class TestHTTPHeaders {
 
-  final HTTPHeaders headers =
+  private final HTTPHeaders headers =
       HTTPHeaders.of(
           HTTPHeader.of("header1", "value1a"),
           HTTPHeader.of("HEADER1", "value1b"),
@@ -71,6 +74,7 @@ void entries() {
     assertThat(headers.entries("HEADER2")).containsExactly(HTTPHeader.of("header2", "value2"));
     assertThat(headers.entries("header3")).isEmpty();
     assertThat(headers.entries("HEADER3")).isEmpty();
+    assertThat(headers.entries(null)).isEmpty();
   }
 
   @Test
@@ -81,6 +85,7 @@ void contains() {
     assertThat(headers.contains("HEADER2")).isTrue();
     assertThat(headers.contains("header3")).isFalse();
     assertThat(headers.contains("HEADER3")).isFalse();
+    assertThat(headers.contains(null)).isFalse();
   }
 
   @Test
@@ -95,6 +100,9 @@ void addIfAbsentHTTPHeader() {
                 "header1", List.of("value1a", "value1b"),
                 "header2", List.of("value2"),
                 "header3", List.of("value3")));
+
+    assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null))
+        .isInstanceOf(NullPointerException.class);
   }
 
   @Test
@@ -117,6 +125,9 @@ void addIfAbsentHTTPHeaders() {
                     HTTPHeader.of("header2", "value2"),
                     HTTPHeader.of("header3", "value3"))
                 .build());
+
+    assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null))
+        .isInstanceOf(NullPointerException.class);
   }
 
   @Test
@@ -133,6 +144,29 @@ void fromMap() {
                 .addEntry(HTTPHeader.of("header1", "value1b"))
                 .addEntry(HTTPHeader.of("header2", "value2"))
                 .build());
+
+    // invalid input (null name or value)
+    assertThatThrownBy(
+            () -> {
+              Map<String, List<String>> map = Maps.newHashMap();
+              map.put(null, Lists.newArrayList("value1"));
+              HTTPHeaders.fromMap(map);
+            })
+        .isInstanceOf(NullPointerException.class);
+    assertThatThrownBy(
+            () -> {
+              Map<String, List<String>> map = Maps.newHashMap();
+              map.put("header", null);
+              HTTPHeaders.fromMap(map);
+            })
+        .isInstanceOf(NullPointerException.class);
+    assertThatThrownBy(
+            () -> HTTPHeaders.fromMap(Map.of("header1", Lists.newArrayList("value1", null))))
+        .isInstanceOf(NullPointerException.class);
+
+    // invalid input (empty name)
+    assertThatThrownBy(() -> HTTPHeaders.fromMap(Map.of("", List.of("value1"))))
+        .isInstanceOf(IllegalArgumentException.class);
   }
 
   @Test
@@ -148,6 +182,26 @@ void fromSimpleMap() {
                 .addEntry(HTTPHeader.of("header1", "value1a"))
                 .addEntry(HTTPHeader.of("header2", "value2"))
                 .build());
+
+    // invalid input (null name or value)
+    assertThatThrownBy(
+            () -> {
+              Map<String, String> map = Maps.newHashMap();
+              map.put(null, "value1");
+              HTTPHeaders.fromSimpleMap(map);
+            })
+        .isInstanceOf(NullPointerException.class);
+    assertThatThrownBy(
+            () -> {
+              Map<String, String> map = Maps.newHashMap();
+              map.put("header", null);
+              HTTPHeaders.fromSimpleMap(map);
+            })
+        .isInstanceOf(NullPointerException.class);
+
+    // invalid input (empty name)
+    assertThatThrownBy(() -> HTTPHeaders.fromSimpleMap(Map.of("", "value1")))
+        .isInstanceOf(IllegalArgumentException.class);
   }
 
   @Test
@@ -163,5 +217,22 @@ void fromMultiMap() {
                 .addEntry(HTTPHeader.of("header1", "value1b"))
                 .addEntry(HTTPHeader.of("header2", "value2"))
                 .build());
+
+    // invalid input (empty name)
+    assertThatThrownBy(() -> HTTPHeaders.fromMultiMap(ImmutableListMultimap.of("", "value1")))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  void invalidHeader() {
+    // invalid input (null name or value)
+    assertThatThrownBy(() -> HTTPHeader.of(null, "value1"))
+        .isInstanceOf(NullPointerException.class);
+    assertThatThrownBy(() -> HTTPHeader.of("header1", null))
+        .isInstanceOf(NullPointerException.class);
+
+    // invalid input (empty name)
+    assertThatThrownBy(() -> HTTPHeader.of("", "value1"))
+        .isInstanceOf(IllegalArgumentException.class);
   }
 }