From 7db53d8543d40cb767757e45807b158706dd6f42 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Thu, 12 Dec 2024 14:43:11 +0100 Subject: [PATCH 1/8] Auth Manager API part 1: HTTPRequest, HTTPHeader --- .../org/apache/iceberg/rest/HTTPHeaders.java | 168 ++++++++++++++++++ .../org/apache/iceberg/rest/HTTPRequest.java | 91 ++++++++++ .../org/apache/iceberg/rest/RESTUtil.java | 46 +++++ .../apache/iceberg/rest/TestHTTPHeaders.java | 167 +++++++++++++++++ .../org/apache/iceberg/rest/TestRESTUtil.java | 107 +++++++++++ 5 files changed, 579 insertions(+) create mode 100644 core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java create mode 100644 core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java create mode 100644 core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java new file mode 100644 index 000000000000..3407c80b4819 --- /dev/null +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java @@ -0,0 +1,168 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest; + +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.stream.Collectors; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableListMultimap; +import org.apache.iceberg.relocated.com.google.common.collect.ListMultimap; +import org.apache.iceberg.relocated.com.google.common.collect.Multimap; +import org.immutables.value.Value; + +/** + * Represents a group of HTTP headers. + * + *

Header name comparison in this class is always case-insensitive, in accordance with RFC 2616. + * + *

This class exposes methods to convert to and from different representations such as maps and + * multimap, for easier access and manipulation – especially when dealing with multiple headers with + * the same name. + */ +@Value.Style(depluralize = true) +@Value.Immutable +@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"}) +public interface HTTPHeaders { + + HTTPHeaders EMPTY = of(); + + /** Returns all the header entries in this group. */ + List entries(); + + /** + * Returns a map representation of the headers where each header name is mapped to a list of its + * values. Header names are case-insensitive. + */ + @Value.Lazy + default Map> asMap() { + return entries().stream() + .collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT))) + .values() + .stream() + .collect( + Collectors.toMap( + headers -> headers.get(0).name(), + headers -> headers.stream().map(HTTPHeader::value).collect(Collectors.toList()))); + } + + /** + * Returns a simple map representation of the headers where each header name is mapped to its + * first value. If a header has multiple values, only the first value is used. Header names are + * case-insensitive. + */ + @Value.Lazy + default Map asSimpleMap() { + return entries().stream() + .collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT))) + .values() + .stream() + .collect( + Collectors.toMap(headers -> headers.get(0).name(), headers -> headers.get(0).value())); + } + + /** + * Returns a {@link ListMultimap} representation of the headers. Header names are + * case-insensitive. + */ + @Value.Lazy + default ListMultimap asMultiMap() { + return entries().stream() + .collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT))) + .values() + .stream() + .collect( + ImmutableListMultimap.flatteningToImmutableListMultimap( + headers -> headers.get(0).name(), + headers -> headers.stream().map(HTTPHeader::value))); + } + + /** Returns all the entries in this group for the given name (case-insensitive). */ + default List entries(String name) { + return entries().stream() + .filter(header -> header.name().equalsIgnoreCase(name)) + .collect(Collectors.toList()); + } + + /** Returns whether this group contains an entry with the given name (case-insensitive). */ + default boolean contains(String name) { + return entries().stream().anyMatch(header -> header.name().equalsIgnoreCase(name)); + } + + /** + * Adds the given header to the current group if no entry with the same name is already present. + * Returns a new instance with the added header, or the current instance if the header is already + * present. + */ + default HTTPHeaders addIfAbsent(HTTPHeader header) { + return contains(header.name()) + ? this + : ImmutableHTTPHeaders.builder().from(this).addEntry(header).build(); + } + + /** + * Adds the given headers to the current group if no entries with same names are already present. + * Returns a new instance with the added headers, or the current instance if all headers are + * already present. + */ + default HTTPHeaders addIfAbsent(HTTPHeaders headers) { + List newHeaders = + headers.entries().stream().filter(e -> !contains(e.name())).collect(Collectors.toList()); + return newHeaders.isEmpty() + ? this + : ImmutableHTTPHeaders.builder().from(this).addAllEntries(newHeaders).build(); + } + + static HTTPHeaders of(HTTPHeader... headers) { + return ImmutableHTTPHeaders.builder().addEntries(headers).build(); + } + + static HTTPHeaders fromMap(Map> headers) { + ImmutableHTTPHeaders.Builder builder = ImmutableHTTPHeaders.builder(); + headers.forEach( + (name, values) -> values.forEach(value -> builder.addEntry(HTTPHeader.of(name, value)))); + return builder.build(); + } + + static HTTPHeaders fromSimpleMap(Map headers) { + ImmutableHTTPHeaders.Builder builder = ImmutableHTTPHeaders.builder(); + headers.forEach((name, value) -> builder.addEntry(HTTPHeader.of(name, value))); + return builder.build(); + } + + static HTTPHeaders fromMultiMap(Multimap headers) { + return fromMap(headers.asMap()); + } + + /** Represents an HTTP header as a name-value pair. */ + @Value.Style(redactedMask = "****", depluralize = true) + @Value.Immutable + @SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"}) + interface HTTPHeader { + + String name(); + + @Value.Redacted + String value(); + + static HTTPHeader of(String name, String value) { + return ImmutableHTTPHeader.builder().name(name).value(value).build(); + } + } +} diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java b/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java new file mode 100644 index 000000000000..e655a6e9ef22 --- /dev/null +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest; + +import com.fasterxml.jackson.databind.ObjectMapper; +import java.net.URI; +import java.util.Map; +import javax.annotation.Nullable; +import org.immutables.value.Value; + +/** Represents an HTTP request. */ +@Value.Style(redactedMask = "****", depluralize = true) +@Value.Immutable +@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"}) +public interface HTTPRequest { + + enum HTTPMethod { + GET, + HEAD, + POST, + DELETE + } + + /** + * Returns the base URI configured at the REST client level. The base URI is used to construct the + * full {@link #requestUri()}. + */ + URI baseUri(); + + /** + * Returns the full URI of this request. The URI is constructed from the base URI, path, and query + * parameters. It cannot be modified directly. + */ + @Value.Lazy + default URI requestUri() { + return RESTUtil.buildRequestUri(this); + } + + /** Returns the HTTP method of this request. */ + HTTPMethod method(); + + /** Returns the path of this request. */ + String path(); + + /** Returns the query parameters of this request. */ + Map queryParameters(); + + /** Returns the headers of this request. */ + @Value.Default + default HTTPHeaders headers() { + return HTTPHeaders.EMPTY; + } + + /** Returns the raw, unencoded request body. */ + @Nullable + @Value.Redacted + Object body(); + + /** Returns the encoded request body as a string. */ + @Value.Lazy + @Nullable + @Value.Redacted + default String encodedBody() { + return RESTUtil.encodeRequestBody(this); + } + + /** + * Returns the {@link ObjectMapper} to use for encoding the request body. The default is {@link + * RESTObjectMapper#mapper()}. + */ + @Value.Default + default ObjectMapper mapper() { + return RESTObjectMapper.mapper(); + } +} diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java b/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java index fab01162cad7..4074fbdba32b 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java @@ -18,13 +18,18 @@ */ 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; @@ -215,4 +220,45 @@ 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; + } } diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java new file mode 100644 index 000000000000..221f94077120 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest; + +import static org.assertj.core.api.Assertions.assertThat; + +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.rest.HTTPHeaders.HTTPHeader; +import org.junit.jupiter.api.Test; + +class TestHTTPHeaders { + + final HTTPHeaders headers = + HTTPHeaders.of( + HTTPHeader.of("header1", "value1a"), + HTTPHeader.of("HEADER1", "value1b"), + HTTPHeader.of("header2", "value2")); + + @Test + void asMap() { + assertThat(headers.asMap()) + .isEqualTo( + Map.of( + "header1", List.of("value1a", "value1b"), + "header2", List.of("value2"))); + } + + @Test + void asSimpleMap() { + assertThat(headers.asSimpleMap()) + .isEqualTo( + Map.of( + "header1", "value1a", + "header2", "value2")); + } + + @Test + void asMultiMap() { + assertThat(headers.asMultiMap()) + .isEqualTo( + ImmutableListMultimap.of( + "header1", "value1a", "header1", "value1b", "header2", "value2")); + } + + @Test + void entries() { + assertThat(headers.entries("header1")) + .containsExactly(HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b")); + assertThat(headers.entries("HEADER1")) + .containsExactly(HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b")); + assertThat(headers.entries("header2")).containsExactly(HTTPHeader.of("header2", "value2")); + assertThat(headers.entries("HEADER2")).containsExactly(HTTPHeader.of("header2", "value2")); + assertThat(headers.entries("header3")).isEmpty(); + assertThat(headers.entries("HEADER3")).isEmpty(); + } + + @Test + void contains() { + assertThat(headers.contains("header1")).isTrue(); + assertThat(headers.contains("HEADER1")).isTrue(); + assertThat(headers.contains("header2")).isTrue(); + assertThat(headers.contains("HEADER2")).isTrue(); + assertThat(headers.contains("header3")).isFalse(); + assertThat(headers.contains("HEADER3")).isFalse(); + } + + @Test + void addIfAbsentHTTPHeader() { + HTTPHeaders actual = headers.addIfAbsent(HTTPHeader.of("Header1", "value1c")); + assertThat(actual).isSameAs(headers); + + actual = headers.addIfAbsent(HTTPHeader.of("header3", "value3")); + assertThat(actual.asMap()) + .isEqualTo( + Map.of( + "header1", List.of("value1a", "value1b"), + "header2", List.of("value2"), + "header3", List.of("value3"))); + } + + @Test + void addIfAbsentHTTPHeaders() { + HTTPHeaders actual = headers.addIfAbsent(HTTPHeaders.of(HTTPHeader.of("Header1", "value1c"))); + assertThat(actual).isSameAs(headers); + + actual = + headers.addIfAbsent( + ImmutableHTTPHeaders.builder() + .addEntry(HTTPHeader.of("Header1", "value1c")) + .addEntry(HTTPHeader.of("header3", "value3")) + .build()); + assertThat(actual) + .isEqualTo( + ImmutableHTTPHeaders.builder() + .addEntries( + HTTPHeader.of("header1", "value1a"), + HTTPHeader.of("HEADER1", "value1b"), + HTTPHeader.of("header2", "value2"), + HTTPHeader.of("header3", "value3")) + .build()); + } + + @Test + void fromMap() { + HTTPHeaders actual = + HTTPHeaders.fromMap( + ImmutableMap.of( + "header1", List.of("value1a", "value1b"), + "header2", List.of("value2"))); + assertThat(actual) + .isEqualTo( + ImmutableHTTPHeaders.builder() + .addEntry(HTTPHeader.of("header1", "value1a")) + .addEntry(HTTPHeader.of("header1", "value1b")) + .addEntry(HTTPHeader.of("header2", "value2")) + .build()); + } + + @Test + void fromSimpleMap() { + HTTPHeaders actual = + HTTPHeaders.fromSimpleMap( + ImmutableMap.of( + "header1", "value1a", + "header2", "value2")); + assertThat(actual) + .isEqualTo( + ImmutableHTTPHeaders.builder() + .addEntry(HTTPHeader.of("header1", "value1a")) + .addEntry(HTTPHeader.of("header2", "value2")) + .build()); + } + + @Test + void fromMultiMap() { + HTTPHeaders actual = + HTTPHeaders.fromMultiMap( + ImmutableListMultimap.of( + "header1", "value1a", "header2", "value2", "header1", "value1b")); + assertThat(actual) + .isEqualTo( + ImmutableHTTPHeaders.builder() + .addEntry(HTTPHeader.of("header1", "value1a")) + .addEntry(HTTPHeader.of("header1", "value1b")) + .addEntry(HTTPHeader.of("header2", "value2")) + .build()); + } +} diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java b/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java index c7667d90ac6f..1ca4aabb822c 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java @@ -20,11 +20,20 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +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.rest.HTTPRequest.HTTPMethod; +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; public class TestRESTUtil { @@ -139,4 +148,102 @@ public void testOAuth2FormDataDecoding() { assertThat(RESTUtil.decodeFormData(formString)).isEqualTo(expected); } + + @ParameterizedTest + @MethodSource("validRequestUris") + public void validRequestUris(HTTPRequest request, URI expected) { + assertThat(RESTUtil.buildRequestUri(request)).isEqualTo(expected); + } + + public static Stream validRequestUris() { + return Stream.of( + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost:8080/foo")) + .method(HTTPMethod.GET) + .path("v1/namespaces/ns/tables/") // trailing slash should be removed + .putQueryParameter("pageToken", "1234") + .putQueryParameter("pageSize", "10") + .build(), + URI.create( + "http://localhost:8080/foo/v1/namespaces/ns/tables?pageToken=1234&pageSize=10")), + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost:8080/foo")) + .method(HTTPMethod.GET) + .path("https://authserver.com/token") // absolute path HTTPS + .build(), + URI.create("https://authserver.com/token")), + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost:8080/foo")) + .method(HTTPMethod.GET) + .path("http://authserver.com/token") // absolute path HTTP + .build(), + URI.create("http://authserver.com/token"))); + } + + @Test + public void buildRequestUriFailures() { + HTTPRequest request = + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost")) + .method(HTTPMethod.GET) + .path("/v1/namespaces") // wrong leading slash + .build(); + assertThatThrownBy(() -> RESTUtil.buildRequestUri(request)) + .isInstanceOf(RESTException.class) + .hasMessage( + "Received a malformed path for a REST request: /v1/namespaces. Paths should not start with /"); + HTTPRequest request2 = + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost")) + .method(HTTPMethod.GET) + .path(" not a valid path") // wrong path + .build(); + assertThatThrownBy(() -> RESTUtil.buildRequestUri(request2)) + .isInstanceOf(RESTException.class) + .hasMessage( + "Failed to create request URI from base http://localhost/ not a valid path, params {}"); + } + + @ParameterizedTest + @MethodSource("encodeRequestBody") + public void encodeRequestBody(HTTPRequest request, String expected) { + assertThat(RESTUtil.encodeRequestBody(request)).isEqualTo(expected); + } + + public static Stream encodeRequestBody() { + return Stream.of( + // form data + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost")) + .method(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(), + "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(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\"}}")); + } } From e486e0d9faf046de84c1cdb9a3545eb0458f4a72 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 13 Dec 2024 10:27:39 +0100 Subject: [PATCH 2/8] review --- .../org/apache/iceberg/rest/HTTPHeaders.java | 7 + .../org/apache/iceberg/rest/HTTPRequest.java | 39 ++++- .../org/apache/iceberg/rest/RESTUtil.java | 46 ------ .../apache/iceberg/rest/TestHTTPHeaders.java | 73 ++++++++- .../apache/iceberg/rest/TestHTTPRequest.java | 138 ++++++++++++++++++ .../org/apache/iceberg/rest/TestRESTUtil.java | 107 -------------- 6 files changed, 254 insertions(+), 156 deletions(-) create mode 100644 core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java 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 3407c80b4819..5b1cac7f2d2b 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java +++ b/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(); } diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java b/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java index e655a6e9ef22..41921d946ca8 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPRequest.java +++ b/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()); + } + } } diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java b/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java index 4074fbdba32b..fab01162cad7 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTUtil.java +++ b/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; - } } 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 221f94077120..7bd25672484d 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java +++ b/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> map = Maps.newHashMap(); + map.put(null, Lists.newArrayList("value1")); + HTTPHeaders.fromMap(map); + }) + .isInstanceOf(NullPointerException.class); + assertThatThrownBy( + () -> { + Map> 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 map = Maps.newHashMap(); + map.put(null, "value1"); + HTTPHeaders.fromSimpleMap(map); + }) + .isInstanceOf(NullPointerException.class); + assertThatThrownBy( + () -> { + Map 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); } } diff --git a/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java b/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java new file mode 100644 index 000000000000..3cc6d520b591 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.net.URI; +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.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; + +class TestHTTPRequest { + + @ParameterizedTest + @MethodSource("validRequestUris") + public void requestUriSuccess(HTTPRequest request, URI expected) { + assertThat(request.requestUri()).isEqualTo(expected); + } + + public static Stream validRequestUris() { + return Stream.of( + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost:8080/foo")) + .method(HTTPRequest.HTTPMethod.GET) + .path("v1/namespaces/ns/tables/") // trailing slash should be removed + .putQueryParameter("pageToken", "1234") + .putQueryParameter("pageSize", "10") + .build(), + URI.create( + "http://localhost:8080/foo/v1/namespaces/ns/tables?pageToken=1234&pageSize=10")), + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost:8080/foo")) + .method(HTTPRequest.HTTPMethod.GET) + .path("https://authserver.com/token") // absolute path HTTPS + .build(), + URI.create("https://authserver.com/token")), + Arguments.of( + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost:8080/foo")) + .method(HTTPRequest.HTTPMethod.GET) + .path("http://authserver.com/token") // absolute path HTTP + .build(), + URI.create("http://authserver.com/token"))); + } + + @Test + public void malformedPath() { + assertThatThrownBy( + () -> + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost")) + .method(HTTPRequest.HTTPMethod.GET) + .path("/v1/namespaces") // wrong leading slash + .build()) + .isInstanceOf(RESTException.class) + .hasMessage( + "Received a malformed path for a REST request: /v1/namespaces. Paths should not start with /"); + } + + @Test + public void invalidPath() { + HTTPRequest request = + ImmutableHTTPRequest.builder() + .baseUri(URI.create("http://localhost")) + .method(HTTPRequest.HTTPMethod.GET) + .path(" not a valid path") // wrong path + .build(); + assertThatThrownBy(request::requestUri) + .isInstanceOf(RESTException.class) + .hasMessage( + "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); + } + + public static Stream 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(), + "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\"}}")); + } +} diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java b/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java index 1ca4aabb822c..c7667d90ac6f 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java @@ -20,20 +20,11 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -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.rest.HTTPRequest.HTTPMethod; -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; public class TestRESTUtil { @@ -148,102 +139,4 @@ public void testOAuth2FormDataDecoding() { assertThat(RESTUtil.decodeFormData(formString)).isEqualTo(expected); } - - @ParameterizedTest - @MethodSource("validRequestUris") - public void validRequestUris(HTTPRequest request, URI expected) { - assertThat(RESTUtil.buildRequestUri(request)).isEqualTo(expected); - } - - public static Stream validRequestUris() { - return Stream.of( - Arguments.of( - ImmutableHTTPRequest.builder() - .baseUri(URI.create("http://localhost:8080/foo")) - .method(HTTPMethod.GET) - .path("v1/namespaces/ns/tables/") // trailing slash should be removed - .putQueryParameter("pageToken", "1234") - .putQueryParameter("pageSize", "10") - .build(), - URI.create( - "http://localhost:8080/foo/v1/namespaces/ns/tables?pageToken=1234&pageSize=10")), - Arguments.of( - ImmutableHTTPRequest.builder() - .baseUri(URI.create("http://localhost:8080/foo")) - .method(HTTPMethod.GET) - .path("https://authserver.com/token") // absolute path HTTPS - .build(), - URI.create("https://authserver.com/token")), - Arguments.of( - ImmutableHTTPRequest.builder() - .baseUri(URI.create("http://localhost:8080/foo")) - .method(HTTPMethod.GET) - .path("http://authserver.com/token") // absolute path HTTP - .build(), - URI.create("http://authserver.com/token"))); - } - - @Test - public void buildRequestUriFailures() { - HTTPRequest request = - ImmutableHTTPRequest.builder() - .baseUri(URI.create("http://localhost")) - .method(HTTPMethod.GET) - .path("/v1/namespaces") // wrong leading slash - .build(); - assertThatThrownBy(() -> RESTUtil.buildRequestUri(request)) - .isInstanceOf(RESTException.class) - .hasMessage( - "Received a malformed path for a REST request: /v1/namespaces. Paths should not start with /"); - HTTPRequest request2 = - ImmutableHTTPRequest.builder() - .baseUri(URI.create("http://localhost")) - .method(HTTPMethod.GET) - .path(" not a valid path") // wrong path - .build(); - assertThatThrownBy(() -> RESTUtil.buildRequestUri(request2)) - .isInstanceOf(RESTException.class) - .hasMessage( - "Failed to create request URI from base http://localhost/ not a valid path, params {}"); - } - - @ParameterizedTest - @MethodSource("encodeRequestBody") - public void encodeRequestBody(HTTPRequest request, String expected) { - assertThat(RESTUtil.encodeRequestBody(request)).isEqualTo(expected); - } - - public static Stream encodeRequestBody() { - return Stream.of( - // form data - Arguments.of( - ImmutableHTTPRequest.builder() - .baseUri(URI.create("http://localhost")) - .method(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(), - "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(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\"}}")); - } } From 0449744f7227af8a7507cdffdc71e24e3172518f Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 13 Dec 2024 12:44:32 +0100 Subject: [PATCH 3/8] remove static methods --- .../org/apache/iceberg/rest/HTTPHeaders.java | 69 --------- .../apache/iceberg/rest/TestHTTPHeaders.java | 137 +----------------- 2 files changed, 6 insertions(+), 200 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 5b1cac7f2d2b..4d99f0494224 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java @@ -19,12 +19,7 @@ package org.apache.iceberg.rest; import java.util.List; -import java.util.Locale; -import java.util.Map; import java.util.stream.Collectors; -import org.apache.iceberg.relocated.com.google.common.collect.ImmutableListMultimap; -import org.apache.iceberg.relocated.com.google.common.collect.ListMultimap; -import org.apache.iceberg.relocated.com.google.common.collect.Multimap; import org.immutables.value.Value; /** @@ -46,53 +41,6 @@ public interface HTTPHeaders { /** Returns all the header entries in this group. */ List entries(); - /** - * Returns a map representation of the headers where each header name is mapped to a list of its - * values. Header names are case-insensitive. - */ - @Value.Lazy - default Map> asMap() { - return entries().stream() - .collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT))) - .values() - .stream() - .collect( - Collectors.toMap( - headers -> headers.get(0).name(), - headers -> headers.stream().map(HTTPHeader::value).collect(Collectors.toList()))); - } - - /** - * Returns a simple map representation of the headers where each header name is mapped to its - * first value. If a header has multiple values, only the first value is used. Header names are - * case-insensitive. - */ - @Value.Lazy - default Map asSimpleMap() { - return entries().stream() - .collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT))) - .values() - .stream() - .collect( - Collectors.toMap(headers -> headers.get(0).name(), headers -> headers.get(0).value())); - } - - /** - * Returns a {@link ListMultimap} representation of the headers. Header names are - * case-insensitive. - */ - @Value.Lazy - default ListMultimap asMultiMap() { - return entries().stream() - .collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT))) - .values() - .stream() - .collect( - ImmutableListMultimap.flatteningToImmutableListMultimap( - headers -> headers.get(0).name(), - headers -> headers.stream().map(HTTPHeader::value))); - } - /** Returns all the entries in this group for the given name (case-insensitive). */ default List entries(String name) { return entries().stream() @@ -133,23 +81,6 @@ static HTTPHeaders of(HTTPHeader... headers) { return ImmutableHTTPHeaders.builder().addEntries(headers).build(); } - static HTTPHeaders fromMap(Map> headers) { - ImmutableHTTPHeaders.Builder builder = ImmutableHTTPHeaders.builder(); - headers.forEach( - (name, values) -> values.forEach(value -> builder.addEntry(HTTPHeader.of(name, value)))); - return builder.build(); - } - - static HTTPHeaders fromSimpleMap(Map headers) { - ImmutableHTTPHeaders.Builder builder = ImmutableHTTPHeaders.builder(); - headers.forEach((name, value) -> builder.addEntry(HTTPHeader.of(name, value))); - return builder.build(); - } - - static HTTPHeaders fromMultiMap(Multimap headers) { - return fromMap(headers.asMap()); - } - /** Represents an HTTP header as a name-value pair. */ @Value.Style(redactedMask = "****", depluralize = true) @Value.Immutable 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 7bd25672484d..2dcc8bd1af4b 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java @@ -21,12 +21,6 @@ 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; @@ -38,32 +32,6 @@ class TestHTTPHeaders { HTTPHeader.of("HEADER1", "value1b"), HTTPHeader.of("header2", "value2")); - @Test - void asMap() { - assertThat(headers.asMap()) - .isEqualTo( - Map.of( - "header1", List.of("value1a", "value1b"), - "header2", List.of("value2"))); - } - - @Test - void asSimpleMap() { - assertThat(headers.asSimpleMap()) - .isEqualTo( - Map.of( - "header1", "value1a", - "header2", "value2")); - } - - @Test - void asMultiMap() { - assertThat(headers.asMultiMap()) - .isEqualTo( - ImmutableListMultimap.of( - "header1", "value1a", "header1", "value1b", "header2", "value2")); - } - @Test void entries() { assertThat(headers.entries("header1")) @@ -94,12 +62,12 @@ void addIfAbsentHTTPHeader() { assertThat(actual).isSameAs(headers); actual = headers.addIfAbsent(HTTPHeader.of("header3", "value3")); - assertThat(actual.asMap()) - .isEqualTo( - Map.of( - "header1", List.of("value1a", "value1b"), - "header2", List.of("value2"), - "header3", List.of("value3"))); + assertThat(actual.entries()) + .containsExactly( + HTTPHeader.of("header1", "value1a"), + HTTPHeader.of("HEADER1", "value1b"), + HTTPHeader.of("header2", "value2"), + HTTPHeader.of("header3", "value3")); assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null)) .isInstanceOf(NullPointerException.class); @@ -130,99 +98,6 @@ void addIfAbsentHTTPHeaders() { .isInstanceOf(NullPointerException.class); } - @Test - void fromMap() { - HTTPHeaders actual = - HTTPHeaders.fromMap( - ImmutableMap.of( - "header1", List.of("value1a", "value1b"), - "header2", List.of("value2"))); - assertThat(actual) - .isEqualTo( - ImmutableHTTPHeaders.builder() - .addEntry(HTTPHeader.of("header1", "value1a")) - .addEntry(HTTPHeader.of("header1", "value1b")) - .addEntry(HTTPHeader.of("header2", "value2")) - .build()); - - // invalid input (null name or value) - assertThatThrownBy( - () -> { - Map> map = Maps.newHashMap(); - map.put(null, Lists.newArrayList("value1")); - HTTPHeaders.fromMap(map); - }) - .isInstanceOf(NullPointerException.class); - assertThatThrownBy( - () -> { - Map> 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 - void fromSimpleMap() { - HTTPHeaders actual = - HTTPHeaders.fromSimpleMap( - ImmutableMap.of( - "header1", "value1a", - "header2", "value2")); - assertThat(actual) - .isEqualTo( - ImmutableHTTPHeaders.builder() - .addEntry(HTTPHeader.of("header1", "value1a")) - .addEntry(HTTPHeader.of("header2", "value2")) - .build()); - - // invalid input (null name or value) - assertThatThrownBy( - () -> { - Map map = Maps.newHashMap(); - map.put(null, "value1"); - HTTPHeaders.fromSimpleMap(map); - }) - .isInstanceOf(NullPointerException.class); - assertThatThrownBy( - () -> { - Map 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 - void fromMultiMap() { - HTTPHeaders actual = - HTTPHeaders.fromMultiMap( - ImmutableListMultimap.of( - "header1", "value1a", "header2", "value2", "header1", "value1b")); - assertThat(actual) - .isEqualTo( - ImmutableHTTPHeaders.builder() - .addEntry(HTTPHeader.of("header1", "value1a")) - .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) From d9a05fb1ea5ce3ed552a444d6b0e834220316dd4 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 13 Dec 2024 12:49:59 +0100 Subject: [PATCH 4/8] verify error messages --- .../org/apache/iceberg/rest/HTTPHeaders.java | 3 +++ .../apache/iceberg/rest/TestHTTPHeaders.java | 17 +++++++++++------ 2 files changed, 14 insertions(+), 6 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 4d99f0494224..2767fc6c5a49 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java @@ -19,6 +19,7 @@ package org.apache.iceberg.rest; import java.util.List; +import java.util.Objects; import java.util.stream.Collectors; import org.immutables.value.Value; @@ -59,6 +60,7 @@ default boolean contains(String name) { * present. */ default HTTPHeaders addIfAbsent(HTTPHeader header) { + Objects.requireNonNull(header, "header"); return contains(header.name()) ? this : ImmutableHTTPHeaders.builder().from(this).addEntry(header).build(); @@ -70,6 +72,7 @@ default HTTPHeaders addIfAbsent(HTTPHeader header) { * already present. */ default HTTPHeaders addIfAbsent(HTTPHeaders headers) { + Objects.requireNonNull(headers, "headers"); List newHeaders = headers.entries().stream().filter(e -> !contains(e.name())).collect(Collectors.toList()); return newHeaders.isEmpty() 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 2dcc8bd1af4b..bff1f6121c57 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java @@ -69,8 +69,9 @@ void addIfAbsentHTTPHeader() { HTTPHeader.of("header2", "value2"), HTTPHeader.of("header3", "value3")); - assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null)) - .isInstanceOf(NullPointerException.class); + assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeader) null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("header"); } @Test @@ -95,19 +96,23 @@ void addIfAbsentHTTPHeaders() { .build()); assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null)) - .isInstanceOf(NullPointerException.class); + .isInstanceOf(NullPointerException.class) + .hasMessage("headers"); } @Test void invalidHeader() { // invalid input (null name or value) assertThatThrownBy(() -> HTTPHeader.of(null, "value1")) - .isInstanceOf(NullPointerException.class); + .isInstanceOf(NullPointerException.class) + .hasMessage("name"); assertThatThrownBy(() -> HTTPHeader.of("header1", null)) - .isInstanceOf(NullPointerException.class); + .isInstanceOf(NullPointerException.class) + .hasMessage("value"); // invalid input (empty name) assertThatThrownBy(() -> HTTPHeader.of("", "value1")) - .isInstanceOf(IllegalArgumentException.class); + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Header name cannot be empty"); } } From 4c0e650e2faf7e538c4c570bff76b231a1b6912b Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 13 Dec 2024 13:09:15 +0100 Subject: [PATCH 5/8] checkstyle --- core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java | 6 +++--- 1 file changed, 3 insertions(+), 3 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 2767fc6c5a49..9563266b28a4 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java @@ -19,8 +19,8 @@ package org.apache.iceberg.rest; import java.util.List; -import java.util.Objects; import java.util.stream.Collectors; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.immutables.value.Value; /** @@ -60,7 +60,7 @@ default boolean contains(String name) { * present. */ default HTTPHeaders addIfAbsent(HTTPHeader header) { - Objects.requireNonNull(header, "header"); + Preconditions.checkNotNull(header, "header"); return contains(header.name()) ? this : ImmutableHTTPHeaders.builder().from(this).addEntry(header).build(); @@ -72,7 +72,7 @@ default HTTPHeaders addIfAbsent(HTTPHeader header) { * already present. */ default HTTPHeaders addIfAbsent(HTTPHeaders headers) { - Objects.requireNonNull(headers, "headers"); + Preconditions.checkNotNull(headers, "headers"); List newHeaders = headers.entries().stream().filter(e -> !contains(e.name())).collect(Collectors.toList()); return newHeaders.isEmpty() From 56ff7b21c91e5ae664102d7e76cceb431a0f93f1 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Fri, 13 Dec 2024 14:41:24 +0100 Subject: [PATCH 6/8] 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 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 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 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(); } } From ca6eb1e7a385c775efe25dde8bd8f947b2dec248 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Mon, 16 Dec 2024 19:12:22 +0100 Subject: [PATCH 7/8] review --- .../org/apache/iceberg/rest/HTTPHeaders.java | 11 +++---- .../apache/iceberg/rest/TestHTTPHeaders.java | 29 ++++++++++--------- 2 files changed, 22 insertions(+), 18 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 e7807a8cb071..35710bd9a9b7 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java @@ -19,6 +19,7 @@ package org.apache.iceberg.rest; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.immutables.value.Value; @@ -40,13 +41,13 @@ public interface HTTPHeaders { HTTPHeaders EMPTY = of(); /** Returns all the header entries in this group. */ - List entries(); + Set entries(); /** Returns all the entries in this group for the given name (case-insensitive). */ - default List entries(String name) { + default Set entries(String name) { return entries().stream() .filter(header -> header.name().equalsIgnoreCase(name)) - .collect(Collectors.toList()); + .collect(Collectors.toSet()); } /** Returns whether this group contains an entry with the given name (case-insensitive). */ @@ -59,7 +60,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 withHeaderIfAbsent(HTTPHeader header) { + default HTTPHeaders putIfAbsent(HTTPHeader header) { Preconditions.checkNotNull(header, "header"); return contains(header.name()) ? this @@ -71,7 +72,7 @@ default HTTPHeaders withHeaderIfAbsent(HTTPHeader header) { * Returns a new instance with the added headers, or the current instance if all headers are * already present. */ - default HTTPHeaders withHeaderIfAbsent(HTTPHeaders headers) { + default HTTPHeaders putIfAbsent(HTTPHeaders headers) { Preconditions.checkNotNull(headers, "headers"); List 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 e8fc493ad393..53d37186f7cc 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java @@ -35,11 +35,15 @@ class TestHTTPHeaders { @Test void entries() { assertThat(headers.entries("header1")) - .containsExactly(HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b")); + .containsExactlyInAnyOrder( + HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b")); assertThat(headers.entries("HEADER1")) - .containsExactly(HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b")); - assertThat(headers.entries("header2")).containsExactly(HTTPHeader.of("header2", "value2")); - assertThat(headers.entries("HEADER2")).containsExactly(HTTPHeader.of("header2", "value2")); + .containsExactlyInAnyOrder( + HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b")); + assertThat(headers.entries("header2")) + .containsExactlyInAnyOrder(HTTPHeader.of("header2", "value2")); + assertThat(headers.entries("HEADER2")) + .containsExactlyInAnyOrder(HTTPHeader.of("header2", "value2")); assertThat(headers.entries("header3")).isEmpty(); assertThat(headers.entries("HEADER3")).isEmpty(); assertThat(headers.entries(null)).isEmpty(); @@ -57,11 +61,11 @@ void contains() { } @Test - void withHeaderIfAbsentHTTPHeader() { - HTTPHeaders actual = headers.withHeaderIfAbsent(HTTPHeader.of("Header1", "value1c")); + void putIfAbsentHTTPHeader() { + HTTPHeaders actual = headers.putIfAbsent(HTTPHeader.of("Header1", "value1c")); assertThat(actual).isSameAs(headers); - actual = headers.withHeaderIfAbsent(HTTPHeader.of("header3", "value3")); + actual = headers.putIfAbsent(HTTPHeader.of("header3", "value3")); assertThat(actual.entries()) .containsExactly( HTTPHeader.of("header1", "value1a"), @@ -69,19 +73,18 @@ void withHeaderIfAbsentHTTPHeader() { HTTPHeader.of("header2", "value2"), HTTPHeader.of("header3", "value3")); - assertThatThrownBy(() -> headers.withHeaderIfAbsent((HTTPHeader) null)) + assertThatThrownBy(() -> headers.putIfAbsent((HTTPHeader) null)) .isInstanceOf(NullPointerException.class) .hasMessage("header"); } @Test - void withHeaderIfAbsentHTTPHeaders() { - HTTPHeaders actual = - headers.withHeaderIfAbsent(HTTPHeaders.of(HTTPHeader.of("Header1", "value1c"))); + void putIfAbsentHTTPHeaders() { + HTTPHeaders actual = headers.putIfAbsent(HTTPHeaders.of(HTTPHeader.of("Header1", "value1c"))); assertThat(actual).isSameAs(headers); actual = - headers.withHeaderIfAbsent( + headers.putIfAbsent( ImmutableHTTPHeaders.builder() .addEntry(HTTPHeader.of("Header1", "value1c")) .addEntry(HTTPHeader.of("header3", "value3")) @@ -96,7 +99,7 @@ void withHeaderIfAbsentHTTPHeaders() { HTTPHeader.of("header3", "value3")) .build()); - assertThatThrownBy(() -> headers.withHeaderIfAbsent((HTTPHeaders) null)) + assertThatThrownBy(() -> headers.putIfAbsent((HTTPHeaders) null)) .isInstanceOf(NullPointerException.class) .hasMessage("headers"); } From 2848e4205e6559d4d37dfb45fb51e49c5534e423 Mon Sep 17 00:00:00 2001 From: Alexandre Dutra Date: Tue, 17 Dec 2024 11:29:19 +0100 Subject: [PATCH 8/8] review --- .../org/apache/iceberg/rest/TestHTTPHeaders.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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 53d37186f7cc..9380073f7643 100644 --- a/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java +++ b/core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java @@ -34,6 +34,21 @@ class TestHTTPHeaders { @Test void entries() { + assertThat(headers.entries()) + .containsExactlyInAnyOrder( + HTTPHeader.of("header1", "value1a"), + HTTPHeader.of("HEADER1", "value1b"), + HTTPHeader.of("header2", "value2")); + + // duplicated entries + assertThat( + HTTPHeaders.of(HTTPHeader.of("header1", "value1"), HTTPHeader.of("header1", "value1")) + .entries()) + .containsExactly(HTTPHeader.of("header1", "value1")); + } + + @Test + void entriesByName() { assertThat(headers.entries("header1")) .containsExactlyInAnyOrder( HTTPHeader.of("header1", "value1a"), HTTPHeader.of("HEADER1", "value1b"));