This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 90da2776aa [#14432] fix(iceberg-rest): answer invalid requests with
400 and an ErrorModel (#14433)
90da2776aa is described below
commit 90da2776aaefc4d4b0534c90581bd7f86d380e8e
Author: Mark Hoerth <[email protected]>
AuthorDate: Sun Nov 1 18:44:33 2026 -0800
[#14432] fix(iceberg-rest): answer invalid requests with 400 and an
ErrorModel (#14433)
### What changes were proposed in this pull request?
Answer invalid Iceberg REST requests with 400 and an ErrorModel instead
of 5xx, 403, 404 or 406:
- `IcebergRESTUtils.validateRequest`: every route that takes a body
checks it is present and runs the request's own `validate()`; failures
are 400.
- Create-namespace and rename-table/view authorization handlers reject a
missing body, namespace, source or destination with 400 instead of 403
or NPE.
- Register `JacksonFeature.withoutExceptionMappers()` so Jackson errors
reach `IcebergExceptionMapper` and get an ErrorModel; the mapper reports
them as 400 with the Iceberg parser's message or the field path, never
Jackson internals.
- New `IcebergRequestBodyReaderInterceptor`: Iceberg's parsers reject an
unknown update action or requirement type with
`UnsupportedOperationException` / `IllegalArgumentException` during body
reading; these become 400.
- `IcebergExceptionMapper`: matches the exception's superclasses (so
`NumberFormatException` is 400), maps Jersey `ParamException` (bad
`pageSize`) to 400, and maps storage SDK errors (AWS, GCS, Azure)
without echoing their message: unusable location (missing bucket, wrong
region/endpoint, unbuildable request) is 400, anything else 503. A
missing object keeps its existing handling.
- `IcebergRESTUtils.errorResponse`: an exception with a null message
gets a default message, since ErrorModel requires one.
- `IcebergCatalogWrapper`:
- `registerTable`: a metadata location that does not exist is 400.
- `createTable`: an explicit format-version must be an integer from 1 to
3, and the schema must be valid for it (`Schema.checkCompatibility`);
invalid transforms are 400.
- `updateTable`: set-default-spec / set-default-sort-order must name an
id the table has (or -1, or a spec added in the same request); invalid
updates are 400.
Format version 4 is accepted by the Iceberg 1.11 library but is not a
released spec version and current clients cannot read it, so it is
refused.
### Why are the changes needed?
The REST spec defines these as client errors (400 with an ErrorModel).
Today clients get 500s, a 404 that looks like a missing namespace, 406,
or text/plain bodies, and some invalid creates are committed and leave
tables that clients cannot load.
Fixes #14432
### Does this PR introduce _any_ user-facing change?
Yes. Responses to invalid requests change from 5xx/403/404/406 to 400
with an ErrorModel; storage-service failures are 503 without the SDK
message. Tables can no longer be created with format-version outside 1
to 3.
### How was this patch tested?
New unit tests:
- `TestIcebergExceptionMapper`: JSON errors, `ParamException`, subclass
mapping, storage errors (no SDK message leak), default message.
- `TestIcebergRequestBodyReaderInterceptor`,
`TestIcebergRESTUtils#testValidateRequestRejectsMissingOrInvalidBody`.
- `TestIcebergRequestBodyAuthzHandlers`: create namespace and rename
table/view with incomplete bodies.
- `TestIcebergCatalogWrapper`: format-version and schema validation,
unknown default spec/sort order, register with a missing metadata file.
- `TestIcebergTableOperations`: HTTP-level checks for malformed JSON,
`{}`, unknown update action, invalid transform, bad format versions,
unknown default spec/sort order (and that the table still loads
afterwards).
`./gradlew :iceberg:iceberg-common:test
:iceberg:iceberg-rest-server:test -PskipITs`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Eu6fb9pLfmrSJzGQDVLi1g
Co-authored-by: Claude Opus 5.5 <[email protected]>
---
.../iceberg/common/ops/IcebergCatalogWrapper.java | 149 ++++++++++++++++++-
.../common/ops/TestIcebergCatalogWrapper.java | 129 +++++++++++++++++
.../org/apache/gravitino/iceberg/RESTService.java | 8 +-
.../iceberg/service/IcebergExceptionMapper.java | 157 ++++++++++++++++++++-
.../iceberg/service/IcebergRESTUtils.java | 36 ++++-
.../IcebergRequestBodyReaderInterceptor.java | 52 +++++++
.../service/rest/IcebergNamespaceOperations.java | 7 +
.../service/rest/IcebergTableOperations.java | 5 +
.../service/rest/IcebergTableRenameOperations.java | 1 +
.../service/rest/IcebergViewOperations.java | 2 +
.../service/rest/IcebergViewRenameOperations.java | 1 +
.../web/filter/CreateNamespaceAuthzHandler.java | 5 +
.../server/web/filter/RenameTableAuthzHandler.java | 11 +-
.../server/web/filter/RenameViewAuthzHandler.java | 11 +-
.../service/TestIcebergExceptionMapper.java | 106 +++++++++++++-
.../iceberg/service/TestIcebergRESTUtils.java | 32 +++++
.../TestIcebergRequestBodyReaderInterceptor.java | 70 +++++++++
.../iceberg/service/rest/IcebergRestTestUtil.java | 6 +-
.../service/rest/TestIcebergTableOperations.java | 94 ++++++++++++
.../TestIcebergRequestBodyAuthzHandlers.java | 120 ++++++++++++++++
20 files changed, 986 insertions(+), 16 deletions(-)
diff --git
a/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java
b/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java
index 0304cbf635..cc69b8eb4d 100644
---
a/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java
+++
b/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java
@@ -18,10 +18,13 @@
*/
package org.apache.gravitino.iceberg.common.ops;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import java.util.HashSet;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
+import java.util.Set;
import lombok.Getter;
import lombok.Setter;
import org.apache.commons.lang3.StringUtils;
@@ -38,16 +41,22 @@ import org.apache.hadoop.security.UserGroupInformation;
import org.apache.iceberg.BaseMetadataTable;
import org.apache.iceberg.BaseTable;
import org.apache.iceberg.CatalogUtil;
+import org.apache.iceberg.MetadataUpdate;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.TableProperties;
import org.apache.iceberg.Transaction;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.NoSuchTableException;
import org.apache.iceberg.exceptions.NoSuchViewException;
+import org.apache.iceberg.exceptions.NotFoundException;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.io.ResolvingFileIO;
import org.apache.iceberg.jdbc.JdbcCatalogWithMetadataLocationSupport;
@@ -84,6 +93,11 @@ public class IcebergCatalogWrapper implements AutoCloseable {
public static final Logger LOG =
LoggerFactory.getLogger(IcebergCatalogWrapper.class);
+ /** Format versions a table can be created with: the versions the Iceberg
table spec defines. */
+ private static final int MIN_FORMAT_VERSION = 1;
+
+ private static final int MAX_FORMAT_VERSION = 3;
+
private final Object initializationLock = new Object();
private volatile Catalog catalog;
private volatile SupportsNamespaces asNamespaceCatalog;
@@ -208,7 +222,15 @@ public class IcebergCatalogWrapper implements
AutoCloseable {
}
public LoadTableResponse registerTable(Namespace namespace,
RegisterTableRequest request) {
- return CatalogHandlers.registerTable(getCatalog(), namespace, request);
+ try {
+ return CatalogHandlers.registerTable(getCatalog(), namespace, request);
+ } catch (NotFoundException e) {
+ // The metadata location comes from the request, so a file that does not
exist is a client
+ // error. 404 is reserved by the REST spec for a namespace that does not
exist.
+ throw new BadRequestException(
+ "Cannot register table %s: metadata location %s does not exist",
+ request.name(), request.metadataLocation());
+ }
}
/**
@@ -224,12 +246,26 @@ public class IcebergCatalogWrapper implements
AutoCloseable {
public LoadTableResponse createTable(Namespace namespace, CreateTableRequest
request) {
request.validate();
+ validateTableDefinition(request);
Catalog loadedCatalog = getCatalog();
- if (request.stageCreate()) {
- return CatalogHandlers.stageTableCreate(loadedCatalog, namespace,
request);
+ LoadTableResponse loadTableResponse;
+ try {
+ if (request.stageCreate()) {
+ return CatalogHandlers.stageTableCreate(loadedCatalog, namespace,
request);
+ }
+ loadTableResponse = CatalogHandlers.createTable(loadedCatalog,
namespace, request);
+ } catch (UnsupportedOperationException e) {
+ // Iceberg reports an invalid table definition, such as a transform the
column type does
+ // not support, as UnsupportedOperationException. createTable has no 406
response.
+ throw new BadRequestException("Invalid table definition: %s",
e.getMessage());
+ } catch (IllegalStateException e) {
+ // With the format version taken from the catalog defaults, a column
type that version does
+ // not support is only detected while Iceberg builds the table metadata.
+ if (e.getMessage() != null && e.getMessage().startsWith("Invalid schema
for v")) {
+ throw new BadRequestException("%s", e.getMessage());
+ }
+ throw e;
}
- LoadTableResponse loadTableResponse =
- CatalogHandlers.createTable(loadedCatalog, namespace, request);
if (loadTableResponse != null) {
TableIdentifier tableIdentifier = TableIdentifier.of(namespace,
request.name());
getMetadataCache().updateTableMetadata(tableIdentifier,
loadTableResponse.tableMetadata());
@@ -351,8 +387,15 @@ public class IcebergCatalogWrapper implements
AutoCloseable {
public LoadTableResponse updateTable(
TableIdentifier tableIdentifier, UpdateTableRequest updateTableRequest) {
getMetadataCache().invalidate(tableIdentifier);
- LoadTableResponse loadTableResponse =
- CatalogHandlers.updateTable(getCatalog(), tableIdentifier,
updateTableRequest);
+ validateDefaultSpecAndSortOrder(tableIdentifier, updateTableRequest);
+ LoadTableResponse loadTableResponse;
+ try {
+ loadTableResponse =
+ CatalogHandlers.updateTable(getCatalog(), tableIdentifier,
updateTableRequest);
+ } catch (UnsupportedOperationException e) {
+ // As in createTable: an invalid update, not an unsupported operation.
updateTable has no 406.
+ throw new BadRequestException("Invalid table update: %s",
e.getMessage());
+ }
if (loadTableResponse != null) {
getMetadataCache().updateTableMetadata(tableIdentifier,
loadTableResponse.tableMetadata());
}
@@ -506,4 +549,96 @@ public class IcebergCatalogWrapper implements
AutoCloseable {
expireMinutes);
return cache;
}
+
+ /**
+ * Rejects a table definition Iceberg would otherwise accept or fail on with
a server error: a
+ * format version that is not a number or not a defined version, and column
types the format
+ * version does not support.
+ */
+ @VisibleForTesting
+ static void validateTableDefinition(CreateTableRequest request) {
+ Map<String, String> properties = request.properties();
+ String value = properties == null ? null :
properties.get(TableProperties.FORMAT_VERSION);
+ if (value == null) {
+ // The catalog's table defaults decide the version; Iceberg checks the
schema against it.
+ return;
+ }
+ int formatVersion;
+ try {
+ formatVersion = Integer.parseInt(value.trim());
+ } catch (NumberFormatException e) {
+ throw new BadRequestException(
+ "Invalid %s '%s': must be an integer",
TableProperties.FORMAT_VERSION, value);
+ }
+ if (formatVersion < MIN_FORMAT_VERSION || formatVersion >
MAX_FORMAT_VERSION) {
+ throw new BadRequestException(
+ "Unsupported %s %s: must be between %s and %s",
+ TableProperties.FORMAT_VERSION, formatVersion, MIN_FORMAT_VERSION,
MAX_FORMAT_VERSION);
+ }
+ if (request.schema() != null) {
+ try {
+ Schema.checkCompatibility(request.schema(), formatVersion);
+ } catch (IllegalStateException | IllegalArgumentException e) {
+ throw new BadRequestException("%s", e.getMessage());
+ }
+ }
+ }
+
+ /**
+ * Rejects set-default-spec and set-default-sort-order updates naming an id
the table does not
+ * have; Iceberg applies them unchecked and then fails with a
NullPointerException.
+ */
+ private void validateDefaultSpecAndSortOrder(
+ TableIdentifier tableIdentifier, UpdateTableRequest request) {
+ if (request == null || request.updates() == null) {
+ return;
+ }
+ Set<Integer> specIds = new HashSet<>();
+ Set<Integer> sortOrderIds = new HashSet<>();
+ boolean addsSortOrder = false;
+ for (MetadataUpdate update : request.updates()) {
+ if (update instanceof MetadataUpdate.SetDefaultPartitionSpec) {
+ specIds.add(((MetadataUpdate.SetDefaultPartitionSpec)
update).specId());
+ } else if (update instanceof MetadataUpdate.SetDefaultSortOrder) {
+ sortOrderIds.add(((MetadataUpdate.SetDefaultSortOrder)
update).sortOrderId());
+ } else if (update instanceof MetadataUpdate.AddSortOrder) {
+ addsSortOrder = true;
+ }
+ }
+ // -1 means "the spec or sort order added last in this request".
+ specIds.remove(-1);
+ sortOrderIds.remove(-1);
+ if (specIds.isEmpty() && sortOrderIds.isEmpty()) {
+ return;
+ }
+ for (MetadataUpdate update : request.updates()) {
+ if (update instanceof MetadataUpdate.AddPartitionSpec) {
+ specIds.remove(((MetadataUpdate.AddPartitionSpec)
update).spec().specId());
+ }
+ }
+ Table table;
+ try {
+ table = getCatalog().loadTable(tableIdentifier);
+ } catch (NoSuchTableException e) {
+ // A create-table transaction commits to a table that does not exist
yet; Iceberg checks its
+ // requirements itself.
+ return;
+ }
+ for (Integer id : specIds) {
+ if (!table.specs().containsKey(id)) {
+ throw new BadRequestException(
+ "Cannot set default partition spec to %s: the table has no spec
with that id", id);
+ }
+ }
+ // A sort order added in the same request gets its id when it is applied,
so only ids the
+ // table already has can be checked here.
+ if (!addsSortOrder) {
+ for (Integer id : sortOrderIds) {
+ if (!table.sortOrders().containsKey(id)) {
+ throw new BadRequestException(
+ "Cannot set default sort order to %s: the table has no sort
order with that id", id);
+ }
+ }
+ }
+ }
}
diff --git
a/iceberg/iceberg-common/src/test/java/org/apache/gravitino/iceberg/common/ops/TestIcebergCatalogWrapper.java
b/iceberg/iceberg-common/src/test/java/org/apache/gravitino/iceberg/common/ops/TestIcebergCatalogWrapper.java
index 9b87546e25..c21e951fa3 100644
---
a/iceberg/iceberg-common/src/test/java/org/apache/gravitino/iceberg/common/ops/TestIcebergCatalogWrapper.java
+++
b/iceberg/iceberg-common/src/test/java/org/apache/gravitino/iceberg/common/ops/TestIcebergCatalogWrapper.java
@@ -18,11 +18,14 @@
*/
package org.apache.gravitino.iceberg.common.ops;
+import com.google.common.collect.ImmutableMap;
import java.io.IOException;
import java.lang.reflect.Method;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
@@ -34,6 +37,7 @@ import
org.apache.gravitino.iceberg.common.cache.SupportsMetadataLocation;
import org.apache.gravitino.iceberg.common.cache.TableMetadataCache;
import org.apache.gravitino.iceberg.common.utils.IcebergCatalogUtil;
import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.MetadataUpdate;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.TableMetadata;
@@ -41,9 +45,14 @@ import org.apache.iceberg.TableMetadataParser;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.NoSuchViewException;
import org.apache.iceberg.jdbc.JdbcCatalog;
import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
+import org.apache.iceberg.rest.requests.CreateTableRequest;
+import org.apache.iceberg.rest.requests.ImmutableRegisterTableRequest;
+import org.apache.iceberg.rest.requests.RegisterTableRequest;
+import org.apache.iceberg.rest.requests.UpdateTableRequest;
import org.apache.iceberg.rest.responses.LoadViewResponse;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.view.ViewOperations;
@@ -189,6 +198,114 @@ public class TestIcebergCatalogWrapper {
Assertions.assertEquals(schema.asStruct(), actual.schema().asStruct());
}
+ @Test
+ public void testValidateTableDefinitionRejectsInvalidFormatVersion() {
+ Schema schema = new Schema(Types.NestedField.required(1, "id",
Types.LongType.get()));
+ for (String version : new String[] {"abc", "0", "4", "-1"}) {
+ CreateTableRequest request =
+ CreateTableRequest.builder()
+ .withName("t")
+ .withSchema(schema)
+ .setProperties(ImmutableMap.of("format-version", version))
+ .build();
+ Assertions.assertThrows(
+ BadRequestException.class,
+ () -> IcebergCatalogWrapper.validateTableDefinition(request),
+ "format-version " + version);
+ }
+ for (String version : new String[] {"1", "2", "3"}) {
+ CreateTableRequest request =
+ CreateTableRequest.builder()
+ .withName("t")
+ .withSchema(schema)
+ .setProperties(ImmutableMap.of("format-version", version))
+ .build();
+ Assertions.assertDoesNotThrow(() ->
IcebergCatalogWrapper.validateTableDefinition(request));
+ }
+ }
+
+ @Test
+ public void
testValidateTableDefinitionRejectsTypesTheVersionDoesNotSupport() {
+ Schema nanos =
+ new Schema(Types.NestedField.required(1, "ts",
Types.TimestampNanoType.withZone()));
+ CreateTableRequest v2 =
+ CreateTableRequest.builder()
+ .withName("t")
+ .withSchema(nanos)
+ .setProperties(ImmutableMap.of("format-version", "2"))
+ .build();
+ Assertions.assertThrows(
+ BadRequestException.class, () ->
IcebergCatalogWrapper.validateTableDefinition(v2));
+
+ CreateTableRequest v3 =
+ CreateTableRequest.builder()
+ .withName("t")
+ .withSchema(nanos)
+ .setProperties(ImmutableMap.of("format-version", "3"))
+ .build();
+ Assertions.assertDoesNotThrow(() ->
IcebergCatalogWrapper.validateTableDefinition(v3));
+
+ // Without an explicit version the catalog defaults decide; nothing is
rejected up front.
+ CreateTableRequest unspecified =
+ CreateTableRequest.builder().withName("t").withSchema(nanos).build();
+ Assertions.assertDoesNotThrow(() ->
IcebergCatalogWrapper.validateTableDefinition(unspecified));
+ }
+
+ @Test
+ public void
testUpdateTableToUnknownDefaultSpecOrSortOrderIsBadRequest(@TempDir Path
warehouseDir)
+ throws Exception {
+ try (IcebergCatalogWrapper wrapper =
+ new IcebergCatalogWrapper(new
IcebergConfig(sqliteConfig(warehouseDir)))) {
+ Namespace namespace = Namespace.of("db");
+ TableIdentifier ident = TableIdentifier.of(namespace, "t");
+
wrapper.createNamespace(CreateNamespaceRequest.builder().withNamespace(namespace).build());
+ wrapper.createTable(
+ namespace,
+ CreateTableRequest.builder()
+ .withName("t")
+ .withSchema(new Schema(Types.NestedField.required(1, "id",
Types.LongType.get())))
+ .build());
+
+ for (MetadataUpdate update :
+ new MetadataUpdate[] {
+ new MetadataUpdate.SetDefaultPartitionSpec(7), new
MetadataUpdate.SetDefaultSortOrder(7)
+ }) {
+ UpdateTableRequest request =
+ new UpdateTableRequest(Collections.emptyList(),
Collections.singletonList(update));
+ Assertions.assertThrows(
+ BadRequestException.class, () -> wrapper.updateTable(ident,
request));
+ }
+
+ // Ids the table has, and -1 ("the last added"), are still accepted.
+ UpdateTableRequest existing =
+ new UpdateTableRequest(
+ Collections.emptyList(),
+ Arrays.asList(
+ new MetadataUpdate.SetDefaultPartitionSpec(0),
+ new MetadataUpdate.SetDefaultSortOrder(0)));
+ Assertions.assertDoesNotThrow(() -> wrapper.updateTable(ident,
existing));
+ }
+ }
+
+ @Test
+ public void testRegisterTableWithMissingMetadataIsBadRequest(@TempDir Path
warehouseDir)
+ throws Exception {
+ try (IcebergCatalogWrapper wrapper =
+ new IcebergCatalogWrapper(new
IcebergConfig(sqliteConfig(warehouseDir)))) {
+ Namespace namespace = Namespace.of("db");
+
wrapper.createNamespace(CreateNamespaceRequest.builder().withNamespace(namespace).build());
+ RegisterTableRequest request =
+ ImmutableRegisterTableRequest.builder()
+ .name("ghost")
+
.metadataLocation(warehouseDir.resolve("missing/v1.metadata.json").toUri().toString())
+ .build();
+ BadRequestException e =
+ Assertions.assertThrows(
+ BadRequestException.class, () ->
wrapper.registerTable(namespace, request));
+ Assertions.assertTrue(e.getMessage().contains("does not exist"),
e.getMessage());
+ }
+ }
+
private static TableMetadataCache
invokeGetMetadataCache(IcebergCatalogWrapper wrapper)
throws Exception {
Method method =
IcebergCatalogWrapper.class.getDeclaredMethod("getMetadataCache");
@@ -204,6 +321,18 @@ public class TestIcebergCatalogWrapper {
return config;
}
+ private static Map<String, String> sqliteConfig(Path warehouseDir) {
+ Map<String, String> config = new HashMap<>();
+ config.put(IcebergConstants.CATALOG_BACKEND, "jdbc");
+ config.put(IcebergConstants.URI, "jdbc:sqlite:" +
warehouseDir.resolve("catalog.db"));
+ config.put(IcebergConstants.WAREHOUSE, warehouseDir.toUri().toString());
+ config.put(IcebergConstants.GRAVITINO_JDBC_DRIVER, "org.sqlite.JDBC");
+ config.put(IcebergConstants.ICEBERG_JDBC_USER, "test");
+ config.put(IcebergConstants.ICEBERG_JDBC_PASSWORD, "test");
+ config.put(IcebergConstants.ICEBERG_JDBC_INITIALIZE, "true");
+ return config;
+ }
+
private static Map<String, String> metadataConfig(Path warehouseDir) {
Map<String, String> config = new HashMap<>();
config.put(IcebergConstants.CATALOG_BACKEND, "jdbc");
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
index 3907b1940b..47a1344de3 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
@@ -35,6 +35,7 @@ import
org.apache.gravitino.iceberg.service.IcebergExceptionMapper;
import org.apache.gravitino.iceberg.service.IcebergHealthCheckPathMatcher;
import org.apache.gravitino.iceberg.service.IcebergObjectMapperProvider;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
+import
org.apache.gravitino.iceberg.service.IcebergRequestBodyReaderInterceptor;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
import org.apache.gravitino.iceberg.service.cleanup.IcebergCleanupJobStore;
import org.apache.gravitino.iceberg.service.cleanup.IcebergCleanupManager;
@@ -119,8 +120,13 @@ public class RESTService implements
GravitinoAuxiliaryService {
ResourceConfig config = new ResourceConfig();
config.packages(getIcebergRESTPackages(icebergConfig));
-
config.register(IcebergObjectMapperProvider.class).register(JacksonFeature.class);
+ config
+ .register(IcebergObjectMapperProvider.class)
+ // Without the bundled mappers, Jackson failures reach
IcebergExceptionMapper and are
+ // answered with an Iceberg ErrorModel instead of a text/plain Jackson
message.
+ .register(JacksonFeature.withoutExceptionMappers());
config.register(IcebergExceptionMapper.class);
+ config.register(IcebergRequestBodyReaderInterceptor.class);
config.register(new OutOfMemoryErrorListener());
HttpServerMetricsSource httpServerMetricsSource =
new
HttpServerMetricsSource(MetricsSource.ICEBERG_REST_SERVER_METRIC_NAME, config,
server);
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
index 913dad38d9..b452facd6e 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
@@ -18,8 +18,12 @@
*/
package org.apache.gravitino.iceberg.service;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
import com.google.common.collect.ImmutableMap;
+import java.lang.reflect.Method;
import java.util.Map;
+import java.util.stream.Collectors;
import javax.ws.rs.NotFoundException;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.Response;
@@ -47,6 +51,7 @@ import org.apache.iceberg.exceptions.ServiceFailureException;
import org.apache.iceberg.exceptions.ServiceUnavailableException;
import org.apache.iceberg.exceptions.UnprocessableEntityException;
import org.apache.iceberg.exceptions.ValidationException;
+import org.glassfish.jersey.server.ParamException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -99,9 +104,13 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Throwable> {
* @return the HTTP status code
*/
public static int getErrorCode(Exception ex) {
- Integer code = EXCEPTION_ERROR_CODES.get(ex.getClass());
- if (code != null) {
- return code;
+ // Match the exception's own class first, then its superclasses, so a
subclass such as
+ // NumberFormatException gets the status of IllegalArgumentException.
+ for (Class<?> c = ex.getClass(); c != null; c = c.getSuperclass()) {
+ Integer code = EXCEPTION_ERROR_CODES.get(c);
+ if (code != null) {
+ return code;
+ }
}
if (ex instanceof WebApplicationException) {
return ((WebApplicationException) ex).getResponse().getStatus();
@@ -150,6 +159,7 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Throwable> {
public static Response toRESTResponse(Throwable ex) {
ServerHealth.getInstance().recordFailure(ex);
+ ex = toClientFacingException(ex);
int status =
ex instanceof Exception
? getErrorCode((Exception) ex)
@@ -165,4 +175,145 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Throwable> {
}
return IcebergRESTUtils.errorResponse(ex, status);
}
+
+ /**
+ * Replaces exceptions whose type or message would expose server internals,
and client errors that
+ * reach the server as framework or storage-SDK exceptions, with the Iceberg
exception the REST
+ * spec expects for the condition.
+ *
+ * @param ex the failure raised while processing the request
+ * @return the exception to report to the client
+ */
+ static Throwable toClientFacingException(Throwable ex) {
+ JsonProcessingException jsonError = findCause(ex,
JsonProcessingException.class);
+ if (jsonError != null) {
+ return new BadRequestException("%s", describeJsonError(jsonError));
+ }
+ if (ex instanceof ParamException) {
+ return new BadRequestException(
+ "Invalid value for parameter '%s'", ((ParamException)
ex).getParameterName());
+ }
+ Throwable storageError = findStorageError(ex);
+ if (storageError != null) {
+ RuntimeException mapped = toStorageException(storageError);
+ return mapped == null ? ex : mapped;
+ }
+ return ex;
+ }
+
+ private static String describeJsonError(JsonProcessingException e) {
+ // Iceberg's parsers report invalid input (an unknown update action or
requirement type) as
+ // IllegalArgumentException or UnsupportedOperationException with a
message meant for users.
+ for (Throwable t = e.getCause(); t != null; t = t.getCause()) {
+ if ((t instanceof IllegalArgumentException || t instanceof
UnsupportedOperationException)
+ && t.getMessage() != null) {
+ return "Invalid request body: " + t.getMessage();
+ }
+ }
+ if (e instanceof JsonMappingException && !((JsonMappingException)
e).getPath().isEmpty()) {
+ String field =
+ ((JsonMappingException) e)
+ .getPath().stream()
+ .map(
+ r ->
+ r.getFieldName() != null
+ ? r.getFieldName()
+ : String.valueOf(r.getIndex()))
+ .collect(Collectors.joining("."));
+ return "Invalid request body: unexpected value for field '" + field +
"'";
+ }
+ return "Invalid request body: malformed JSON";
+ }
+
+ private static Throwable findStorageError(Throwable ex) {
+ for (Throwable t = ex; t != null; t = t.getCause()) {
+ String name = t.getClass().getName();
+ if (name.startsWith("software.amazon.awssdk.")
+ || name.startsWith("com.google.cloud.storage.")
+ || name.startsWith("com.google.cloud.BaseServiceException")
+ || name.startsWith("com.azure.")) {
+ return t;
+ }
+ if (t.getCause() == t) {
+ break;
+ }
+ }
+ return null;
+ }
+
+ private static RuntimeException toStorageException(Throwable storageError) {
+ Integer status = storageStatusCode(storageError);
+ String reason = storageErrorCode(storageError);
+ String message = storageError.getMessage() == null ? "" :
storageError.getMessage();
+ // A missing object (for example a deleted metadata file) keeps its
existing handling: it is not
+ // a problem with the request.
+ if (status != null && status == 404 && !"NoSuchBucket".equals(reason)) {
+ return null;
+ }
+ // A bucket that does not exist, a bucket in another region or behind
another endpoint (301,
+ // 307), a request the storage service rejects as malformed (400), and a
request the SDK cannot
+ // even build (a location without a bucket) are problems with the location
the client asked
+ // for. Anything else is the storage service failing. The SDK message is
never returned: it
+ // carries request ids and SDK details.
+ boolean clientLocation =
+ status != null && (status == 400 || status == 404 || status == 301 ||
status == 307);
+ if (clientLocation || (status == null && message.startsWith("Unable to
marshall request"))) {
+ return new BadRequestException(
+ "The storage location cannot be used by this catalog (%s%s). Check
the location"
+ + " and that the catalog's storage configuration can reach it.",
+ reason, status == null ? "" : ", HTTP " + status);
+ }
+ return new ServiceUnavailableException(
+ "The catalog's storage service did not complete the request (%s%s)",
+ reason, status == null ? "" : ", HTTP " + status);
+ }
+
+ private static String storageErrorCode(Throwable storageError) {
+ // AWS SDK v2 reports the service error code (for example NoSuchBucket) in
awsErrorDetails().
+ Object details = invoke(storageError, "awsErrorDetails");
+ Object code = details == null ? null : invoke(details, "errorCode");
+ if (code instanceof String && !((String) code).isEmpty()) {
+ return (String) code;
+ }
+ return storageError.getClass().getSimpleName().replaceAll("Exception$",
"");
+ }
+
+ private static Integer storageStatusCode(Throwable storageError) {
+ // AWS SDK v2: statusCode(); Google Cloud: getCode(); Azure:
getResponse().getStatusCode().
+ Integer status = invokeInt(storageError, "statusCode");
+ if (status == null) {
+ status = invokeInt(storageError, "getCode");
+ }
+ if (status == null) {
+ Object response = invoke(storageError, "getResponse");
+ status = response == null ? null : invokeInt(response, "getStatusCode");
+ }
+ return status != null && status > 0 ? status : null;
+ }
+
+ private static Integer invokeInt(Object target, String methodName) {
+ Object value = invoke(target, methodName);
+ return value instanceof Integer ? (Integer) value : null;
+ }
+
+ private static Object invoke(Object target, String methodName) {
+ try {
+ Method method = target.getClass().getMethod(methodName);
+ return method.invoke(target);
+ } catch (ReflectiveOperationException | RuntimeException e) {
+ return null;
+ }
+ }
+
+ private static <T extends Throwable> T findCause(Throwable ex, Class<T>
type) {
+ for (Throwable t = ex; t != null; t = t.getCause()) {
+ if (type.isInstance(t)) {
+ return type.cast(t);
+ }
+ if (t.getCause() == t) {
+ break;
+ }
+ }
+ return null;
+ }
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
index 1a02022b8a..1d9e85bb97 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
@@ -53,9 +53,11 @@ import org.apache.gravitino.server.web.JettyServerConfig;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.io.StorageCredential;
import org.apache.iceberg.io.SupportsStorageCredentials;
+import org.apache.iceberg.rest.RESTRequest;
import org.apache.iceberg.rest.RESTUtil;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.ImmutableLoadCredentialsResponse;
@@ -459,12 +461,44 @@ public class IcebergRESTUtils {
includeErrorStackTrace = include;
}
+ /**
+ * Checks that a request body was sent and passes the request's own
validation, so a missing or
+ * incomplete body is answered with 400 instead of failing later with an
internal error.
+ *
+ * @param request the deserialized request body, which is null when no body
was sent
+ * @param requestType the request type named in the error message
+ * @throws BadRequestException if the body is missing or fails validation
+ */
+ public static void validateRequest(Object request, String requestType) {
+ if (request == null) {
+ throw new BadRequestException("Invalid request: a %s body is required",
requestType);
+ }
+ if (request instanceof RESTRequest) {
+ try {
+ ((RESTRequest) request).validate();
+ } catch (IllegalArgumentException | IllegalStateException |
NullPointerException e) {
+ throw new BadRequestException(
+ "Invalid %s: %s",
+ requestType, e.getMessage() == null ? "a required field is
missing" : e.getMessage());
+ }
+ }
+ }
+
public static Response errorResponse(Throwable ex, int httpStatus) {
+ String message = ex.getMessage();
+ if (message == null) {
+ message =
+ httpStatus >= Status.INTERNAL_SERVER_ERROR.getStatusCode()
+ ? "Internal server error"
+ : Status.fromStatusCode(httpStatus) == null
+ ? "Request failed"
+ : Status.fromStatusCode(httpStatus).getReasonPhrase();
+ }
ErrorResponse.Builder builder =
ErrorResponse.builder()
.responseCode(httpStatus)
.withType(ex.getClass().getSimpleName())
- .withMessage(ex.getMessage());
+ .withMessage(message);
if (includeErrorStackTrace) {
builder.withStackTrace(ex);
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRequestBodyReaderInterceptor.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRequestBodyReaderInterceptor.java
new file mode 100644
index 0000000000..6a34418dd9
--- /dev/null
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRequestBodyReaderInterceptor.java
@@ -0,0 +1,52 @@
+/*
+ * 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.gravitino.iceberg.service;
+
+import java.io.IOException;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.ext.ReaderInterceptor;
+import javax.ws.rs.ext.ReaderInterceptorContext;
+import org.apache.iceberg.exceptions.BadRequestException;
+
+/**
+ * Reports a request body that Iceberg's parsers reject as a 400 Bad Request.
+ *
+ * <p>Iceberg's JSON parsers reject invalid input, such as an unknown metadata
update action or
+ * requirement type, with {@link IllegalArgumentException}, {@link
UnsupportedOperationException} or
+ * {@link IllegalStateException} instead of a Jackson exception. Raised while
the body is read,
+ * these would otherwise be reported as server errors or as 406 Not Acceptable.
+ */
+public class IcebergRequestBodyReaderInterceptor implements ReaderInterceptor {
+
+ @Override
+ public Object aroundReadFrom(ReaderInterceptorContext context) throws
IOException {
+ try {
+ return context.proceed();
+ } catch (WebApplicationException e) {
+ throw e;
+ } catch (IllegalArgumentException
+ | UnsupportedOperationException
+ | IllegalStateException
+ | NullPointerException e) {
+ throw new BadRequestException(
+ "Invalid request body: %s",
+ e.getMessage() == null ? "a required field is missing or invalid" :
e.getMessage());
+ }
+ }
+}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
index 0e0c2cf2dd..f62c6048cc 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
@@ -248,6 +248,7 @@ public class IcebergNamespaceOperations {
@IcebergAuthorizationMetadata(
type = IcebergAuthorizationMetadata.RequestType.CREATE_NAMESPACE)
CreateNamespaceRequest createNamespaceRequest) {
+ IcebergRESTUtils.validateRequest(createNamespaceRequest,
"CreateNamespaceRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
LOG.info(
"Create Iceberg namespace, catalog: {}, createNamespaceRequest: {}",
@@ -281,6 +282,8 @@ public class IcebergNamespaceOperations {
@AuthorizationMetadata(type = Entity.EntityType.SCHEMA) @Encoded()
@PathParam("namespace")
String namespace,
UpdateNamespacePropertiesRequest updateNamespacePropertiesRequest) {
+ IcebergRESTUtils.validateRequest(
+ updateNamespacePropertiesRequest, "UpdateNamespacePropertiesRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
RESTUtil.decodeNamespace(namespace,
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
@@ -322,6 +325,7 @@ public class IcebergNamespaceOperations {
String namespace,
RegisterTableRequest registerTableRequest,
@HeaderParam(IcebergTableOperations.X_ICEBERG_ACCESS_DELEGATION) String
accessDelegation) {
+ IcebergRESTUtils.validateRequest(registerTableRequest,
"RegisterTableRequest");
boolean isCredentialVending =
IcebergTableOperations.isCredentialVending(accessDelegation);
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
@@ -363,6 +367,7 @@ public class IcebergNamespaceOperations {
@AuthorizationMetadata(type = Entity.EntityType.SCHEMA) @Encoded()
@PathParam("namespace")
String namespace,
RegisterViewRequest registerViewRequest) {
+ IcebergRESTUtils.validateRequest(registerViewRequest,
"RegisterViewRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
RESTUtil.decodeNamespace(namespace,
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
@@ -428,6 +433,8 @@ public class IcebergNamespaceOperations {
private String SerializeUpdateNamespacePropertiesRequest(
UpdateNamespacePropertiesRequest updateNamespacePropertiesRequest) {
+ IcebergRESTUtils.validateRequest(
+ updateNamespacePropertiesRequest, "UpdateNamespacePropertiesRequest");
try {
return
icebergObjectMapper.writeValueAsString(updateNamespacePropertiesRequest);
} catch (JsonProcessingException e) {
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
index d75213e8c0..e098f2ef57 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
@@ -187,6 +187,7 @@ public class IcebergTableOperations {
String namespace,
CreateTableRequest createTableRequest,
@HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation) {
+ IcebergRESTUtils.validateRequest(createTableRequest, "CreateTableRequest");
boolean isCredentialVending = isCredentialVending(accessDelegation);
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
@@ -229,6 +230,7 @@ public class IcebergTableOperations {
@AuthorizationMetadata(type = Entity.EntityType.TABLE) @Encoded()
@PathParam("table")
String table,
UpdateTableRequest updateTableRequest) {
+ IcebergRESTUtils.validateRequest(updateTableRequest, "UpdateTableRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
RESTUtil.decodeNamespace(namespace,
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
@@ -430,6 +432,7 @@ public class IcebergTableOperations {
String namespace,
@AuthorizationMetadata(type = EntityType.TABLE) @Encoded()
@PathParam("table") String table,
ReportMetricsRequest request) {
+ IcebergRESTUtils.validateRequest(request, "ReportMetricsRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
RESTUtil.decodeNamespace(namespace,
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
@@ -562,6 +565,7 @@ public class IcebergTableOperations {
@Encoded() @PathParam("table") @AuthorizationMetadata(type =
EntityType.TABLE) String table,
PlanTableScanRequest scanRequest,
@HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation) {
+ IcebergRESTUtils.validateRequest(scanRequest, "PlanTableScanRequest");
if (scanRequest == null) {
return missingRequestBody("plan table scan");
}
@@ -627,6 +631,7 @@ public class IcebergTableOperations {
String namespace,
@Encoded() @PathParam("table") @AuthorizationMetadata(type =
EntityType.TABLE) String table,
FetchScanTasksRequest request) {
+ IcebergRESTUtils.validateRequest(request, "FetchScanTasksRequest");
if (request == null) {
return missingRequestBody("fetch scan tasks");
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
index 95555cfe3e..830b21daa8 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableRenameOperations.java
@@ -75,6 +75,7 @@ public class IcebergTableRenameOperations {
@AuthorizationMetadata(type = Entity.EntityType.CATALOG)
@PathParam("prefix") String prefix,
@IcebergAuthorizationMetadata(type = RequestType.RENAME_TABLE)
RenameTableRequest renameTableRequest) {
+ IcebergRESTUtils.validateRequest(renameTableRequest, "RenameTableRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
LOG.info(
"Rename Iceberg tables, catalog: {}, source: {}, destination: {}.",
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java
index b33a53b2e6..f4900cd658 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewOperations.java
@@ -145,6 +145,7 @@ public class IcebergViewOperations {
@AuthorizationMetadata(type = Entity.EntityType.SCHEMA) @Encoded()
@PathParam("namespace")
String namespace,
CreateViewRequest createViewRequest) {
+ IcebergRESTUtils.validateRequest(createViewRequest, "CreateViewRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
RESTUtil.decodeNamespace(namespace,
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
@@ -227,6 +228,7 @@ public class IcebergViewOperations {
String namespace,
@AuthorizationMetadata(type = EntityType.VIEW) @Encoded()
@PathParam("view") String view,
UpdateTableRequest replaceViewRequest) {
+ IcebergRESTUtils.validateRequest(replaceViewRequest, "UpdateTableRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS =
RESTUtil.decodeNamespace(namespace,
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewRenameOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewRenameOperations.java
index 40c3f6da07..a5b4dc5f12 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewRenameOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergViewRenameOperations.java
@@ -74,6 +74,7 @@ public class IcebergViewRenameOperations {
@AuthorizationMetadata(type = Entity.EntityType.CATALOG)
@PathParam("prefix") String prefix,
@IcebergAuthorizationMetadata(type = RequestType.RENAME_VIEW)
RenameTableRequest renameViewRequest) {
+ IcebergRESTUtils.validateRequest(renameViewRequest, "RenameTableRequest");
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
LOG.info(
"Rename Iceberg view, catalog: {}, source: {}, destination: {}.",
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/CreateNamespaceAuthzHandler.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/CreateNamespaceAuthzHandler.java
index 52bf8429e5..1d12ee4364 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/CreateNamespaceAuthzHandler.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/CreateNamespaceAuthzHandler.java
@@ -29,6 +29,7 @@ import
org.apache.gravitino.server.web.filter.BaseMetadataAuthorizationMethodInt
import org.apache.gravitino.utils.HierarchicalSchemaUtil;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
@@ -53,6 +54,10 @@ public class CreateNamespaceAuthzHandler implements
AuthorizationHandler {
@Override
public void process(Map<EntityType, NameIdentifier> nameIdentifierMap) {
CreateNamespaceRequest request = extractRequest();
+ if (request == null || request.namespace() == null ||
request.namespace().isEmpty()) {
+ throw new BadRequestException(
+ "Invalid namespace creation request: a non-empty namespace is
required");
+ }
NameIdentifier catalogIdent = nameIdentifierMap.get(EntityType.CATALOG);
if (catalogIdent == null) {
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
index 07c480bf70..ef408e7048 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameTableAuthzHandler.java
@@ -34,6 +34,7 @@ import
org.apache.gravitino.server.web.filter.BaseMetadataAuthorizationMethodInt
import org.apache.gravitino.utils.HierarchicalSchemaUtil;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.rest.requests.RenameTableRequest;
@@ -55,19 +56,27 @@ public class RenameTableAuthzHandler implements
AuthorizationHandler {
@Override
public void process(Map<EntityType, NameIdentifier> nameIdentifierMap) {
RenameTableRequest renameTableRequest = null;
+ boolean found = false;
for (int i = 0; i < parameters.length; i++) {
IcebergAuthorizationMetadata metadata =
parameters[i].getAnnotation(IcebergAuthorizationMetadata.class);
if (metadata != null
&& metadata.type() ==
IcebergAuthorizationMetadata.RequestType.RENAME_TABLE) {
renameTableRequest = (RenameTableRequest) args[i];
+ found = true;
break;
}
}
- if (renameTableRequest == null) {
+ if (!found) {
throw new ForbiddenException("RenameTableRequest not found in
parameters");
}
+ if (renameTableRequest == null
+ || renameTableRequest.source() == null
+ || renameTableRequest.destination() == null) {
+ throw new BadRequestException(
+ "Invalid table rename request: both source and destination are
required");
+ }
// Extract metalake and catalog from nameIdentifierMap
NameIdentifier metalakeIdent = nameIdentifierMap.get(EntityType.METALAKE);
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameViewAuthzHandler.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameViewAuthzHandler.java
index 865498e989..bc2f43b8a2 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameViewAuthzHandler.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/RenameViewAuthzHandler.java
@@ -33,6 +33,7 @@ import
org.apache.gravitino.server.web.filter.BaseMetadataAuthorizationMethodInt
import org.apache.gravitino.utils.HierarchicalSchemaUtil;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.ForbiddenException;
import org.apache.iceberg.rest.requests.RenameTableRequest;
@@ -54,19 +55,27 @@ public class RenameViewAuthzHandler implements
AuthorizationHandler {
@Override
public void process(Map<EntityType, NameIdentifier> nameIdentifierMap) {
RenameTableRequest renameViewRequest = null;
+ boolean found = false;
for (int i = 0; i < parameters.length; i++) {
IcebergAuthorizationMetadata metadata =
parameters[i].getAnnotation(IcebergAuthorizationMetadata.class);
if (metadata != null
&& metadata.type() ==
IcebergAuthorizationMetadata.RequestType.RENAME_VIEW) {
renameViewRequest = (RenameTableRequest) args[i];
+ found = true;
break;
}
}
- if (renameViewRequest == null) {
+ if (!found) {
throw new ForbiddenException("RenameViewRequest not found in
parameters");
}
+ if (renameViewRequest == null
+ || renameViewRequest.source() == null
+ || renameViewRequest.destination() == null) {
+ throw new BadRequestException(
+ "Invalid view rename request: both source and destination are
required");
+ }
// Extract metalake and catalog from nameIdentifierMap
NameIdentifier metalakeIdent = nameIdentifierMap.get(EntityType.METALAKE);
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java
index 1ce5ebc6ba..1c88bd5220 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java
@@ -18,14 +18,18 @@
*/
package org.apache.gravitino.iceberg.service;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.JsonMappingException;
import javax.ws.rs.NotAcceptableException;
import javax.ws.rs.NotAllowedException;
import javax.ws.rs.NotFoundException;
import javax.ws.rs.NotSupportedException;
+import javax.ws.rs.ProcessingException;
import javax.ws.rs.core.Response;
import org.apache.gravitino.exceptions.TokenExpiredException;
import org.apache.gravitino.server.web.ServerHealth;
import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.CommitStateUnknownException;
import org.apache.iceberg.exceptions.ForbiddenException;
@@ -38,10 +42,14 @@ import
org.apache.iceberg.exceptions.ServiceUnavailableException;
import org.apache.iceberg.exceptions.UnprocessableEntityException;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.glassfish.jersey.server.ParamException;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
+import software.amazon.awssdk.awscore.exception.AwsErrorDetails;
+import software.amazon.awssdk.core.exception.SdkClientException;
+import software.amazon.awssdk.services.s3.model.S3Exception;
public class TestIcebergExceptionMapper {
private final IcebergExceptionMapper icebergExceptionMapper = new
IcebergExceptionMapper();
@@ -105,7 +113,10 @@ public class TestIcebergExceptionMapper {
Assertions.assertEquals(500, response.getStatus());
ErrorResponse entity = (ErrorResponse) response.getEntity();
Assertions.assertEquals(error.getClass().getSimpleName(),
entity.type());
- Assertions.assertEquals(error.getMessage(), entity.message());
+ // The ErrorModel message is required, so an error without one gets
a default.
+ Assertions.assertEquals(
+ error.getMessage() == null ? "Internal server error" :
error.getMessage(),
+ entity.message());
Assertions.assertTrue(
String.join("\n", entity.stack())
.contains("Caused by: java.lang.IllegalStateException: root
cause"));
@@ -115,6 +126,99 @@ public class TestIcebergExceptionMapper {
}
}
+ @Test
+ public void testSubclassOfMappedExceptionGetsParentStatus() {
+ checkExceptionStatus(new NumberFormatException("For input string: \"x\""),
400);
+ }
+
+ @Test
+ public void testJsonErrorsAreBadRequest() {
+ // Iceberg's parsers explain an invalid body with an
IllegalArgumentException cause.
+ JsonMappingException withParserCause =
+ JsonMappingException.from(
+ (JsonParser) null,
+ "deserialize failed",
+ new IllegalArgumentException("Cannot parse metadata update.
Invalid action: bogus"));
+ assertBadRequest(new ProcessingException(withParserCause), "Invalid
action: bogus");
+
+ JsonMappingException withPath = JsonMappingException.from((JsonParser)
null, "wrong type");
+ withPath.prependPath(new JsonMappingException.Reference(null,
"page-size"));
+ assertBadRequest(withPath, "page-size");
+
+ assertBadRequest(
+ JsonMappingException.from((JsonParser) null, "Unexpected
end-of-input"), "malformed JSON");
+ }
+
+ @Test
+ public void testInvalidParameterIsBadRequest() {
+ assertBadRequest(
+ new ParamException.QueryParamException(new
NumberFormatException("abc"), "pageSize", null),
+ "pageSize");
+ }
+
+ @Test
+ public void testStorageErrorsDoNotLeakSdkMessages() {
+ // A bucket that does not exist is a problem with the requested location.
+ S3Exception noSuchBucket =
+ (S3Exception)
+ S3Exception.builder()
+ .statusCode(404)
+ .message("The specified bucket does not exist (Request ID:
SECRET-ID)")
+
.awsErrorDetails(AwsErrorDetails.builder().errorCode("NoSuchBucket").build())
+ .build();
+ Throwable mapped =
+ IcebergExceptionMapper.toClientFacingException(new
RuntimeException(noSuchBucket));
+ Assertions.assertInstanceOf(BadRequestException.class, mapped);
+
+ // Access denied by the storage service is the service failing for this
catalog: 503, and the
+ // SDK message, with its request id, is not returned.
+ S3Exception denied =
+ (S3Exception)
+ S3Exception.builder()
+ .statusCode(403)
+ .message("Access Denied (Service: S3, Status Code: 403,
Request ID: SECRET-ID)")
+ .build();
+ try (Response response = icebergExceptionMapper.toResponse(new
RuntimeException(denied))) {
+ Assertions.assertEquals(503, response.getStatus());
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+ Assertions.assertFalse(entity.message().contains("SECRET-ID"),
entity.message());
+ }
+
+ // A location the SDK cannot turn into a request (for example s3://
without a bucket).
+ SdkClientException marshall =
+ SdkClientException.create(
+ "Unable to marshall request to JSON: Parameter 'Bucket' is empty");
+ Assertions.assertInstanceOf(
+ BadRequestException.class,
IcebergExceptionMapper.toClientFacingException(marshall));
+
+ // A missing object keeps its existing handling.
+ S3Exception missingObject = (S3Exception)
S3Exception.builder().statusCode(404).build();
+ RuntimeException wrapper = new RuntimeException(missingObject);
+ Assertions.assertSame(wrapper,
IcebergExceptionMapper.toClientFacingException(wrapper));
+ }
+
+ @Test
+ public void testErrorWithoutMessageGetsDefaultMessage() {
+ try (Response response = icebergExceptionMapper.toResponse(new
RuntimeException())) {
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+ Assertions.assertEquals("Internal server error", entity.message());
+ }
+ try (Response response = icebergExceptionMapper.toResponse(new
ForbiddenException(""))) {
+ Assertions.assertEquals(403, response.getStatus());
+ }
+ }
+
+ private void assertBadRequest(Throwable exception, String expectedInMessage)
{
+ try (Response response = icebergExceptionMapper.toResponse(exception)) {
+ Assertions.assertEquals(400, response.getStatus());
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+ Assertions.assertEquals("BadRequestException", entity.type());
+ Assertions.assertTrue(
+ entity.message().contains(expectedInMessage),
+ "Expected '" + expectedInMessage + "' in: " + entity.message());
+ }
+ }
+
private void checkExceptionStatus(Exception exception, int statusCode) {
Response response = icebergExceptionMapper.toResponse(exception);
Assertions.assertEquals(statusCode, response.getStatus());
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
index af01018630..ee56ba31b9 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java
@@ -43,9 +43,11 @@ import org.apache.iceberg.SortOrder;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.io.StorageCredential;
import org.apache.iceberg.io.SupportsStorageCredentials;
+import org.apache.iceberg.rest.RESTRequest;
import org.apache.iceberg.rest.credentials.Credential;
import org.apache.iceberg.rest.requests.CreateTableRequest;
import org.apache.iceberg.rest.responses.ErrorResponse;
@@ -470,4 +472,34 @@ public class TestIcebergRESTUtils {
IcebergRESTUtils.setIncludeErrorStackTrace(true);
}
}
+
+ @Test
+ void testValidateRequestRejectsMissingOrInvalidBody() {
+ BadRequestException missing =
+ Assertions.assertThrows(
+ BadRequestException.class,
+ () -> IcebergRESTUtils.validateRequest(null,
"CreateTableRequest"));
+ Assertions.assertTrue(missing.getMessage().contains("CreateTableRequest"));
+
+ RESTRequest invalid =
+ () -> {
+ throw new IllegalArgumentException("Invalid name: null");
+ };
+ BadRequestException invalidBody =
+ Assertions.assertThrows(
+ BadRequestException.class,
+ () -> IcebergRESTUtils.validateRequest(invalid,
"CreateTableRequest"));
+ Assertions.assertTrue(invalidBody.getMessage().contains("Invalid name:
null"));
+
+ RESTRequest npe =
+ () -> {
+ throw new NullPointerException();
+ };
+ Assertions.assertThrows(
+ BadRequestException.class,
+ () -> IcebergRESTUtils.validateRequest(npe, "UpdateTableRequest"));
+
+ RESTRequest valid = () -> {};
+ Assertions.assertDoesNotThrow(() ->
IcebergRESTUtils.validateRequest(valid, "X"));
+ }
}
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRequestBodyReaderInterceptor.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRequestBodyReaderInterceptor.java
new file mode 100644
index 0000000000..042eab74c6
--- /dev/null
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRequestBodyReaderInterceptor.java
@@ -0,0 +1,70 @@
+/*
+ * 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.gravitino.iceberg.service;
+
+import javax.ws.rs.NotAcceptableException;
+import javax.ws.rs.ext.ReaderInterceptorContext;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+public class TestIcebergRequestBodyReaderInterceptor {
+
+ private final IcebergRequestBodyReaderInterceptor interceptor =
+ new IcebergRequestBodyReaderInterceptor();
+
+ @Test
+ public void testParserRejectionsBecomeBadRequest() throws Exception {
+ for (RuntimeException parserError :
+ new RuntimeException[] {
+ new UnsupportedOperationException(
+ "Cannot convert metadata update action to json: no-such-action"),
+ new IllegalArgumentException("Cannot parse missing field: term"),
+ new IllegalStateException("Invalid state"),
+ new NullPointerException()
+ }) {
+ ReaderInterceptorContext context =
Mockito.mock(ReaderInterceptorContext.class);
+ Mockito.when(context.proceed()).thenThrow(parserError);
+ BadRequestException e =
+ Assertions.assertThrows(
+ BadRequestException.class, () ->
interceptor.aroundReadFrom(context));
+ Assertions.assertTrue(e.getMessage().startsWith("Invalid request body:
"), e.getMessage());
+ if (parserError.getMessage() != null) {
+
Assertions.assertTrue(e.getMessage().contains(parserError.getMessage()),
e.getMessage());
+ }
+ }
+ }
+
+ @Test
+ public void testOtherOutcomesPassThrough() throws Exception {
+ ReaderInterceptorContext ok = Mockito.mock(ReaderInterceptorContext.class);
+ Object body = new Object();
+ Mockito.when(ok.proceed()).thenReturn(body);
+ Assertions.assertSame(body, interceptor.aroundReadFrom(ok));
+
+ ReaderInterceptorContext webError =
Mockito.mock(ReaderInterceptorContext.class);
+ NotAcceptableException notAcceptable = new NotAcceptableException();
+ Mockito.when(webError.proceed()).thenThrow(notAcceptable);
+ Assertions.assertSame(
+ notAcceptable,
+ Assertions.assertThrows(
+ NotAcceptableException.class, () ->
interceptor.aroundReadFrom(webError)));
+ }
+}
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
index 1b8c5256b3..8d7015dd0b 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
@@ -38,6 +38,7 @@ import
org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergExceptionMapper;
import org.apache.gravitino.iceberg.service.IcebergObjectMapperProvider;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
+import
org.apache.gravitino.iceberg.service.IcebergRequestBodyReaderInterceptor;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
import org.apache.gravitino.iceberg.service.cleanup.IcebergCleanupManager;
import
org.apache.gravitino.iceberg.service.dispatcher.IcebergNamespaceEventDispatcher;
@@ -117,8 +118,11 @@ public class IcebergRestTestUtil {
WrapperManagerFactory wrapperManagerFactory) {
ResourceConfig resourceConfig = new ResourceConfig();
resourceConfig.register(c);
-
resourceConfig.register(IcebergObjectMapperProvider.class).register(JacksonFeature.class);
+ resourceConfig
+ .register(IcebergObjectMapperProvider.class)
+ .register(JacksonFeature.withoutExceptionMappers());
resourceConfig.register(IcebergExceptionMapper.class);
+ resourceConfig.register(IcebergRequestBodyReaderInterceptor.class);
if (DEBUG_SERVER_LOG_ENABLED) {
resourceConfig.register(
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
index 72f21ac8cf..528c9ee699 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java
@@ -617,6 +617,100 @@ public class TestIcebergTableOperations extends
IcebergNamespaceTestBase {
loadTableResponse.config().get(Credential.CREDENTIAL_TYPE));
}
+ @ParameterizedTest
+
@MethodSource("org.apache.gravitino.iceberg.service.rest.IcebergRestTestUtil#testNamespaces")
+ void testInvalidRequestBodiesReturnBadRequest(Namespace namespace) {
+ verifyCreateNamespaceSucc(namespace);
+ verifyCreateTableSucc(namespace, "bad_body_tbl");
+
+ // Malformed JSON must get an Iceberg ErrorModel, not Jackson's text/plain
message.
+ assertBadRequest(
+ getTableClientBuilder(namespace, Optional.empty())
+ .post(Entity.entity("{\"name\": \"t\", ",
MediaType.APPLICATION_JSON_TYPE)),
+ "malformed JSON");
+ // An empty object is valid JSON but not a valid CreateTableRequest.
+ assertBadRequest(
+ getTableClientBuilder(namespace, Optional.empty())
+ .post(Entity.entity("{}", MediaType.APPLICATION_JSON_TYPE)),
+ null);
+ // An update action the server does not know is a client error, with
Iceberg's own message.
+ assertBadRequest(
+ getTableClientBuilder(namespace, Optional.of("bad_body_tbl"))
+ .post(
+ Entity.entity(
+ "{\"requirements\": [], \"updates\": [{\"action\":
\"no-such-action\"}]}",
+ MediaType.APPLICATION_JSON_TYPE)),
+ "no-such-action");
+ // A transform the column type does not support: day() on a string column.
+ assertBadRequest(
+ getTableClientBuilder(namespace, Optional.empty())
+ .post(
+ Entity.entity(
+ "{\"name\": \"bad_transform_tbl\", \"schema\": {\"type\":
\"struct\","
+ + " \"schema-id\": 0, \"fields\": [{\"id\": 1,
\"name\": \"s\","
+ + " \"required\": true, \"type\": \"string\"}]},
\"partition-spec\":"
+ + " {\"spec-id\": 0, \"fields\": [{\"name\":
\"s_day\", \"transform\":"
+ + " \"day\", \"source-id\": 1, \"field-id\": 1000}]}}",
+ MediaType.APPLICATION_JSON_TYPE)),
+ null);
+ // A format version that is not a number, or not one the table spec
defines.
+ for (String version : new String[] {"abc", "0", "4"}) {
+ CreateTableRequest request =
+ CreateTableRequest.builder()
+ .withName("bad_version_tbl")
+ .withSchema(tableSchema)
+ .setProperties(ImmutableMap.of("format-version", version))
+ .build();
+ assertBadRequest(
+ getTableClientBuilder(namespace, Optional.empty())
+ .post(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE)),
+ version);
+ }
+ }
+
+ @ParameterizedTest
+
@MethodSource("org.apache.gravitino.iceberg.service.rest.IcebergRestTestUtil#testNamespaces")
+ void testSetDefaultToUnknownSpecOrSortOrderReturnsBadRequest(Namespace
namespace) {
+ verifyCreateNamespaceSucc(namespace);
+ verifyCreateTableSucc(namespace, "bad_default_tbl");
+
+ for (MetadataUpdate update :
+ new MetadataUpdate[] {
+ new MetadataUpdate.SetDefaultPartitionSpec(42), new
MetadataUpdate.SetDefaultSortOrder(42)
+ }) {
+ UpdateTableRequest request =
+ new UpdateTableRequest(ImmutableList.of(), ImmutableList.of(update));
+ assertBadRequest(
+ getTableClientBuilder(namespace, Optional.of("bad_default_tbl"))
+ .post(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE)),
+ "42");
+ }
+ // The table must still load: the rejected updates were not committed.
+ Assertions.assertEquals(
+ Status.OK.getStatusCode(),
+ getTableClientBuilder(namespace,
Optional.of("bad_default_tbl")).get().getStatus());
+ }
+
+ private static void assertBadRequest(Response response, String
expectedInMessage) {
+ Assertions.assertTrue(
+ response.getMediaType().isCompatible(MediaType.APPLICATION_JSON_TYPE),
+ "Expected a JSON error body, got " + response.getMediaType());
+ ErrorResponse error = response.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(
+ Status.BAD_REQUEST.getStatusCode(),
+ response.getStatus(),
+ () -> error.type() + ": " + error.message() + "\n" + String.join("\n",
error.stack()));
+ Assertions.assertEquals(400, error.code());
+ Assertions.assertTrue(
+ ImmutableSet.of("BadRequestException",
"IllegalArgumentException").contains(error.type()),
+ "Unexpected error type " + error.type());
+ if (expectedInMessage != null) {
+ Assertions.assertTrue(
+ error.message().contains(expectedInMessage),
+ "Expected '" + expectedInMessage + "' in: " + error.message());
+ }
+ }
+
private Response doCreateTableWithCredentialVending(Namespace ns, String
name, String location) {
CreateTableRequest createTableRequest =
CreateTableRequest.builder()
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergRequestBodyAuthzHandlers.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergRequestBodyAuthzHandlers.java
new file mode 100644
index 0000000000..a34ccf037f
--- /dev/null
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/server/web/filter/TestIcebergRequestBodyAuthzHandlers.java
@@ -0,0 +1,120 @@
+/*
+ * 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.gravitino.server.web.filter;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Parameter;
+import java.util.HashMap;
+import
org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata;
+import
org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata.RequestType;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
+import org.apache.iceberg.rest.requests.RenameTableRequest;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+/**
+ * Tests that authorization handlers answer a missing or incomplete request
body with 400 instead of
+ * a 403 or an internal error.
+ */
+public class TestIcebergRequestBodyAuthzHandlers {
+
+ @Test
+ public void testCreateNamespaceWithoutBodyOrNamespaceIsBadRequest() throws
Exception {
+ Parameter[] parameters = parameters("createNamespace",
CreateNamespaceRequest.class);
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ new CreateNamespaceAuthzHandler(parameters, new Object[] {null})
+ .process(new HashMap<>()));
+
+ CreateNamespaceRequest empty = Mockito.mock(CreateNamespaceRequest.class);
+ Mockito.when(empty.namespace()).thenReturn(Namespace.empty());
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ new CreateNamespaceAuthzHandler(parameters, new Object[] {empty})
+ .process(new HashMap<>()));
+
+ CreateNamespaceRequest noNamespace =
Mockito.mock(CreateNamespaceRequest.class);
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ new CreateNamespaceAuthzHandler(parameters, new Object[]
{noNamespace})
+ .process(new HashMap<>()));
+ }
+
+ @Test
+ public void testRenameTableWithoutSourceOrDestinationIsBadRequest() throws
Exception {
+ Parameter[] parameters = parameters("renameTable",
RenameTableRequest.class);
+ for (RenameTableRequest request : incompleteRenames()) {
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ new RenameTableAuthzHandler(parameters, new Object[] {request})
+ .process(new HashMap<>()));
+ }
+ }
+
+ @Test
+ public void testRenameViewWithoutSourceOrDestinationIsBadRequest() throws
Exception {
+ Parameter[] parameters = parameters("renameView",
RenameTableRequest.class);
+ for (RenameTableRequest request : incompleteRenames()) {
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ new RenameViewAuthzHandler(parameters, new Object[] {request})
+ .process(new HashMap<>()));
+ }
+ }
+
+ private static RenameTableRequest[] incompleteRenames() {
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("db"), "t");
+ RenameTableRequest noDestination = Mockito.mock(RenameTableRequest.class);
+ Mockito.when(noDestination.source()).thenReturn(ident);
+ RenameTableRequest noSource = Mockito.mock(RenameTableRequest.class);
+ Mockito.when(noSource.destination()).thenReturn(ident);
+ return new RenameTableRequest[] {null, noDestination, noSource};
+ }
+
+ private static Parameter[] parameters(String methodName, Class<?>
requestClass) throws Exception {
+ Method method = TestOperations.class.getMethod(methodName, requestClass);
+ return method.getParameters();
+ }
+
+ /** Provides annotated parameters like the REST operations do. */
+ @SuppressWarnings("unused")
+ public static class TestOperations {
+ public void createNamespace(
+ @IcebergAuthorizationMetadata(type = RequestType.CREATE_NAMESPACE)
+ CreateNamespaceRequest request) {}
+
+ public void renameTable(
+ @IcebergAuthorizationMetadata(type = RequestType.RENAME_TABLE)
+ RenameTableRequest request) {}
+
+ public void renameView(
+ @IcebergAuthorizationMetadata(type = RequestType.RENAME_VIEW)
RenameTableRequest request) {}
+ }
+}
