Skip to content

Commit

Permalink
Core: Remove namespace/table/view HEAD endpoints from defaults (#12351)
Browse files Browse the repository at this point in the history
  • Loading branch information
nastra authored Feb 21, 2025
1 parent d4fe23a commit 4958663
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,17 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
.addAll(TOKEN_PREFERENCE_ORDER)
.build();

// these default endpoints must not be updated in order to maintain backwards compatibility with
// legacy servers
private static final Set<Endpoint> DEFAULT_ENDPOINTS =
ImmutableSet.<Endpoint>builder()
.add(Endpoint.V1_LIST_NAMESPACES)
.add(Endpoint.V1_LOAD_NAMESPACE)
.add(Endpoint.V1_NAMESPACE_EXISTS)
.add(Endpoint.V1_CREATE_NAMESPACE)
.add(Endpoint.V1_UPDATE_NAMESPACE)
.add(Endpoint.V1_DELETE_NAMESPACE)
.add(Endpoint.V1_LIST_TABLES)
.add(Endpoint.V1_LOAD_TABLE)
.add(Endpoint.V1_TABLE_EXISTS)
.add(Endpoint.V1_CREATE_TABLE)
.add(Endpoint.V1_UPDATE_TABLE)
.add(Endpoint.V1_DELETE_TABLE)
Expand All @@ -155,11 +155,12 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
.add(Endpoint.V1_COMMIT_TRANSACTION)
.build();

// these view endpoints must not be updated in order to maintain backwards compatibility with
// legacy servers
private static final Set<Endpoint> VIEW_ENDPOINTS =
ImmutableSet.<Endpoint>builder()
.add(Endpoint.V1_LIST_VIEWS)
.add(Endpoint.V1_LOAD_VIEW)
.add(Endpoint.V1_VIEW_EXISTS)
.add(Endpoint.V1_CREATE_VIEW)
.add(Endpoint.V1_UPDATE_VIEW)
.add(Endpoint.V1_DELETE_VIEW)
Expand Down
46 changes: 32 additions & 14 deletions core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Original file line number Diff line number Diff line change
Expand Up @@ -2508,6 +2508,15 @@ public void testNamespaceExistsViaHEADRequest() {

@Test
public void testNamespaceExistsFallbackToGETRequest() {
// server indicates support of loading a namespace only via GET, which is
// what older REST servers would send back too
verifyNamespaceExistsFallbackToGETRequest(
ConfigResponse.builder()
.withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_NAMESPACE))
.build());
}

private void verifyNamespaceExistsFallbackToGETRequest(ConfigResponse configResponse) {
RESTCatalogAdapter adapter =
Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
Expand All @@ -2518,13 +2527,7 @@ public <T extends RESTResponse> T execute(
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders) {
if ("v1/config".equals(request.path())) {
return castResponse(
responseType,
ConfigResponse.builder()
// server indicates support of loading a namespace only via GET, which is
// what older REST servers would send back too
.withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_NAMESPACE))
.build());
return castResponse(responseType, configResponse);
}

return super.execute(request, responseType, errorHandler, responseHeaders);
Expand Down Expand Up @@ -2553,6 +2556,13 @@ public <T extends RESTResponse> T execute(
any());
}

@Test
public void testNamespaceExistsFallbackToGETRequestWithLegacyServer() {
// simulate a legacy server that doesn't send back supported endpoints, thus the
// client relies on the default endpoints
verifyNamespaceExistsFallbackToGETRequest(ConfigResponse.builder().build());
}

@Test
public void testTableExistsViaHEADRequest() {
RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
Expand All @@ -2578,6 +2588,13 @@ public void testTableExistsViaHEADRequest() {

@Test
public void testTableExistsFallbackToGETRequest() {
// server indicates support of loading a table only via GET, which is
// what older REST servers would send back too
verifyTableExistsFallbackToGETRequest(
ConfigResponse.builder().withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_TABLE)).build());
}

private void verifyTableExistsFallbackToGETRequest(ConfigResponse configResponse) {
RESTCatalogAdapter adapter =
Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
Expand All @@ -2588,13 +2605,7 @@ public <T extends RESTResponse> T execute(
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders) {
if ("v1/config".equals(request.path())) {
return castResponse(
responseType,
ConfigResponse.builder()
// server indicates support of loading a table only via GET, which is
// what older REST servers would send back too
.withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_TABLE))
.build());
return castResponse(responseType, configResponse);
}

return super.execute(request, responseType, errorHandler, responseHeaders);
Expand Down Expand Up @@ -2627,6 +2638,13 @@ public <T extends RESTResponse> T execute(
any());
}

@Test
public void testTableExistsFallbackToGETRequestWithLegacyServer() {
// simulate a legacy server that doesn't send back supported endpoints, thus the
// client relies on the default endpoints
verifyTableExistsFallbackToGETRequest(ConfigResponse.builder().build());
}

private RESTCatalog catalog(RESTCatalogAdapter adapter) {
RESTCatalog catalog =
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,15 @@ public void viewExistsViaHEADRequest() {

@Test
public void viewExistsFallbackToGETRequest() {
// server indicates support of loading a view only via GET, which is
// what older REST servers would send back too
verifyViewExistsFallbackToGETRequest(
ConfigResponse.builder().withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_VIEW)).build(),
ImmutableMap.of());
}

private void verifyViewExistsFallbackToGETRequest(
ConfigResponse configResponse, Map<String, String> catalogProperties) {
RESTCatalogAdapter adapter =
Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
Expand All @@ -256,13 +265,7 @@ public <T extends RESTResponse> T execute(
Consumer<ErrorResponse> errorHandler,
Consumer<Map<String, String>> responseHeaders) {
if ("v1/config".equals(request.path())) {
return castResponse(
responseType,
ConfigResponse.builder()
// server indicates support of loading a view only via GET, which is
// what older REST servers would send back too
.withEndpoints(ImmutableList.of(Endpoint.V1_LOAD_VIEW))
.build());
return castResponse(responseType, configResponse);
}

return super.execute(request, responseType, errorHandler, responseHeaders);
Expand All @@ -271,8 +274,7 @@ public <T extends RESTResponse> T execute(

RESTCatalog catalog =
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
catalog.initialize("test", ImmutableMap.of());

catalog.initialize("test", catalogProperties);
assertThat(catalog.viewExists(TableIdentifier.of("ns", "view"))).isFalse();

Mockito.verify(adapter)
Expand All @@ -289,6 +291,15 @@ public <T extends RESTResponse> T execute(
any());
}

@Test
public void viewExistsFallbackToGETRequestWithLegacyServer() {
// simulate a legacy server that doesn't send back supported endpoints, thus the
// client relies on the default endpoints
verifyViewExistsFallbackToGETRequest(
ConfigResponse.builder().build(),
ImmutableMap.of(RESTSessionCatalog.VIEW_ENDPOINTS_SUPPORTED, "true"));
}

@Override
protected RESTCatalog catalog() {
return restCatalog;
Expand Down
4 changes: 0 additions & 4 deletions open-api/rest-catalog-open-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,6 @@ paths:
- GET /v1/{prefix}/namespaces/{namespace}
- HEAD /v1/{prefix}/namespaces/{namespace}
- DELETE /v1/{prefix}/namespaces/{namespace}
- POST /v1/{prefix}/namespaces/{namespace}/properties
Expand All @@ -123,8 +121,6 @@ paths:
- GET /v1/{prefix}/namespaces/{namespace}/tables/{table}
- HEAD /v1/{prefix}/namespaces/{namespace}/tables/{table}
- POST /v1/{prefix}/namespaces/{namespace}/tables/{table}
- DELETE /v1/{prefix}/namespaces/{namespace}/tables/{table}
Expand Down

0 comments on commit 4958663

Please sign in to comment.