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: Fallback to GET requests for namespace/table/view exists checks #12314

Merged
merged 2 commits into from
Feb 19, 2025
Merged

Core: Fallback to GET requests for namespace/table/view exists checks #12314

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
587372d
CORE: return false when view exists endpoint isn't supported
danielcweeks Feb 13, 2025
970e6a7
Core: Fallback to GET requests
nastra Feb 14, 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
35 changes: 22 additions & 13 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 @@ -444,12 +444,15 @@ public void renameTable(SessionContext context, TableIdentifier from, TableIdent

@Override
public boolean tableExists(SessionContext context, TableIdentifier identifier) {
Endpoint.check(endpoints, Endpoint.V1_TABLE_EXISTS);

try {
checkIdentifierIsValid(identifier);
client.head(paths.table(identifier), headers(context), ErrorHandlers.tableErrorHandler());
return true;
if (endpoints.contains(Endpoint.V1_TABLE_EXISTS)) {
client.head(paths.table(identifier), headers(context), ErrorHandlers.tableErrorHandler());
return true;
} else {
// fallback in order to work with 1.7.x and older servers
return super.tableExists(context, identifier);
}
} catch (NoSuchTableException e) {
return false;
}
Expand Down Expand Up @@ -665,13 +668,16 @@ public List<Namespace> listNamespaces(SessionContext context, Namespace namespac

@Override
public boolean namespaceExists(SessionContext context, Namespace namespace) {
Endpoint.check(endpoints, Endpoint.V1_NAMESPACE_EXISTS);

try {
checkNamespaceIsValid(namespace);
client.head(
paths.namespace(namespace), headers(context), ErrorHandlers.namespaceErrorHandler());
return true;
if (endpoints.contains(Endpoint.V1_NAMESPACE_EXISTS)) {
client.head(
paths.namespace(namespace), headers(context), ErrorHandlers.namespaceErrorHandler());
return true;
} else {
// fallback in order to work with 1.7.x and older servers
return super.namespaceExists(context, namespace);
}
} catch (NoSuchNamespaceException e) {
return false;
}
Expand Down Expand Up @@ -1239,12 +1245,15 @@ public List<TableIdentifier> listViews(SessionContext context, Namespace namespa

@Override
public boolean viewExists(SessionContext context, TableIdentifier identifier) {
Endpoint.check(endpoints, Endpoint.V1_VIEW_EXISTS);

try {
checkViewIdentifierIsValid(identifier);
client.head(paths.view(identifier), headers(context), ErrorHandlers.viewErrorHandler());
return true;
if (endpoints.contains(Endpoint.V1_VIEW_EXISTS)) {
client.head(paths.view(identifier), headers(context), ErrorHandlers.viewErrorHandler());
return true;
} else {
// fallback in order to work with 1.7.x and older servers
return super.viewExists(context, identifier);
}
} catch (NoSuchViewException e) {
return false;
}
Expand Down
122 changes: 122 additions & 0 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 @@ -66,6 +66,7 @@
import org.apache.iceberg.exceptions.ServiceFailureException;
import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.inmemory.InMemoryCatalog;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
Expand Down Expand Up @@ -2505,6 +2506,127 @@ public void testNamespaceExistsViaHEADRequest() {
any());
}

@Test
public void testNamespaceExistsFallbackToGETRequest() {
RESTCatalogAdapter adapter =
Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
@Override
public <T extends RESTResponse> T execute(
HTTPRequest request,
Class<T> responseType,
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 super.execute(request, responseType, errorHandler, responseHeaders);
}
});

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

assertThat(catalog.namespaceExists(Namespace.of("non-existing"))).isFalse();

Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.GET, "v1/config", Map.of(), Map.of()),
eq(ConfigResponse.class),
any(),
any());

// verifies that the namespace is loaded via a GET instead of HEAD (V1_NAMESPACE_EXISTS)
Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.GET, "v1/namespaces/non-existing", Map.of(), Map.of()),
any(),
any(),
any());
}

@Test
public void testTableExistsViaHEADRequest() {
RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
RESTCatalog catalog =
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
catalog.initialize("test", ImmutableMap.of());

assertThat(catalog.tableExists(TABLE)).isFalse();

Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.GET, "v1/config", Map.of(), Map.of()),
eq(ConfigResponse.class),
any(),
any());
Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.HEAD, "v1/namespaces/newdb/tables/table", Map.of(), Map.of()),
any(),
any(),
any());
}

@Test
public void testTableExistsFallbackToGETRequest() {
RESTCatalogAdapter adapter =
Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
@Override
public <T extends RESTResponse> T execute(
HTTPRequest request,
Class<T> responseType,
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 super.execute(request, responseType, errorHandler, responseHeaders);
}
});

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

assertThat(catalog.tableExists(TABLE)).isFalse();

Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.GET, "v1/config", Map.of(), Map.of()),
eq(ConfigResponse.class),
any(),
any());

// verifies that the table is loaded via a GET instead of HEAD (V1_LOAD_TABLE)
Mockito.verify(adapter)
.execute(
reqMatcher(
HTTPMethod.GET,
"v1/namespaces/newdb/tables/table",
Map.of(),
Map.of("snapshots", "all")),
any(),
any(),
any());
}

private RESTCatalog catalog(RESTCatalogAdapter adapter) {
RESTCatalog catalog =
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
Expand Down
46 changes: 46 additions & 0 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 @@ -38,6 +38,7 @@
import org.apache.iceberg.catalog.SessionCatalog;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.inmemory.InMemoryCatalog;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
import org.apache.iceberg.rest.responses.ConfigResponse;
Expand Down Expand Up @@ -243,6 +244,51 @@ public void viewExistsViaHEADRequest() {
any());
}

@Test
public void viewExistsFallbackToGETRequest() {
RESTCatalogAdapter adapter =
Mockito.spy(
new RESTCatalogAdapter(backendCatalog) {
@Override
public <T extends RESTResponse> T execute(
HTTPRequest request,
Class<T> responseType,
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 super.execute(request, responseType, errorHandler, responseHeaders);
}
});

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

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

Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.GET, "v1/config", Map.of(), Map.of()),
eq(ConfigResponse.class),
any(),
any());
Mockito.verify(adapter)
.execute(
reqMatcher(HTTPMethod.GET, "v1/namespaces/ns/views/view", Map.of(), Map.of()),
any(),
any(),
any());
}

@Override
protected RESTCatalog catalog() {
return restCatalog;
Expand Down