Skip to content

Commit

Permalink
Core: Fix CreateTableRequest to use field names from the REST spec (a…
Browse files Browse the repository at this point in the history 
…pache#5135)
  • Loading branch information
@kbendick
kbendick authored Jun 28, 2022
1 parent 2c6f097 commit 71cbe8b
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 57 deletions.
33 changes: 17 additions & 16 deletions core/src/main/java/org/apache/iceberg/rest/requests/CreateTableRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,22 +40,22 @@ public class CreateTableRequest implements RESTRequest {
private String name;
private String location;
private Schema schema;
private UnboundPartitionSpec spec;
private UnboundSortOrder order;
private UnboundPartitionSpec partitionSpec;
private UnboundSortOrder writeOrder;
private Map<String, String> properties;
private Boolean stageCreate = false;

public CreateTableRequest() {
// Needed for Jackson Deserialization.
}

private CreateTableRequest(String name, String location, Schema schema, PartitionSpec spec, SortOrder order,
Map<String, String> properties, boolean stageCreate) {
private CreateTableRequest(String name, String location, Schema schema, PartitionSpec partitionSpec,
SortOrder writeOrder, Map<String, String> properties, boolean stageCreate) {
this.name = name;
this.location = location;
this.schema = schema;
this.spec = spec != null ? spec.toUnbound() : null;
this.order = order != null ? order.toUnbound() : null;
this.partitionSpec = partitionSpec != null ? partitionSpec.toUnbound() : null;
this.writeOrder = writeOrder != null ? writeOrder.toUnbound() : null;
this.properties = properties;
this.stageCreate = stageCreate;
validate();
Expand All @@ -81,11 +81,11 @@ public Schema schema() {
}

public PartitionSpec spec() {
return spec != null ? spec.bind(schema) : null;
return partitionSpec != null ? partitionSpec.bind(schema) : null;
}

public SortOrder writeOrder() {
return order != null ? order.bind(schema) : null;
return writeOrder != null ? writeOrder.bind(schema) : null;
}

public Map<String, String> properties() {
Expand All @@ -103,8 +103,9 @@ public String toString() {
.add("location", location)
.add("properties", properties)
.add("schema", schema)
.add("spec", spec)
.add("order", order)
.add("partitionSpec", partitionSpec)
.add("writeOrder", writeOrder)
.add("stageCreate", stageCreate)
.toString();
}

Expand All @@ -116,8 +117,8 @@ public static class Builder {
private String name;
private String location;
private Schema schema;
private PartitionSpec spec;
private SortOrder order;
private PartitionSpec partitionSpec;
private SortOrder writeOrder;
private final ImmutableMap.Builder<String, String> properties = ImmutableMap.builder();
private boolean stageCreate = false;

Expand Down Expand Up @@ -158,12 +159,12 @@ public Builder withSchema(Schema tableSchema) {
}

public Builder withPartitionSpec(PartitionSpec tableSpec) {
this.spec = tableSpec;
this.partitionSpec = tableSpec;
return this;
}

public Builder withWriteOrder(SortOrder writeOrder) {
this.order = writeOrder;
public Builder withWriteOrder(SortOrder order) {
this.writeOrder = order;
return this;
}

Expand All @@ -173,7 +174,7 @@ public Builder stageCreate() {
}

public CreateTableRequest build() {
return new CreateTableRequest(name, location, schema, spec, order, properties.build(), stageCreate);
return new CreateTableRequest(name, location, schema, partitionSpec, writeOrder, properties.build(), stageCreate);
}
}
}
28 changes: 23 additions & 5 deletions core/src/test/java/org/apache/iceberg/rest/RequestResponseTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,13 @@
package org.apache.iceberg.rest;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.util.Collections;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

public abstract class RequestResponseTestBase<T extends RESTMessage> {
Expand Down Expand Up @@ -66,15 +71,28 @@ public String serialize(T object) throws JsonProcessingException {
}

/**
* This test ensures that only the fields that are expected, e.g. from the spec, are found on the class.
* If new fields are added to the spec, they should be added to the function
* {@link RequestResponseTestBase#allFieldsFromSpec()}
* This test ensures that the serialized JSON of each class has only fields that are expected from the spec.
* Only top level fields are checked presently, as nested fields generally come from some existing type that is
* tested elsewhere.
* The fields from the spec should be populated into each subclass's
* {@link RequestResponseTestBase#allFieldsFromSpec()}.
*/
@Test
public void testHasOnlyKnownFields() {
T value = createExampleInstance();
Set<String> fieldsFromSpec = Sets.newHashSet();
Collections.addAll(fieldsFromSpec, allFieldsFromSpec());
try {
JsonNode node = mapper().readValue(serialize(createExampleInstance()), JsonNode.class);
for (String field : fieldsFromSpec) {
Assert.assertTrue("Should have field: " + field, node.has(field));
}

Assertions.assertThat(value).hasOnlyFields(allFieldsFromSpec());
for (String field : ((Iterable<? extends String>) node::fieldNames)) {
Assert.assertTrue("Should not have field: " + field, fieldsFromSpec.contains(field));
}
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}

/**
Expand Down
27 changes: 14 additions & 13 deletions core/src/test/java/org/apache/iceberg/rest/requests/TestCreateTableRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ public void testRoundTripSerDe() throws JsonProcessingException {
String fullJsonRaw =
"{\"name\":\"test_tbl\",\"location\":\"file://tmp/location/\",\"schema\":{\"type\":\"struct\"," +
"\"schema-id\":0,\"fields\":[{\"id\":1,\"name\":\"id\",\"required\":true,\"type\":\"int\"}," +
"{\"id\":2,\"name\":\"data\",\"required\":false,\"type\":\"string\"}]},\"spec\":{\"spec-id\":0," +
"{\"id\":2,\"name\":\"data\",\"required\":false,\"type\":\"string\"}]},\"partition-spec\":{\"spec-id\":0," +
"\"fields\":[{\"name\":\"id_bucket\",\"transform\":\"bucket[16]\",\"source-id\":1,\"field-id\":1000}]}," +
"\"order\":{\"order-id\":1,\"fields\":" +
"\"write-order\":{\"order-id\":1,\"fields\":" +
"[{\"transform\":\"identity\",\"source-id\":2,\"direction\":\"asc\",\"null-order\":\"nulls-last\"}]}," +
"\"properties\":{\"owner\":\"Hank\"},\"stage-create\":false}";

Expand All @@ -82,8 +82,8 @@ public void testRoundTripSerDe() throws JsonProcessingException {

// The same JSON but using existing parsers for clarity and staging the request instead of committing
String jsonStagedReq = String.format(
"{\"name\":\"%s\",\"location\":\"%s\",\"schema\":%s,\"spec\":%s," +
"\"order\":%s,\"properties\":%s,\"stage-create\":%b}",
"{\"name\":\"%s\",\"location\":\"%s\",\"schema\":%s,\"partition-spec\":%s," +
"\"write-order\":%s,\"properties\":%s,\"stage-create\":%b}",
SAMPLE_NAME, SAMPLE_LOCATION, SchemaParser.toJson(SAMPLE_SCHEMA),
PartitionSpecParser.toJson(SAMPLE_SPEC.toUnbound()), SortOrderParser.toJson(SAMPLE_WRITE_ORDER.toUnbound()),
mapper().writeValueAsString(SAMPLE_PROPERTIES), true);
Expand All @@ -102,8 +102,8 @@ public void testRoundTripSerDe() throws JsonProcessingException {

// Partition spec and write order can be null or use PartitionSpec.unpartitioned() and SortOrder.unsorted()
String jsonWithExplicitUnsortedUnordered = String.format(
"{\"name\":\"%s\",\"location\":null,\"schema\":%s,\"spec\":%s," +
"\"order\":%s,\"properties\":{},\"stage-create\":%b}",
"{\"name\":\"%s\",\"location\":null,\"schema\":%s,\"partition-spec\":%s," +
"\"write-order\":%s,\"properties\":{},\"stage-create\":%b}",
SAMPLE_NAME, SchemaParser.toJson(SAMPLE_SCHEMA),
PartitionSpecParser.toJson(PartitionSpec.unpartitioned()),
SortOrderParser.toJson(SortOrder.unsorted().toUnbound()),
Expand All @@ -122,8 +122,8 @@ public void testRoundTripSerDe() throws JsonProcessingException {
jsonWithExplicitUnsortedUnordered, reqOnlyRequiredFieldsExplicitDefaults);

String jsonOnlyRequiredFieldsNullAsDefault = String.format(
"{\"name\":\"%s\",\"location\":null,\"schema\":%s,\"spec\":null,\"order\":null,\"properties\":{}," +
"\"stage-create\":false}",
"{\"name\":\"%s\",\"location\":null,\"schema\":%s,\"partition-spec\":null,\"write-order\":null," +
"\"properties\":{},\"stage-create\":false}",
SAMPLE_NAME, SchemaParser.toJson(SAMPLE_SCHEMA));

CreateTableRequest reqOnlyRequiredFieldsMissingDefaults = CreateTableRequest.builder()
Expand Down Expand Up @@ -155,7 +155,8 @@ public void testCanDeserializeWithoutDefaultValues() throws JsonProcessingExcept
@Test
public void testDeserializeInvalidRequest() {
String jsonMissingSchema =
"{\"name\":\"foo\",\"location\":null,\"spec\":null,\"order\":null,\"properties\":{},\"stage-create\":false}";
"{\"name\":\"foo\",\"location\":null,\"partition-spec\":null,\"write-order\":null,\"properties\":{}," +
"\"stage-create\":false}";
AssertHelpers.assertThrows(
"A JSON request with the keys spelled incorrectly should fail to deserialize and validate",
IllegalArgumentException.class,
Expand All @@ -164,7 +165,7 @@ public void testDeserializeInvalidRequest() {
);

String jsonMissingName = String.format(
"{\"location\":null,\"schema\":%s,\"spec\":null,\"order\":null,\"properties\":{}," +
"{\"location\":null,\"schema\":%s,\"spec\":null,\"write-order\":null,\"properties\":{}," +
"\"stage-create\":false}", SAMPLE_SCHEMA_JSON);
AssertHelpers.assertThrows(
"A JSON request with the keys spelled incorrectly should fail to deserialize and validate",
Expand All @@ -174,8 +175,8 @@ public void testDeserializeInvalidRequest() {
);

String jsonIncorrectTypeForProperties = String.format(
"{\"name\":\"foo\",\"location\":null,\"schema\":%s,\"spec\":null,\"order\":null,\"properties\":[]," +
"\"stage-create\":false}", SAMPLE_SCHEMA_JSON);
"{\"name\":\"foo\",\"location\":null,\"schema\":%s,\"partition-spec\":null,\"write-order\":null," +
"\"properties\":[],\"stage-create\":false}", SAMPLE_SCHEMA_JSON);
AssertHelpers.assertThrows(
"A JSON request with incorrect types for fields should fail to parse and validate",
JsonProcessingException.class,
Expand Down Expand Up @@ -255,7 +256,7 @@ public void testBuilderDoesNotBuildInvalidRequests() {

@Override
public String[] allFieldsFromSpec() {
return new String[] {"name", "location", "schema", "spec", "order", "properties", "stageCreate"};
return new String[] {"name", "location", "schema", "partition-spec", "write-order", "stage-create", "properties"};
}

@Override
Expand Down
23 changes: 0 additions & 23 deletions core/src/test/java/org/apache/iceberg/rest/responses/TestOAuthTokenResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,9 @@
package org.apache.iceberg.rest.responses;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import java.util.Collections;
import java.util.Set;
import org.apache.iceberg.AssertHelpers;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.rest.RequestResponseTestBase;
import org.apache.iceberg.rest.auth.OAuth2Util;
import org.apache.iceberg.util.JsonUtil;
import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -67,24 +62,6 @@ public String serialize(OAuthTokenResponse response) throws JsonProcessingExcept
return OAuth2Util.tokenResponseToJson(response);
}

@Override
public void testHasOnlyKnownFields() {
Set<String> fieldsFromSpec = Sets.newHashSet();
Collections.addAll(fieldsFromSpec, allFieldsFromSpec());
try {
JsonNode node = JsonUtil.mapper().readValue(serialize(createExampleInstance()), JsonNode.class);
for (String field : fieldsFromSpec) {
Assert.assertTrue("Should have field: " + field, node.has(field));
}

for (String field : ((Iterable<? extends String>) node::fieldNames)) {
Assert.assertTrue("Should not have field: " + field, fieldsFromSpec.contains(field));
}
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}

@Test
public void testRoundTrip() throws Exception {
assertRoundTripSerializesEquallyFrom(
Expand Down

0 comments on commit 71cbe8b

Please sign in to comment.