This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-90da2776-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 8d10102cc079ac995cf5170b127f0b582d932509 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]> # Conflicts: # iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java # iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java # iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java # iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java # iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java # iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java --- .../iceberg/common/ops/IcebergCatalogWrapper.java | 152 +++++++++++++++++++- .../common/ops/TestIcebergCatalogWrapper.java | 129 +++++++++++++++++ .../org/apache/gravitino/iceberg/RESTService.java | 12 +- .../iceberg/service/IcebergExceptionMapper.java | 157 ++++++++++++++++++++- .../iceberg/service/IcebergRESTUtils.java | 57 ++++++++ .../IcebergRequestBodyReaderInterceptor.java | 52 +++++++ .../service/rest/IcebergNamespaceOperations.java | 46 ++++++ .../service/rest/IcebergTableOperations.java | 82 +++++++++++ .../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 | 55 ++++++++ .../TestIcebergRequestBodyReaderInterceptor.java | 70 +++++++++ .../iceberg/service/rest/IcebergRestTestUtil.java | 6 +- .../service/rest/TestIcebergTableOperations.java | 94 ++++++++++++ .../TestIcebergRequestBodyAuthzHandlers.java | 120 ++++++++++++++++ 20 files changed, 1154 insertions(+), 15 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..21a94042da 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,18 @@ */ package org.apache.gravitino.iceberg.common.ops; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +<<<<<<< HEAD import java.sql.Driver; import java.sql.DriverManager; +======= +import java.util.HashSet; +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) 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 +47,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 +98,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 +231,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 +255,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 +396,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 +620,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 3710708c57..25f53a5fb6 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,11 @@ 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; +<<<<<<< HEAD +======= +import org.apache.gravitino.iceberg.service.IcebergRESTUtils; +import org.apache.gravitino.iceberg.service.IcebergRequestBodyReaderInterceptor; +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) 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 +109,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..127831e8cc 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; @@ -96,9 +101,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 +156,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 +172,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..ba3ed73e78 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,6 +383,7 @@ public class IcebergRESTUtils { return Response.status(Status.NOT_FOUND).build(); } +<<<<<<< HEAD public static Response errorResponse(Throwable ex, int httpStatus) { ErrorResponse errorResponse = ErrorResponse.builder() @@ -389,6 +392,60 @@ public class IcebergRESTUtils { .withMessage(ex.getMessage()) .withStackTrace(ex) .build(); +======= + /** + * Sets whether error responses built by {@link #errorResponse(Throwable, int)} include the + * exception's stack trace. Called once when the service starts. + * + * @param include whether to include stack traces + */ + public static void setIncludeErrorStackTrace(boolean include) { + 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(message); + if (includeErrorStackTrace) { + builder.withStackTrace(ex); + } + ErrorResponse errorResponse = builder.build(); +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) return Response.status(httpStatus) .entity(errorResponse) .type(MediaType.APPLICATION_JSON) 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..470e9327dc 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 = @@ -349,6 +353,46 @@ public class IcebergNamespaceOperations { } } +<<<<<<< HEAD +======= + @POST + @Path("{namespace}/register-view") + @Produces(MediaType.APPLICATION_JSON) + @Timed(name = "register-view." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "register-view", absolute = true) + @AuthorizationExpression( + expression = AuthorizationExpressionConstants.ICEBERG_CREATE_VIEW_AUTHORIZATION_EXPRESSION, + accessMetadataType = MetadataObject.Type.SCHEMA) + public Response registerView( + @AuthorizationMetadata(type = Entity.EntityType.CATALOG) @PathParam("prefix") String prefix, + @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); + LOG.info( + "Register Iceberg view, catalog: {}, namespace: {}, registerViewRequest: {}", + catalogName, + icebergNS, + registerViewRequest); + try { + return Utils.doAs( + httpRequest, + () -> { + IcebergRequestContext context = + new IcebergRequestContext(httpServletRequest(), catalogName); + LoadViewResponse loadViewResponse = + namespaceOperationDispatcher.registerView(context, icebergNS, registerViewRequest); + return IcebergRESTUtils.ok(loadViewResponse); + }); + } catch (Exception e) { + return IcebergExceptionMapper.toRESTResponse(e); + } + } + +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) private NameIdentifier[] toNameIdentifiers( ListNamespacesResponse listNamespacesResponse, String metalake, String catalogName) { String separator = HierarchicalSchemaUtil.schemaSeparator(); @@ -391,6 +435,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..fc0de5e0cc 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); @@ -546,7 +549,17 @@ public class IcebergTableOperations { @Encoded() @PathParam("namespace") @AuthorizationMetadata(type = EntityType.SCHEMA) String namespace, @Encoded() @PathParam("table") @AuthorizationMetadata(type = EntityType.TABLE) String table, +<<<<<<< HEAD PlanTableScanRequest scanRequest) { +======= + PlanTableScanRequest scanRequest, + @HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation) { + IcebergRESTUtils.validateRequest(scanRequest, "PlanTableScanRequest"); + if (scanRequest == null) { + return missingRequestBody("plan table scan"); + } + boolean isCredentialVending = isCredentialVending(accessDelegation); +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) String catalogName = IcebergRESTUtils.getCatalogName(prefix); Namespace icebergNS = RESTUtil.decodeNamespace(namespace, IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8); @@ -555,7 +568,76 @@ public class IcebergTableOperations { "Plan table scan, catalog: {}, namespace: {}, table: {}", catalogName, icebergNS, +<<<<<<< HEAD tableName); +======= + tableName, + accessDelegation, + isCredentialVending); + + try { + return Utils.doAs( + httpRequest, + () -> { + TableIdentifier tableIdentifier = TableIdentifier.of(icebergNS, tableName); + IcebergRequestContext context = + new IcebergRequestContext(httpServletRequest(), catalogName, isCredentialVending); + + PlanTableScanResponse scanResponse = + tableOperationDispatcher.planTableScan(context, tableIdentifier, scanRequest); + + return IcebergRESTUtils.ok(scanResponse); + }); + } catch (Exception e) { + LOG.error("Failed to plan table scan: {}", e.getMessage(), e); + return IcebergExceptionMapper.toRESTResponse(e); + } + } + + /** + * Fetch scan tasks endpoint. Completes the second step of the Iceberg REST two-step scan planning + * protocol: a client exchanges a {@code plan-task} handed out by {@code POST + * .../tables/{table}/plan} for the scan tasks it covers. + * + * @param prefix The catalog prefix + * @param namespace The namespace + * @param table The table name + * @param request The request containing the {@code plan-task} + * @return Response containing the scan tasks for the given plan task + */ + @POST + @Path("{table}/tasks") + @Produces(MediaType.APPLICATION_JSON) + @Consumes(MediaType.APPLICATION_JSON) + @Timed(name = "fetch-scan-tasks." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "fetch-scan-tasks", absolute = true) + @AuthorizationExpression( + expression = + "ANY(OWNER, METALAKE, CATALOG) || " + + "SCHEMA_OWNER_WITH_USE_CATALOG || " + + "ANY_USE_CATALOG && ANY_USE_SCHEMA && (TABLE::OWNER || ANY_SELECT_TABLE || ANY_MODIFY_TABLE)", + accessMetadataType = MetadataObject.Type.TABLE) + public Response fetchScanTasks( + @PathParam("prefix") @AuthorizationMetadata(type = EntityType.CATALOG) String prefix, + @Encoded() @PathParam("namespace") @AuthorizationMetadata(type = EntityType.SCHEMA) + 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"); + } + String catalogName = IcebergRESTUtils.getCatalogName(prefix); + Namespace icebergNS = + RESTUtil.decodeNamespace(namespace, IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8); + String tableName = RESTUtil.decodeString(table); + LOG.info( + "Fetch scan tasks, catalog: {}, namespace: {}, table: {}, planTask: {}", + catalogName, + icebergNS, + tableName, + request.planTask()); +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) try { return Utils.doAs( 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..f24dbbace1 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,57 @@ public class TestIcebergRESTUtils { "v1/irc1/namespaces/db/tables/tbl/credentials", rewritten.credentials().get(0).config().get("client.refresh-credentials-endpoint")); } +<<<<<<< HEAD +======= + + @Test + void testErrorResponseStackTraceFollowsSetting() throws Exception { + RuntimeException failure = new RuntimeException("failure"); + try { + ErrorResponse withStack = + (ErrorResponse) IcebergRESTUtils.errorResponse(failure, 500).getEntity(); + Assertions.assertTrue( + IcebergObjectMapper.getInstance().writeValueAsString(withStack).contains("\"stack\"")); + + IcebergRESTUtils.setIncludeErrorStackTrace(false); + ErrorResponse withoutStack = + (ErrorResponse) IcebergRESTUtils.errorResponse(failure, 500).getEntity(); + String json = IcebergObjectMapper.getInstance().writeValueAsString(withoutStack); + Assertions.assertFalse(json.contains("\"stack\""), json); + Assertions.assertEquals("failure", withoutStack.message()); + } finally { + 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")); + } +>>>>>>> 90da2776a ([#14432] fix(iceberg-rest): answer invalid requests with 400 and an ErrorModel (#14433)) } 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..f0cf5ee9fc 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 @@ -536,6 +536,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) {} + } +}
