diff --git a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java index f1267ba6d45d..215a0499b2f7 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -40,6 +40,8 @@ import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.AlreadyExistsException; import org.apache.iceberg.exceptions.CommitFailedException; +import org.apache.iceberg.exceptions.NoSuchNamespaceException; +import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; @@ -48,8 +50,6 @@ import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest; import org.apache.iceberg.rest.requests.UpdateTableRequest; import org.apache.iceberg.rest.responses.CreateNamespaceResponse; -import org.apache.iceberg.rest.responses.DropNamespaceResponse; -import org.apache.iceberg.rest.responses.DropTableResponse; import org.apache.iceberg.rest.responses.GetNamespaceResponse; import org.apache.iceberg.rest.responses.ListNamespacesResponse; import org.apache.iceberg.rest.responses.ListTablesResponse; @@ -117,11 +117,11 @@ public static GetNamespaceResponse loadNamespace(SupportsNamespaces catalog, Nam .build(); } - public static DropNamespaceResponse dropNamespace(SupportsNamespaces catalog, Namespace namespace) { + public static void dropNamespace(SupportsNamespaces catalog, Namespace namespace) { boolean dropped = catalog.dropNamespace(namespace); - return DropNamespaceResponse.builder() - .dropped(dropped) - .build(); + if (!dropped) { + throw new NoSuchNamespaceException("Namespace does not exist: %s", namespace); + } } public static UpdateNamespacePropertiesResponse updateNamespaceProperties( @@ -199,9 +199,11 @@ public static LoadTableResponse createTable(Catalog catalog, Namespace namespace throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); } - public static DropTableResponse dropTable(Catalog catalog, TableIdentifier ident) { + public static void dropTable(Catalog catalog, TableIdentifier ident) { boolean dropped = catalog.dropTable(ident); - return DropTableResponse.builder().dropped(dropped).build(); + if (!dropped) { + throw new NoSuchTableException("Table does not exist: %s", ident); + } } public static LoadTableResponse loadTable(Catalog catalog, TableIdentifier ident) { diff --git a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java index d5d8533088f2..18f936a9f4a0 100644 --- a/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java +++ b/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java @@ -85,7 +85,7 @@ private static String extractResponseBodyAsString(CloseableHttpResponse response // Per the spec, the only currently defined / used "success" responses are 200 and 202. private static boolean isSuccessful(CloseableHttpResponse response) { int code = response.getCode(); - return code == HttpStatus.SC_OK || code == HttpStatus.SC_ACCEPTED; + return code == HttpStatus.SC_OK || code == HttpStatus.SC_ACCEPTED || code == HttpStatus.SC_NO_CONTENT; } private static ErrorResponse buildDefaultErrorResponse(CloseableHttpResponse response) { @@ -165,7 +165,7 @@ private T execute( try (CloseableHttpResponse response = httpClient.execute(request)) { // Skip parsing the response stream for any successful request not expecting a response body - if (responseType == null && isSuccessful(response)) { + if (response.getCode() == HttpStatus.SC_NO_CONTENT || (responseType == null && isSuccessful(response))) { return null; } diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java index 66162c4d590e..703eb9723a05 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java @@ -53,8 +53,6 @@ import org.apache.iceberg.rest.requests.CreateTableRequest; import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest; import org.apache.iceberg.rest.responses.CreateNamespaceResponse; -import org.apache.iceberg.rest.responses.DropNamespaceResponse; -import org.apache.iceberg.rest.responses.DropTableResponse; import org.apache.iceberg.rest.responses.GetNamespaceResponse; import org.apache.iceberg.rest.responses.ListNamespacesResponse; import org.apache.iceberg.rest.responses.ListTablesResponse; @@ -105,9 +103,12 @@ public List listTables(Namespace namespace) { public boolean dropTable(TableIdentifier identifier, boolean purge) { String tablePath = tablePath(identifier); // TODO: support purge flagN - DropTableResponse response = client.delete( - tablePath, DropTableResponse.class, ErrorHandlers.tableErrorHandler()); - return response.isDropped(); + try { + client.delete(tablePath, null, ErrorHandlers.tableErrorHandler()); + return true; + } catch (NoSuchTableException e) { + return false; + } } @Override @@ -162,9 +163,12 @@ public Map loadNamespaceMetadata(Namespace namespace) throws NoS @Override public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { String ns = RESTUtil.urlEncode(namespace); - DropNamespaceResponse response = client - .delete("v1/namespaces/" + ns, DropNamespaceResponse.class, ErrorHandlers.namespaceErrorHandler()); - return response.isDropped(); + try { + client.delete("v1/namespaces/" + ns, null, ErrorHandlers.namespaceErrorHandler()); + return true; + } catch (NoSuchNamespaceException e) { + return false; + } } @Override diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/DropNamespaceResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/DropNamespaceResponse.java deleted file mode 100644 index c827133ec249..000000000000 --- a/core/src/main/java/org/apache/iceberg/rest/responses/DropNamespaceResponse.java +++ /dev/null @@ -1,82 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.iceberg.rest.responses; - -import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; -import org.apache.iceberg.relocated.com.google.common.base.Preconditions; - -/** - * Represents a REST response to drop a namespace. - *

- * The server may choose to return an error if the namespace is not empty. - */ -public class DropNamespaceResponse { - - // For Jackson to properly fail when deserializing, this needs to be boxed. - // Otherwise, the boolean is parsed according to "loose" javascript JSON rules. - private Boolean dropped; - - public DropNamespaceResponse() { - // Required for Jackson deserialization - } - - private DropNamespaceResponse(boolean dropped) { - this.dropped = dropped; - validate(); - } - - DropNamespaceResponse validate() { - Preconditions.checkArgument(dropped != null, "Invalid response, missing field: dropped"); - return this; - } - - public boolean isDropped() { - return dropped; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("dropped", dropped) - .toString(); - } - - public static Builder builder() { - return new Builder(); - } - - public static class Builder { - private Boolean dropped; - - private Builder() { - } - - public Builder dropped(boolean isDropped) { - this.dropped = isDropped; - return this; - } - - public DropNamespaceResponse build() { - Preconditions.checkArgument(dropped != null, "Invalid response, missing field: dropped"); - return new DropNamespaceResponse(dropped); - } - } -} - diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/DropTableResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/DropTableResponse.java deleted file mode 100644 index 42393ea67f44..000000000000 --- a/core/src/main/java/org/apache/iceberg/rest/responses/DropTableResponse.java +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.iceberg.rest.responses; - -import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; -import org.apache.iceberg.relocated.com.google.common.base.Preconditions; - -/** - * Represents a REST response to drop a table. - */ -public class DropTableResponse { - - // For Jackson to properly fail when deserializing, this needs to be boxed. - // Otherwise, the boolean is parsed according to "loose" javascript JSON rules. - private Boolean dropped; - - public DropTableResponse() { - // Required for Jackson deserialization - } - - private DropTableResponse(boolean dropped) { - this.dropped = dropped; - validate(); - } - - DropTableResponse validate() { - Preconditions.checkArgument(dropped != null, "Invalid response, missing field: dropped"); - return this; - } - - public boolean isDropped() { - return dropped; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("dropped", dropped) - .toString(); - } - - public static Builder builder() { - return new Builder(); - } - - public static class Builder { - private Boolean dropped; - - private Builder() { - } - - public Builder dropped(boolean isDropped) { - this.dropped = isDropped; - return this; - } - - public DropTableResponse build() { - Preconditions.checkArgument(dropped != null, "Invalid response, missing field: dropped"); - return new DropTableResponse(dropped); - } - } -} - diff --git a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java index 6495354e033d..ad65b0076b10 100644 --- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java +++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java @@ -180,8 +180,8 @@ public T handleRequest(Route route, Map vars, Object body, C case DROP_NAMESPACE: if (asNamespaceCatalog != null) { - Namespace namespace = namespaceFromPathVars(vars); - return castResponse(responseType, CatalogHandlers.dropNamespace(asNamespaceCatalog, namespace)); + CatalogHandlers.dropNamespace(asNamespaceCatalog, namespaceFromPathVars(vars)); + return null; } break; @@ -212,8 +212,8 @@ public T handleRequest(Route route, Map vars, Object body, C } case DROP_TABLE: { - TableIdentifier ident = identFromPathVars(vars); - return castResponse(responseType, CatalogHandlers.dropTable(catalog, ident)); + CatalogHandlers.dropTable(catalog, identFromPathVars(vars)); + return null; } case LOAD_TABLE: { @@ -230,7 +230,7 @@ public T handleRequest(Route route, Map vars, Object body, C default: } - return null; // will be converted to a 400 + return null; } public T execute(HTTPMethod method, String path, Object body, Class responseType, @@ -239,16 +239,7 @@ public T execute(HTTPMethod method, String path, Object body, Class respo Pair> routeAndVars = Route.from(method, path); if (routeAndVars != null) { try { - T response = handleRequest(routeAndVars.first(), routeAndVars.second(), body, responseType); - if (response != null) { - return response; - } - - // if a response was not returned, there was no handler for the route - errorBuilder - .responseCode(400) - .withType("BadRequestException") - .withMessage(String.format("No handler for request: %s %s", method, path)); + return handleRequest(routeAndVars.first(), routeAndVars.second(), body, responseType); } catch (RuntimeException e) { configureResponseFromException(e, errorBuilder); diff --git a/core/src/test/java/org/apache/iceberg/rest/responses/TestDropNamespaceResponse.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestDropNamespaceResponse.java deleted file mode 100644 index 7c29afb04dff..000000000000 --- a/core/src/test/java/org/apache/iceberg/rest/responses/TestDropNamespaceResponse.java +++ /dev/null @@ -1,108 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.iceberg.rest.responses; - -import com.fasterxml.jackson.core.JsonProcessingException; -import org.apache.iceberg.AssertHelpers; -import org.apache.iceberg.rest.RequestResponseTestBase; -import org.junit.Assert; -import org.junit.Test; - -public class TestDropNamespaceResponse extends RequestResponseTestBase { - - @Test - public void testRoundTripSerDe() throws JsonProcessingException { - assertRoundTripSerializesEquallyFrom( - "{\"dropped\":true}", DropNamespaceResponse.builder().dropped(true).build()); - - assertRoundTripSerializesEquallyFrom( - "{\"dropped\":false}", DropNamespaceResponse.builder().dropped(false).build()); - } - - @Test - public void testFailParsingWhenNullOrEmptyJson() { - String nullJson = null; - AssertHelpers.assertThrows("DropNamespaceResponse should fail to deserialize from a null JSON string", - IllegalArgumentException.class, "argument \"content\" is null", - () -> deserialize(nullJson)); - - String emptyString = ""; - AssertHelpers.assertThrows("DropNamespaceResponse should fail to deserialize empty string as JSON", - JsonProcessingException.class, "No content to map due to end-of-input", - () -> deserialize(emptyString)); - - String emptyJson = "{}"; - AssertHelpers.assertThrows( - "DropNamespaceResponse should fail to deserialize and validate if missing the field dropped", - IllegalArgumentException.class, - "Invalid response, missing field: dropped", - () -> deserialize(emptyJson)); - } - - @Test - public void testFailWhenFieldsHaveInvalidValues() { - String invalidJsonWithStringType = "{\"dropped\":\"421\"}"; - AssertHelpers.assertThrows("DropNamespaceResponse should fail to deserialize if the dropped field is invalid", - JsonProcessingException.class, - () -> deserialize(invalidJsonWithStringType)); - - // Note that Jackson follows JSON rules to some degree, so integers wil parse into booleans. - String invalidJsonWithFloatingPointType = "{\"dropped\":421.62}"; - AssertHelpers.assertThrows("DropNamespaceResponse should fail to deserialize if the dropped field is invalid", - JsonProcessingException.class, - () -> deserialize(invalidJsonWithFloatingPointType)); - - String invalidJsonWithNullType = "{\"dropped\":null}"; - AssertHelpers.assertThrows("DropNamespaceResponse should fail to deserialize if the dropped field is invalid", - IllegalArgumentException.class, - "Invalid response, missing field: dropped", - () -> deserialize(invalidJsonWithNullType)); - } - - @Test - public void testBuilderDoesNotBuildInvalidResponse() { - AssertHelpers.assertThrows( - "The builder should not allow not setting the dropped field", - IllegalArgumentException.class, - "Invalid response, missing field: dropped", - () -> DropNamespaceResponse.builder().build() - ); - } - - @Override - public String[] allFieldsFromSpec() { - return new String[] { "dropped" }; - } - - @Override - public DropNamespaceResponse createExampleInstance() { - return DropNamespaceResponse.builder().dropped(true).build(); - } - - @Override - public void assertEquals(DropNamespaceResponse actual, DropNamespaceResponse expected) { - Assert.assertEquals(actual.isDropped(), expected.isDropped()); - } - - @Override - public DropNamespaceResponse deserialize(String json) throws JsonProcessingException { - return mapper().readValue(json, DropNamespaceResponse.class).validate(); - } -}