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(); } }