From 95ac203ebd516023b05e9f9d0ebb192b9e4191ca Mon Sep 17 00:00:00 2001 From: Honah J Date: Tue, 1 Apr 2025 23:00:29 -0500 Subject: [PATCH 1/6] Add CRUD operations and ListPolicy --- api/polaris-catalog-service/build.gradle.kts | 5 +- .../service/types/PolicyIdentifier.java | 122 ++++ .../exceptions/NoSuchPolicyException.java | 32 + .../PolicyVersionMismatchException.java | 31 + .../quarkus/catalog/PolicyCatalogTest.java | 562 ++++++++++++++++++ service/common/build.gradle.kts | 1 + .../service/catalog/policy/PolicyCatalog.java | 334 +++++++++++ .../exception/PolarisExceptionMapper.java | 6 + .../PolarisPassthroughResolutionView.java | 16 + 9 files changed, 1108 insertions(+), 1 deletion(-) create mode 100644 api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java create mode 100644 polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/NoSuchPolicyException.java create mode 100644 polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyVersionMismatchException.java create mode 100644 quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java create mode 100644 service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java diff --git a/api/polaris-catalog-service/build.gradle.kts b/api/polaris-catalog-service/build.gradle.kts index 2fd861d80b..5f0a8134fd 100644 --- a/api/polaris-catalog-service/build.gradle.kts +++ b/api/polaris-catalog-service/build.gradle.kts @@ -36,7 +36,6 @@ val policyManagementModels = "CatalogIdentifier", "CreatePolicyRequest", "LoadPolicyResponse", - "PolicyIdentifier", "Policy", "PolicyAttachmentTarget", "AttachPolicyRequest", @@ -59,6 +58,7 @@ dependencies { implementation(libs.jakarta.inject.api) implementation(libs.jakarta.validation.api) implementation(libs.swagger.annotations) + implementation(libs.guava) implementation(libs.jakarta.servlet.api) implementation(libs.jakarta.ws.rs.api) @@ -103,6 +103,9 @@ openApiGenerate { "ErrorModel" to "org.apache.iceberg.rest.responses.ErrorResponse", "IcebergErrorResponse" to "org.apache.iceberg.rest.responses.ErrorResponse", "TableIdentifier" to "org.apache.iceberg.catalog.TableIdentifier", + + // Custom types defined below + "PolicyIdentifier" to "org.apache.polaris.service.types.PolicyIdentifier", ) } diff --git a/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java b/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java new file mode 100644 index 0000000000..ba504e73ac --- /dev/null +++ b/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java @@ -0,0 +1,122 @@ +/* + * 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.types; + +import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; +import java.util.Arrays; +import java.util.Locale; +import java.util.Objects; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; + +public class PolicyIdentifier { + + private static final Splitter DOT = Splitter.on('.'); + + private final Namespace namespace; + private final String name; + + public static PolicyIdentifier of(String... names) { + Preconditions.checkArgument(names != null, "Cannot create policy identifier from null array"); + Preconditions.checkArgument(names.length > 0, "Cannot create policy identifier without a name"); + return new PolicyIdentifier( + Namespace.of(Arrays.copyOf(names, names.length - 1)), names[names.length - 1]); + } + + public static PolicyIdentifier of(Namespace namespace, String name) { + return new PolicyIdentifier(namespace, name); + } + + public static PolicyIdentifier parse(String identifier) { + Preconditions.checkArgument(identifier != null, "Cannot parse policy identifier: null"); + Iterable parts = DOT.split(identifier); + return PolicyIdentifier.of(Iterables.toArray(parts, String.class)); + } + + public static PolicyIdentifier fromTableIdentifier(TableIdentifier tableIdentifier) { + return new PolicyIdentifier(tableIdentifier.namespace(), tableIdentifier.name()); + } + + private PolicyIdentifier(Namespace namespace, String name) { + Preconditions.checkArgument(name != null && !name.isEmpty(), "Invalid name: null or empty"); + Preconditions.checkArgument(namespace != null, "Invalid Namespace: null"); + this.namespace = namespace; + this.name = name; + } + + /** + * Whether the namespace is empty. + * + * @return true if the namespace is not empty, false otherwise + */ + public boolean hasNamespace() { + return !namespace.isEmpty(); + } + + /** Returns the identifier namespace. */ + public Namespace namespace() { + return namespace; + } + + /** Returns the identifier name. */ + public String name() { + return name; + } + + public TableIdentifier toTableIdentifier() { + return TableIdentifier.of(namespace, name); + } + + public PolicyIdentifier toLowerCase() { + String[] newLevels = + Arrays.stream(namespace().levels()).map(String::toLowerCase).toArray(String[]::new); + String newName = name().toLowerCase(Locale.ROOT); + return PolicyIdentifier.of(Namespace.of(newLevels), newName); + } + + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + + if (other == null || getClass() != other.getClass()) { + return false; + } + + PolicyIdentifier that = (PolicyIdentifier) other; + return namespace.equals(that.namespace) && name.equals(that.name); + } + + @Override + public int hashCode() { + return Objects.hash(namespace, name); + } + + @Override + public String toString() { + if (hasNamespace()) { + return namespace.toString() + "." + name; + } else { + return name; + } + } +} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/NoSuchPolicyException.java b/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/NoSuchPolicyException.java new file mode 100644 index 0000000000..642bd36adb --- /dev/null +++ b/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/NoSuchPolicyException.java @@ -0,0 +1,32 @@ +/* + * 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.policy.exceptions; + +import org.apache.polaris.core.exceptions.PolarisException; + +public class NoSuchPolicyException extends PolarisException { + + public NoSuchPolicyException(String message) { + super(message); + } + + public NoSuchPolicyException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyVersionMismatchException.java b/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyVersionMismatchException.java new file mode 100644 index 0000000000..c2f8f8f78e --- /dev/null +++ b/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyVersionMismatchException.java @@ -0,0 +1,31 @@ +/* + * 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.policy.exceptions; + +import org.apache.polaris.core.exceptions.PolarisException; + +public class PolicyVersionMismatchException extends PolarisException { + public PolicyVersionMismatchException(String message) { + super(message); + } + + public PolicyVersionMismatchException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java new file mode 100644 index 0000000000..0c79828cc3 --- /dev/null +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java @@ -0,0 +1,562 @@ +/* + * 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.quarkus.catalog; + +import static org.apache.iceberg.types.Types.NestedField.required; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.when; + +import com.google.common.collect.ImmutableMap; +import io.quarkus.test.junit.QuarkusMock; +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.junit.QuarkusTestProfile; +import io.quarkus.test.junit.TestProfile; +import jakarta.inject.Inject; +import jakarta.ws.rs.core.SecurityContext; +import java.io.IOException; +import java.lang.reflect.Method; +import java.time.Clock; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Supplier; +import org.apache.commons.lang3.NotImplementedException; +import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.Schema; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.AlreadyExistsException; +import org.apache.iceberg.types.Types; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; +import org.apache.polaris.core.admin.model.StorageConfigInfo; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.auth.PolarisAuthorizerImpl; +import org.apache.polaris.core.config.FeatureConfiguration; +import org.apache.polaris.core.config.PolarisConfigurationStore; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.entity.PrincipalEntity; +import org.apache.polaris.core.persistence.MetaStoreManagerFactory; +import org.apache.polaris.core.persistence.PolarisEntityManager; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet; +import org.apache.polaris.core.persistence.cache.EntityCache; +import org.apache.polaris.core.persistence.dao.entity.BaseResult; +import org.apache.polaris.core.persistence.dao.entity.PrincipalSecretsResult; +import org.apache.polaris.core.persistence.transactional.TransactionalPersistence; +import org.apache.polaris.core.policy.PredefinedPolicyTypes; +import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; +import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; +import org.apache.polaris.core.policy.validator.InvalidPolicyException; +import org.apache.polaris.core.storage.PolarisStorageIntegration; +import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider; +import org.apache.polaris.core.storage.aws.AwsCredentialsStorageIntegration; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.core.storage.cache.StorageCredentialCache; +import org.apache.polaris.service.admin.PolarisAdminService; +import org.apache.polaris.service.catalog.PolarisPassthroughResolutionView; +import org.apache.polaris.service.catalog.iceberg.IcebergCatalog; +import org.apache.polaris.service.catalog.io.DefaultFileIOFactory; +import org.apache.polaris.service.catalog.io.FileIOFactory; +import org.apache.polaris.service.catalog.policy.PolicyCatalog; +import org.apache.polaris.service.config.RealmEntityManagerFactory; +import org.apache.polaris.service.storage.PolarisStorageIntegrationProviderImpl; +import org.apache.polaris.service.task.TaskExecutor; +import org.apache.polaris.service.types.Policy; +import org.apache.polaris.service.types.PolicyIdentifier; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.mockito.Mockito; +import software.amazon.awssdk.services.sts.StsClient; +import software.amazon.awssdk.services.sts.model.AssumeRoleRequest; +import software.amazon.awssdk.services.sts.model.AssumeRoleResponse; +import software.amazon.awssdk.services.sts.model.Credentials; + +@QuarkusTest +@TestProfile(PolicyCatalogTest.Profile.class) +public class PolicyCatalogTest { + public static class Profile implements QuarkusTestProfile { + + @Override + public Map getConfigOverrides() { + return Map.of( + "polaris.features.defaults.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"", + "true", + "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"", + "true", + "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"", + "[\"FILE\"]"); + } + } + + protected static final Namespace NS = Namespace.of("newdb"); + protected static final TableIdentifier TABLE = TableIdentifier.of(NS, "table"); + public static final String CATALOG_NAME = "polaris-catalog"; + public static final String TEST_ACCESS_KEY = "test_access_key"; + public static final String SECRET_ACCESS_KEY = "secret_access_key"; + public static final String SESSION_TOKEN = "session_token"; + + @Inject MetaStoreManagerFactory managerFactory; + @Inject PolarisConfigurationStore configurationStore; + @Inject PolarisStorageIntegrationProvider storageIntegrationProvider; + @Inject PolarisDiagnostics diagServices; + + private PolicyCatalog policyCatalog; + private IcebergCatalog icebergCatalog; + private CallContext callContext; + private AwsStorageConfigInfo storageConfigModel; + private String realmName; + private PolarisMetaStoreManager metaStoreManager; + private PolarisCallContext polarisContext; + private PolarisAdminService adminService; + private PolarisEntityManager entityManager; + private FileIOFactory fileIOFactory; + private AuthenticatedPolarisPrincipal authenticatedRoot; + private PolarisEntity catalogEntity; + private SecurityContext securityContext; + + protected static final Schema SCHEMA = + new Schema( + required(3, "id", Types.IntegerType.get(), "unique ID 🤪"), + required(4, "data", Types.StringType.get())); + + @BeforeAll + public static void setUpMocks() { + PolarisStorageIntegrationProviderImpl mock = + Mockito.mock(PolarisStorageIntegrationProviderImpl.class); + QuarkusMock.installMockForType(mock, PolarisStorageIntegrationProviderImpl.class); + } + + @BeforeEach + @SuppressWarnings("unchecked") + public void before(TestInfo testInfo) { + realmName = + "realm_%s_%s" + .formatted( + testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime()); + RealmContext realmContext = () -> realmName; + metaStoreManager = managerFactory.getOrCreateMetaStoreManager(realmContext); + polarisContext = + new PolarisCallContext( + managerFactory.getOrCreateSessionSupplier(realmContext).get(), + diagServices, + configurationStore, + Clock.systemDefaultZone()); + entityManager = + new PolarisEntityManager( + metaStoreManager, new StorageCredentialCache(), new EntityCache(metaStoreManager)); + + callContext = CallContext.of(realmContext, polarisContext); + + PrincipalEntity rootEntity = + new PrincipalEntity( + PolarisEntity.of( + metaStoreManager + .readEntityByName( + polarisContext, + null, + PolarisEntityType.PRINCIPAL, + PolarisEntitySubType.NULL_SUBTYPE, + "root") + .getEntity())); + + authenticatedRoot = new AuthenticatedPolarisPrincipal(rootEntity, Set.of()); + + securityContext = Mockito.mock(SecurityContext.class); + when(securityContext.getUserPrincipal()).thenReturn(authenticatedRoot); + when(securityContext.isUserInRole(isA(String.class))).thenReturn(true); + adminService = + new PolarisAdminService( + callContext, + entityManager, + metaStoreManager, + securityContext, + new PolarisAuthorizerImpl(new PolarisConfigurationStore() {})); + + String storageLocation = "s3://my-bucket/path/to/data"; + storageConfigModel = + AwsStorageConfigInfo.builder() + .setRoleArn("arn:aws:iam::012345678901:role/jdoe") + .setExternalId("externalId") + .setUserArn("aws::a:user:arn") + .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) + .setAllowedLocations(List.of(storageLocation, "s3://externally-owned-bucket")) + .build(); + catalogEntity = + adminService.createCatalog( + new CatalogEntity.Builder() + .setName(CATALOG_NAME) + .setDefaultBaseLocation(storageLocation) + .setReplaceNewLocationPrefixWithCatalogDefault("file:") + .addProperty( + FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "true") + .addProperty( + FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true") + .setStorageConfigurationInfo(storageConfigModel, storageLocation) + .build()); + + PolarisPassthroughResolutionView passthroughView = + new PolarisPassthroughResolutionView( + callContext, entityManager, securityContext, CATALOG_NAME); + TaskExecutor taskExecutor = Mockito.mock(); + RealmEntityManagerFactory realmEntityManagerFactory = + new RealmEntityManagerFactory(createMockMetaStoreManagerFactory()); + this.fileIOFactory = + new DefaultFileIOFactory(realmEntityManagerFactory, managerFactory, configurationStore); + + StsClient stsClient = Mockito.mock(StsClient.class); + when(stsClient.assumeRole(isA(AssumeRoleRequest.class))) + .thenReturn( + AssumeRoleResponse.builder() + .credentials( + Credentials.builder() + .accessKeyId(TEST_ACCESS_KEY) + .secretAccessKey(SECRET_ACCESS_KEY) + .sessionToken(SESSION_TOKEN) + .build()) + .build()); + PolarisStorageIntegration storageIntegration = + new AwsCredentialsStorageIntegration(stsClient); + when(storageIntegrationProvider.getStorageIntegrationForConfig( + isA(AwsStorageConfigurationInfo.class))) + .thenReturn((PolarisStorageIntegration) storageIntegration); + + this.policyCatalog = + new PolicyCatalog( + entityManager, + metaStoreManager, + callContext, + passthroughView, + securityContext, + taskExecutor); + this.icebergCatalog = + new IcebergCatalog( + entityManager, + metaStoreManager, + callContext, + passthroughView, + securityContext, + taskExecutor, + fileIOFactory); + this.icebergCatalog.initialize( + CATALOG_NAME, + ImmutableMap.of( + CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.inmemory.InMemoryFileIO")); + } + + @AfterEach + public void after() throws IOException { + metaStoreManager.purge(polarisContext); + } + + private MetaStoreManagerFactory createMockMetaStoreManagerFactory() { + return new MetaStoreManagerFactory() { + @Override + public PolarisMetaStoreManager getOrCreateMetaStoreManager(RealmContext realmContext) { + return metaStoreManager; + } + + @Override + public Supplier getOrCreateSessionSupplier( + RealmContext realmContext) { + return () -> ((TransactionalPersistence) polarisContext.getMetaStore()); + } + + @Override + public StorageCredentialCache getOrCreateStorageCredentialCache(RealmContext realmContext) { + return new StorageCredentialCache(); + } + + @Override + public EntityCache getOrCreateEntityCache(RealmContext realmContext) { + return new EntityCache(metaStoreManager); + } + + @Override + public Map bootstrapRealms( + Iterable realms, RootCredentialsSet rootCredentialsSet) { + throw new NotImplementedException("Bootstrapping realms is not supported"); + } + + @Override + public Map purgeRealms(Iterable realms) { + throw new NotImplementedException("Purging realms is not supported"); + } + }; + } + + @Test + public void testCreatePolicyDoesNotThrow() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + Assertions.assertThatCode( + () -> + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}")) + .doesNotThrowAnyException(); + } + + @Test + public void testCreatePolicyAlreadyExists() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + Assertions.assertThatThrownBy( + () -> + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}")) + .isInstanceOf(AlreadyExistsException.class); + + Assertions.assertThatThrownBy( + () -> + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), + "test", + "{\"enable\": false}")) + .isInstanceOf(AlreadyExistsException.class); + } + + @Test + public void testListPolicies() { + Namespace namespace = Namespace.of("ns"); + PolicyIdentifier identifer1 = PolicyIdentifier.of("ns", "p1"); + PolicyIdentifier identifer2 = PolicyIdentifier.of("ns", "p2"); + PolicyIdentifier identifer3 = PolicyIdentifier.of("ns", "p3"); + PolicyIdentifier identifer4 = PolicyIdentifier.of("ns", "p4"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + identifer1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + + policyCatalog.createPolicy( + identifer2, + PredefinedPolicyTypes.METADATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + policyCatalog.createPolicy( + identifer3, + PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), + "test", + "{\"enable\": false}"); + + policyCatalog.createPolicy( + identifer4, + PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL.getName(), + "test", + "{\"enable\": false}"); + + List listResult = policyCatalog.listPolicies(Namespace.of("ns"), null); + Assertions.assertThat(listResult).hasSize(4); + Assertions.assertThat(listResult) + .containsExactlyInAnyOrder(identifer1, identifer2, identifer3, identifer4); + } + + @Test + public void testListPoliciesFilterByPolicyType() { + Namespace namespace = Namespace.of("ns"); + PolicyIdentifier identifer1 = PolicyIdentifier.of("ns", "p1"); + PolicyIdentifier identifer2 = PolicyIdentifier.of("ns", "p2"); + PolicyIdentifier identifer3 = PolicyIdentifier.of("ns", "p3"); + PolicyIdentifier identifer4 = PolicyIdentifier.of("ns", "p4"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + identifer1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + + policyCatalog.createPolicy( + identifer2, + PredefinedPolicyTypes.METADATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + policyCatalog.createPolicy( + identifer3, + PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), + "test", + "{\"enable\": false}"); + + policyCatalog.createPolicy( + identifer4, + PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL.getName(), + "test", + "{\"enable\": false}"); + + List listResult = + policyCatalog.listPolicies(Namespace.of("ns"), PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL); + Assertions.assertThat(listResult).hasSize(1); + Assertions.assertThat(listResult).containsExactlyInAnyOrder(identifer4); + } + + @Test + public void testLoadPolicy() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + Policy policy = policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1")); + Assertions.assertThat(policy.getVersion()).isEqualTo(0); + Assertions.assertThat(policy.getPolicyType()) + .isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); + Assertions.assertThat(policy.getContent()).isEqualTo("{\"enable\": false}"); + Assertions.assertThat(policy.getName()).isEqualTo("p1"); + Assertions.assertThat(policy.getDescription()).isEqualTo("test"); + } + + @Test + public void testCreatePolicyWithInvalidContent() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + + Assertions.assertThatThrownBy( + () -> + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "invalid")) + .isInstanceOf(InvalidPolicyException.class); + } + + @Test + public void testLoadPolicyNotExist() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + + Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1"))) + .isInstanceOf(NoSuchPolicyException.class); + } + + @Test + public void testUpdatePolicy() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + policyCatalog.updatePolicy(PolicyIdentifier.of("ns", "p1"), "updated", "{\"enable\": true}", 0); + + Policy policy = policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1")); + Assertions.assertThat(policy.getVersion()).isEqualTo(1); + Assertions.assertThat(policy.getPolicyType()) + .isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); + Assertions.assertThat(policy.getContent()).isEqualTo("{\"enable\": true}"); + Assertions.assertThat(policy.getName()).isEqualTo("p1"); + Assertions.assertThat(policy.getDescription()).isEqualTo("updated"); + } + + @Test + public void testUpdatePolicyWithWrongVersion() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + Assertions.assertThatThrownBy( + () -> + policyCatalog.updatePolicy( + PolicyIdentifier.of("ns", "p1"), "updated", "{\"enable\": true}", 1)) + .isInstanceOf(PolicyVersionMismatchException.class); + } + + @Test + public void testUpdatePolicyWithInvalidContent() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + Assertions.assertThatThrownBy( + () -> + policyCatalog.updatePolicy( + PolicyIdentifier.of("ns", "p1"), "updated", "invalid", 0)) + .isInstanceOf(InvalidPolicyException.class); + } + + @Test + public void testUpdatePolicyNotExist() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + + Assertions.assertThatThrownBy( + () -> + policyCatalog.updatePolicy( + PolicyIdentifier.of("ns", "p1"), "updated", "{\"enable\": true}", 0)) + .isInstanceOf(NoSuchPolicyException.class); + } + + @Test + public void testDropPolicy() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + policyCatalog.createPolicy( + PolicyIdentifier.of("ns", "p1"), + PredefinedPolicyTypes.DATA_COMPACTION.getName(), + "test", + "{\"enable\": false}"); + + Assertions.assertThat(policyCatalog.dropPolicy(PolicyIdentifier.of("ns", "p1"), false)) + .isTrue(); + Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1"))) + .isInstanceOf(NoSuchPolicyException.class); + } + + @Test + public void testDropPolicyNotExist() { + Namespace namespace = Namespace.of("ns"); + icebergCatalog.createNamespace(namespace); + + Assertions.assertThatThrownBy( + () -> policyCatalog.dropPolicy(PolicyIdentifier.of("ns", "p1"), false)) + .isInstanceOf(NoSuchPolicyException.class); + } +} diff --git a/service/common/build.gradle.kts b/service/common/build.gradle.kts index fec8f12c74..ca00893e07 100644 --- a/service/common/build.gradle.kts +++ b/service/common/build.gradle.kts @@ -102,6 +102,7 @@ dependencies { testFixturesImplementation(project(":polaris-api-management-model")) testFixturesImplementation(project(":polaris-api-management-service")) testFixturesImplementation(project(":polaris-api-iceberg-service")) + testFixturesImplementation(project(":polaris-api-catalog-service")) testFixturesImplementation(libs.jakarta.enterprise.cdi.api) testFixturesImplementation(libs.jakarta.annotation.api) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java new file mode 100644 index 0000000000..19d41b9b55 --- /dev/null +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -0,0 +1,334 @@ +/* + * 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.policy; + +import jakarta.ws.rs.core.SecurityContext; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.exceptions.AlreadyExistsException; +import org.apache.iceberg.exceptions.BadRequestException; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.catalog.PolarisCatalogHelpers; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.persistence.PolarisEntityManager; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; +import org.apache.polaris.core.persistence.dao.entity.DropEntityResult; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; +import org.apache.polaris.core.policy.PolicyEntity; +import org.apache.polaris.core.policy.PolicyType; +import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; +import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; +import org.apache.polaris.core.policy.validator.PolicyValidators; +import org.apache.polaris.service.task.TaskExecutor; +import org.apache.polaris.service.types.Policy; +import org.apache.polaris.service.types.PolicyIdentifier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class PolicyCatalog { + private static final Logger LOGGER = LoggerFactory.getLogger(PolicyCatalog.class); + + private final PolarisEntityManager entityManager; + private final CallContext callContext; + private final PolarisResolutionManifestCatalogView resolvedEntityView; + private final CatalogEntity catalogEntity; + private final TaskExecutor taskExecutor; + private final SecurityContext securityContext; + private final String catalogName; + private long catalogId = -1; + private PolarisMetaStoreManager metaStoreManager; + + public PolicyCatalog( + PolarisEntityManager entityManager, + PolarisMetaStoreManager metaStoreManager, + CallContext callContext, + PolarisResolutionManifestCatalogView resolvedEntityView, + SecurityContext securityContext, + TaskExecutor taskExecutor) { + // TODO: clean out unecessary fields + this.entityManager = entityManager; + this.callContext = callContext; + this.resolvedEntityView = resolvedEntityView; + this.catalogEntity = + CatalogEntity.of(resolvedEntityView.getResolvedReferenceCatalogEntity().getRawLeafEntity()); + this.securityContext = securityContext; + this.taskExecutor = taskExecutor; + this.catalogId = catalogEntity.getId(); + this.catalogName = catalogEntity.getName(); + this.metaStoreManager = metaStoreManager; + } + + public List listPolicies(Namespace namespace, PolicyType policyType) { + PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(namespace); + if (resolvedEntities == null) { + throw new IllegalStateException( + String.format("Failed to fetch resolved namespace '%s'", namespace)); + } + + List catalogPath = resolvedEntities.getRawFullPath(); + List policyEntities = + getMetaStoreManager() + .listEntities( + getCurrentPolarisContext(), + PolarisEntity.toCoreList(catalogPath), + PolarisEntityType.POLICY, + PolarisEntitySubType.ANY_SUBTYPE) + .getEntities() + .stream() + .map( + polarisEntityActiveRecord -> + PolicyEntity.of( + getMetaStoreManager() + .loadEntity( + getCurrentPolarisContext(), + polarisEntityActiveRecord.getCatalogId(), + polarisEntityActiveRecord.getId(), + polarisEntityActiveRecord.getType()) + .getEntity())) + .filter( + policyEntity -> policyType == null || policyEntity.getPolicyType() == policyType) + .toList(); + + List entities = + policyEntities.stream().map(PolarisEntity::nameAndId).toList(); + + return PolarisCatalogHelpers.nameAndIdToTableIdentifiers(catalogPath, entities).stream() + .map(PolicyIdentifier::fromTableIdentifier) + .toList(); + } + + public Policy createPolicy( + PolicyIdentifier policyIdentifier, String type, String description, String content) { + PolarisResolvedPathWrapper resolvedParent = + resolvedEntityView.getResolvedPath(policyIdentifier.namespace()); + if (resolvedParent == null) { + // Illegal state because the namespace should've already been in the static resolution set. + throw new IllegalStateException( + String.format( + "Failed to fetch resolved parent for PolicyIdentifier '%s'", policyIdentifier)); + } + PolarisResolvedPathWrapper resolvedPolicyEntities = + resolvedEntityView.getPassthroughResolvedPath( + policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); + + PolicyEntity entity = + PolicyEntity.of( + resolvedPolicyEntities == null ? null : resolvedPolicyEntities.getRawLeafEntity()); + + if (entity == null) { + PolicyType policyType = PolicyType.fromName(type); + if (policyType == null) { + throw new BadRequestException("Unknown policy type: %s", type); + } + + entity = + new PolicyEntity.Builder( + policyIdentifier.namespace(), policyIdentifier.name(), policyType) + .setCatalogId(catalogEntity.getId()) + .setDescription(description) + .setContent(content) + .setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()) + .build(); + + PolicyValidators.validate(entity); + + } else { + throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); + } + + return constructPolicy(createPolicyEntity(policyIdentifier, entity)); + } + + public Policy loadPolicy(PolicyIdentifier policyIdentifier) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath( + policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); + + PolicyEntity policy = null; + + if (resolvedEntities != null) { + if (resolvedEntities.getRawLeafEntity().getType() == PolarisEntityType.POLICY) { + policy = PolicyEntity.of(resolvedEntities.getRawLeafEntity()); + } + } + + if (policy == null) { + throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); + } + return constructPolicy(policy); + } + + public Policy updatePolicy( + PolicyIdentifier policyIdentifier, + String newDescription, + String newContent, + int currentPolicyVersion) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath( + policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); + + PolicyEntity policy = null; + + if (resolvedEntities != null) { + if (resolvedEntities.getRawLeafEntity().getType() == PolarisEntityType.POLICY) { + policy = PolicyEntity.of(resolvedEntities.getRawLeafEntity()); + } + } + + if (policy == null) { + throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); + } + + PolicyEntity.Builder newPolicyBuilder = new PolicyEntity.Builder(policy); + int policyVersion = policy.getPolicyVersion(); + if (currentPolicyVersion != policyVersion) { + throw new PolicyVersionMismatchException( + String.format("Policy version mismatch. Current version is %d", policyVersion)); + } + boolean hasUpdate = false; + if (newContent != null) { + newPolicyBuilder.setContent(newContent); + hasUpdate = true; + } + + if (newDescription != null) { + newPolicyBuilder.setDescription(newDescription); + hasUpdate = true; + } + + if (!hasUpdate) { + return constructPolicy(policy); + } + + newPolicyBuilder.setPolicyVersion(policyVersion + 1); + PolicyEntity newPolicyEntity = newPolicyBuilder.build(); + PolicyValidators.validate(newPolicyEntity); + newPolicyEntity = PolicyEntity.of(updatePolicy(policyIdentifier, newPolicyEntity)); + + return constructPolicy(newPolicyEntity); + } + + public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getPassthroughResolvedPath( + policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); + if (resolvedEntities == null) { + throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); + } + + List catalogPath = resolvedEntities.getRawParentPath(); + PolarisEntity leafEntity = resolvedEntities.getRawLeafEntity(); + + DropEntityResult dropEntityResult = + getMetaStoreManager() + .dropEntityIfExists( + getCurrentPolarisContext(), + PolarisEntity.toCoreList(catalogPath), + leafEntity, + Map.of(), + false); + + return dropEntityResult.isSuccess(); + } + + private PolicyEntity createPolicyEntity(PolicyIdentifier identifier, PolarisEntity entity) { + PolarisResolvedPathWrapper resolvedParent = + resolvedEntityView.getResolvedPath(identifier.namespace()); + if (resolvedParent == null) { + // Illegal state because the namespace should've already been in the static resolution set. + throw new IllegalStateException( + String.format("Failed to fetch resolved parent for Policy '%s'", identifier)); + } + + List catalogPath = resolvedParent.getRawFullPath(); + if (entity.getParentId() <= 0) { + // TODO: Validate catalogPath size is at least 1 for catalog entity? + entity = + new PolarisEntity.Builder(entity) + .setParentId(resolvedParent.getRawLeafEntity().getId()) + .build(); + } + + entity = + new PolarisEntity.Builder(entity).setCreateTimestamp(System.currentTimeMillis()).build(); + + PolarisEntity returnedEntity = + PolarisEntity.of( + getMetaStoreManager() + .createEntityIfNotExists( + getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity)); + + LOGGER.debug("Created Policy entity {} with Identifier {}", entity, identifier); + if (returnedEntity == null) { + throw new IllegalStateException("Failed to create Policy entity"); + } + + return PolicyEntity.of(returnedEntity); + } + + private PolarisEntity updatePolicy(PolicyIdentifier identifier, PolarisEntity entity) { + PolarisResolvedPathWrapper resolvedEntities = + resolvedEntityView.getResolvedPath(identifier, entity.getType(), entity.getSubType()); + if (resolvedEntities == null) { + throw new IllegalStateException( + String.format("Failed to fetch resolved PolicyIdentifier '%s'", identifier)); + } + + List catalogPath = resolvedEntities.getRawParentPath(); + PolarisEntity returnedEntity = + Optional.ofNullable( + getMetaStoreManager() + .updateEntityPropertiesIfNotChanged( + getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity) + .getEntity()) + .map(PolarisEntity::new) + .orElse(null); + if (returnedEntity == null) { + throw new IllegalStateException("Failed to update Policy entity"); + } + + return returnedEntity; + } + + private PolarisMetaStoreManager getMetaStoreManager() { + return metaStoreManager; + } + + private PolarisCallContext getCurrentPolarisContext() { + return callContext.getPolarisCallContext(); + } + + private static Policy constructPolicy(PolicyEntity policyEntity) { + return Policy.builder() + .setPolicyType(policyEntity.getPolicyType().getName()) + .setInheritable(policyEntity.getPolicyType().isInheritable()) + .setName(policyEntity.getName()) + .setDescription(policyEntity.getDescription()) + .setContent(policyEntity.getContent()) + .setVersion(policyEntity.getPolicyVersion()) + .build(); + } +} diff --git a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java index 0636f5b97b..22b9e187db 100644 --- a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java @@ -25,6 +25,8 @@ import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.polaris.core.exceptions.AlreadyExistsException; import org.apache.polaris.core.exceptions.PolarisException; +import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; +import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; import org.apache.polaris.core.policy.validator.InvalidPolicyException; import org.apache.polaris.service.context.UnresolvableRealmContextException; import org.slf4j.Logger; @@ -47,6 +49,10 @@ private Response.Status getStatus(PolarisException exception) { return Response.Status.NOT_FOUND; } else if (exception instanceof InvalidPolicyException) { return Response.Status.BAD_REQUEST; + } else if (exception instanceof NoSuchPolicyException) { + return Response.Status.NOT_FOUND; + } else if (exception instanceof PolicyVersionMismatchException) { + return Response.Status.CONFLICT; } else { return Response.Status.INTERNAL_SERVER_ERROR; } diff --git a/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java b/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java index 8c98e910b1..87b0696a52 100644 --- a/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java +++ b/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java @@ -31,6 +31,7 @@ import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; import org.apache.polaris.core.persistence.resolver.ResolverPath; +import org.apache.polaris.service.types.PolicyIdentifier; /** * For test purposes or for elevated-privilege scenarios where entity resolution is allowed to @@ -94,6 +95,14 @@ public PolarisResolvedPathWrapper getResolvedPath( identifier); manifest.resolveAll(); return manifest.getResolvedPath(identifier, entityType, subType); + } else if (key instanceof PolicyIdentifier identifier) { + manifest.addPath( + new ResolverPath( + PolarisCatalogHelpers.tableIdentifierToList(identifier.toTableIdentifier()), + entityType), + identifier); + manifest.resolveAll(); + return manifest.getResolvedPath(identifier, entityType, subType); } else { throw new IllegalStateException( String.format( @@ -130,6 +139,13 @@ public PolarisResolvedPathWrapper getPassthroughResolvedPath( new ResolverPath(PolarisCatalogHelpers.tableIdentifierToList(identifier), entityType), identifier); return manifest.getPassthroughResolvedPath(identifier, entityType, subType); + } else if (key instanceof PolicyIdentifier policyIdentifier) { + manifest.addPassthroughPath( + new ResolverPath( + PolarisCatalogHelpers.tableIdentifierToList(policyIdentifier.toTableIdentifier()), + entityType), + policyIdentifier); + return manifest.getPassthroughResolvedPath(policyIdentifier, entityType, subType); } else { throw new IllegalStateException( String.format( From 77fc063a365c1079d194e07ee5f00576a630f4d4 Mon Sep 17 00:00:00 2001 From: Honah J Date: Wed, 2 Apr 2025 13:47:36 -0500 Subject: [PATCH 2/6] Fix test and catalog --- .../quarkus/catalog/PolicyCatalogTest.java | 9 +- .../service/catalog/policy/PolicyCatalog.java | 112 ++++++------------ 2 files changed, 40 insertions(+), 81 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java index 0c79828cc3..b49e997288 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java @@ -247,14 +247,7 @@ public void before(TestInfo testInfo) { isA(AwsStorageConfigurationInfo.class))) .thenReturn((PolarisStorageIntegration) storageIntegration); - this.policyCatalog = - new PolicyCatalog( - entityManager, - metaStoreManager, - callContext, - passthroughView, - securityContext, - taskExecutor); + this.policyCatalog = new PolicyCatalog(metaStoreManager, callContext, passthroughView); this.icebergCatalog = new IcebergCatalog( entityManager, diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 19d41b9b55..163065acb6 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -18,7 +18,6 @@ */ package org.apache.polaris.service.catalog.policy; -import jakarta.ws.rs.core.SecurityContext; import java.util.List; import java.util.Map; import java.util.Optional; @@ -32,7 +31,6 @@ import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; -import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; import org.apache.polaris.core.persistence.dao.entity.DropEntityResult; @@ -42,7 +40,6 @@ import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; import org.apache.polaris.core.policy.validator.PolicyValidators; -import org.apache.polaris.service.task.TaskExecutor; import org.apache.polaris.service.types.Policy; import org.apache.polaris.service.types.PolicyIdentifier; import org.slf4j.Logger; @@ -51,36 +48,58 @@ public class PolicyCatalog { private static final Logger LOGGER = LoggerFactory.getLogger(PolicyCatalog.class); - private final PolarisEntityManager entityManager; private final CallContext callContext; private final PolarisResolutionManifestCatalogView resolvedEntityView; private final CatalogEntity catalogEntity; - private final TaskExecutor taskExecutor; - private final SecurityContext securityContext; - private final String catalogName; private long catalogId = -1; private PolarisMetaStoreManager metaStoreManager; public PolicyCatalog( - PolarisEntityManager entityManager, PolarisMetaStoreManager metaStoreManager, CallContext callContext, - PolarisResolutionManifestCatalogView resolvedEntityView, - SecurityContext securityContext, - TaskExecutor taskExecutor) { - // TODO: clean out unecessary fields - this.entityManager = entityManager; + PolarisResolutionManifestCatalogView resolvedEntityView) { this.callContext = callContext; this.resolvedEntityView = resolvedEntityView; this.catalogEntity = CatalogEntity.of(resolvedEntityView.getResolvedReferenceCatalogEntity().getRawLeafEntity()); - this.securityContext = securityContext; - this.taskExecutor = taskExecutor; this.catalogId = catalogEntity.getId(); - this.catalogName = catalogEntity.getName(); this.metaStoreManager = metaStoreManager; } + public Policy createPolicy( + PolicyIdentifier policyIdentifier, String type, String description, String content) { + PolarisResolvedPathWrapper resolvedPolicyEntities = + resolvedEntityView.getPassthroughResolvedPath( + policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); + + PolicyEntity entity = + PolicyEntity.of( + resolvedPolicyEntities == null ? null : resolvedPolicyEntities.getRawLeafEntity()); + + if (entity == null) { + PolicyType policyType = PolicyType.fromName(type); + if (policyType == null) { + throw new BadRequestException("Unknown policy type: %s", type); + } + + entity = + new PolicyEntity.Builder( + policyIdentifier.namespace(), policyIdentifier.name(), policyType) + .setCatalogId(catalogId) + .setDescription(description) + .setContent(content) + .setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()) + .build(); + + PolicyValidators.validate(entity); + + } else { + throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); + } + + return constructPolicy(createPolicyEntity(policyIdentifier, entity)); + } + public List listPolicies(Namespace namespace, PolicyType policyType) { PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(namespace); if (resolvedEntities == null) { @@ -120,60 +139,13 @@ public List listPolicies(Namespace namespace, PolicyType polic .toList(); } - public Policy createPolicy( - PolicyIdentifier policyIdentifier, String type, String description, String content) { - PolarisResolvedPathWrapper resolvedParent = - resolvedEntityView.getResolvedPath(policyIdentifier.namespace()); - if (resolvedParent == null) { - // Illegal state because the namespace should've already been in the static resolution set. - throw new IllegalStateException( - String.format( - "Failed to fetch resolved parent for PolicyIdentifier '%s'", policyIdentifier)); - } - PolarisResolvedPathWrapper resolvedPolicyEntities = - resolvedEntityView.getPassthroughResolvedPath( - policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); - - PolicyEntity entity = - PolicyEntity.of( - resolvedPolicyEntities == null ? null : resolvedPolicyEntities.getRawLeafEntity()); - - if (entity == null) { - PolicyType policyType = PolicyType.fromName(type); - if (policyType == null) { - throw new BadRequestException("Unknown policy type: %s", type); - } - - entity = - new PolicyEntity.Builder( - policyIdentifier.namespace(), policyIdentifier.name(), policyType) - .setCatalogId(catalogEntity.getId()) - .setDescription(description) - .setContent(content) - .setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()) - .build(); - - PolicyValidators.validate(entity); - - } else { - throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); - } - - return constructPolicy(createPolicyEntity(policyIdentifier, entity)); - } - public Policy loadPolicy(PolicyIdentifier policyIdentifier) { PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getPassthroughResolvedPath( policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); - PolicyEntity policy = null; - - if (resolvedEntities != null) { - if (resolvedEntities.getRawLeafEntity().getType() == PolarisEntityType.POLICY) { - policy = PolicyEntity.of(resolvedEntities.getRawLeafEntity()); - } - } + PolicyEntity policy = + PolicyEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); if (policy == null) { throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); @@ -190,13 +162,8 @@ public Policy updatePolicy( resolvedEntityView.getPassthroughResolvedPath( policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); - PolicyEntity policy = null; - - if (resolvedEntities != null) { - if (resolvedEntities.getRawLeafEntity().getType() == PolarisEntityType.POLICY) { - policy = PolicyEntity.of(resolvedEntities.getRawLeafEntity()); - } - } + PolicyEntity policy = + PolicyEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); if (policy == null) { throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); @@ -265,7 +232,6 @@ private PolicyEntity createPolicyEntity(PolicyIdentifier identifier, PolarisEnti List catalogPath = resolvedParent.getRawFullPath(); if (entity.getParentId() <= 0) { - // TODO: Validate catalogPath size is at least 1 for catalog entity? entity = new PolarisEntity.Builder(entity) .setParentId(resolvedParent.getRawLeafEntity().getId()) From 872a5d669d7a9b8f5e8b03e6ddac8fc32cabd1d9 Mon Sep 17 00:00:00 2001 From: Honah J Date: Thu, 3 Apr 2025 17:57:51 -0500 Subject: [PATCH 3/6] revise PolicyIdentifier --- .../service/types/PolicyIdentifier.java | 137 ++++++++------- .../core/catalog/PolarisCatalogHelpers.java | 10 +- .../quarkus/catalog/PolicyCatalogTest.java | 161 ++++++------------ .../service/catalog/policy/PolicyCatalog.java | 14 +- .../PolarisPassthroughResolutionView.java | 12 +- 5 files changed, 150 insertions(+), 184 deletions(-) diff --git a/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java b/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java index ba504e73ac..597e534fa1 100644 --- a/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java +++ b/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java @@ -18,92 +18,95 @@ */ package org.apache.polaris.service.types; -import com.google.common.base.Preconditions; -import com.google.common.base.Splitter; -import com.google.common.collect.Iterables; -import java.util.Arrays; -import java.util.Locale; +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import io.swagger.annotations.ApiModelProperty; import java.util.Objects; import org.apache.iceberg.catalog.Namespace; -import org.apache.iceberg.catalog.TableIdentifier; +/** + * Represents a modified version of the PolicyIdentifier that is different from the one generated by + * the OpenAPI generator + * + *

This version uses {@link org.apache.iceberg.catalog.Namespace} instead a list of strings for + * namespace field. + */ public class PolicyIdentifier { - private static final Splitter DOT = Splitter.on('.'); - private final Namespace namespace; private final String name; - public static PolicyIdentifier of(String... names) { - Preconditions.checkArgument(names != null, "Cannot create policy identifier from null array"); - Preconditions.checkArgument(names.length > 0, "Cannot create policy identifier without a name"); - return new PolicyIdentifier( - Namespace.of(Arrays.copyOf(names, names.length - 1)), names[names.length - 1]); + /** Reference to one or more levels of a namespace */ + @ApiModelProperty( + example = "[\"accounting\",\"tax\"]", + required = true, + value = "Reference to one or more levels of a namespace") + @JsonProperty(value = "namespace", required = true) + public Namespace getNamespace() { + return namespace; } - public static PolicyIdentifier of(Namespace namespace, String name) { - return new PolicyIdentifier(namespace, name); + /** */ + @ApiModelProperty(required = true, value = "") + @JsonProperty(value = "name", required = true) + public String getName() { + return name; } - public static PolicyIdentifier parse(String identifier) { - Preconditions.checkArgument(identifier != null, "Cannot parse policy identifier: null"); - Iterable parts = DOT.split(identifier); - return PolicyIdentifier.of(Iterables.toArray(parts, String.class)); + @JsonCreator + public PolicyIdentifier( + @JsonProperty(value = "namespace", required = true) Namespace namespace, + @JsonProperty(value = "name", required = true) String name) { + this.namespace = Objects.requireNonNullElse(namespace, Namespace.empty()); + this.name = name; } - public static PolicyIdentifier fromTableIdentifier(TableIdentifier tableIdentifier) { - return new PolicyIdentifier(tableIdentifier.namespace(), tableIdentifier.name()); + public static Builder builder() { + return new Builder(); } - private PolicyIdentifier(Namespace namespace, String name) { - Preconditions.checkArgument(name != null && !name.isEmpty(), "Invalid name: null or empty"); - Preconditions.checkArgument(namespace != null, "Invalid Namespace: null"); - this.namespace = namespace; - this.name = name; + public static Builder builder(Namespace namespace, String name) { + return new Builder(namespace, name); } - /** - * Whether the namespace is empty. - * - * @return true if the namespace is not empty, false otherwise - */ - public boolean hasNamespace() { - return !namespace.isEmpty(); - } + public static final class Builder { + private Namespace namespace; + private String name; - /** Returns the identifier namespace. */ - public Namespace namespace() { - return namespace; - } + private Builder() {} - /** Returns the identifier name. */ - public String name() { - return name; - } + private Builder(Namespace namespace, String name) { + this.namespace = Objects.requireNonNullElse(namespace, Namespace.empty()); + this.name = name; + } - public TableIdentifier toTableIdentifier() { - return TableIdentifier.of(namespace, name); - } + public Builder setNamespace(Namespace namespace) { + this.namespace = namespace; + return this; + } - public PolicyIdentifier toLowerCase() { - String[] newLevels = - Arrays.stream(namespace().levels()).map(String::toLowerCase).toArray(String[]::new); - String newName = name().toLowerCase(Locale.ROOT); - return PolicyIdentifier.of(Namespace.of(newLevels), newName); + public Builder setName(String name) { + this.name = name; + return this; + } + + public PolicyIdentifier build() { + PolicyIdentifier inst = new PolicyIdentifier(namespace, name); + return inst; + } } @Override - public boolean equals(Object other) { - if (this == other) { + public boolean equals(Object o) { + if (this == o) { return true; } - - if (other == null || getClass() != other.getClass()) { + if (o == null || getClass() != o.getClass()) { return false; } - - PolicyIdentifier that = (PolicyIdentifier) other; - return namespace.equals(that.namespace) && name.equals(that.name); + PolicyIdentifier policyIdentifier = (PolicyIdentifier) o; + return Objects.equals(this.namespace, policyIdentifier.namespace) + && Objects.equals(this.name, policyIdentifier.name); } @Override @@ -113,10 +116,22 @@ public int hashCode() { @Override public String toString() { - if (hasNamespace()) { - return namespace.toString() + "." + name; - } else { - return name; + StringBuilder sb = new StringBuilder(); + sb.append("class PolicyIdentifier {\n"); + + sb.append(" namespace: ").append(toIndentedString(namespace)).append("\n"); + sb.append(" name: ").append(toIndentedString(name)).append("\n"); + sb.append("}"); + return sb.toString(); + } + + /** + * Convert the given object to string with each line indented by 4 spaces (except the first line). + */ + private String toIndentedString(Object o) { + if (o == null) { + return "null"; } + return o.toString().replace("\n", "\n "); } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/catalog/PolarisCatalogHelpers.java b/polaris-core/src/main/java/org/apache/polaris/core/catalog/PolarisCatalogHelpers.java index ce0345bf39..e10c24f2f1 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/catalog/PolarisCatalogHelpers.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/catalog/PolarisCatalogHelpers.java @@ -36,10 +36,14 @@ public class PolarisCatalogHelpers { private PolarisCatalogHelpers() {} public static List tableIdentifierToList(TableIdentifier identifier) { + return identifierToList(identifier.namespace(), identifier.name()); + } + + public static List identifierToList(Namespace namespace, String name) { ImmutableList.Builder fullList = - ImmutableList.builderWithExpectedSize(identifier.namespace().length() + 1); - fullList.addAll(Arrays.asList(identifier.namespace().levels())); - fullList.add(identifier.name()); + ImmutableList.builderWithExpectedSize(namespace.length() + 1); + fullList.addAll(Arrays.asList(namespace.levels())); + fullList.add(name); return fullList.build(); } diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java index b49e997288..83bdecaaa4 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java @@ -18,7 +18,6 @@ */ package org.apache.polaris.service.quarkus.catalog; -import static org.apache.iceberg.types.Types.NestedField.required; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.when; @@ -38,11 +37,9 @@ import java.util.function.Supplier; import org.apache.commons.lang3.NotImplementedException; import org.apache.iceberg.CatalogProperties; -import org.apache.iceberg.Schema; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.AlreadyExistsException; -import org.apache.iceberg.types.Types; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; @@ -122,6 +119,12 @@ public Map getConfigOverrides() { public static final String SECRET_ACCESS_KEY = "secret_access_key"; public static final String SESSION_TOKEN = "session_token"; + private static final Namespace NS1 = Namespace.of("ns1"); + private static final PolicyIdentifier POLICY1 = new PolicyIdentifier(NS1, "p1"); + private static final PolicyIdentifier POLICY2 = new PolicyIdentifier(NS1, "p2"); + private static final PolicyIdentifier POLICY3 = new PolicyIdentifier(NS1, "p3"); + private static final PolicyIdentifier POLICY4 = new PolicyIdentifier(NS1, "p4"); + @Inject MetaStoreManagerFactory managerFactory; @Inject PolarisConfigurationStore configurationStore; @Inject PolarisStorageIntegrationProvider storageIntegrationProvider; @@ -141,11 +144,6 @@ public Map getConfigOverrides() { private PolarisEntity catalogEntity; private SecurityContext securityContext; - protected static final Schema SCHEMA = - new Schema( - required(3, "id", Types.IntegerType.get(), "unique ID 🤪"), - required(4, "data", Types.StringType.get())); - @BeforeAll public static void setUpMocks() { PolarisStorageIntegrationProviderImpl mock = @@ -306,12 +304,11 @@ public Map purgeRealms(Iterable realms) { @Test public void testCreatePolicyDoesNotThrow() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); Assertions.assertThatCode( () -> policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}")) @@ -320,18 +317,14 @@ public void testCreatePolicyDoesNotThrow() { @Test public void testCreatePolicyAlreadyExists() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); Assertions.assertThatThrownBy( () -> policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}")) @@ -340,7 +333,7 @@ public void testCreatePolicyAlreadyExists() { Assertions.assertThatThrownBy( () -> policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), + POLICY1, PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), "test", "{\"enable\": false}")) @@ -349,85 +342,64 @@ public void testCreatePolicyAlreadyExists() { @Test public void testListPolicies() { - Namespace namespace = Namespace.of("ns"); - PolicyIdentifier identifer1 = PolicyIdentifier.of("ns", "p1"); - PolicyIdentifier identifer2 = PolicyIdentifier.of("ns", "p2"); - PolicyIdentifier identifer3 = PolicyIdentifier.of("ns", "p3"); - PolicyIdentifier identifer4 = PolicyIdentifier.of("ns", "p4"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - identifer1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - identifer2, + POLICY2, PredefinedPolicyTypes.METADATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - identifer3, - PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), - "test", - "{\"enable\": false}"); + POLICY3, PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - identifer4, + POLICY4, PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL.getName(), "test", "{\"enable\": false}"); - List listResult = policyCatalog.listPolicies(Namespace.of("ns"), null); + List listResult = policyCatalog.listPolicies(NS1, null); Assertions.assertThat(listResult).hasSize(4); - Assertions.assertThat(listResult) - .containsExactlyInAnyOrder(identifer1, identifer2, identifer3, identifer4); + Assertions.assertThat(listResult).containsExactlyInAnyOrder(POLICY1, POLICY2, POLICY3, POLICY4); } @Test public void testListPoliciesFilterByPolicyType() { - Namespace namespace = Namespace.of("ns"); - PolicyIdentifier identifer1 = PolicyIdentifier.of("ns", "p1"); - PolicyIdentifier identifer2 = PolicyIdentifier.of("ns", "p2"); - PolicyIdentifier identifer3 = PolicyIdentifier.of("ns", "p3"); - PolicyIdentifier identifer4 = PolicyIdentifier.of("ns", "p4"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - identifer1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - identifer2, + POLICY2, PredefinedPolicyTypes.METADATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - identifer3, - PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), - "test", - "{\"enable\": false}"); + POLICY3, PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - identifer4, + POLICY4, PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL.getName(), "test", "{\"enable\": false}"); List listResult = - policyCatalog.listPolicies(Namespace.of("ns"), PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL); + policyCatalog.listPolicies(NS1, PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL); Assertions.assertThat(listResult).hasSize(1); - Assertions.assertThat(listResult).containsExactlyInAnyOrder(identifer4); + Assertions.assertThat(listResult).containsExactlyInAnyOrder(POLICY4); } @Test public void testLoadPolicy() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - Policy policy = policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1")); + Policy policy = policyCatalog.loadPolicy(POLICY1); Assertions.assertThat(policy.getVersion()).isEqualTo(0); Assertions.assertThat(policy.getPolicyType()) .isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); @@ -438,41 +410,32 @@ public void testLoadPolicy() { @Test public void testCreatePolicyWithInvalidContent() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); Assertions.assertThatThrownBy( () -> policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "invalid")) + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "invalid")) .isInstanceOf(InvalidPolicyException.class); } @Test public void testLoadPolicyNotExist() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); - Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1"))) + Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) .isInstanceOf(NoSuchPolicyException.class); } @Test public void testUpdatePolicy() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - policyCatalog.updatePolicy(PolicyIdentifier.of("ns", "p1"), "updated", "{\"enable\": true}", 0); + policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 0); - Policy policy = policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1")); + Policy policy = policyCatalog.loadPolicy(POLICY1); Assertions.assertThat(policy.getVersion()).isEqualTo(1); Assertions.assertThat(policy.getPolicyType()) .isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); @@ -483,73 +446,51 @@ public void testUpdatePolicy() { @Test public void testUpdatePolicyWithWrongVersion() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); Assertions.assertThatThrownBy( - () -> - policyCatalog.updatePolicy( - PolicyIdentifier.of("ns", "p1"), "updated", "{\"enable\": true}", 1)) + () -> policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 1)) .isInstanceOf(PolicyVersionMismatchException.class); } @Test public void testUpdatePolicyWithInvalidContent() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); Assertions.assertThatThrownBy( - () -> - policyCatalog.updatePolicy( - PolicyIdentifier.of("ns", "p1"), "updated", "invalid", 0)) + () -> policyCatalog.updatePolicy(POLICY1, "updated", "invalid", 0)) .isInstanceOf(InvalidPolicyException.class); } @Test public void testUpdatePolicyNotExist() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); Assertions.assertThatThrownBy( - () -> - policyCatalog.updatePolicy( - PolicyIdentifier.of("ns", "p1"), "updated", "{\"enable\": true}", 0)) + () -> policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 0)) .isInstanceOf(NoSuchPolicyException.class); } @Test public void testDropPolicy() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( - PolicyIdentifier.of("ns", "p1"), - PredefinedPolicyTypes.DATA_COMPACTION.getName(), - "test", - "{\"enable\": false}"); + POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - Assertions.assertThat(policyCatalog.dropPolicy(PolicyIdentifier.of("ns", "p1"), false)) - .isTrue(); - Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(PolicyIdentifier.of("ns", "p1"))) + Assertions.assertThat(policyCatalog.dropPolicy(POLICY1, false)).isTrue(); + Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) .isInstanceOf(NoSuchPolicyException.class); } @Test public void testDropPolicyNotExist() { - Namespace namespace = Namespace.of("ns"); - icebergCatalog.createNamespace(namespace); + icebergCatalog.createNamespace(NS1); - Assertions.assertThatThrownBy( - () -> policyCatalog.dropPolicy(PolicyIdentifier.of("ns", "p1"), false)) + Assertions.assertThatThrownBy(() -> policyCatalog.dropPolicy(POLICY1, false)) .isInstanceOf(NoSuchPolicyException.class); } } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 163065acb6..fe72f04a20 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -25,7 +25,6 @@ import org.apache.iceberg.exceptions.AlreadyExistsException; import org.apache.iceberg.exceptions.BadRequestException; import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.catalog.PolarisCatalogHelpers; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.entity.CatalogEntity; import org.apache.polaris.core.entity.PolarisEntity; @@ -84,7 +83,7 @@ public Policy createPolicy( entity = new PolicyEntity.Builder( - policyIdentifier.namespace(), policyIdentifier.name(), policyType) + policyIdentifier.getNamespace(), policyIdentifier.getName(), policyType) .setCatalogId(catalogId) .setDescription(description) .setContent(content) @@ -134,8 +133,13 @@ public List listPolicies(Namespace namespace, PolicyType polic List entities = policyEntities.stream().map(PolarisEntity::nameAndId).toList(); - return PolarisCatalogHelpers.nameAndIdToTableIdentifiers(catalogPath, entities).stream() - .map(PolicyIdentifier::fromTableIdentifier) + return entities.stream() + .map( + entity -> + PolicyIdentifier.builder() + .setNamespace(namespace) + .setName(entity.getName()) + .build()) .toList(); } @@ -223,7 +227,7 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) private PolicyEntity createPolicyEntity(PolicyIdentifier identifier, PolarisEntity entity) { PolarisResolvedPathWrapper resolvedParent = - resolvedEntityView.getResolvedPath(identifier.namespace()); + resolvedEntityView.getResolvedPath(identifier.getNamespace()); if (resolvedParent == null) { // Illegal state because the namespace should've already been in the static resolution set. throw new IllegalStateException( diff --git a/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java b/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java index 87b0696a52..8919ef5f02 100644 --- a/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java +++ b/service/common/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java @@ -95,14 +95,15 @@ public PolarisResolvedPathWrapper getResolvedPath( identifier); manifest.resolveAll(); return manifest.getResolvedPath(identifier, entityType, subType); - } else if (key instanceof PolicyIdentifier identifier) { + } else if (key instanceof PolicyIdentifier policyIdentifier) { manifest.addPath( new ResolverPath( - PolarisCatalogHelpers.tableIdentifierToList(identifier.toTableIdentifier()), + PolarisCatalogHelpers.identifierToList( + policyIdentifier.getNamespace(), policyIdentifier.getName()), entityType), - identifier); + policyIdentifier); manifest.resolveAll(); - return manifest.getResolvedPath(identifier, entityType, subType); + return manifest.getResolvedPath(policyIdentifier, entityType, subType); } else { throw new IllegalStateException( String.format( @@ -142,7 +143,8 @@ public PolarisResolvedPathWrapper getPassthroughResolvedPath( } else if (key instanceof PolicyIdentifier policyIdentifier) { manifest.addPassthroughPath( new ResolverPath( - PolarisCatalogHelpers.tableIdentifierToList(policyIdentifier.toTableIdentifier()), + PolarisCatalogHelpers.identifierToList( + policyIdentifier.getNamespace(), policyIdentifier.getName()), entityType), policyIdentifier); return manifest.getPassthroughResolvedPath(policyIdentifier, entityType, subType); From c52987dc38eb447a9c3cee4d368310939b0155fb Mon Sep 17 00:00:00 2001 From: Honah J Date: Thu, 3 Apr 2025 18:38:43 -0500 Subject: [PATCH 4/6] resolve comments --- .../service/catalog/policy/PolicyCatalog.java | 157 ++++++++---------- 1 file changed, 70 insertions(+), 87 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index fe72f04a20..da7ebaf0b2 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -32,7 +32,9 @@ import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; +import org.apache.polaris.core.persistence.dao.entity.BaseResult; import org.apache.polaris.core.persistence.dao.entity.DropEntityResult; +import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; import org.apache.polaris.core.policy.PolicyEntity; import org.apache.polaris.core.policy.PolicyType; @@ -67,6 +69,16 @@ public PolicyCatalog( public Policy createPolicy( PolicyIdentifier policyIdentifier, String type, String description, String content) { + PolarisResolvedPathWrapper resolvedParent = + resolvedEntityView.getResolvedPath(policyIdentifier.getNamespace()); + if (resolvedParent == null) { + // Illegal state because the namespace should've already been in the static resolution set. + throw new IllegalStateException( + String.format("Failed to fetch resolved parent for Policy '%s'", policyIdentifier)); + } + + List catalogPath = resolvedParent.getRawFullPath(); + PolarisResolvedPathWrapper resolvedPolicyEntities = resolvedEntityView.getPassthroughResolvedPath( policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); @@ -85,18 +97,39 @@ public Policy createPolicy( new PolicyEntity.Builder( policyIdentifier.getNamespace(), policyIdentifier.getName(), policyType) .setCatalogId(catalogId) + .setParentId(resolvedParent.getRawLeafEntity().getId()) .setDescription(description) .setContent(content) - .setId(getMetaStoreManager().generateNewEntityId(getCurrentPolarisContext()).getId()) + .setId(metaStoreManager.generateNewEntityId(getCurrentPolarisContext()).getId()) + .setCreateTimestamp(System.currentTimeMillis()) .build(); PolicyValidators.validate(entity); - } else { throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); } - return constructPolicy(createPolicyEntity(policyIdentifier, entity)); + EntityResult res = + metaStoreManager.createEntityIfNotExists( + getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); + + if (!res.isSuccess()) { + switch (res.getReturnStatus()) { + case BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS: + throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); + + default: + throw new IllegalStateException( + String.format( + "Unknown error status for identifier %s: %s with extraInfo: %s", + policyIdentifier, res.getReturnStatus(), res.getExtraInformation())); + } + } + + PolicyEntity resultEntity = PolicyEntity.of(res.getEntity()); + LOGGER.debug( + "Created Policy entity {} with PolicyIdentifier {}", resultEntity, policyIdentifier); + return constructPolicy(resultEntity); } public List listPolicies(Namespace namespace, PolicyType policyType) { @@ -108,7 +141,7 @@ public List listPolicies(Namespace namespace, PolicyType polic List catalogPath = resolvedEntities.getRawFullPath(); List policyEntities = - getMetaStoreManager() + metaStoreManager .listEntities( getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), @@ -119,7 +152,7 @@ public List listPolicies(Namespace namespace, PolicyType polic .map( polarisEntityActiveRecord -> PolicyEntity.of( - getMetaStoreManager() + metaStoreManager .loadEntity( getCurrentPolarisContext(), polarisEntityActiveRecord.getCatalogId(), @@ -173,36 +206,49 @@ public Policy updatePolicy( throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); } - PolicyEntity.Builder newPolicyBuilder = new PolicyEntity.Builder(policy); + // Verify that the current version of the policy matches the version that the user is trying to + // update int policyVersion = policy.getPolicyVersion(); if (currentPolicyVersion != policyVersion) { throw new PolicyVersionMismatchException( String.format("Policy version mismatch. Current version is %d", policyVersion)); } - boolean hasUpdate = false; - if (newContent != null) { - newPolicyBuilder.setContent(newContent); - hasUpdate = true; - } - if (newDescription != null) { - newPolicyBuilder.setDescription(newDescription); - hasUpdate = true; - } - - if (!hasUpdate) { + if (newDescription.equals(policy.getDescription()) && newContent.equals(policy.getContent())) { + // No need to update the policy if the new description and content are the same as the current return constructPolicy(policy); } + PolicyEntity.Builder newPolicyBuilder = new PolicyEntity.Builder(policy); + newPolicyBuilder.setContent(newContent); + newPolicyBuilder.setDescription(newDescription); newPolicyBuilder.setPolicyVersion(policyVersion + 1); PolicyEntity newPolicyEntity = newPolicyBuilder.build(); + PolicyValidators.validate(newPolicyEntity); - newPolicyEntity = PolicyEntity.of(updatePolicy(policyIdentifier, newPolicyEntity)); + + List catalogPath = resolvedEntities.getRawParentPath(); + newPolicyEntity = + Optional.ofNullable( + metaStoreManager + .updateEntityPropertiesIfNotChanged( + getCurrentPolarisContext(), + PolarisEntity.toCoreList(catalogPath), + newPolicyEntity) + .getEntity()) + .map(PolicyEntity::of) + .orElse(null); + + if (newPolicyEntity == null) { + throw new IllegalStateException( + String.format("Failed to update policy %s", policyIdentifier)); + } return constructPolicy(newPolicyEntity); } public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { + // TODO: Implement detachAll when we support attach/detach policy PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getPassthroughResolvedPath( policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); @@ -214,79 +260,16 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) PolarisEntity leafEntity = resolvedEntities.getRawLeafEntity(); DropEntityResult dropEntityResult = - getMetaStoreManager() - .dropEntityIfExists( - getCurrentPolarisContext(), - PolarisEntity.toCoreList(catalogPath), - leafEntity, - Map.of(), - false); + metaStoreManager.dropEntityIfExists( + getCurrentPolarisContext(), + PolarisEntity.toCoreList(catalogPath), + leafEntity, + Map.of(), + false); return dropEntityResult.isSuccess(); } - private PolicyEntity createPolicyEntity(PolicyIdentifier identifier, PolarisEntity entity) { - PolarisResolvedPathWrapper resolvedParent = - resolvedEntityView.getResolvedPath(identifier.getNamespace()); - if (resolvedParent == null) { - // Illegal state because the namespace should've already been in the static resolution set. - throw new IllegalStateException( - String.format("Failed to fetch resolved parent for Policy '%s'", identifier)); - } - - List catalogPath = resolvedParent.getRawFullPath(); - if (entity.getParentId() <= 0) { - entity = - new PolarisEntity.Builder(entity) - .setParentId(resolvedParent.getRawLeafEntity().getId()) - .build(); - } - - entity = - new PolarisEntity.Builder(entity).setCreateTimestamp(System.currentTimeMillis()).build(); - - PolarisEntity returnedEntity = - PolarisEntity.of( - getMetaStoreManager() - .createEntityIfNotExists( - getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity)); - - LOGGER.debug("Created Policy entity {} with Identifier {}", entity, identifier); - if (returnedEntity == null) { - throw new IllegalStateException("Failed to create Policy entity"); - } - - return PolicyEntity.of(returnedEntity); - } - - private PolarisEntity updatePolicy(PolicyIdentifier identifier, PolarisEntity entity) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getResolvedPath(identifier, entity.getType(), entity.getSubType()); - if (resolvedEntities == null) { - throw new IllegalStateException( - String.format("Failed to fetch resolved PolicyIdentifier '%s'", identifier)); - } - - List catalogPath = resolvedEntities.getRawParentPath(); - PolarisEntity returnedEntity = - Optional.ofNullable( - getMetaStoreManager() - .updateEntityPropertiesIfNotChanged( - getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity) - .getEntity()) - .map(PolarisEntity::new) - .orElse(null); - if (returnedEntity == null) { - throw new IllegalStateException("Failed to update Policy entity"); - } - - return returnedEntity; - } - - private PolarisMetaStoreManager getMetaStoreManager() { - return metaStoreManager; - } - private PolarisCallContext getCurrentPolarisContext() { return callContext.getPolarisCallContext(); } From dfb4992f6efe8ab9fd2a63a9ad1645a91de2a753 Mon Sep 17 00:00:00 2001 From: Honah J Date: Fri, 4 Apr 2025 10:57:05 -0500 Subject: [PATCH 5/6] resolve comments --- .../service/types/PolicyIdentifier.java | 7 +- .../service/catalog/policy/PolicyCatalog.java | 77 +++++++++---------- 2 files changed, 42 insertions(+), 42 deletions(-) diff --git a/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java b/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java index 597e534fa1..59dfd30249 100644 --- a/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java +++ b/api/polaris-catalog-service/src/main/java/org/apache/polaris/service/types/PolicyIdentifier.java @@ -28,8 +28,11 @@ * Represents a modified version of the PolicyIdentifier that is different from the one generated by * the OpenAPI generator * - *

This version uses {@link org.apache.iceberg.catalog.Namespace} instead a list of strings for - * namespace field. + *

the open api generation inlines the namespace definition, generates a {@code List} + * directly, instead of generating a Namespace class. This version uses {@link + * org.apache.iceberg.catalog.Namespace} for namespace field. + * + *

TODO: make code generation use {@link org.apache.iceberg.catalog.Namespace} directly */ public class PolicyIdentifier { diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index da7ebaf0b2..c7a86926a2 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -24,7 +24,6 @@ import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.exceptions.AlreadyExistsException; import org.apache.iceberg.exceptions.BadRequestException; -import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.entity.CatalogEntity; import org.apache.polaris.core.entity.PolarisEntity; @@ -87,42 +86,42 @@ public Policy createPolicy( PolicyEntity.of( resolvedPolicyEntities == null ? null : resolvedPolicyEntities.getRawLeafEntity()); - if (entity == null) { - PolicyType policyType = PolicyType.fromName(type); - if (policyType == null) { - throw new BadRequestException("Unknown policy type: %s", type); - } - - entity = - new PolicyEntity.Builder( - policyIdentifier.getNamespace(), policyIdentifier.getName(), policyType) - .setCatalogId(catalogId) - .setParentId(resolvedParent.getRawLeafEntity().getId()) - .setDescription(description) - .setContent(content) - .setId(metaStoreManager.generateNewEntityId(getCurrentPolarisContext()).getId()) - .setCreateTimestamp(System.currentTimeMillis()) - .build(); - - PolicyValidators.validate(entity); - } else { + if (entity != null) { throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); } + PolicyType policyType = PolicyType.fromName(type); + if (policyType == null) { + throw new BadRequestException("Unknown policy type: %s", type); + } + + entity = + new PolicyEntity.Builder( + policyIdentifier.getNamespace(), policyIdentifier.getName(), policyType) + .setCatalogId(catalogId) + .setParentId(resolvedParent.getRawLeafEntity().getId()) + .setDescription(description) + .setContent(content) + .setId( + metaStoreManager.generateNewEntityId(callContext.getPolarisCallContext()).getId()) + .setCreateTimestamp(System.currentTimeMillis()) + .build(); + + PolicyValidators.validate(entity); + EntityResult res = metaStoreManager.createEntityIfNotExists( - getCurrentPolarisContext(), PolarisEntity.toCoreList(catalogPath), entity); + callContext.getPolarisCallContext(), PolarisEntity.toCoreList(catalogPath), entity); if (!res.isSuccess()) { - switch (res.getReturnStatus()) { - case BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS: - throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); - - default: - throw new IllegalStateException( - String.format( - "Unknown error status for identifier %s: %s with extraInfo: %s", - policyIdentifier, res.getReturnStatus(), res.getExtraInformation())); + + if (res.getReturnStatus() == BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS) { + throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); + } else { + throw new IllegalStateException( + String.format( + "Unknown error status for identifier %s: %s with extraInfo: %s", + policyIdentifier, res.getReturnStatus(), res.getExtraInformation())); } } @@ -143,10 +142,10 @@ public List listPolicies(Namespace namespace, PolicyType polic List policyEntities = metaStoreManager .listEntities( - getCurrentPolarisContext(), + callContext.getPolarisCallContext(), PolarisEntity.toCoreList(catalogPath), PolarisEntityType.POLICY, - PolarisEntitySubType.ANY_SUBTYPE) + PolarisEntitySubType.NULL_SUBTYPE) .getEntities() .stream() .map( @@ -154,7 +153,7 @@ public List listPolicies(Namespace namespace, PolicyType polic PolicyEntity.of( metaStoreManager .loadEntity( - getCurrentPolarisContext(), + callContext.getPolarisCallContext(), polarisEntityActiveRecord.getCatalogId(), polarisEntityActiveRecord.getId(), polarisEntityActiveRecord.getType()) @@ -211,7 +210,9 @@ public Policy updatePolicy( int policyVersion = policy.getPolicyVersion(); if (currentPolicyVersion != policyVersion) { throw new PolicyVersionMismatchException( - String.format("Policy version mismatch. Current version is %d", policyVersion)); + String.format( + "Policy version mismatch. Given version is %d, current version is %d", + currentPolicyVersion, policyVersion)); } if (newDescription.equals(policy.getDescription()) && newContent.equals(policy.getContent())) { @@ -232,7 +233,7 @@ public Policy updatePolicy( Optional.ofNullable( metaStoreManager .updateEntityPropertiesIfNotChanged( - getCurrentPolarisContext(), + callContext.getPolarisCallContext(), PolarisEntity.toCoreList(catalogPath), newPolicyEntity) .getEntity()) @@ -261,7 +262,7 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) DropEntityResult dropEntityResult = metaStoreManager.dropEntityIfExists( - getCurrentPolarisContext(), + callContext.getPolarisCallContext(), PolarisEntity.toCoreList(catalogPath), leafEntity, Map.of(), @@ -270,10 +271,6 @@ public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) return dropEntityResult.isSuccess(); } - private PolarisCallContext getCurrentPolarisContext() { - return callContext.getPolarisCallContext(); - } - private static Policy constructPolicy(PolicyEntity policyEntity) { return Policy.builder() .setPolicyType(policyEntity.getPolicyType().getName()) From 8e48c55b3415bf71fd0e92daad87a20617be25a8 Mon Sep 17 00:00:00 2001 From: Honah J Date: Fri, 4 Apr 2025 11:04:33 -0500 Subject: [PATCH 6/6] Change back to switch --- .../service/catalog/policy/PolicyCatalog.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index c7a86926a2..a4fb23da7d 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -31,7 +31,6 @@ import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; -import org.apache.polaris.core.persistence.dao.entity.BaseResult; import org.apache.polaris.core.persistence.dao.entity.DropEntityResult; import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; @@ -115,13 +114,15 @@ public Policy createPolicy( if (!res.isSuccess()) { - if (res.getReturnStatus() == BaseResult.ReturnStatus.ENTITY_ALREADY_EXISTS) { - throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); - } else { - throw new IllegalStateException( - String.format( - "Unknown error status for identifier %s: %s with extraInfo: %s", - policyIdentifier, res.getReturnStatus(), res.getExtraInformation())); + switch (res.getReturnStatus()) { + case ENTITY_ALREADY_EXISTS: + throw new AlreadyExistsException("Policy already exists %s", policyIdentifier); + + default: + throw new IllegalStateException( + String.format( + "Unknown error status for identifier %s: %s with extraInfo: %s", + policyIdentifier, res.getReturnStatus(), res.getExtraInformation())); } }