Skip to content

Commit

Permalink
review
Browse files Browse the repository at this point in the history
  • Loading branch information
@adutra
adutra committed Dec 13, 2024
1 parent 4c0e650 commit 56ff7b2
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 43 deletions.
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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());
Expand Down
17 changes: 9 additions & 8 deletions core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,30 +57,31 @@ 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"),
HTTPHeader.of("HEADER1", "value1b"),
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"))
Expand All @@ -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");
}
Expand Down
115 changes: 82 additions & 33 deletions core/src/test/java/org/apache/iceberg/rest/TestHTTPRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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();
}
}

0 comments on commit 56ff7b2

Please sign in to comment.