Skip to content

Commit

Permalink
Core: Rename REST config response to ConfigResponse (apache#4554)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kbendick
kbendick authored Apr 15, 2022
1 parent 30c76ee commit 993666d
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 53 deletions.
10 changes: 5 additions & 5 deletions core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@
import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
import org.apache.iceberg.rest.requests.CreateTableRequest;
import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest;
import org.apache.iceberg.rest.responses.ConfigResponse;
import org.apache.iceberg.rest.responses.CreateNamespaceResponse;
import org.apache.iceberg.rest.responses.GetNamespaceResponse;
import org.apache.iceberg.rest.responses.ListNamespacesResponse;
import org.apache.iceberg.rest.responses.ListTablesResponse;
import org.apache.iceberg.rest.responses.LoadTableResponse;
import org.apache.iceberg.rest.responses.RESTCatalogConfigResponse;
import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse;
import org.apache.iceberg.util.Pair;
import org.slf4j.Logger;
Expand All @@ -85,7 +85,7 @@ public RESTCatalog() {

@Override
public void initialize(String name, Map<String, String> props) {
RESTCatalogConfigResponse config = fetchConfig(props);
ConfigResponse config = fetchConfig(props);
Map<String, String> mergedProps = config.merge(props);
this.client = clientBuilder.apply(mergedProps);
this.catalogName = name;
Expand Down Expand Up @@ -429,14 +429,14 @@ private Pair<RESTClient, FileIO> tableClients(Map<String, String> config) {
return Pair.of(tableClient, tableIO);
}

private RESTCatalogConfigResponse fetchConfig(Map<String, String> props) {
private ConfigResponse fetchConfig(Map<String, String> props) {
// Create a client for one time use, as we will reconfigure the client using the merged server and application
// defined configuration.
RESTClient singleUseClient = clientBuilder.apply(props);

try {
RESTCatalogConfigResponse configResponse = singleUseClient
.get(ResourcePaths.config(), RESTCatalogConfigResponse.class, ErrorHandlers.defaultErrorHandler());
ConfigResponse configResponse = singleUseClient
.get(ResourcePaths.config(), ConfigResponse.class, ErrorHandlers.defaultErrorHandler());
configResponse.validate();
return configResponse;
} finally {
Expand Down
10 changes: 5 additions & 5 deletions .../responses/RESTCatalogConfigResponse.java → ...ceberg/rest/responses/ConfigResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,16 @@
* <li> overrides - properties that should be used to override client configuration </li>
* </ul>
*/
public class RESTCatalogConfigResponse implements RESTResponse {
public class ConfigResponse implements RESTResponse {

private Map<String, String> defaults;
private Map<String, String> overrides;

public RESTCatalogConfigResponse() {
public ConfigResponse() {
// Required for Jackson deserialization
}

private RESTCatalogConfigResponse(Map<String, String> defaults, Map<String, String> overrides) {
private ConfigResponse(Map<String, String> defaults, Map<String, String> overrides) {
this.defaults = defaults;
this.overrides = overrides;
validate();
Expand Down Expand Up @@ -152,8 +152,8 @@ public Builder withOverrides(Map<String, String> overridesToAdd) {
return this;
}

public RESTCatalogConfigResponse build() {
return new RESTCatalogConfigResponse(defaults, overrides);
public ConfigResponse build() {
return new ConfigResponse(defaults, overrides);
}
}
}
4 changes: 2 additions & 2 deletions core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@
import org.apache.iceberg.rest.requests.CreateTableRequest;
import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest;
import org.apache.iceberg.rest.requests.UpdateTableRequest;
import org.apache.iceberg.rest.responses.ConfigResponse;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.RESTCatalogConfigResponse;
import org.apache.iceberg.util.Pair;

/**
Expand Down Expand Up @@ -154,7 +154,7 @@ public <T extends RESTResponse> T handleRequest(Route route, Map<String, String>
Object body, Class<T> responseType) {
switch (route) {
case CONFIG:
return castResponse(responseType, RESTCatalogConfigResponse.builder().build());
return castResponse(responseType, ConfigResponse.builder().build());

case LIST_NAMESPACES:
if (asNamespaceCatalog != null) {
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
import org.apache.iceberg.catalog.CatalogTests;
import org.apache.iceberg.jdbc.JdbcCatalog;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.rest.responses.ConfigResponse;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.RESTCatalogConfigResponse;
import org.junit.Assert;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -112,7 +112,7 @@ public <T extends RESTResponse> T delete(String path, Class<T> responseType,

@Override
public <T extends RESTResponse> T get(String path, Class<T> responseType, Consumer<ErrorResponse> errorHandler) {
return (T) RESTCatalogConfigResponse
return (T) ConfigResponse
.builder()
.withDefaults(ImmutableMap.of(CatalogProperties.CLIENT_POOL_SIZE, "1"))
.withOverrides(ImmutableMap.of(CatalogProperties.CACHE_ENABLED, "false"))
Expand Down
60 changes: 30 additions & 30 deletions core/src/test/java/org/apache/iceberg/rest/responses/TestRESTCatalogConfigResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import org.junit.BeforeClass;
import org.junit.Test;

public class TestRESTCatalogConfigResponse extends RequestResponseTestBase<RESTCatalogConfigResponse> {
public class TestRESTCatalogConfigResponse extends RequestResponseTestBase<ConfigResponse> {

private static final Map<String, String> DEFAULTS = ImmutableMap.of("warehouse", "s3://bucket/warehouse");
private static final Map<String, String> OVERRIDES = ImmutableMap.of("clients", "5");
Expand All @@ -50,63 +50,63 @@ public void testRoundTripSerDe() throws JsonProcessingException {
String fullJson = "{\"defaults\":{\"warehouse\":\"s3://bucket/warehouse\"},\"overrides\":{\"clients\":\"5\"}}";
assertRoundTripSerializesEquallyFrom(
fullJson,
RESTCatalogConfigResponse.builder()
ConfigResponse.builder()
.withOverrides(OVERRIDES).withDefaults(DEFAULTS).build());
assertRoundTripSerializesEquallyFrom(
fullJson,
RESTCatalogConfigResponse.builder()
ConfigResponse.builder()
.withOverride("clients", "5").withDefault("warehouse", "s3://bucket/warehouse").build());

// `defaults` is empty
String jsonEmptyDefaults = "{\"defaults\":{},\"overrides\":{\"clients\":\"5\"}}";
assertRoundTripSerializesEquallyFrom(
jsonEmptyDefaults,
RESTCatalogConfigResponse.builder().withOverrides(OVERRIDES).build());
ConfigResponse.builder().withOverrides(OVERRIDES).build());
assertRoundTripSerializesEquallyFrom(
jsonEmptyDefaults,
RESTCatalogConfigResponse.builder().withOverrides(OVERRIDES).withDefaults(ImmutableMap.of()).build());
ConfigResponse.builder().withOverrides(OVERRIDES).withDefaults(ImmutableMap.of()).build());
assertRoundTripSerializesEquallyFrom(
jsonEmptyDefaults,
RESTCatalogConfigResponse.builder().withOverride("clients", "5").build());
ConfigResponse.builder().withOverride("clients", "5").build());

// `overrides` is empty
String jsonEmptyOverrides = "{\"defaults\":{\"warehouse\":\"s3://bucket/warehouse\"},\"overrides\":{}}";
assertRoundTripSerializesEquallyFrom(
jsonEmptyOverrides,
RESTCatalogConfigResponse.builder().withDefaults(DEFAULTS).build());
ConfigResponse.builder().withDefaults(DEFAULTS).build());
assertRoundTripSerializesEquallyFrom(
jsonEmptyOverrides,
RESTCatalogConfigResponse.builder().withDefault("warehouse", "s3://bucket/warehouse").build());
ConfigResponse.builder().withDefault("warehouse", "s3://bucket/warehouse").build());
assertRoundTripSerializesEquallyFrom(
jsonEmptyOverrides,
RESTCatalogConfigResponse.builder().withDefaults(DEFAULTS).withOverrides(ImmutableMap.of()).build());
ConfigResponse.builder().withDefaults(DEFAULTS).withOverrides(ImmutableMap.of()).build());

// Both are empty
String emptyJson = "{\"defaults\":{},\"overrides\":{}}";
assertRoundTripSerializesEquallyFrom(
emptyJson,
RESTCatalogConfigResponse.builder().build());
ConfigResponse.builder().build());
assertRoundTripSerializesEquallyFrom(
emptyJson,
RESTCatalogConfigResponse.builder().withOverrides(ImmutableMap.of()).withDefaults(ImmutableMap.of()).build());
ConfigResponse.builder().withOverrides(ImmutableMap.of()).withDefaults(ImmutableMap.of()).build());
}

@Test
// Test cases that cannot be built with our builder, but that are accepted when parsed
public void testCanDeserializeWithoutDefaultValues() throws JsonProcessingException {
RESTCatalogConfigResponse noOverrides = RESTCatalogConfigResponse.builder().withDefaults(DEFAULTS).build();
ConfigResponse noOverrides = ConfigResponse.builder().withDefaults(DEFAULTS).build();
String jsonMissingOverrides = "{\"defaults\":{\"warehouse\":\"s3://bucket/warehouse\"}}";
assertEquals(deserialize(jsonMissingOverrides), noOverrides);
String jsonNullOverrides = "{\"defaults\":{\"warehouse\":\"s3://bucket/warehouse\"},\"overrides\":null}";
assertEquals(deserialize(jsonNullOverrides), noOverrides);

RESTCatalogConfigResponse noDefaults = RESTCatalogConfigResponse.builder().withOverrides(OVERRIDES).build();
ConfigResponse noDefaults = ConfigResponse.builder().withOverrides(OVERRIDES).build();
String jsonMissingDefaults = "{\"overrides\":{\"clients\":\"5\"}}";
assertEquals(deserialize(jsonMissingDefaults), noDefaults);
String jsonNullDefaults = "{\"defaults\":null,\"overrides\":{\"clients\":\"5\"}}";
assertEquals(deserialize(jsonNullDefaults), noDefaults);

RESTCatalogConfigResponse noValues = RESTCatalogConfigResponse.builder().build();
ConfigResponse noValues = ConfigResponse.builder().build();
String jsonEmptyObject = "{}";
assertEquals(deserialize(jsonEmptyObject), noValues);
String jsonNullForAllFields = "{\"defaults\":null,\"overrides\":null}";
Expand All @@ -119,22 +119,22 @@ public void testCanUseNullAsPropertyValue() throws JsonProcessingException {
"{\"defaults\":{\"warehouse\":null},\"overrides\":{\"clients\":\"5\"}}";
assertRoundTripSerializesEquallyFrom(
jsonNullValueInDefaults,
RESTCatalogConfigResponse.builder()
ConfigResponse.builder()
.withDefaults(DEFAULTS_WITH_NULL_VALUE).withOverrides(OVERRIDES).build());
assertRoundTripSerializesEquallyFrom(
jsonNullValueInDefaults,
RESTCatalogConfigResponse.builder()
ConfigResponse.builder()
.withDefault("warehouse", null).withOverrides(OVERRIDES).build());

String jsonNullValueInOverrides =
"{\"defaults\":{\"warehouse\":\"s3://bucket/warehouse\"},\"overrides\":{\"clients\":null}}";
assertRoundTripSerializesEquallyFrom(
jsonNullValueInOverrides,
RESTCatalogConfigResponse.builder()
ConfigResponse.builder()
.withDefaults(DEFAULTS).withOverrides(OVERRIDES_WITH_NULL_VALUE).build());
assertRoundTripSerializesEquallyFrom(
jsonNullValueInOverrides,
RESTCatalogConfigResponse.builder()
ConfigResponse.builder()
.withDefaults(DEFAULTS).withOverride("clients", null).build());
}

Expand Down Expand Up @@ -177,28 +177,28 @@ public void testBuilderDoesNotCreateInvalidObjects() {
"The builder should not allow using null as a key in the properties to override",
NullPointerException.class,
"Invalid override property: null",
() -> RESTCatalogConfigResponse.builder().withOverride(null, "100").build()
() -> ConfigResponse.builder().withOverride(null, "100").build()
);

AssertHelpers.assertThrows(
"The builder should not allow using null as a key in the default properties",
NullPointerException.class,
"Invalid default property: null",
() -> RESTCatalogConfigResponse.builder().withDefault(null, "100").build()
() -> ConfigResponse.builder().withDefault(null, "100").build()
);

AssertHelpers.assertThrows(
"The builder should not allow passing a null map of config properties to override",
NullPointerException.class,
"Invalid override properties map: null",
() -> RESTCatalogConfigResponse.builder().withOverrides(null).build()
() -> ConfigResponse.builder().withOverrides(null).build()
);

AssertHelpers.assertThrows(
"The builder should not allow passing a null map of default config properties",
NullPointerException.class,
"Invalid default properties map: null",
() -> RESTCatalogConfigResponse.builder().withDefaults(null).build()
() -> ConfigResponse.builder().withDefaults(null).build()
);

Map<String, String> mapWithNullKey = Maps.newHashMap();
Expand All @@ -208,14 +208,14 @@ public void testBuilderDoesNotCreateInvalidObjects() {
"The builder should not allow passing a map of default config properties with a null key",
IllegalArgumentException.class,
"Invalid default property: null",
() -> RESTCatalogConfigResponse.builder().withDefaults(mapWithNullKey).build()
() -> ConfigResponse.builder().withDefaults(mapWithNullKey).build()
);

AssertHelpers.assertThrows(
"The builder should not allow passing a map of properties to override with a null key",
IllegalArgumentException.class,
"Invalid override property: null",
() -> RESTCatalogConfigResponse.builder().withOverrides(mapWithNullKey).build()
() -> ConfigResponse.builder().withOverrides(mapWithNullKey).build()
);
}

Expand All @@ -229,7 +229,7 @@ public void testMergeStripsNullValuedEntries() {
Map<String, String> defaults = ImmutableMap.of("a", "from_defaults");
Map<String, String> clientConfig = ImmutableMap.of("a", "from_client", "c", "from_client");

RESTCatalogConfigResponse resp = RESTCatalogConfigResponse.builder()
ConfigResponse resp = ConfigResponse.builder()
.withOverrides(overrides).withDefaults(defaults).build();

// "a" isn't present as it was marked as `null` in the overrides, so the provided client configuration is discarded
Expand All @@ -251,24 +251,24 @@ public String[] allFieldsFromSpec() {
}

@Override
public RESTCatalogConfigResponse createExampleInstance() {
return RESTCatalogConfigResponse.builder()
public ConfigResponse createExampleInstance() {
return ConfigResponse.builder()
.withDefaults(DEFAULTS)
.withOverrides(OVERRIDES)
.build();
}

@Override
public void assertEquals(RESTCatalogConfigResponse actual, RESTCatalogConfigResponse expected) {
public void assertEquals(ConfigResponse actual, ConfigResponse expected) {
Assert.assertEquals("Config properties to use as defaults should be equal",
actual.defaults(), expected.defaults());
Assert.assertEquals("Config properties to use as overrides should be equal",
actual.overrides(), expected.overrides());
}

@Override
public RESTCatalogConfigResponse deserialize(String json) throws JsonProcessingException {
RESTCatalogConfigResponse resp = mapper().readValue(json, RESTCatalogConfigResponse.class);
public ConfigResponse deserialize(String json) throws JsonProcessingException {
ConfigResponse resp = mapper().readValue(json, ConfigResponse.class);
resp.validate();
return resp;
}
Expand Down
16 changes: 7 additions & 9 deletions open-api/rest-catalog-open-api.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,13 @@ paths:
content:
application/json:
schema:
$ref: '#/components/schemas/CatalogConfiguration'
$ref: '#/components/schemas/CatalogConfig'
example: {
"data": {
"overrides": {
"warehouse": "s3://bucket/warehouse/"
},
"defaults": {
"clients": "4"
}
"overrides": {
"warehouse": "s3://bucket/warehouse/"
},
"defaults": {
"clients": "4"
}
}
400:
Expand Down Expand Up @@ -757,7 +755,7 @@ components:
items:
type: string

CatalogConfiguration:
CatalogConfig:
type: object
description: Server-provided configuration for the catalog.
required:
Expand Down

0 comments on commit 993666d

Please sign in to comment.