From eaa786a50f1026a9850925611c8fa79c26721a28 Mon Sep 17 00:00:00 2001 From: Dennis Huo Date: Thu, 3 Apr 2025 03:42:06 +0000 Subject: [PATCH 1/3] Improve code-containment and efficiency of etag-aware loading -Make the hash generation resilient against null metadataLocation -Use getResolvedPath instead of getPassthroughResolvedPath to avoid redundant persistence round-trip -Only try to calculate the etag for comparison against ifNoneMatch if the ifNoneMatch is actually provided --- .../iceberg/IcebergCatalogHandler.java | 31 ++++++++++--------- .../polaris/service/http/IcebergHttpUtil.java | 4 +++ 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 78fecf4797..5222e5641b 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -503,8 +503,7 @@ public boolean sendNotification(TableIdentifier identifier, NotificationRequest * @return the Polaris table entity for the table */ private IcebergTableLikeEntity getTableEntity(TableIdentifier tableIdentifier) { - PolarisResolvedPathWrapper target = - resolutionManifest.getPassthroughResolvedPath(tableIdentifier); + PolarisResolvedPathWrapper target = resolutionManifest.getResolvedPath(tableIdentifier); return IcebergTableLikeEntity.of(target.getRawLeafEntity()); } @@ -529,12 +528,14 @@ public Optional loadTableIfStale( authorizeBasicTableLikeOperationOrThrow( op, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier); - IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier); - String tableEntityTag = - IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); - - if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableEntityTag)) { - return Optional.empty(); + if (ifNoneMatch != null) { + // Perform freshness-aware table loading if caller specified ifNoneMatch. + IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier); + String tableEntityTag = + IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); + if (ifNoneMatch.anyMatch(tableEntityTag)) { + return Optional.empty(); + } } return Optional.of(CatalogHandlers.loadTable(baseCatalog, tableIdentifier)); @@ -601,12 +602,14 @@ public Optional loadTableWithAccessDelegationIfStale( FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig()); } - IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier); - String tableETag = - IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); - - if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableETag)) { - return Optional.empty(); + if (ifNoneMatch != null) { + // Perform freshness-aware table loading if caller specified ifNoneMatch. + IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier); + String tableETag = + IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); + if (ifNoneMatch.anyMatch(tableETag)) { + return Optional.empty(); + } } // TODO: Find a way for the configuration or caller to better express whether to fail or omit diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java b/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java index 88c056aa70..663e8d70ac 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java @@ -32,6 +32,10 @@ private IcebergHttpUtil() {} * @return the generated ETag */ public static String generateETagForMetadataFileLocation(String metadataFileLocation) { + if (metadataFileLocation == null) { + metadataFileLocation = ""; + } + // Use hash of metadata location since we don't want clients to use the ETag to try to extract // the metadata file location String hashedMetadataFileLocation = DigestUtils.sha256Hex(metadataFileLocation); From c70b2cc950cc0670f8b2f9c28062df903ef9fbad Mon Sep 17 00:00:00 2001 From: Dennis Huo Date: Thu, 3 Apr 2025 04:52:36 +0000 Subject: [PATCH 2/3] Add strict null-checking at callsites to generateETag, disallow passing null to generator --- .../iceberg/IcebergCatalogAdapter.java | 52 ++++++++++++------- .../iceberg/IcebergCatalogHandler.java | 32 +++++++++--- .../polaris/service/http/IcebergHttpUtil.java | 3 +- 3 files changed, 58 insertions(+), 29 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java index 6db06cb01a..ccb6b44f57 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java @@ -235,6 +235,31 @@ private static Namespace decodeNamespace(String namespace) { return RESTUtil.decodeNamespace(URLEncoder.encode(namespace, Charset.defaultCharset())); } + /** + * For situations where we typically expect a metadataLocation to be present in the response and + * so expect to insert an etag header, this helper gracefully falls back to omitting the header if + * unable to get metadata location and logs a warning. + */ + private Response.ResponseBuilder tryInsertETagHeader( + Response.ResponseBuilder builder, + LoadTableResponse response, + String namespace, + String tableName) { + if (response.metadataLocation() != null) { + builder = + builder.header( + HttpHeaders.ETAG, + IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation())); + } else { + LOGGER + .atWarn() + .addKeyValue("namespace", namespace) + .addKeyValue("tableName", tableName) + .log("Response has null metadataLocation; omitting etag"); + } + return builder; + } + @Override public Response namespaceExists( String prefix, String namespace, RealmContext realmContext, SecurityContext securityContext) { @@ -312,20 +337,14 @@ public Response createTable( } } else if (delegationModes.isEmpty()) { LoadTableResponse response = catalog.createTableDirect(ns, createTableRequest); - return Response.ok(response) - .header( - HttpHeaders.ETAG, - IcebergHttpUtil.generateETagForMetadataFileLocation( - response.metadataLocation())) + return tryInsertETagHeader( + Response.ok(response), response, namespace, createTableRequest.name()) .build(); } else { LoadTableResponse response = catalog.createTableDirectWithWriteDelegation(ns, createTableRequest); - return Response.ok(response) - .header( - HttpHeaders.ETAG, - IcebergHttpUtil.generateETagForMetadataFileLocation( - response.metadataLocation())) + return tryInsertETagHeader( + Response.ok(response), response, namespace, createTableRequest.name()) .build(); } }); @@ -384,11 +403,7 @@ public Response loadTable( .orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED)); } - return Response.ok(response) - .header( - HttpHeaders.ETAG, - IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation())) - .build(); + return tryInsertETagHeader(Response.ok(response), response, namespace, table).build(); }); } @@ -446,11 +461,8 @@ public Response registerTable( prefix, catalog -> { LoadTableResponse response = catalog.registerTable(ns, registerTableRequest); - - return Response.ok(response) - .header( - HttpHeaders.ETAG, - IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation())) + return tryInsertETagHeader( + Response.ok(response), response, namespace, registerTableRequest.name()) .build(); }); } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 5222e5641b..b282d1b7ce 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -531,10 +531,18 @@ public Optional loadTableIfStale( if (ifNoneMatch != null) { // Perform freshness-aware table loading if caller specified ifNoneMatch. IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier); - String tableEntityTag = - IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); - if (ifNoneMatch.anyMatch(tableEntityTag)) { - return Optional.empty(); + if (tableEntity == null || tableEntity.getMetadataLocation() == null) { + LOGGER + .atWarn() + .addKeyValue("tableIdentifier", tableIdentifier) + .addKeyValue("tableEntity", tableEntity) + .log("Failed to getMetadataLocation to generate ETag when loading table"); + } else { + String tableEntityTag = + IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); + if (ifNoneMatch.anyMatch(tableEntityTag)) { + return Optional.empty(); + } } } @@ -605,10 +613,18 @@ public Optional loadTableWithAccessDelegationIfStale( if (ifNoneMatch != null) { // Perform freshness-aware table loading if caller specified ifNoneMatch. IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier); - String tableETag = - IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); - if (ifNoneMatch.anyMatch(tableETag)) { - return Optional.empty(); + if (tableEntity == null || tableEntity.getMetadataLocation() == null) { + LOGGER + .atWarn() + .addKeyValue("tableIdentifier", tableIdentifier) + .addKeyValue("tableEntity", tableEntity) + .log("Failed to getMetadataLocation to generate ETag when loading table"); + } else { + String tableETag = + IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); + if (ifNoneMatch.anyMatch(tableETag)) { + return Optional.empty(); + } } } diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java b/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java index 663e8d70ac..2fd0445d93 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java @@ -33,7 +33,8 @@ private IcebergHttpUtil() {} */ public static String generateETagForMetadataFileLocation(String metadataFileLocation) { if (metadataFileLocation == null) { - metadataFileLocation = ""; + // Throw a more appropriate exception than letting DigestUtils die randomly. + throw new IllegalArgumentException("Unable to generate etag for null metadataFileLocation"); } // Use hash of metadata location since we don't want clients to use the ETag to try to extract From 52247837bdeea012dbdb01505a3b1ddc7030a5b8 Mon Sep 17 00:00:00 2001 From: Dennis Huo Date: Thu, 3 Apr 2025 05:03:17 +0000 Subject: [PATCH 3/3] Add TODO to refactor shared logic for etag generation --- .../service/catalog/iceberg/IcebergCatalogHandler.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index b282d1b7ce..b5e1a0edb9 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -538,6 +538,8 @@ public Optional loadTableIfStale( .addKeyValue("tableEntity", tableEntity) .log("Failed to getMetadataLocation to generate ETag when loading table"); } else { + // TODO: Refactor null-checking into the helper method once we create a more canonical + // interface for associate etags with entities. String tableEntityTag = IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); if (ifNoneMatch.anyMatch(tableEntityTag)) { @@ -620,6 +622,8 @@ public Optional loadTableWithAccessDelegationIfStale( .addKeyValue("tableEntity", tableEntity) .log("Failed to getMetadataLocation to generate ETag when loading table"); } else { + // TODO: Refactor null-checking into the helper method once we create a more canonical + // interface for associate etags with entities. String tableETag = IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation()); if (ifNoneMatch.anyMatch(tableETag)) {