Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Remove namespace/table/view HEAD endpoints from defaults #12351

Merged
merged 2 commits into from
Feb 21, 2025
+56 −30
Merged

Core: Remove namespace/table/view HEAD endpoints from defaults #12351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb94311
Core: Remove namespace/table/view HEAD requests from defaults
nastra Feb 20, 2025
5d0fe8c
improvements
nastra Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
View file
Open in desktop
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
View file
Open in desktop
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
29 changes: 20 additions & 9 deletions core/src/test/java/org/apache/iceberg/rest/TestRESTViewCatalog.java
View file
Open in desktop
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
View file
Open in desktop
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}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this was added after #11756 and needs to be removed again, since we can't actually modify/update the default endpoints in the RESTSessionCatalog that were initially defined by #10929


- 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