This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 9591bbebdb [Cherry-pick to branch-1.3] [#14432] fix(iceberg-rest):
answer invalid requests with 400 and an ErrorModel (#14434)
9591bbebdb is described below
commit 9591bbebdbf229ecc848a38b399498c1218fec4d
Author: Mark Hoerth <[email protected]>
AuthorDate: Sun Nov 1 18:45:17 2026 -0800
[Cherry-pick to branch-1.3] [#14432] fix(iceberg-rest): answer invalid
requests with 400 and an ErrorModel (#14434)
Backport to branch-1.3 of the fix for #14432 (same change as the PR to
main). Adapted for branch-1.3: adds the BadRequestException to 400
mapping that main already has (without it these 400s would be 500), adds
the AWS SDK to the test compile classpath as main does, and leaves out
main-only routes (register-view, fetch-scan-tasks) and the error
stack-trace setting.
### 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 +++++++++++++++++
iceberg/iceberg-rest-server/build.gradle.kts | 2 +
.../org/apache/gravitino/iceberg/RESTService.java | 8 +-
.../iceberg/service/IcebergExceptionMapper.java | 158 ++++++++++++++++++++-
.../iceberg/service/IcebergRESTUtils.java | 36 ++++-
.../IcebergRequestBodyReaderInterceptor.java | 52 +++++++
.../service/rest/IcebergNamespaceOperations.java | 6 +
.../service/rest/IcebergTableOperations.java | 4 +
.../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 | 95 +++++++++++++
.../TestIcebergRequestBodyAuthzHandlers.java | 120 ++++++++++++++++
21 files changed, 988 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 706b8f71e2..231bc58424 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,12 +18,15 @@
*/
package org.apache.gravitino.iceberg.common.ops;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import java.sql.Driver;
import java.sql.DriverManager;
+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;
@@ -41,16 +44,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;
@@ -86,6 +95,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;
@@ -214,7 +228,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());
+ }
}
/**
@@ -230,12 +252,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());
@@ -357,8 +393,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());
}
@@ -574,4 +617,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/build.gradle.kts
b/iceberg/iceberg-rest-server/build.gradle.kts
index f69bb0ea66..015b9c3b27 100644
--- a/iceberg/iceberg-rest-server/build.gradle.kts
+++ b/iceberg/iceberg-rest-server/build.gradle.kts
@@ -121,6 +121,8 @@ dependencies {
exclude("org.ow2.asm")
}
testImplementation(libs.asm)
+ // Compile-time access to the AWS SDK exception types; at runtime the
iceberg-aws-bundle supplies them.
+ testCompileOnly(libs.aws.s3)
testImplementation(libs.testcontainers.postgresql)
// Add Hadoop 3.3+ dependencies since Spark's Hadoop is excluded
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 3710708c57..bcc4995ac5 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
@@ -34,6 +34,7 @@ import
org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
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.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;
@@ -104,8 +105,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 d12e6e8019..de60c69dcc 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;
@@ -46,6 +50,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;
@@ -59,6 +64,7 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Throwable> {
private static final Map<Class<? extends Exception>, Integer>
EXCEPTION_ERROR_CODES =
ImmutableMap.<Class<? extends Exception>, Integer>builder()
.put(IllegalArgumentException.class, 400)
+ .put(BadRequestException.class, 400)
.put(ValidationException.class, 400)
.put(IllegalNameIdentifierException.class, 400)
.put(NamespaceNotEmptyException.class, 409)
@@ -96,9 +102,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();
@@ -147,6 +157,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)
@@ -162,4 +173,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 bedd609d37..7b2998caaa 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
@@ -52,9 +52,11 @@ import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerConte
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;
@@ -381,12 +383,44 @@ public class IcebergRESTUtils {
return Response.status(Status.NOT_FOUND).build();
}
+ /**
+ * 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 errorResponse =
ErrorResponse.builder()
.responseCode(httpStatus)
.withType(ex.getClass().getSimpleName())
- .withMessage(ex.getMessage())
+ .withMessage(message)
.withStackTrace(ex)
.build();
return Response.status(httpStatus)
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 9d33d5ddfd..405d26c484 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
@@ -247,6 +247,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: {}",
@@ -280,6 +281,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);
@@ -321,6 +324,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 =
@@ -391,6 +395,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 e6a9333692..9be8e9aceb 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
@@ -181,6 +181,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 =
@@ -223,6 +224,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);
@@ -416,6 +418,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);
@@ -547,6 +550,7 @@ public class IcebergTableOperations {
String namespace,
@Encoded() @PathParam("table") @AuthorizationMetadata(type =
EntityType.TABLE) String table,
PlanTableScanRequest scanRequest) {
+ IcebergRESTUtils.validateRequest(scanRequest, "PlanTableScanRequest");
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/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 e3e4f3225d..6253f57edc 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.ImmutableLoadCredentialsResponse;
@@ -449,4 +451,34 @@ public class TestIcebergRESTUtils {
"v1/irc1/namespaces/db/tables/tbl/credentials",
rewritten.credentials().get(0).config().get("client.refresh-credentials-endpoint"));
}
+
+ @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 8c572999b6..89ad43dfb7 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;
@@ -97,8 +98,11 @@ public class IcebergRestTestUtil {
Class c, boolean bindIcebergTableOps, List<EventListenerPlugin>
eventListenerPlugins) {
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 8e8f2efb16..58cd3a0d75 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
@@ -93,6 +93,7 @@ import org.apache.iceberg.rest.requests.PlanTableScanRequest;
import org.apache.iceberg.rest.requests.RenameTableRequest;
import org.apache.iceberg.rest.requests.ReportMetricsRequest;
import org.apache.iceberg.rest.requests.UpdateTableRequest;
+import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.ListTablesResponse;
import org.apache.iceberg.rest.responses.LoadTableResponse;
import org.apache.iceberg.types.Types.NestedField;
@@ -536,6 +537,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) {}
+ }
+}
