From 39e0ea482b4dab4c16b7c2a8e61318df8defa531 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Tue, 18 Feb 2025 14:53:02 -0800 Subject: [PATCH 01/10] Pulled in iceberg 1.8.0 spec changes for freshness aware table loading and added feature to Polaris --- .../catalog/IcebergCatalogAdapter.java | 22 +++++++-- .../catalog/PolarisCatalogHandlerWrapper.java | 46 ++++++++++++++++--- spec/rest-catalog-open-api.yaml | 27 +++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index ce8b3264b..dbd3e8691 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -26,6 +26,8 @@ import com.google.common.collect.ImmutableSet; import jakarta.enterprise.context.RequestScoped; import jakarta.inject.Inject; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; import jakarta.ws.rs.core.SecurityContext; @@ -54,6 +56,7 @@ import org.apache.iceberg.rest.requests.ReportMetricsRequest; import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest; import org.apache.iceberg.rest.responses.ConfigResponse; +import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.polaris.core.PolarisConfigurationStore; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; @@ -308,9 +311,11 @@ public Response createTable( .build(); } } else if (delegationModes.isEmpty()) { - return Response.ok(catalog.createTableDirect(ns, createTableRequest)).build(); + LoadTableResponse loadTableResponse = catalog.createTableDirect(ns, createTableRequest); + return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()).build(); } else { - return Response.ok(catalog.createTableDirectWithWriteDelegation(ns, createTableRequest)) + LoadTableResponse loadTableResponse = catalog.createTableDirectWithWriteDelegation(ns, createTableRequest); + return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()) .build(); } }); @@ -335,6 +340,7 @@ public Response loadTable( String namespace, String table, String accessDelegationMode, + String etag, String snapshots, RealmContext realmContext, SecurityContext securityContext) { @@ -347,9 +353,12 @@ public Response loadTable( prefix, catalog -> { if (delegationModes.isEmpty()) { - return Response.ok(catalog.loadTable(tableIdentifier, snapshots)).build(); + Optional optionalLoadTableResponse = catalog.loadTableFreshnessAware(tableIdentifier, etag, snapshots); + LoadTableResponse loadTableResponse = optionalLoadTableResponse.orElseThrow(() -> new WebApplicationException(Status.NOT_MODIFIED)); + return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()).build(); } else { - return Response.ok(catalog.loadTableWithAccessDelegation(tableIdentifier, snapshots)) + LoadTableResponse loadTableResponse = catalog.loadTableWithAccessDelegation(tableIdentifier, snapshots); + return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()) .build(); } }); @@ -407,7 +416,10 @@ public Response registerTable( return withCatalog( securityContext, prefix, - catalog -> Response.ok(catalog.registerTable(ns, registerTableRequest)).build()); + catalog -> { + LoadTableResponse loadTableResponse = catalog.registerTable(ns, registerTableRequest); + return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()).build(); + }); } @Override diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 95c8c4856..f54addaf2 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -81,6 +81,7 @@ import org.apache.polaris.core.entity.CatalogEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.entity.TableLikeEntity; import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisMetaStoreSession; @@ -786,15 +787,44 @@ public boolean sendNotification(TableIdentifier identifier, NotificationRequest && notificationCatalog.sendNotification(identifier, request); } + private boolean etagMatchesCurrentMetadataLocation(String etag, TableIdentifier tableIdentifier) { + if (etag != null) { + PolarisResolvedPathWrapper target = resolutionManifest + .getResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE, true); + + String currentEntityMetadataVersion = target + .getRawLeafEntity() + .getInternalPropertiesAsMap() + .get(TableLikeEntity.METADATA_LOCATION_KEY); + + if (etag.equals(currentEntityMetadataVersion)) { + return true; + } + } + return false; + } + public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snapshots) { - PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LOAD_TABLE; - authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); + return loadTableFreshnessAware(tableIdentifier, null, snapshots).get(); + } - return CatalogHandlers.loadTable(baseCatalog, tableIdentifier); + public Optional loadTableFreshnessAware(TableIdentifier tableIdentifier, String etag, String snapshots) { + PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LOAD_TABLE; + authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); + + if (etagMatchesCurrentMetadataLocation(etag, tableIdentifier)) { + return Optional.empty(); + } + + return Optional.of(CatalogHandlers.loadTable(baseCatalog, tableIdentifier)); + } + + public LoadTableResponse loadTableWithAccessDelegation(TableIdentifier tableIdentifier, String snapshots) { + return loadTableWithAccessDelegationFreshnessAware(tableIdentifier, null, snapshots).get(); } - public LoadTableResponse loadTableWithAccessDelegation( - TableIdentifier tableIdentifier, String snapshots) { + public Optional loadTableWithAccessDelegationFreshnessAware( + TableIdentifier tableIdentifier, String etag, String snapshots) { // Here we have a single method that falls through multiple candidate // PolarisAuthorizableOperations because instead of identifying the desired operation up-front // and @@ -832,6 +862,10 @@ public LoadTableResponse loadTableWithAccessDelegation( PolarisConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig()); } + if (etagMatchesCurrentMetadataLocation(etag, tableIdentifier)) { + return Optional.empty(); + } + // TODO: Find a way for the configuration or caller to better express whether to fail or omit // when data-access is specified but access delegation grants are not found. Table table = baseCatalog.loadTable(tableIdentifier); @@ -850,7 +884,7 @@ public LoadTableResponse loadTableWithAccessDelegation( credentialDelegation.getCredentialConfig( tableIdentifier, tableMetadata, actionsRequested)); } - return responseBuilder.build(); + return Optional.of(responseBuilder.build()); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); diff --git a/spec/rest-catalog-open-api.yaml b/spec/rest-catalog-open-api.yaml index cfcd4987d..487c44652 100644 --- a/spec/rest-catalog-open-api.yaml +++ b/spec/rest-catalog-open-api.yaml @@ -683,6 +683,15 @@ paths: key. For example, "urn:ietf:params:oauth:token-type:jwt=". parameters: - $ref: '#/components/parameters/data-access' + - name: If-None-Match + in: header + description: + An optional header that allows the server to return 304 (Not Modified) if the metadata + is current. The content is the value of the ETag received in a CreateTableResponse or + LoadTableResponse. + required: false + schema: + type: string - in: query name: snapshots description: @@ -698,6 +707,10 @@ paths: responses: 200: $ref: '#/components/responses/LoadTableResponse' + 304: + description: + Not Modified - Based on the content of the 'If-None-Match' header the table metadata has + not changed since. 400: $ref: '#/components/responses/BadRequestErrorResponse' 401: @@ -1688,6 +1701,14 @@ components: type: integer minimum: 1 + etag: + name: ETag + in: header + description: Identifies a unique version of the table metadata. + required: false + schema: + type: string + ############################## # Application Schema Objects # ############################## @@ -4193,6 +4214,9 @@ components: application/json: schema: $ref: '#/components/schemas/LoadTableResult' + headers: + etag: + $ref: '#/components/parameters/etag' LoadTableResponse: description: Table metadata result when loading a table @@ -4200,6 +4224,9 @@ components: application/json: schema: $ref: '#/components/schemas/LoadTableResult' + headers: + etag: + $ref: '#/components/parameters/etag' LoadViewResponse: description: View metadata result when loading a view From 0ef4de8b9a0627a61efc5549641f74d7ce993703 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Tue, 25 Feb 2025 17:18:30 -0800 Subject: [PATCH 02/10] Changed etag support to use entityId:version tuple --- .../PolarisRestCatalogIntegrationTest.java | 127 ++++++++++++++++++ .../polaris/core/entity/ETaggableEntity.java | 40 ++++++ .../polaris/core/entity/TableLikeEntity.java | 14 +- ...PolarisCatalogHandlerWrapperAuthzTest.java | 81 +++++++++++ .../catalog/IcebergCatalogAdapter.java | 31 ++--- .../catalog/PolarisCatalogHandlerWrapper.java | 97 ++++++++----- .../catalog/response/ETaggedResponse.java | 46 +++++++ 7 files changed, 390 insertions(+), 46 deletions(-) create mode 100644 polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java create mode 100644 service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index bc774c1cd..9df1622a7 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableMap; import jakarta.ws.rs.client.Entity; import jakarta.ws.rs.client.Invocation; +import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.Response; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -54,6 +55,7 @@ import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.io.ResolvingFileIO; import org.apache.iceberg.rest.RESTCatalog; +import org.apache.iceberg.rest.requests.CreateTableRequest; import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.iceberg.types.Types; import org.apache.polaris.core.PolarisConfiguration; @@ -626,6 +628,131 @@ public void testLoadTableWithAccessDelegationForExternalCatalogWithConfigEnabled } } + /** + * Register a table. Then, invoke an initial loadTable request to fetch and ensure ETag is present. + * Then, invoke a second loadTable to ensure that ETag is matched. + */ + @Test + public void testLoadTableTwiceWithETag() { + Namespace ns1 = Namespace.of("ns1"); + restCatalog.createNamespace(ns1); + TableMetadata tableMetadata = + TableMetadata.newTableMetadata( + new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))), + PartitionSpec.unpartitioned(), + "file:///tmp/ns1/my_table", + Map.of()); + try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) { + resolvingFileIO.initialize(Map.of()); + resolvingFileIO.setConf(new Configuration()); + String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json"; + TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation)); + restCatalog.registerTable(TableIdentifier.of(ns1, "my_table_etagged"), fileLocation); + Invocation invocation = catalogApi.request("v1/" + currentCatalogName + "/namespaces/ns1/tables/my_table_etagged").build("GET"); + try (Response initialLoadTable = invocation.invoke()) { + assertThat(initialLoadTable.getHeaders()).containsKey(HttpHeaders.ETAG); + String etag = initialLoadTable.getHeaders().getFirst(HttpHeaders.ETAG).toString(); + + Invocation etaggedInvocation = catalogApi + .request("v1/" + currentCatalogName + "/namespaces/ns1/tables/my_table_etagged") + .header(HttpHeaders.IF_NONE_MATCH, etag) + .build("GET"); + + try (Response etaggedLoadTable = etaggedInvocation.invoke()) { + assertThat(etaggedLoadTable.getStatus()).isEqualTo(Response.Status.NOT_MODIFIED.getStatusCode()); + } + } finally { + resolvingFileIO.deleteFile(fileLocation); + } + } + } + + /** + * Invoke an initial registerTable request to fetch and ensure ETag is present. + * Then, invoke a second loadTable to ensure that ETag is matched. + */ + @Test + public void testRegisterAndLoadTableWithReturnedETag() { + Namespace ns1 = Namespace.of("ns1"); + restCatalog.createNamespace(ns1); + TableMetadata tableMetadata = + TableMetadata.newTableMetadata( + new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))), + PartitionSpec.unpartitioned(), + "file:///tmp/ns1/my_table", + Map.of()); + try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) { + resolvingFileIO.initialize(Map.of()); + resolvingFileIO.setConf(new Configuration()); + String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json"; + TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation)); + + Invocation registerInvocation = catalogApi + .request("v1/" + currentCatalogName + "/namespaces/ns1/register") + .buildPost(Entity.json(Map.of("name", "my_etagged_table", "metadata-location", fileLocation))); + try (Response registerResponse = registerInvocation.invoke()) { + assertThat(registerResponse.getHeaders()).containsKey(HttpHeaders.ETAG); + String etag = registerResponse.getHeaders().getFirst(HttpHeaders.ETAG).toString(); + + Invocation etaggedInvocation = catalogApi + .request("v1/" + currentCatalogName + "/namespaces/ns1/tables/my_etagged_table") + .header(HttpHeaders.IF_NONE_MATCH, etag) + .build("GET"); + + try (Response etaggedLoadTable = etaggedInvocation.invoke()) { + assertThat(etaggedLoadTable.getStatus()).isEqualTo(Response.Status.NOT_MODIFIED.getStatusCode()); + } + + } finally { + resolvingFileIO.deleteFile(fileLocation); + } + } + } + + @Test + public void testCreateAndLoadTableWithReturnedEtag() { + Namespace ns1 = Namespace.of("ns1"); + restCatalog.createNamespace(ns1); + TableMetadata tableMetadata = + TableMetadata.newTableMetadata( + new Schema(List.of(Types.NestedField.of(1, false, "col1", new Types.StringType()))), + PartitionSpec.unpartitioned(), + "file:///tmp/ns1/my_table", + Map.of()); + try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) { + resolvingFileIO.initialize(Map.of()); + resolvingFileIO.setConf(new Configuration()); + String fileLocation = "file:///tmp/ns1/my_table/metadata/v1.metadata.json"; + TableMetadataParser.write(tableMetadata, resolvingFileIO.newOutputFile(fileLocation)); + + Invocation createInvocation = catalogApi + .request("v1/" + currentCatalogName + "/namespaces/ns1/tables") + .buildPost(Entity.json(CreateTableRequest.builder() + .withName("my_etagged_table") + .withLocation(tableMetadata.location()) + .withPartitionSpec(tableMetadata.spec()) + .withSchema(tableMetadata.schema()) + .withWriteOrder(tableMetadata.sortOrder()) + .build() + )); + try (Response createResponse = createInvocation.invoke()) { + assertThat(createResponse.getHeaders()).containsKey(HttpHeaders.ETAG); + String etag = createResponse.getHeaders().getFirst(HttpHeaders.ETAG).toString(); + + Invocation etaggedInvocation = catalogApi + .request("v1/" + currentCatalogName + "/namespaces/ns1/tables/my_etagged_table") + .header(HttpHeaders.IF_NONE_MATCH, etag) + .build("GET"); + + try (Response etaggedLoadTable = etaggedInvocation.invoke()) { + assertThat(etaggedLoadTable.getStatus()).isEqualTo(Response.Status.NOT_MODIFIED.getStatusCode()); + } + } finally { + resolvingFileIO.deleteFile(fileLocation); + } + } + } + @Test public void testSendNotificationInternalCatalog() { Map payload = diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java new file mode 100644 index 000000000..8b3f7599b --- /dev/null +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java @@ -0,0 +1,40 @@ +/* + * 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.polaris.core.entity; + +/** + * Entities that can expose an ETag that can uniquely identify their current state. + */ +public interface ETaggableEntity { + + /** + * Obtain an ETag that uniquely identifies the current state of the entity. + * @return the ETag + */ + String getETag(); + + /** + * Determines if the provided etag identifies the current version of the entity. + * @param etag The etag to compare the entity against + * @return true if the etag identifies the current state of the entity, false otherwise + */ + boolean isCurrent(String etag); + +} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java index 968598b93..c29b391c3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java @@ -24,7 +24,7 @@ import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.rest.RESTUtil; -public class TableLikeEntity extends PolarisEntity { +public class TableLikeEntity extends PolarisEntity implements ETaggableEntity { // For applicable types, this key on the "internalProperties" map will return the location // of the internalProperties JSON file. public static final String METADATA_LOCATION_KEY = "metadata-location"; @@ -79,6 +79,18 @@ public String getBaseLocation() { return getPropertiesAsMap().get(PolarisEntityConstants.ENTITY_BASE_LOCATION); } + @Override + @JsonIgnore + public String getETag() { + return id + ":" + entityVersion; + } + + @Override + @JsonIgnore + public boolean isCurrent(String etag) { + return getETag().equals(etag); + } + public static class Builder extends PolarisEntity.BaseBuilder { public Builder(TableIdentifier identifier, String metadataLocation) { super(); diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java index 6ba5cef12..57c22cfa8 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java @@ -875,6 +875,32 @@ public void testLoadTableInsufficientPermissions() { () -> newWrapper().loadTable(TABLE_NS1A_2, "all")); } + @Test + public void testLoadTableIfStaleSufficientPrivileges() { + doTestSufficientPrivileges( + List.of( + PolarisPrivilege.TABLE_READ_PROPERTIES, + PolarisPrivilege.TABLE_WRITE_PROPERTIES, + PolarisPrivilege.TABLE_READ_DATA, + PolarisPrivilege.TABLE_WRITE_DATA, + PolarisPrivilege.TABLE_FULL_METADATA, + PolarisPrivilege.CATALOG_MANAGE_CONTENT), + () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, "0:0", "all"), + null /* cleanupAction */); + } + + @Test + public void testLoadTableIfStaleInsufficientPermissions() { + doTestInsufficientPrivileges( + List.of( + PolarisPrivilege.NAMESPACE_FULL_METADATA, + PolarisPrivilege.VIEW_FULL_METADATA, + PolarisPrivilege.TABLE_CREATE, + PolarisPrivilege.TABLE_LIST, + PolarisPrivilege.TABLE_DROP), + () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, "0:0", "all")); + } + @Test public void testLoadTableWithReadAccessDelegationSufficientPrivileges() { doTestSufficientPrivileges( @@ -930,6 +956,61 @@ public void testLoadTableWithWriteAccessDelegationInsufficientPermissions() { () -> newWrapper().loadTableWithAccessDelegation(TABLE_NS1A_2, "all")); } + @Test + public void testLoadTableWithReadAccessDelegationIfStaleSufficientPrivileges() { + doTestSufficientPrivileges( + List.of( + PolarisPrivilege.TABLE_READ_DATA, + PolarisPrivilege.TABLE_WRITE_DATA, + PolarisPrivilege.CATALOG_MANAGE_CONTENT), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all"), + null /* cleanupAction */); + } + + @Test + public void testLoadTableWithReadAccessDelegationIfStaleInsufficientPermissions() { + doTestInsufficientPrivileges( + List.of( + PolarisPrivilege.NAMESPACE_FULL_METADATA, + PolarisPrivilege.VIEW_FULL_METADATA, + PolarisPrivilege.TABLE_FULL_METADATA, + PolarisPrivilege.TABLE_READ_PROPERTIES, + PolarisPrivilege.TABLE_WRITE_PROPERTIES, + PolarisPrivilege.TABLE_CREATE, + PolarisPrivilege.TABLE_LIST, + PolarisPrivilege.TABLE_DROP), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all")); + } + + @Test + public void testLoadTableWithWriteAccessDelegationIfStaleSufficientPrivileges() { + doTestSufficientPrivileges( + List.of( + // TODO: Once we give different creds for read/write privilege, move this + // TABLE_READ_DATA into a special-case test; with only TABLE_READ_DATA we'd expet + // to receive a read-only credential. + PolarisPrivilege.TABLE_READ_DATA, + PolarisPrivilege.TABLE_WRITE_DATA, + PolarisPrivilege.CATALOG_MANAGE_CONTENT), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all"), + null /* cleanupAction */); + } + + @Test + public void testLoadTableWithWriteAccessDelegationIfStaleInsufficientPermissions() { + doTestInsufficientPrivileges( + List.of( + PolarisPrivilege.NAMESPACE_FULL_METADATA, + PolarisPrivilege.VIEW_FULL_METADATA, + PolarisPrivilege.TABLE_FULL_METADATA, + PolarisPrivilege.TABLE_READ_PROPERTIES, + PolarisPrivilege.TABLE_WRITE_PROPERTIES, + PolarisPrivilege.TABLE_CREATE, + PolarisPrivilege.TABLE_LIST, + PolarisPrivilege.TABLE_DROP), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all")); + } + @Test public void testUpdateTableSufficientPrivileges() { doTestSufficientPrivileges( diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index dbd3e8691..100663f8e 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -71,6 +71,7 @@ import org.apache.polaris.core.persistence.resolver.ResolverStatus; import org.apache.polaris.service.catalog.api.IcebergRestCatalogApiService; import org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService; +import org.apache.polaris.service.catalog.response.ETaggedResponse; import org.apache.polaris.service.context.CallContextCatalogFactory; import org.apache.polaris.service.types.CommitTableRequest; import org.apache.polaris.service.types.CommitViewRequest; @@ -311,11 +312,11 @@ public Response createTable( .build(); } } else if (delegationModes.isEmpty()) { - LoadTableResponse loadTableResponse = catalog.createTableDirect(ns, createTableRequest); - return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()).build(); + ETaggedResponse createTableResult = catalog.createTableDirect(ns, createTableRequest); + return Response.ok(createTableResult.getResponse()).header(HttpHeaders.ETAG, createTableResult.getETag()).build(); } else { - LoadTableResponse loadTableResponse = catalog.createTableDirectWithWriteDelegation(ns, createTableRequest); - return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()) + ETaggedResponse createTableResult = catalog.createTableDirectWithWriteDelegation(ns, createTableRequest); + return Response.ok(createTableResult.getResponse()).header(HttpHeaders.ETAG, createTableResult.getETag()) .build(); } }); @@ -352,15 +353,15 @@ public Response loadTable( securityContext, prefix, catalog -> { - if (delegationModes.isEmpty()) { - Optional optionalLoadTableResponse = catalog.loadTableFreshnessAware(tableIdentifier, etag, snapshots); - LoadTableResponse loadTableResponse = optionalLoadTableResponse.orElseThrow(() -> new WebApplicationException(Status.NOT_MODIFIED)); - return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()).build(); - } else { - LoadTableResponse loadTableResponse = catalog.loadTableWithAccessDelegation(tableIdentifier, snapshots); - return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()) - .build(); - } + ETaggedResponse loadTableResult; + if (delegationModes.isEmpty()) { + loadTableResult = catalog.loadTableIfStale(tableIdentifier, etag, snapshots) + .orElseThrow(() -> new WebApplicationException(Status.NOT_MODIFIED)); + } else { + loadTableResult = catalog.loadTableWithAccessDelegationIfStale(tableIdentifier, etag, snapshots) + .orElseThrow(() -> new WebApplicationException(Status.NOT_MODIFIED)); + } + return Response.ok(loadTableResult.getResponse()).header(HttpHeaders.ETAG, loadTableResult.getETag()).build(); }); } @@ -417,8 +418,8 @@ public Response registerTable( securityContext, prefix, catalog -> { - LoadTableResponse loadTableResponse = catalog.registerTable(ns, registerTableRequest); - return Response.ok(loadTableResponse).header(HttpHeaders.ETAG, loadTableResponse.metadataLocation()).build(); + ETaggedResponse registerTableResult = catalog.registerTable(ns, registerTableRequest); + return Response.ok(registerTableResult.getResponse()).header(HttpHeaders.ETAG, registerTableResult.getETag()).build(); }); } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index f54addaf2..a87bcb99d 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -91,6 +91,7 @@ import org.apache.polaris.core.persistence.resolver.ResolverPath; import org.apache.polaris.core.persistence.resolver.ResolverStatus; import org.apache.polaris.core.storage.PolarisStorageActions; +import org.apache.polaris.service.catalog.response.ETaggedResponse; import org.apache.polaris.service.context.CallContextCatalogFactory; import org.apache.polaris.service.types.NotificationRequest; import org.slf4j.Logger; @@ -563,10 +564,17 @@ public ListTablesResponse listTables(Namespace namespace) { return CatalogHandlers.listTables(baseCatalog, namespace); } - public LoadTableResponse createTableDirect(Namespace namespace, CreateTableRequest request) { + /** + * Create a table. + * @param namespace the namespace to create the table in + * @param request the table creation request + * @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata + */ + public ETaggedResponse createTableDirect(Namespace namespace, CreateTableRequest request) { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_TABLE_DIRECT; + TableIdentifier identifier = TableIdentifier.of(namespace, request.name()); authorizeCreateTableLikeUnderNamespaceOperationOrThrow( - op, TableIdentifier.of(namespace, request.name())); + op, identifier); CatalogEntity catalog = CatalogEntity.of( @@ -577,10 +585,17 @@ public LoadTableResponse createTableDirect(Namespace namespace, CreateTableReque if (isExternal(catalog)) { throw new BadRequestException("Cannot create table on external catalogs."); } - return CatalogHandlers.createTable(baseCatalog, namespace, request); + + return new ETaggedResponse<>(CatalogHandlers.createTable(baseCatalog, namespace, request), getTableEntity(identifier).getETag()); } - public LoadTableResponse createTableDirectWithWriteDelegation( + /** + * Create a table. + * @param namespace the namespace to create the table in + * @param request the table creation request + * @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata + */ + public ETaggedResponse createTableDirectWithWriteDelegation( Namespace namespace, CreateTableRequest request) { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_TABLE_DIRECT_WITH_WRITE_DELEGATION; @@ -635,7 +650,7 @@ public LoadTableResponse createTableDirectWithWriteDelegation( PolarisStorageActions.WRITE, PolarisStorageActions.LIST))); } - return responseBuilder.build(); + return new ETaggedResponse<>(responseBuilder.build(), getTableEntity(tableIdentifier).getETag()); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -741,12 +756,21 @@ public LoadTableResponse createTableStagedWithWriteDelegation( return responseBuilder.build(); } - public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) { + /** + * Register a table. + * @param namespace The namespace to register the table in + * @param request the register table request + * @return ETagged {@link LoadTableResponse} to uniquely identify the table metadata + */ + public ETaggedResponse registerTable(Namespace namespace, RegisterTableRequest request) { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.REGISTER_TABLE; + TableIdentifier identifier = TableIdentifier.of(namespace, request.name()); + authorizeCreateTableLikeUnderNamespaceOperationOrThrow( - op, TableIdentifier.of(namespace, request.name())); + op, identifier); - return CatalogHandlers.registerTable(baseCatalog, namespace, request); + LoadTableResponse registerTableResponse = CatalogHandlers.registerTable(baseCatalog, namespace, request); + return new ETaggedResponse<>(registerTableResponse, getTableEntity(identifier).getETag()); } public boolean sendNotification(TableIdentifier identifier, NotificationRequest request) { @@ -787,43 +811,54 @@ public boolean sendNotification(TableIdentifier identifier, NotificationRequest && notificationCatalog.sendNotification(identifier, request); } - private boolean etagMatchesCurrentMetadataLocation(String etag, TableIdentifier tableIdentifier) { - if (etag != null) { - PolarisResolvedPathWrapper target = resolutionManifest - .getResolvedPath(tableIdentifier, PolarisEntitySubType.TABLE, true); - - String currentEntityMetadataVersion = target - .getRawLeafEntity() - .getInternalPropertiesAsMap() - .get(TableLikeEntity.METADATA_LOCATION_KEY); + /** + * Fetch the metastore table entity for the given table identifier + * @param tableIdentifier The identifier of the table + * @return the Polaris table entity for the table + */ + private TableLikeEntity getTableEntity(TableIdentifier tableIdentifier) { + PolarisResolvedPathWrapper target = resolutionManifest.getPassthroughResolvedPath(tableIdentifier); - if (etag.equals(currentEntityMetadataVersion)) { - return true; - } - } - return false; + return TableLikeEntity.of(target.getRawLeafEntity()); } public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snapshots) { - return loadTableFreshnessAware(tableIdentifier, null, snapshots).get(); + return loadTableIfStale(tableIdentifier, null, snapshots).get().getResponse(); } - public Optional loadTableFreshnessAware(TableIdentifier tableIdentifier, String etag, String snapshots) { + /** + * Attempt to perform a loadTable operation only when the specified etag does not match the current state of the table. + * @param tableIdentifier The identifier of the table to load + * @param etag The ETag which identifies the metadata currently known + * @param snapshots + * @return {@link Optional#empty()} if the ETag is current, an {@link Optional} containing the load table response, otherwise + */ + public Optional> loadTableIfStale(TableIdentifier tableIdentifier, String etag, String snapshots) { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LOAD_TABLE; authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); - if (etagMatchesCurrentMetadataLocation(etag, tableIdentifier)) { + TableLikeEntity tableEntity = getTableEntity(tableIdentifier); + + if (tableEntity.isCurrent(etag)) { return Optional.empty(); } - return Optional.of(CatalogHandlers.loadTable(baseCatalog, tableIdentifier)); + return Optional.of(new ETaggedResponse<>( + CatalogHandlers.loadTable(baseCatalog, tableIdentifier), tableEntity.getETag())); } public LoadTableResponse loadTableWithAccessDelegation(TableIdentifier tableIdentifier, String snapshots) { - return loadTableWithAccessDelegationFreshnessAware(tableIdentifier, null, snapshots).get(); + return loadTableWithAccessDelegationIfStale(tableIdentifier, null, snapshots).get().getResponse(); } - public Optional loadTableWithAccessDelegationFreshnessAware( + /** + * Attempt to perform a loadTable operation with access delegation only when the specified etag does not match the current state of the table. + * @param tableIdentifier The identifier of the table to load + * @param etag The ETag which identifies the metadata currently known + * @param snapshots + * @return {@link Optional#empty()} if the ETag is current, an {@link Optional} containing the load table response, otherwise + */ + public Optional> loadTableWithAccessDelegationIfStale( TableIdentifier tableIdentifier, String etag, String snapshots) { // Here we have a single method that falls through multiple candidate // PolarisAuthorizableOperations because instead of identifying the desired operation up-front @@ -862,7 +897,9 @@ public Optional loadTableWithAccessDelegationFreshnessAware( PolarisConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig()); } - if (etagMatchesCurrentMetadataLocation(etag, tableIdentifier)) { + TableLikeEntity tableEntity = getTableEntity(tableIdentifier); + + if (tableEntity.isCurrent(etag)) { return Optional.empty(); } @@ -884,7 +921,7 @@ public Optional loadTableWithAccessDelegationFreshnessAware( credentialDelegation.getCredentialConfig( tableIdentifier, tableMetadata, actionsRequested)); } - return Optional.of(responseBuilder.build()); + return Optional.of(new ETaggedResponse<>(responseBuilder.build(), tableEntity.getETag())); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java new file mode 100644 index 000000000..1bf727c90 --- /dev/null +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java @@ -0,0 +1,46 @@ +/* + * 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.polaris.service.catalog.response; + +/** + * Allows Iceberg response types to be wrapped alongside an ETag that + * can be unwrapped into the HTTP ETag header. + * @param The type of the encapsulated response object + */ +public class ETaggedResponse { + + private final T response; + + private final String etag; + + public ETaggedResponse(T response, String etag) { + this.response = response; + this.etag = etag; + } + + public T getResponse() { + return response; + } + + public String getETag() { + return etag; + } + +} From 44b1859e6eb842facabf29409451c8d61796c946 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Tue, 25 Feb 2025 23:25:43 -0800 Subject: [PATCH 03/10] fixed getresponse call --- .../polaris/service/catalog/IcebergCatalogAdapter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index 4039d68e5..21da6af66 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -306,12 +306,12 @@ public Response createTable( } else if (delegationModes.isEmpty()) { ETaggedResponse createResult = newHandlerWrapper(realmContext, securityContext, prefix) .createTableDirect(ns, createTableRequest); - return Response.ok(createResult).header(HttpHeaders.ETAG, createResult.getETag()) + return Response.ok(createResult.getResponse()).header(HttpHeaders.ETAG, createResult.getETag()) .build(); } else { ETaggedResponse createResult = newHandlerWrapper(realmContext, securityContext, prefix) .createTableDirectWithWriteDelegation(ns, createTableRequest); - return Response.ok(createResult).header(HttpHeaders.ETAG, createResult.getETag()) + return Response.ok(createResult.getResponse()).header(HttpHeaders.ETAG, createResult.getETag()) .build(); } } @@ -353,7 +353,7 @@ public Response loadTable( .loadTableWithAccessDelegationIfStale(tableIdentifier, etag, snapshots) .orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED)); } - return Response.ok(loadTableResult).header(HttpHeaders.ETAG, loadTableResult.getETag()) + return Response.ok(loadTableResult.getResponse()).header(HttpHeaders.ETAG, loadTableResult.getETag()) .build(); } From be3da8d7859d0100a3cd308d4c2bb41156d47f3f Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Thu, 27 Feb 2025 10:08:53 -0800 Subject: [PATCH 04/10] Changed etagged response to record and gave default implementation to ETaggableEntity --- .../polaris/core/entity/ETaggableEntity.java | 8 ++++++- .../polaris/core/entity/TableLikeEntity.java | 6 ----- .../catalog/IcebergCatalogAdapter.java | 8 +++---- .../catalog/PolarisCatalogHandlerWrapper.java | 4 ++-- .../catalog/response/ETaggedResponse.java | 23 +++---------------- 5 files changed, 16 insertions(+), 33 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java index 8b3f7599b..419d874a4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java @@ -19,6 +19,8 @@ package org.apache.polaris.core.entity; +import net.minidev.json.annotate.JsonIgnore; + /** * Entities that can expose an ETag that can uniquely identify their current state. */ @@ -28,6 +30,7 @@ public interface ETaggableEntity { * Obtain an ETag that uniquely identifies the current state of the entity. * @return the ETag */ + @JsonIgnore String getETag(); /** @@ -35,6 +38,9 @@ public interface ETaggableEntity { * @param etag The etag to compare the entity against * @return true if the etag identifies the current state of the entity, false otherwise */ - boolean isCurrent(String etag); + @JsonIgnore + default boolean isCurrent(String etag) { + return getETag().equals(etag); + } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java index c29b391c3..9bd60acab 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java @@ -85,12 +85,6 @@ public String getETag() { return id + ":" + entityVersion; } - @Override - @JsonIgnore - public boolean isCurrent(String etag) { - return getETag().equals(etag); - } - public static class Builder extends PolarisEntity.BaseBuilder { public Builder(TableIdentifier identifier, String metadataLocation) { super(); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index 21da6af66..bcef69c0d 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -306,12 +306,12 @@ public Response createTable( } else if (delegationModes.isEmpty()) { ETaggedResponse createResult = newHandlerWrapper(realmContext, securityContext, prefix) .createTableDirect(ns, createTableRequest); - return Response.ok(createResult.getResponse()).header(HttpHeaders.ETAG, createResult.getETag()) + return Response.ok(createResult.response()).header(HttpHeaders.ETAG, createResult.etag()) .build(); } else { ETaggedResponse createResult = newHandlerWrapper(realmContext, securityContext, prefix) .createTableDirectWithWriteDelegation(ns, createTableRequest); - return Response.ok(createResult.getResponse()).header(HttpHeaders.ETAG, createResult.getETag()) + return Response.ok(createResult.response()).header(HttpHeaders.ETAG, createResult.etag()) .build(); } } @@ -353,7 +353,7 @@ public Response loadTable( .loadTableWithAccessDelegationIfStale(tableIdentifier, etag, snapshots) .orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED)); } - return Response.ok(loadTableResult.getResponse()).header(HttpHeaders.ETAG, loadTableResult.getETag()) + return Response.ok(loadTableResult.response()).header(HttpHeaders.ETAG, loadTableResult.etag()) .build(); } @@ -400,7 +400,7 @@ public Response registerTable( Namespace ns = decodeNamespace(namespace); ETaggedResponse registerTableResult = newHandlerWrapper(realmContext, securityContext, prefix) .registerTable(ns, registerTableRequest); - return Response.ok(registerTableResult.getResponse()).header(HttpHeaders.ETAG, registerTableResult.getETag()) + return Response.ok(registerTableResult.response()).header(HttpHeaders.ETAG, registerTableResult.etag()) .build(); } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index b913de3f9..9897ae47b 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -846,7 +846,7 @@ private TableLikeEntity getTableEntity(TableIdentifier tableIdentifier) { } public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snapshots) { - return loadTableIfStale(tableIdentifier, null, snapshots).get().getResponse(); + return loadTableIfStale(tableIdentifier, null, snapshots).get().response(); } /** @@ -873,7 +873,7 @@ public Optional> loadTableIfStale(TableIdenti } public LoadTableResponse loadTableWithAccessDelegation(TableIdentifier tableIdentifier, String snapshots) { - return loadTableWithAccessDelegationIfStale(tableIdentifier, null, snapshots).get().getResponse(); + return loadTableWithAccessDelegationIfStale(tableIdentifier, null, snapshots).get().response(); } /** diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java index 1bf727c90..42b5b84e9 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java @@ -19,28 +19,11 @@ package org.apache.polaris.service.catalog.response; +import jakarta.annotation.Nonnull; + /** * Allows Iceberg response types to be wrapped alongside an ETag that * can be unwrapped into the HTTP ETag header. * @param The type of the encapsulated response object */ -public class ETaggedResponse { - - private final T response; - - private final String etag; - - public ETaggedResponse(T response, String etag) { - this.response = response; - this.etag = etag; - } - - public T getResponse() { - return response; - } - - public String getETag() { - return etag; - } - -} +public record ETaggedResponse (@Nonnull T response, @Nonnull String etag) {} From e96bbdba35836224b07e5733e6eb74170667f222 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Thu, 27 Feb 2025 10:16:24 -0800 Subject: [PATCH 05/10] Made iceberg rest spec docs clearer --- spec/iceberg-rest-catalog-open-api.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/iceberg-rest-catalog-open-api.yaml b/spec/iceberg-rest-catalog-open-api.yaml index 4ff97c373..65e912f13 100644 --- a/spec/iceberg-rest-catalog-open-api.yaml +++ b/spec/iceberg-rest-catalog-open-api.yaml @@ -938,8 +938,8 @@ paths: in: header description: An optional header that allows the server to return 304 (Not Modified) if the metadata - is current. The content is the value of the ETag received in a CreateTableResponse or - LoadTableResponse. + is current. The content is the value of the ETag received in a previous CreateTableResponse or + previous LoadTableResponse. required: false schema: type: string @@ -960,8 +960,8 @@ paths: $ref: '#/components/responses/LoadTableResponse' 304: description: - Not Modified - Based on the content of the 'If-None-Match' header the table metadata has - not changed since. + Not Modified - The metadata of the table has not changed since the content of the 'If-None-Match' header + was generated 400: $ref: '#/components/responses/BadRequestErrorResponse' 401: From 772c715a516c2087b815d975afebcc12c3d736e6 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Thu, 27 Feb 2025 16:16:54 -0800 Subject: [PATCH 06/10] Added HTTP Compliant ETag and IfNoneMatch representations and separated persistence from etag logic --- .../polaris/core/entity/ETaggableEntity.java | 46 -------- .../polaris/core/entity/TableLikeEntity.java | 8 +- ...PolarisCatalogHandlerWrapperAuthzTest.java | 13 ++- .../catalog/IcebergCatalogAdapter.java | 10 +- .../catalog/PolarisCatalogHandlerWrapper.java | 42 ++++--- .../catalog/response/ETaggedResponse.java | 3 +- .../org/apache/polaris/service/http/ETag.java | 102 ++++++++++++++++ .../polaris/service/http/IfNoneMatch.java | 93 +++++++++++++++ .../apache/polaris/service/http/ETagTest.java | 59 ++++++++++ .../polaris/service/http/IfNoneMatchTest.java | 110 ++++++++++++++++++ 10 files changed, 410 insertions(+), 76 deletions(-) delete mode 100644 polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java create mode 100644 service/common/src/main/java/org/apache/polaris/service/http/ETag.java create mode 100644 service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java create mode 100644 service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java create mode 100644 service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java deleted file mode 100644 index 419d874a4..000000000 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/ETaggableEntity.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.polaris.core.entity; - -import net.minidev.json.annotate.JsonIgnore; - -/** - * Entities that can expose an ETag that can uniquely identify their current state. - */ -public interface ETaggableEntity { - - /** - * Obtain an ETag that uniquely identifies the current state of the entity. - * @return the ETag - */ - @JsonIgnore - String getETag(); - - /** - * Determines if the provided etag identifies the current version of the entity. - * @param etag The etag to compare the entity against - * @return true if the etag identifies the current state of the entity, false otherwise - */ - @JsonIgnore - default boolean isCurrent(String etag) { - return getETag().equals(etag); - } - -} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java index 9bd60acab..968598b93 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/TableLikeEntity.java @@ -24,7 +24,7 @@ import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.rest.RESTUtil; -public class TableLikeEntity extends PolarisEntity implements ETaggableEntity { +public class TableLikeEntity extends PolarisEntity { // For applicable types, this key on the "internalProperties" map will return the location // of the internalProperties JSON file. public static final String METADATA_LOCATION_KEY = "metadata-location"; @@ -79,12 +79,6 @@ public String getBaseLocation() { return getPropertiesAsMap().get(PolarisEntityConstants.ENTITY_BASE_LOCATION); } - @Override - @JsonIgnore - public String getETag() { - return id + ":" + entityVersion; - } - public static class Builder extends PolarisEntity.BaseBuilder { public Builder(TableIdentifier identifier, String metadataLocation) { super(); diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java index 916620e56..3048b7410 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java @@ -69,6 +69,7 @@ import org.apache.polaris.service.config.RealmEntityManagerFactory; import org.apache.polaris.service.context.CallContextCatalogFactory; import org.apache.polaris.service.context.PolarisCallContextCatalogFactory; +import org.apache.polaris.service.http.IfNoneMatch; import org.apache.polaris.service.quarkus.admin.PolarisAuthzTestBase; import org.apache.polaris.service.types.NotificationRequest; import org.apache.polaris.service.types.NotificationType; @@ -875,7 +876,7 @@ public void testLoadTableIfStaleSufficientPrivileges() { PolarisPrivilege.TABLE_WRITE_DATA, PolarisPrivilege.TABLE_FULL_METADATA, PolarisPrivilege.CATALOG_MANAGE_CONTENT), - () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, "0:0", "all"), + () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all"), null /* cleanupAction */); } @@ -888,7 +889,7 @@ public void testLoadTableIfStaleInsufficientPermissions() { PolarisPrivilege.TABLE_CREATE, PolarisPrivilege.TABLE_LIST, PolarisPrivilege.TABLE_DROP), - () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, "0:0", "all")); + () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all")); } @Test @@ -953,7 +954,7 @@ public void testLoadTableWithReadAccessDelegationIfStaleSufficientPrivileges() { PolarisPrivilege.TABLE_READ_DATA, PolarisPrivilege.TABLE_WRITE_DATA, PolarisPrivilege.CATALOG_MANAGE_CONTENT), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all"), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all"), null /* cleanupAction */); } @@ -969,7 +970,7 @@ public void testLoadTableWithReadAccessDelegationIfStaleInsufficientPermissions( PolarisPrivilege.TABLE_CREATE, PolarisPrivilege.TABLE_LIST, PolarisPrivilege.TABLE_DROP), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all")); + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all")); } @Test @@ -982,7 +983,7 @@ public void testLoadTableWithWriteAccessDelegationIfStaleSufficientPrivileges() PolarisPrivilege.TABLE_READ_DATA, PolarisPrivilege.TABLE_WRITE_DATA, PolarisPrivilege.CATALOG_MANAGE_CONTENT), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all"), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all"), null /* cleanupAction */); } @@ -998,7 +999,7 @@ public void testLoadTableWithWriteAccessDelegationIfStaleInsufficientPermissions PolarisPrivilege.TABLE_CREATE, PolarisPrivilege.TABLE_LIST, PolarisPrivilege.TABLE_DROP), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, "0:0", "all")); + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all")); } @Test diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index bcef69c0d..176375031 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -73,6 +73,7 @@ import org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService; import org.apache.polaris.service.catalog.response.ETaggedResponse; import org.apache.polaris.service.context.CallContextCatalogFactory; +import org.apache.polaris.service.http.IfNoneMatch; import org.apache.polaris.service.types.CommitTableRequest; import org.apache.polaris.service.types.CommitViewRequest; import org.apache.polaris.service.types.NotificationRequest; @@ -335,7 +336,7 @@ public Response loadTable( String namespace, String table, String accessDelegationMode, - String etag, + String ifNoneMatchHeader, String snapshots, RealmContext realmContext, SecurityContext securityContext) { @@ -344,13 +345,16 @@ public Response loadTable( Namespace ns = decodeNamespace(namespace); TableIdentifier tableIdentifier = TableIdentifier.of(ns, RESTUtil.decodeString(table)); ETaggedResponse loadTableResult; + + IfNoneMatch ifNoneMatch = new IfNoneMatch(ifNoneMatchHeader); + if (delegationModes.isEmpty()) { loadTableResult = newHandlerWrapper(realmContext, securityContext, prefix) - .loadTableIfStale(tableIdentifier, etag, snapshots) + .loadTableIfStale(tableIdentifier, ifNoneMatch, snapshots) .orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED)); } else { loadTableResult = newHandlerWrapper(realmContext, securityContext, prefix) - .loadTableWithAccessDelegationIfStale(tableIdentifier, etag, snapshots) + .loadTableWithAccessDelegationIfStale(tableIdentifier, ifNoneMatch, snapshots) .orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED)); } return Response.ok(loadTableResult.response()).header(HttpHeaders.ETAG, loadTableResult.etag()) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 9897ae47b..04c4bfc85 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -93,6 +93,8 @@ import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.service.catalog.response.ETaggedResponse; import org.apache.polaris.service.context.CallContextCatalogFactory; +import org.apache.polaris.service.http.ETag; +import org.apache.polaris.service.http.IfNoneMatch; import org.apache.polaris.service.types.NotificationRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -596,7 +598,7 @@ public ETaggedResponse createTableDirect(Namespace namespace, } return new ETaggedResponse<>( doCatalogOperation(() -> CatalogHandlers.createTable(baseCatalog, namespace, request)), - getTableEntity(identifier).getETag() + generateETagForTable(getTableEntity(identifier)) ); } @@ -664,7 +666,7 @@ public ETaggedResponse createTableDirectWithWriteDelegation( PolarisStorageActions.WRITE, PolarisStorageActions.LIST))); } - return new ETaggedResponse<>(responseBuilder.build(), getTableEntity(tableIdentifier).getETag()); + return new ETaggedResponse<>(responseBuilder.build(), generateETagForTable(getTableEntity(tableIdentifier))); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -792,7 +794,7 @@ public ETaggedResponse registerTable(Namespace namespace, Reg return new ETaggedResponse<>( doCatalogOperation(() -> CatalogHandlers.registerTable(baseCatalog, namespace, request)), - getTableEntity(identifier).getETag() + generateETagForTable(getTableEntity(identifier)) ); } @@ -845,30 +847,42 @@ private TableLikeEntity getTableEntity(TableIdentifier tableIdentifier) { return TableLikeEntity.of(target.getRawLeafEntity()); } + /** + * Generate an ETag for a table entity + * @param tableLikeEntity the table to generate the ETag for + * @return the generated ETag + */ + private ETag generateETagForTable(TableLikeEntity tableLikeEntity) { + return new ETag(true, tableLikeEntity.getId() + ":" + tableLikeEntity.getEntityVersion()); + } + public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snapshots) { return loadTableIfStale(tableIdentifier, null, snapshots).get().response(); } /** - * Attempt to perform a loadTable operation only when the specified etag does not match the current state of the table. + * Attempt to perform a loadTable operation only when the specified set of etags do not match the current state + * of the table metadata. * @param tableIdentifier The identifier of the table to load - * @param etag The ETag which identifies the metadata currently known + * @param ifNoneMatch set of entity-tags to check the metadata against for staleness * @param snapshots * @return {@link Optional#empty()} if the ETag is current, an {@link Optional} containing the load table response, otherwise */ - public Optional> loadTableIfStale(TableIdentifier tableIdentifier, String etag, String snapshots) { + public Optional> loadTableIfStale( + TableIdentifier tableIdentifier, IfNoneMatch ifNoneMatch, String snapshots) { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LOAD_TABLE; authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); TableLikeEntity tableEntity = getTableEntity(tableIdentifier); + ETag tableEntityTag = generateETagForTable(tableEntity); - if (tableEntity.isCurrent(etag)) { + if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableEntityTag)) { return Optional.empty(); } return Optional.of(new ETaggedResponse<>( doCatalogOperation(() -> CatalogHandlers.loadTable(baseCatalog, tableIdentifier)), - tableEntity.getETag() + tableEntityTag )); } @@ -877,14 +891,15 @@ public LoadTableResponse loadTableWithAccessDelegation(TableIdentifier tableIden } /** - * Attempt to perform a loadTable operation with access delegation only when the specified etag does not match the current state of the table. + * Attempt to perform a loadTable operation with access delegation only when the if none of the provided etags + * match the current state of the table metadata. * @param tableIdentifier The identifier of the table to load - * @param etag The ETag which identifies the metadata currently known + * @param ifNoneMatch set of entity-tags to check the metadata against for staleness * @param snapshots * @return {@link Optional#empty()} if the ETag is current, an {@link Optional} containing the load table response, otherwise */ public Optional> loadTableWithAccessDelegationIfStale( - TableIdentifier tableIdentifier, String etag, String snapshots) { + TableIdentifier tableIdentifier, IfNoneMatch ifNoneMatch, String snapshots) { // Here we have a single method that falls through multiple candidate // PolarisAuthorizableOperations because instead of identifying the desired operation up-front // and @@ -928,8 +943,9 @@ public Optional> loadTableWithAccessDelegatio } TableLikeEntity tableEntity = getTableEntity(tableIdentifier); + ETag tableETag = generateETagForTable(tableEntity); - if (tableEntity.isCurrent(etag)) { + if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableETag)) { return Optional.empty(); } @@ -953,7 +969,7 @@ public Optional> loadTableWithAccessDelegatio credentialDelegation.getCredentialConfig( tableIdentifier, tableMetadata, actionsRequested)); } - return new ETaggedResponse<>(responseBuilder.build(), tableEntity.getETag()); + return new ETaggedResponse<>(responseBuilder.build(), generateETagForTable(tableEntity)); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java index 42b5b84e9..d8f194adb 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java @@ -20,10 +20,11 @@ package org.apache.polaris.service.catalog.response; import jakarta.annotation.Nonnull; +import org.apache.polaris.service.http.ETag; /** * Allows Iceberg response types to be wrapped alongside an ETag that * can be unwrapped into the HTTP ETag header. * @param The type of the encapsulated response object */ -public record ETaggedResponse (@Nonnull T response, @Nonnull String etag) {} +public record ETaggedResponse (@Nonnull T response, @Nonnull ETag etag) {} diff --git a/service/common/src/main/java/org/apache/polaris/service/http/ETag.java b/service/common/src/main/java/org/apache/polaris/service/http/ETag.java new file mode 100644 index 000000000..4a10c5f88 --- /dev/null +++ b/service/common/src/main/java/org/apache/polaris/service/http/ETag.java @@ -0,0 +1,102 @@ +/* + * 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.polaris.service.http; + +import jakarta.annotation.Nonnull; + +import java.util.Objects; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * HTTP Compliant ETag logical wrapper. + */ +public class ETag { + + protected static Pattern ETAG_PATTERN = Pattern.compile("(W/)?\"([^\"]*)\""); + + private final boolean weak; + + private final String value; + + public ETag(boolean weak, String value) { + this.weak = weak; + this.value = value; + } + + /** + * Consumes the raw value of an entity-tag and produces a logical representation + * @param rawValue + */ + public ETag(@Nonnull String rawValue) { + rawValue = rawValue.trim(); + Matcher matcher = ETag.ETAG_PATTERN.matcher(rawValue); + + if (!matcher.matches()) { + throw new IllegalArgumentException("Invalid ETag representation."); + } + + this.weak = rawValue.startsWith("W/"); + this.value = rawValue.substring(rawValue.indexOf('"') + 1, rawValue.length() - 1); + } + + /** + * Obtain the value of the etag. For example, if we had an etag W/"abc", this + * method would return abc + * @return The internal value of the etag + */ + public String value() { + return value; + } + + /** + * Determine if the etag is a weak etag + * @return true if the etag is prefixed by W/, false otherwise + */ + public boolean isWeak() { + return weak; + } + + @Override + public int hashCode() { + return Objects.hash(weak, value); + } + + @Override + public boolean equals(Object o) { + if (o instanceof ETag other) { + return weak == other.weak && value.equals(other.value); + } else if (o instanceof String s) { + return this.toString().equals(s); + } + return false; + } + + /** + * Returns the raw HTTP compliant representation of this tag + * @return + */ + @Override + public String toString() { + String representation = weak ? "W/" : ""; + return representation + "\"" + value + "\""; + } + +} diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java new file mode 100644 index 000000000..adae6970f --- /dev/null +++ b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java @@ -0,0 +1,93 @@ +/* + * 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.polaris.service.http; + +import com.google.common.collect.ImmutableList; + +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.stream.Collectors; + +/** + * Logical representation of an HTTP compliant If-None-Match header. + */ +public class IfNoneMatch { + + private final boolean wildcard; + + private final List etags; + + /** + * Parses a non wildcard If-None-Match header value into ETags + * @param header the header to parse, eg `W\"etag1", "etag2,with,comma", W\"etag3"` + * @return the header parsed into raw string etag values. For the example given, ["W\"etag1"", ""etag2,with,comma"", "W\"etag3""] + */ + private static List parseHeaderIntoParts(String header) { + header = header.trim(); + Matcher matcher = ETag.ETAG_PATTERN.matcher(header); + + return matcher.results() + .map(result -> result.group(0)) + .collect(Collectors.toList()); + } + + public IfNoneMatch(String rawValue) { + if (rawValue == null) { + this.wildcard = false; + this.etags = new ArrayList<>(); + } else { + rawValue = rawValue.trim(); + if (rawValue.equals("*")) { + this.wildcard = true; + this.etags = new ArrayList<>(); + } else { + this.wildcard = false; + List parts = parseHeaderIntoParts(rawValue); + this.etags = parts.stream().map(ETag::new).toList(); + + if (!this.wildcard && this.etags.isEmpty() && !rawValue.isEmpty()) { + throw new IllegalArgumentException("Invalid representation for If-None-Match header."); + } + } + + } + } + + public boolean isWildcard() { + return wildcard; + } + + public List getEtags() { + return ImmutableList.copyOf(etags); + } + + /** + * If any contained ETag matches the provided etag or the header is a wildcard. + * Only matches weak etags to weak etags and strong etags to strong etags. + * @param etag the etag to compare against. + * @return true if any of the contained ETags match the provided etag + */ + public boolean anyMatch(ETag etag) { + if (wildcard) return true; + return etags.contains(etag); + } + +} diff --git a/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java b/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java new file mode 100644 index 000000000..c72ed4aa3 --- /dev/null +++ b/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java @@ -0,0 +1,59 @@ +/* + * 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.polaris.service.http; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class ETagTest { + + @Test + public void validStrongETagRepresentationIsCorrect() { + String rawValue = "\"strong-etag\""; + ETag eTag = new ETag(rawValue); + + Assertions.assertFalse(eTag.isWeak()); + Assertions.assertEquals(rawValue, eTag.toString()); + Assertions.assertEquals("strong-etag", eTag.value()); + } + + @Test + public void validWeakETagRepresentationIsCorrect() { + String rawValue = "W/\"weak-etag\""; + ETag eTag = new ETag(rawValue); + + Assertions.assertTrue(eTag.isWeak()); + Assertions.assertEquals(rawValue, eTag.toString()); + Assertions.assertEquals("weak-etag", eTag.value()); + } + + @Test + public void invalidETagRepresentationThrowsException() { + String rawValue = "clearly_invalid_etag"; + Assertions.assertThrows(IllegalArgumentException.class, () -> new ETag(rawValue)); + } + + @Test + public void eTagWithQuotesInValueThrowsException() { + String rawValue = "W/\"etag\"with_quote\""; + Assertions.assertThrows(IllegalArgumentException.class, () -> new ETag(rawValue)); + } + +} diff --git a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java new file mode 100644 index 000000000..22e4d8f2c --- /dev/null +++ b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java @@ -0,0 +1,110 @@ +/* + * 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.polaris.service.http; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class IfNoneMatchTest { + + @Test + public void validSingleETag() { + String header = "W/\"value\""; + IfNoneMatch ifNoneMatch = new IfNoneMatch(header); + + ETag parsedETag = ifNoneMatch.getEtags().getFirst(); + + Assertions.assertEquals("value", parsedETag.value()); + Assertions.assertTrue(parsedETag.isWeak()); + } + + @Test + public void validMultipleETags() { + String etagValue1 = "W/\"etag1\""; + String etagValue2 = "W/\"etag2,with,comma\""; + String etagValue3 = "W/\"etag3\""; + + String header = etagValue1 + ", " + etagValue2 + ", " + etagValue3; + IfNoneMatch ifNoneMatch = new IfNoneMatch(header); + + Assertions.assertEquals(3, ifNoneMatch.getEtags().size()); + + ETag etag1 = ifNoneMatch.getEtags().get(0); + ETag etag2 = ifNoneMatch.getEtags().get(1); + ETag etag3 = ifNoneMatch.getEtags().get(2); + + Assertions.assertEquals(etagValue1, etag1.toString()); + Assertions.assertEquals(etagValue2, etag2.toString()); + Assertions.assertEquals(etagValue3, etag3.toString()); + } + + @Test + public void validWildcardIfNoneMatch() { + IfNoneMatch ifNoneMatch = new IfNoneMatch("*"); + Assertions.assertTrue(ifNoneMatch.isWildcard()); + Assertions.assertTrue(ifNoneMatch.getEtags().isEmpty()); + } + + @Test + public void nullIfNoneMatchIsValid() { + IfNoneMatch nullIfNoneMatch = new IfNoneMatch(null); + Assertions.assertTrue(nullIfNoneMatch.getEtags().isEmpty()); + } + + @Test + public void invalidETagThrowsException() { + String header = "wrong_value"; + Assertions.assertThrows(IllegalArgumentException.class, () -> new IfNoneMatch(header)); + } + + @Test + public void etagsMatch() { + ETag weakETag = new ETag("W/\"weak\""); + ETag strongETag = new ETag("\"strong\""); + IfNoneMatch ifNoneMatch = new IfNoneMatch("W/\"weak\", \"strong\""); + Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); + Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); + } + + @Test + public void weakETagOnlyMatchesWeak() { + ETag weakETag = new ETag("W/\"etag\""); + IfNoneMatch ifNoneMatch = new IfNoneMatch("\"etag\""); + Assertions.assertFalse(ifNoneMatch.anyMatch(weakETag)); + } + + @Test + public void strongETagOnlyMatchesStrong() { + ETag strongETag = new ETag("\"etag\""); + IfNoneMatch ifNoneMatch = new IfNoneMatch("W/\"etag\""); + Assertions.assertFalse(ifNoneMatch.anyMatch(strongETag)); + } + + @Test + public void wildCardMatchesEverything() { + ETag strongETag = new ETag("\"etag\""); + ETag weakETag = new ETag("W/\"etag\""); + IfNoneMatch ifNoneMatch = new IfNoneMatch("*"); + Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); + Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); + } + + +} From 662f24a41fc3a1019fffcb8e3d664c3a3f974243 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Fri, 28 Feb 2025 10:44:01 -0800 Subject: [PATCH 07/10] Changed ETag to be a record and improved semantics of IfNoneMatch --- ...PolarisCatalogHandlerWrapperAuthzTest.java | 12 +++--- .../catalog/IcebergCatalogAdapter.java | 2 +- .../org/apache/polaris/service/http/ETag.java | 40 ++++--------------- .../polaris/service/http/IfNoneMatch.java | 39 ++++++++++-------- .../apache/polaris/service/http/ETagTest.java | 8 ++-- .../polaris/service/http/IfNoneMatchTest.java | 30 +++++++------- 6 files changed, 57 insertions(+), 74 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java index 3048b7410..6ea1bdcb4 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java @@ -876,7 +876,7 @@ public void testLoadTableIfStaleSufficientPrivileges() { PolarisPrivilege.TABLE_WRITE_DATA, PolarisPrivilege.TABLE_FULL_METADATA, PolarisPrivilege.CATALOG_MANAGE_CONTENT), - () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all"), + () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, IfNoneMatch.fromHeader("W/\"0:0\""), "all"), null /* cleanupAction */); } @@ -889,7 +889,7 @@ public void testLoadTableIfStaleInsufficientPermissions() { PolarisPrivilege.TABLE_CREATE, PolarisPrivilege.TABLE_LIST, PolarisPrivilege.TABLE_DROP), - () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all")); + () -> newWrapper().loadTableIfStale(TABLE_NS1A_2, IfNoneMatch.fromHeader("W/\"0:0\""), "all")); } @Test @@ -954,7 +954,7 @@ public void testLoadTableWithReadAccessDelegationIfStaleSufficientPrivileges() { PolarisPrivilege.TABLE_READ_DATA, PolarisPrivilege.TABLE_WRITE_DATA, PolarisPrivilege.CATALOG_MANAGE_CONTENT), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all"), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, IfNoneMatch.fromHeader("W/\"0:0\""), "all"), null /* cleanupAction */); } @@ -970,7 +970,7 @@ public void testLoadTableWithReadAccessDelegationIfStaleInsufficientPermissions( PolarisPrivilege.TABLE_CREATE, PolarisPrivilege.TABLE_LIST, PolarisPrivilege.TABLE_DROP), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all")); + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, IfNoneMatch.fromHeader("W/\"0:0\""), "all")); } @Test @@ -983,7 +983,7 @@ public void testLoadTableWithWriteAccessDelegationIfStaleSufficientPrivileges() PolarisPrivilege.TABLE_READ_DATA, PolarisPrivilege.TABLE_WRITE_DATA, PolarisPrivilege.CATALOG_MANAGE_CONTENT), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all"), + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, IfNoneMatch.fromHeader("W/\"0:0\""), "all"), null /* cleanupAction */); } @@ -999,7 +999,7 @@ public void testLoadTableWithWriteAccessDelegationIfStaleInsufficientPermissions PolarisPrivilege.TABLE_CREATE, PolarisPrivilege.TABLE_LIST, PolarisPrivilege.TABLE_DROP), - () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, new IfNoneMatch("W/\"0:0\""), "all")); + () -> newWrapper().loadTableWithAccessDelegationIfStale(TABLE_NS1A_2, IfNoneMatch.fromHeader("W/\"0:0\""), "all")); } @Test diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index 176375031..ccf6a382a 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -346,7 +346,7 @@ public Response loadTable( TableIdentifier tableIdentifier = TableIdentifier.of(ns, RESTUtil.decodeString(table)); ETaggedResponse loadTableResult; - IfNoneMatch ifNoneMatch = new IfNoneMatch(ifNoneMatchHeader); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader(ifNoneMatchHeader); if (delegationModes.isEmpty()) { loadTableResult = newHandlerWrapper(realmContext, securityContext, prefix) diff --git a/service/common/src/main/java/org/apache/polaris/service/http/ETag.java b/service/common/src/main/java/org/apache/polaris/service/http/ETag.java index 4a10c5f88..4a977f385 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/ETag.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/ETag.java @@ -28,24 +28,15 @@ /** * HTTP Compliant ETag logical wrapper. */ -public class ETag { +public record ETag(boolean isWeak, @Nonnull String value) { protected static Pattern ETAG_PATTERN = Pattern.compile("(W/)?\"([^\"]*)\""); - private final boolean weak; - - private final String value; - - public ETag(boolean weak, String value) { - this.weak = weak; - this.value = value; - } - /** * Consumes the raw value of an entity-tag and produces a logical representation * @param rawValue */ - public ETag(@Nonnull String rawValue) { + public static ETag fromHeader(@Nonnull String rawValue) { rawValue = rawValue.trim(); Matcher matcher = ETag.ETAG_PATTERN.matcher(rawValue); @@ -53,36 +44,21 @@ public ETag(@Nonnull String rawValue) { throw new IllegalArgumentException("Invalid ETag representation."); } - this.weak = rawValue.startsWith("W/"); - this.value = rawValue.substring(rawValue.indexOf('"') + 1, rawValue.length() - 1); - } + boolean weak = rawValue.startsWith("W/"); + String value = rawValue.substring(rawValue.indexOf('"') + 1, rawValue.length() - 1); - /** - * Obtain the value of the etag. For example, if we had an etag W/"abc", this - * method would return abc - * @return The internal value of the etag - */ - public String value() { - return value; - } - - /** - * Determine if the etag is a weak etag - * @return true if the etag is prefixed by W/, false otherwise - */ - public boolean isWeak() { - return weak; + return new ETag(weak, value); } @Override public int hashCode() { - return Objects.hash(weak, value); + return Objects.hash(isWeak, value); } @Override public boolean equals(Object o) { if (o instanceof ETag other) { - return weak == other.weak && value.equals(other.value); + return isWeak == other.isWeak && value.equals(other.value); } else if (o instanceof String s) { return this.toString().equals(s); } @@ -95,7 +71,7 @@ public boolean equals(Object o) { */ @Override public String toString() { - String representation = weak ? "W/" : ""; + String representation = isWeak ? "W/" : ""; return representation + "\"" + value + "\""; } diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java index adae6970f..3a2e1dfa7 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java @@ -49,25 +49,32 @@ private static List parseHeaderIntoParts(String header) { .collect(Collectors.toList()); } - public IfNoneMatch(String rawValue) { + public IfNoneMatch(boolean wildcard) { + this.wildcard = wildcard; + this.etags = new ArrayList<>(); + } + + public IfNoneMatch(List etags) { + this.wildcard = false; + this.etags = new ArrayList<>(etags); + } + + public static IfNoneMatch fromHeader(String rawValue) { if (rawValue == null) { - this.wildcard = false; - this.etags = new ArrayList<>(); + return new IfNoneMatch(false); + } + + rawValue = rawValue.trim(); + if (rawValue.equals("*")) { + return new IfNoneMatch(true); } else { - rawValue = rawValue.trim(); - if (rawValue.equals("*")) { - this.wildcard = true; - this.etags = new ArrayList<>(); - } else { - this.wildcard = false; - List parts = parseHeaderIntoParts(rawValue); - this.etags = parts.stream().map(ETag::new).toList(); - - if (!this.wildcard && this.etags.isEmpty() && !rawValue.isEmpty()) { - throw new IllegalArgumentException("Invalid representation for If-None-Match header."); - } - } + List parts = parseHeaderIntoParts(rawValue); + List etags = parts.stream().map(ETag::fromHeader).toList(); + if (etags.isEmpty() && !rawValue.isEmpty()) { + throw new IllegalArgumentException("Invalid representation for If-None-Match header."); + } + return new IfNoneMatch(etags); } } diff --git a/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java b/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java index c72ed4aa3..a2de57ef6 100644 --- a/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java @@ -27,7 +27,7 @@ public class ETagTest { @Test public void validStrongETagRepresentationIsCorrect() { String rawValue = "\"strong-etag\""; - ETag eTag = new ETag(rawValue); + ETag eTag = ETag.fromHeader(rawValue); Assertions.assertFalse(eTag.isWeak()); Assertions.assertEquals(rawValue, eTag.toString()); @@ -37,7 +37,7 @@ public void validStrongETagRepresentationIsCorrect() { @Test public void validWeakETagRepresentationIsCorrect() { String rawValue = "W/\"weak-etag\""; - ETag eTag = new ETag(rawValue); + ETag eTag = ETag.fromHeader(rawValue); Assertions.assertTrue(eTag.isWeak()); Assertions.assertEquals(rawValue, eTag.toString()); @@ -47,13 +47,13 @@ public void validWeakETagRepresentationIsCorrect() { @Test public void invalidETagRepresentationThrowsException() { String rawValue = "clearly_invalid_etag"; - Assertions.assertThrows(IllegalArgumentException.class, () -> new ETag(rawValue)); + Assertions.assertThrows(IllegalArgumentException.class, () -> ETag.fromHeader(rawValue)); } @Test public void eTagWithQuotesInValueThrowsException() { String rawValue = "W/\"etag\"with_quote\""; - Assertions.assertThrows(IllegalArgumentException.class, () -> new ETag(rawValue)); + Assertions.assertThrows(IllegalArgumentException.class, () -> ETag.fromHeader(rawValue)); } } diff --git a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java index 22e4d8f2c..dd11fb546 100644 --- a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java @@ -27,7 +27,7 @@ public class IfNoneMatchTest { @Test public void validSingleETag() { String header = "W/\"value\""; - IfNoneMatch ifNoneMatch = new IfNoneMatch(header); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader(header); ETag parsedETag = ifNoneMatch.getEtags().getFirst(); @@ -42,7 +42,7 @@ public void validMultipleETags() { String etagValue3 = "W/\"etag3\""; String header = etagValue1 + ", " + etagValue2 + ", " + etagValue3; - IfNoneMatch ifNoneMatch = new IfNoneMatch(header); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader(header); Assertions.assertEquals(3, ifNoneMatch.getEtags().size()); @@ -57,51 +57,51 @@ public void validMultipleETags() { @Test public void validWildcardIfNoneMatch() { - IfNoneMatch ifNoneMatch = new IfNoneMatch("*"); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("*"); Assertions.assertTrue(ifNoneMatch.isWildcard()); Assertions.assertTrue(ifNoneMatch.getEtags().isEmpty()); } @Test public void nullIfNoneMatchIsValid() { - IfNoneMatch nullIfNoneMatch = new IfNoneMatch(null); + IfNoneMatch nullIfNoneMatch = IfNoneMatch.fromHeader(null); Assertions.assertTrue(nullIfNoneMatch.getEtags().isEmpty()); } @Test public void invalidETagThrowsException() { String header = "wrong_value"; - Assertions.assertThrows(IllegalArgumentException.class, () -> new IfNoneMatch(header)); + Assertions.assertThrows(IllegalArgumentException.class, () -> IfNoneMatch.fromHeader(header)); } @Test public void etagsMatch() { - ETag weakETag = new ETag("W/\"weak\""); - ETag strongETag = new ETag("\"strong\""); - IfNoneMatch ifNoneMatch = new IfNoneMatch("W/\"weak\", \"strong\""); + ETag weakETag = ETag.fromHeader("W/\"weak\""); + ETag strongETag = ETag.fromHeader("\"strong\""); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("W/\"weak\", \"strong\""); Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); } @Test public void weakETagOnlyMatchesWeak() { - ETag weakETag = new ETag("W/\"etag\""); - IfNoneMatch ifNoneMatch = new IfNoneMatch("\"etag\""); + ETag weakETag = ETag.fromHeader("W/\"etag\""); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("\"etag\""); Assertions.assertFalse(ifNoneMatch.anyMatch(weakETag)); } @Test public void strongETagOnlyMatchesStrong() { - ETag strongETag = new ETag("\"etag\""); - IfNoneMatch ifNoneMatch = new IfNoneMatch("W/\"etag\""); + ETag strongETag = ETag.fromHeader("\"etag\""); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("W/\"etag\""); Assertions.assertFalse(ifNoneMatch.anyMatch(strongETag)); } @Test public void wildCardMatchesEverything() { - ETag strongETag = new ETag("\"etag\""); - ETag weakETag = new ETag("W/\"etag\""); - IfNoneMatch ifNoneMatch = new IfNoneMatch("*"); + ETag strongETag = ETag.fromHeader("\"etag\""); + ETag weakETag = ETag.fromHeader("W/\"etag\""); + IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("*"); Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); } From 652cc08ad10b7794c140ca5c66cd9001acb29774 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Fri, 28 Feb 2025 10:59:34 -0800 Subject: [PATCH 08/10] Fixed semantics of if none match --- .../polaris/service/http/IfNoneMatch.java | 40 +++++++++++++------ .../polaris/service/http/IfNoneMatchTest.java | 4 ++ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java index 3a2e1dfa7..c91d939e8 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java @@ -49,28 +49,26 @@ private static List parseHeaderIntoParts(String header) { .collect(Collectors.toList()); } - public IfNoneMatch(boolean wildcard) { - this.wildcard = wildcard; - this.etags = new ArrayList<>(); - } - - public IfNoneMatch(List etags) { - this.wildcard = false; - this.etags = new ArrayList<>(etags); - } - + /** + * Parses the raw content of an If-None-Match header into the logical representation + * @param rawValue The raw value of the If-None-Match header + * @return A logically equivalent representation of the raw header content + */ public static IfNoneMatch fromHeader(String rawValue) { + // parse null header as an empty header if (rawValue == null) { - return new IfNoneMatch(false); + return new IfNoneMatch(List.of()); } rawValue = rawValue.trim(); if (rawValue.equals("*")) { - return new IfNoneMatch(true); + return IfNoneMatch.wildcard(); } else { List parts = parseHeaderIntoParts(rawValue); List etags = parts.stream().map(ETag::fromHeader).toList(); + // If we have no etags parsed, but the raw value of the header contained some content, + // that means there were one or more invalid parts in the header if (etags.isEmpty() && !rawValue.isEmpty()) { throw new IllegalArgumentException("Invalid representation for If-None-Match header."); } @@ -78,6 +76,24 @@ public static IfNoneMatch fromHeader(String rawValue) { } } + /** + * Constructs a new wildcard If-None-Match header + * @return + */ + public static IfNoneMatch wildcard() { + return new IfNoneMatch(true, List.of()); + } + + private IfNoneMatch(boolean wildcard, List etags) { + this.wildcard = wildcard; + this.etags = etags; + } + + public IfNoneMatch(List etags) { + this.wildcard = false; + this.etags = new ArrayList<>(etags); + } + public boolean isWildcard() { return wildcard; } diff --git a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java index dd11fb546..f6c4d885f 100644 --- a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java @@ -104,6 +104,10 @@ public void wildCardMatchesEverything() { IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("*"); Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); + + IfNoneMatch canonicallyBuiltWildcard = IfNoneMatch.wildcard(); + Assertions.assertTrue(canonicallyBuiltWildcard.anyMatch(strongETag)); + Assertions.assertTrue(canonicallyBuiltWildcard.anyMatch(weakETag)); } From 2c4fd1b8367c7d72036e123c03b8fcab9265fcc5 Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Fri, 28 Feb 2025 16:39:56 -0800 Subject: [PATCH 09/10] Removed ETag representation, consolidated in IfNoneMatch --- .../catalog/IcebergCatalogAdapter.java | 11 ++- .../catalog/PolarisCatalogHandlerWrapper.java | 22 ++--- .../catalog/response/ETaggedResponse.java | 9 +- .../org/apache/polaris/service/http/ETag.java | 78 ---------------- .../polaris/service/http/IfNoneMatch.java | 92 +++++++++---------- .../apache/polaris/service/http/ETagTest.java | 59 ------------ .../polaris/service/http/IfNoneMatchTest.java | 52 +++++++---- 7 files changed, 102 insertions(+), 221 deletions(-) delete mode 100644 service/common/src/main/java/org/apache/polaris/service/http/ETag.java delete mode 100644 service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java index ccf6a382a..a0a035867 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java @@ -307,12 +307,12 @@ public Response createTable( } else if (delegationModes.isEmpty()) { ETaggedResponse createResult = newHandlerWrapper(realmContext, securityContext, prefix) .createTableDirect(ns, createTableRequest); - return Response.ok(createResult.response()).header(HttpHeaders.ETAG, createResult.etag()) + return Response.ok(createResult.response()).header(HttpHeaders.ETAG, createResult.eTag()) .build(); } else { ETaggedResponse createResult = newHandlerWrapper(realmContext, securityContext, prefix) .createTableDirectWithWriteDelegation(ns, createTableRequest); - return Response.ok(createResult.response()).header(HttpHeaders.ETAG, createResult.etag()) + return Response.ok(createResult.response()).header(HttpHeaders.ETAG, createResult.eTag()) .build(); } } @@ -348,6 +348,9 @@ public Response loadTable( IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader(ifNoneMatchHeader); + if (ifNoneMatch.isWildcard()) + throw new BadRequestException("If-None-Match may not take the value of '*'"); + if (delegationModes.isEmpty()) { loadTableResult = newHandlerWrapper(realmContext, securityContext, prefix) .loadTableIfStale(tableIdentifier, ifNoneMatch, snapshots) @@ -357,7 +360,7 @@ public Response loadTable( .loadTableWithAccessDelegationIfStale(tableIdentifier, ifNoneMatch, snapshots) .orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED)); } - return Response.ok(loadTableResult.response()).header(HttpHeaders.ETAG, loadTableResult.etag()) + return Response.ok(loadTableResult.response()).header(HttpHeaders.ETAG, loadTableResult.eTag()) .build(); } @@ -404,7 +407,7 @@ public Response registerTable( Namespace ns = decodeNamespace(namespace); ETaggedResponse registerTableResult = newHandlerWrapper(realmContext, securityContext, prefix) .registerTable(ns, registerTableRequest); - return Response.ok(registerTableResult.response()).header(HttpHeaders.ETAG, registerTableResult.etag()) + return Response.ok(registerTableResult.response()).header(HttpHeaders.ETAG, registerTableResult.eTag()) .build(); } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java index 04c4bfc85..1a575c91e 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java @@ -93,7 +93,6 @@ import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.service.catalog.response.ETaggedResponse; import org.apache.polaris.service.context.CallContextCatalogFactory; -import org.apache.polaris.service.http.ETag; import org.apache.polaris.service.http.IfNoneMatch; import org.apache.polaris.service.types.NotificationRequest; import org.slf4j.Logger; @@ -598,7 +597,7 @@ public ETaggedResponse createTableDirect(Namespace namespace, } return new ETaggedResponse<>( doCatalogOperation(() -> CatalogHandlers.createTable(baseCatalog, namespace, request)), - generateETagForTable(getTableEntity(identifier)) + generateETagValueForTable(getTableEntity(identifier)) ); } @@ -666,7 +665,7 @@ public ETaggedResponse createTableDirectWithWriteDelegation( PolarisStorageActions.WRITE, PolarisStorageActions.LIST))); } - return new ETaggedResponse<>(responseBuilder.build(), generateETagForTable(getTableEntity(tableIdentifier))); + return new ETaggedResponse<>(responseBuilder.build(), generateETagValueForTable(getTableEntity(tableIdentifier))); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -794,7 +793,7 @@ public ETaggedResponse registerTable(Namespace namespace, Reg return new ETaggedResponse<>( doCatalogOperation(() -> CatalogHandlers.registerTable(baseCatalog, namespace, request)), - generateETagForTable(getTableEntity(identifier)) + generateETagValueForTable(getTableEntity(identifier)) ); } @@ -852,8 +851,9 @@ private TableLikeEntity getTableEntity(TableIdentifier tableIdentifier) { * @param tableLikeEntity the table to generate the ETag for * @return the generated ETag */ - private ETag generateETagForTable(TableLikeEntity tableLikeEntity) { - return new ETag(true, tableLikeEntity.getId() + ":" + tableLikeEntity.getEntityVersion()); + private String generateETagValueForTable(TableLikeEntity tableLikeEntity) { + // always issue a weak ETag since we never do a byte by byte comparisomn + return "W/\"" + tableLikeEntity.getId() + ":" + tableLikeEntity.getEntityVersion() + "\""; } public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snapshots) { @@ -861,7 +861,7 @@ public LoadTableResponse loadTable(TableIdentifier tableIdentifier, String snaps } /** - * Attempt to perform a loadTable operation only when the specified set of etags do not match the current state + * Attempt to perform a loadTable operation only when the specified set of eTags do not match the current state * of the table metadata. * @param tableIdentifier The identifier of the table to load * @param ifNoneMatch set of entity-tags to check the metadata against for staleness @@ -874,7 +874,7 @@ public Optional> loadTableIfStale( authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.TABLE, tableIdentifier); TableLikeEntity tableEntity = getTableEntity(tableIdentifier); - ETag tableEntityTag = generateETagForTable(tableEntity); + String tableEntityTag = generateETagValueForTable(tableEntity); if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableEntityTag)) { return Optional.empty(); @@ -891,7 +891,7 @@ public LoadTableResponse loadTableWithAccessDelegation(TableIdentifier tableIden } /** - * Attempt to perform a loadTable operation with access delegation only when the if none of the provided etags + * Attempt to perform a loadTable operation with access delegation only when the if none of the provided eTags * match the current state of the table metadata. * @param tableIdentifier The identifier of the table to load * @param ifNoneMatch set of entity-tags to check the metadata against for staleness @@ -943,7 +943,7 @@ public Optional> loadTableWithAccessDelegatio } TableLikeEntity tableEntity = getTableEntity(tableIdentifier); - ETag tableETag = generateETagForTable(tableEntity); + String tableETag = generateETagValueForTable(tableEntity); if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableETag)) { return Optional.empty(); @@ -969,7 +969,7 @@ public Optional> loadTableWithAccessDelegatio credentialDelegation.getCredentialConfig( tableIdentifier, tableMetadata, actionsRequested)); } - return new ETaggedResponse<>(responseBuilder.build(), generateETagForTable(tableEntity)); + return new ETaggedResponse<>(responseBuilder.build(), generateETagValueForTable(tableEntity)); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java index d8f194adb..898339b47 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/response/ETaggedResponse.java @@ -20,11 +20,16 @@ package org.apache.polaris.service.catalog.response; import jakarta.annotation.Nonnull; -import org.apache.polaris.service.http.ETag; /** * Allows Iceberg response types to be wrapped alongside an ETag that * can be unwrapped into the HTTP ETag header. + * + * TODO: Remove this if ever the ETag change to the REST spec is propagated to the iceberg rest library + * representation of {@link org.apache.iceberg.rest.responses.LoadTableResponse} + * + * @param response the iceberg response to encapsulate + * @param eTag the eTag value * @param The type of the encapsulated response object */ -public record ETaggedResponse (@Nonnull T response, @Nonnull ETag etag) {} +public record ETaggedResponse (@Nonnull T response, @Nonnull String eTag) {} \ No newline at end of file diff --git a/service/common/src/main/java/org/apache/polaris/service/http/ETag.java b/service/common/src/main/java/org/apache/polaris/service/http/ETag.java deleted file mode 100644 index 4a977f385..000000000 --- a/service/common/src/main/java/org/apache/polaris/service/http/ETag.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.polaris.service.http; - -import jakarta.annotation.Nonnull; - -import java.util.Objects; -import java.util.regex.Matcher; -import java.util.regex.Pattern; - -/** - * HTTP Compliant ETag logical wrapper. - */ -public record ETag(boolean isWeak, @Nonnull String value) { - - protected static Pattern ETAG_PATTERN = Pattern.compile("(W/)?\"([^\"]*)\""); - - /** - * Consumes the raw value of an entity-tag and produces a logical representation - * @param rawValue - */ - public static ETag fromHeader(@Nonnull String rawValue) { - rawValue = rawValue.trim(); - Matcher matcher = ETag.ETAG_PATTERN.matcher(rawValue); - - if (!matcher.matches()) { - throw new IllegalArgumentException("Invalid ETag representation."); - } - - boolean weak = rawValue.startsWith("W/"); - String value = rawValue.substring(rawValue.indexOf('"') + 1, rawValue.length() - 1); - - return new ETag(weak, value); - } - - @Override - public int hashCode() { - return Objects.hash(isWeak, value); - } - - @Override - public boolean equals(Object o) { - if (o instanceof ETag other) { - return isWeak == other.isWeak && value.equals(other.value); - } else if (o instanceof String s) { - return this.toString().equals(s); - } - return false; - } - - /** - * Returns the raw HTTP compliant representation of this tag - * @return - */ - @Override - public String toString() { - String representation = isWeak ? "W/" : ""; - return representation + "\"" + value + "\""; - } - -} diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java index c91d939e8..338e9766a 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java @@ -19,34 +19,41 @@ package org.apache.polaris.service.http; -import com.google.common.collect.ImmutableList; +import jakarta.annotation.Nonnull; -import java.util.ArrayList; import java.util.List; import java.util.regex.Matcher; -import java.util.stream.Collectors; +import java.util.regex.Pattern; +import java.util.stream.Stream; /** * Logical representation of an HTTP compliant If-None-Match header. */ -public class IfNoneMatch { +public record IfNoneMatch(boolean isWildcard, @Nonnull List eTags) { - private final boolean wildcard; + protected static Pattern ETAG_PATTERN = Pattern.compile("(W/)?\"([^\"]*)\""); - private final List etags; + public IfNoneMatch(List etags) { + this(false, etags); + } - /** - * Parses a non wildcard If-None-Match header value into ETags - * @param header the header to parse, eg `W\"etag1", "etag2,with,comma", W\"etag3"` - * @return the header parsed into raw string etag values. For the example given, ["W\"etag1"", ""etag2,with,comma"", "W\"etag3""] - */ - private static List parseHeaderIntoParts(String header) { - header = header.trim(); - Matcher matcher = ETag.ETAG_PATTERN.matcher(header); + public IfNoneMatch { + if (isWildcard && !eTags.isEmpty()) { + // if the header is a wildcard, it must not be constructed with + // any etags, if it is not a wildcard, it can still contain no ETags, so + // the converse is not true + throw new IllegalArgumentException( + "Invalid representation for If-None-Match header. If-None-Match cannot contain ETags if it takes " + + "the wildcard value '*'"); + } + + for (String etag : eTags) { + Matcher matcher = ETAG_PATTERN.matcher(etag); - return matcher.results() - .map(result -> result.group(0)) - .collect(Collectors.toList()); + if (!matcher.matches()) { + throw new IllegalArgumentException("Invalid ETag representation: " + etag); + } + } } /** @@ -64,53 +71,42 @@ public static IfNoneMatch fromHeader(String rawValue) { if (rawValue.equals("*")) { return IfNoneMatch.wildcard(); } else { - List parts = parseHeaderIntoParts(rawValue); - List etags = parts.stream().map(ETag::fromHeader).toList(); - // If we have no etags parsed, but the raw value of the header contained some content, - // that means there were one or more invalid parts in the header - if (etags.isEmpty() && !rawValue.isEmpty()) { - throw new IllegalArgumentException("Invalid representation for If-None-Match header."); + List parts = Stream.of(rawValue.split("\\s+")) // Tokenizes string , eg. we will now have [`W/"etag1",`, `W/"etag2"`] + .map(s -> s.endsWith(",") ? s.substring(0, s.length() - 1) : s) // Remove trailing comma from each part, so we now have [`W/"etag1"`, `W/"etag2"`] + .toList(); + + // ensure all of the individual tags are valid + boolean allValid = parts.stream().allMatch(s -> { + Matcher matcher = ETAG_PATTERN.matcher(s); + return matcher.matches(); + }); + + if (!allValid) { + throw new IllegalArgumentException("Invalid If-None-Match value provided: " + rawValue); } - return new IfNoneMatch(etags); + + return new IfNoneMatch(parts); } } /** - * Constructs a new wildcard If-None-Match header + * Constructs a new wildcard If-None-Match header * * @return */ public static IfNoneMatch wildcard() { return new IfNoneMatch(true, List.of()); } - private IfNoneMatch(boolean wildcard, List etags) { - this.wildcard = wildcard; - this.etags = etags; - } - - public IfNoneMatch(List etags) { - this.wildcard = false; - this.etags = new ArrayList<>(etags); - } - - public boolean isWildcard() { - return wildcard; - } - - public List getEtags() { - return ImmutableList.copyOf(etags); - } - /** * If any contained ETag matches the provided etag or the header is a wildcard. - * Only matches weak etags to weak etags and strong etags to strong etags. - * @param etag the etag to compare against. + * Only matches weak eTags to weak eTags and strong eTags to strong eTags. + * @param eTag the etag to compare against. * @return true if any of the contained ETags match the provided etag */ - public boolean anyMatch(ETag etag) { - if (wildcard) return true; - return etags.contains(etag); + public boolean anyMatch(String eTag) { + if (this.isWildcard) return true; + return eTags.contains(eTag); } } diff --git a/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java b/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java deleted file mode 100644 index a2de57ef6..000000000 --- a/service/common/src/test/java/org/apache/polaris/service/http/ETagTest.java +++ /dev/null @@ -1,59 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.polaris.service.http; - -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; - -public class ETagTest { - - @Test - public void validStrongETagRepresentationIsCorrect() { - String rawValue = "\"strong-etag\""; - ETag eTag = ETag.fromHeader(rawValue); - - Assertions.assertFalse(eTag.isWeak()); - Assertions.assertEquals(rawValue, eTag.toString()); - Assertions.assertEquals("strong-etag", eTag.value()); - } - - @Test - public void validWeakETagRepresentationIsCorrect() { - String rawValue = "W/\"weak-etag\""; - ETag eTag = ETag.fromHeader(rawValue); - - Assertions.assertTrue(eTag.isWeak()); - Assertions.assertEquals(rawValue, eTag.toString()); - Assertions.assertEquals("weak-etag", eTag.value()); - } - - @Test - public void invalidETagRepresentationThrowsException() { - String rawValue = "clearly_invalid_etag"; - Assertions.assertThrows(IllegalArgumentException.class, () -> ETag.fromHeader(rawValue)); - } - - @Test - public void eTagWithQuotesInValueThrowsException() { - String rawValue = "W/\"etag\"with_quote\""; - Assertions.assertThrows(IllegalArgumentException.class, () -> ETag.fromHeader(rawValue)); - } - -} diff --git a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java index f6c4d885f..000adaa04 100644 --- a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java @@ -22,6 +22,8 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import java.util.List; + public class IfNoneMatchTest { @Test @@ -29,10 +31,9 @@ public void validSingleETag() { String header = "W/\"value\""; IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader(header); - ETag parsedETag = ifNoneMatch.getEtags().getFirst(); + String parsedETag = ifNoneMatch.eTags().getFirst(); - Assertions.assertEquals("value", parsedETag.value()); - Assertions.assertTrue(parsedETag.isWeak()); + Assertions.assertEquals(header, parsedETag); } @Test @@ -44,28 +45,31 @@ public void validMultipleETags() { String header = etagValue1 + ", " + etagValue2 + ", " + etagValue3; IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader(header); - Assertions.assertEquals(3, ifNoneMatch.getEtags().size()); + Assertions.assertEquals(3, ifNoneMatch.eTags().size()); - ETag etag1 = ifNoneMatch.getEtags().get(0); - ETag etag2 = ifNoneMatch.getEtags().get(1); - ETag etag3 = ifNoneMatch.getEtags().get(2); + String etag1 = ifNoneMatch.eTags().get(0); + String etag2 = ifNoneMatch.eTags().get(1); + String etag3 = ifNoneMatch.eTags().get(2); - Assertions.assertEquals(etagValue1, etag1.toString()); - Assertions.assertEquals(etagValue2, etag2.toString()); - Assertions.assertEquals(etagValue3, etag3.toString()); + Assertions.assertEquals(etagValue1, etag1); + Assertions.assertEquals(etagValue2, etag2); + Assertions.assertEquals(etagValue3, etag3); } @Test public void validWildcardIfNoneMatch() { IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("*"); Assertions.assertTrue(ifNoneMatch.isWildcard()); - Assertions.assertTrue(ifNoneMatch.getEtags().isEmpty()); + Assertions.assertTrue(ifNoneMatch.eTags().isEmpty()); } @Test - public void nullIfNoneMatchIsValid() { + public void nullIfNoneMatchIsValidAndMapsToEmptyETags() { + // this test is important because we may not know what the representation of + // the header will be if it is not provided. If it is null, then we should default + // to an empty list of etags, as that has no footprint for logical decisions to be made IfNoneMatch nullIfNoneMatch = IfNoneMatch.fromHeader(null); - Assertions.assertTrue(nullIfNoneMatch.getEtags().isEmpty()); + Assertions.assertTrue(nullIfNoneMatch.eTags().isEmpty()); } @Test @@ -76,8 +80,8 @@ public void invalidETagThrowsException() { @Test public void etagsMatch() { - ETag weakETag = ETag.fromHeader("W/\"weak\""); - ETag strongETag = ETag.fromHeader("\"strong\""); + String weakETag = "W/\"weak\""; + String strongETag = "\"strong\""; IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("W/\"weak\", \"strong\""); Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); @@ -85,22 +89,22 @@ public void etagsMatch() { @Test public void weakETagOnlyMatchesWeak() { - ETag weakETag = ETag.fromHeader("W/\"etag\""); + String weakETag = "W/\"etag\""; IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("\"etag\""); Assertions.assertFalse(ifNoneMatch.anyMatch(weakETag)); } @Test public void strongETagOnlyMatchesStrong() { - ETag strongETag = ETag.fromHeader("\"etag\""); + String strongETag = "\"etag\""; IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("W/\"etag\""); Assertions.assertFalse(ifNoneMatch.anyMatch(strongETag)); } @Test public void wildCardMatchesEverything() { - ETag strongETag = ETag.fromHeader("\"etag\""); - ETag weakETag = ETag.fromHeader("W/\"etag\""); + String strongETag = "\"etag\""; + String weakETag = "W/\"etag\""; IfNoneMatch ifNoneMatch = IfNoneMatch.fromHeader("*"); Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); @@ -110,5 +114,15 @@ public void wildCardMatchesEverything() { Assertions.assertTrue(canonicallyBuiltWildcard.anyMatch(weakETag)); } + @Test + public void cantConstructHeaderWithWildcardAndNonEmptyETag() { + Assertions.assertThrows(IllegalArgumentException.class, () -> new IfNoneMatch(true, List.of("\"etag\""))); + } + + @Test + public void cantConstructHeaderWithOneValidAndOneInvalidPart() { + Assertions.assertThrows(IllegalArgumentException.class, + () -> IfNoneMatch.fromHeader("W/\"etag\" W/invalid-etag")); + } } From 3b4e55351969faccc3a4883991a565d49f55014a Mon Sep 17 00:00:00 2001 From: mansehajsingh Date: Tue, 4 Mar 2025 10:25:10 -0800 Subject: [PATCH 10/10] fixed if none match parsing --- .../polaris/service/http/IfNoneMatch.java | 54 ++++++++++--------- .../polaris/service/http/IfNoneMatchTest.java | 10 +++- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java index 338e9766a..488684d8f 100644 --- a/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java +++ b/service/common/src/main/java/org/apache/polaris/service/http/IfNoneMatch.java @@ -22,16 +22,32 @@ import jakarta.annotation.Nonnull; import java.util.List; +import java.util.regex.MatchResult; import java.util.regex.Matcher; import java.util.regex.Pattern; -import java.util.stream.Stream; /** * Logical representation of an HTTP compliant If-None-Match header. */ public record IfNoneMatch(boolean isWildcard, @Nonnull List eTags) { - protected static Pattern ETAG_PATTERN = Pattern.compile("(W/)?\"([^\"]*)\""); + private static final String WILDCARD_HEADER_VALUE = "*"; + + // Matches but does not capture any content of an ETag + private static final String ETAG_REGEX= "(?:W/)?\"(?:[^\"]*)\""; + + // Builds comma separated list of ETags and disallows trailing comma + private static final String IF_NONE_MATCH_REGEX = String.format("(?:%s, )*%s", ETAG_REGEX, ETAG_REGEX); + + // Wraps pattern in capture group to capture entire ETag + private static final Pattern ETAG_PATTERN = Pattern.compile(String.format("(%s)", ETAG_REGEX)); + + // Used to validate entire header pattern + private static final Pattern IF_NONE_MATCH_PATTERN = Pattern.compile(IF_NONE_MATCH_REGEX); + + public static final IfNoneMatch EMPTY = new IfNoneMatch(List.of()); + + public static final IfNoneMatch WILDCARD = new IfNoneMatch(true, List.of()); public IfNoneMatch(List etags) { this(false, etags); @@ -64,38 +80,24 @@ public IfNoneMatch(List etags) { public static IfNoneMatch fromHeader(String rawValue) { // parse null header as an empty header if (rawValue == null) { - return new IfNoneMatch(List.of()); + return EMPTY; } rawValue = rawValue.trim(); - if (rawValue.equals("*")) { - return IfNoneMatch.wildcard(); + if (rawValue.equals(WILDCARD_HEADER_VALUE)) { + return WILDCARD; } else { - List parts = Stream.of(rawValue.split("\\s+")) // Tokenizes string , eg. we will now have [`W/"etag1",`, `W/"etag2"`] - .map(s -> s.endsWith(",") ? s.substring(0, s.length() - 1) : s) // Remove trailing comma from each part, so we now have [`W/"etag1"`, `W/"etag2"`] - .toList(); - - // ensure all of the individual tags are valid - boolean allValid = parts.stream().allMatch(s -> { - Matcher matcher = ETAG_PATTERN.matcher(s); - return matcher.matches(); - }); - - if (!allValid) { - throw new IllegalArgumentException("Invalid If-None-Match value provided: " + rawValue); + // ensure entire header matches expected pattern + if (!IF_NONE_MATCH_PATTERN.matcher(rawValue).matches()) { + throw new IllegalArgumentException("Invalid If-None-Match header: " + rawValue); } - return new IfNoneMatch(parts); - } - } + // extract out ETags using the capture group + List etags = ETAG_PATTERN.matcher(rawValue).results().map(MatchResult::group).toList(); - /** - * Constructs a new wildcard If-None-Match header * - * @return - */ - public static IfNoneMatch wildcard() { - return new IfNoneMatch(true, List.of()); + return new IfNoneMatch(etags); + } } /** diff --git a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java index 000adaa04..a962f9515 100644 --- a/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/http/IfNoneMatchTest.java @@ -109,7 +109,7 @@ public void wildCardMatchesEverything() { Assertions.assertTrue(ifNoneMatch.anyMatch(strongETag)); Assertions.assertTrue(ifNoneMatch.anyMatch(weakETag)); - IfNoneMatch canonicallyBuiltWildcard = IfNoneMatch.wildcard(); + IfNoneMatch canonicallyBuiltWildcard = IfNoneMatch.WILDCARD; Assertions.assertTrue(canonicallyBuiltWildcard.anyMatch(strongETag)); Assertions.assertTrue(canonicallyBuiltWildcard.anyMatch(weakETag)); } @@ -122,7 +122,13 @@ public void cantConstructHeaderWithWildcardAndNonEmptyETag() { @Test public void cantConstructHeaderWithOneValidAndOneInvalidPart() { Assertions.assertThrows(IllegalArgumentException.class, - () -> IfNoneMatch.fromHeader("W/\"etag\" W/invalid-etag")); + () -> IfNoneMatch.fromHeader("W/\"etag\", W/invalid-etag")); + } + + @Test + public void invalidHeaderWithOnlyWhitespacesBetween() { + Assertions.assertThrows(IllegalArgumentException.class, + () -> IfNoneMatch.fromHeader("W/\"etag\" \"valid-etag\"")); } }