From 5437c55806d6b1f7841355139ac002dcb2fb81be Mon Sep 17 00:00:00 2001 From: andrew4699 Date: Wed, 25 Jun 2025 12:02:13 -0700 Subject: [PATCH 1/3] Filter null values in listCatalogs --- .../org/apache/polaris/service/admin/PolarisServiceImpl.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java index 719e6d44b0..9320c551ed 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java @@ -24,6 +24,7 @@ import jakarta.ws.rs.core.SecurityContext; import java.util.List; import java.util.Locale; +import java.util.Objects; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.NotAuthorizedException; @@ -230,6 +231,7 @@ public Response listCatalogs(RealmContext realmContext, SecurityContext security PolarisAdminService adminService = newAdminService(realmContext, securityContext); List catalogList = adminService.listCatalogs().stream() + .filter(Objects::nonNull) .map(CatalogEntity::new) .map(CatalogEntity::asCatalog) .toList(); From bc7db74f69c30aca4bd253fb57616ea362348f28 Mon Sep 17 00:00:00 2001 From: andrew4699 Date: Thu, 26 Jun 2025 10:37:17 -0700 Subject: [PATCH 2/3] update --- .../quarkus/admin/ManagementServiceTest.java | 43 ++++++++++++++++ .../admin/TestPolarisMetaStoreManager.java | 49 +++++++++++++++++++ .../service/admin/PolarisAdminService.java | 16 ++++-- .../service/admin/PolarisServiceImpl.java | 2 - 4 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java index 47128c86d1..1dc72a154b 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java @@ -43,10 +43,16 @@ import org.apache.polaris.core.auth.PolarisAuthorizerImpl; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisBaseEntity; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PolarisEntityConstants; +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.entity.PrincipalRoleEntity; import org.apache.polaris.core.persistence.MetaStoreManagerFactory; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.dao.entity.CreateCatalogResult; import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.secrets.UnsafeInMemorySecretsManager; import org.apache.polaris.service.TestServices; @@ -276,4 +282,41 @@ public void testCannotAssignFederatedEntities() { () -> polarisAdminService.assignPrincipalRole(principal.getName(), role.getName())) .isInstanceOf(ValidationException.class); } + + /** Simulates the case when a catalog is dropped after being found while listing all catalogs. */ + @Test + public void testCatalogNotReturnedWhenDeletedAfterListBeforeGet() { + TestPolarisMetaStoreManager metaStoreManager = new TestPolarisMetaStoreManager(); + PolarisCallContext callContext = setupCallContext(metaStoreManager); + PolarisAdminService polarisAdminService = + setupPolarisAdminService(metaStoreManager, callContext); + + CreateCatalogResult catalog1 = + metaStoreManager.createCatalog( + callContext, + new PolarisBaseEntity( + PolarisEntityConstants.getNullId(), + metaStoreManager.generateNewEntityId(callContext).getId(), + PolarisEntityType.CATALOG, + PolarisEntitySubType.NULL_SUBTYPE, + PolarisEntityConstants.getRootEntityId(), + "my-catalog-1"), + List.of()); + CreateCatalogResult catalog2 = + metaStoreManager.createCatalog( + callContext, + new PolarisBaseEntity( + PolarisEntityConstants.getNullId(), + metaStoreManager.generateNewEntityId(callContext).getId(), + PolarisEntityType.CATALOG, + PolarisEntitySubType.NULL_SUBTYPE, + PolarisEntityConstants.getRootEntityId(), + "my-catalog-2"), + List.of()); + + metaStoreManager.fakeEntityNotFoundIds.add(catalog1.getCatalog().getId()); + List catalogs = polarisAdminService.listCatalogs(); + assertThat(catalogs.size()).isEqualTo(1); + assertThat(catalogs.getFirst().getId()).isEqualTo(catalog2.getCatalog().getId()); + } } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java new file mode 100644 index 0000000000..aef62747a6 --- /dev/null +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java @@ -0,0 +1,49 @@ +/* + * 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.admin; + +import jakarta.annotation.Nonnull; +import java.util.HashSet; +import java.util.Set; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.persistence.dao.entity.BaseResult; +import org.apache.polaris.core.persistence.dao.entity.EntityResult; +import org.apache.polaris.core.persistence.transactional.TransactionalMetaStoreManagerImpl; + +/** + * Intended to be a delegate to TransactionalMetaStoreManagerImpl with the ability to inject faults. + * Currently, you can force loadEntity() to return ENTITY_NOT_FOUND for a set of entity IDs. + */ +public class TestPolarisMetaStoreManager extends TransactionalMetaStoreManagerImpl { + public Set fakeEntityNotFoundIds = new HashSet<>(); + + @Override + public @Nonnull EntityResult loadEntity( + @Nonnull PolarisCallContext callCtx, + long entityCatalogId, + long entityId, + @Nonnull PolarisEntityType entityType) { + if (fakeEntityNotFoundIds.contains(entityId)) { + return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, ""); + } + return super.loadEntity(callCtx, entityCatalogId, entityId, entityType); + } +} diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java index 1a1914ad10..164d92d8c7 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java @@ -614,7 +614,6 @@ private boolean catalogOverlapsWithExistingCatalog(CatalogEntity catalogEntity) Set newCatalogLocations = getCatalogLocations(catalogEntity); return listCatalogsUnsafe().stream() - .filter(Objects::nonNull) .map(CatalogEntity::new) .anyMatch( existingCatalog -> { @@ -923,18 +922,24 @@ private void validateUpdateCatalogDiffOrThrow( return returnedEntity; } + /** + * List all catalogs after checking for permission. Nulls due to non-atomic list-then-get are + * filtered out. + */ public List listCatalogs() { authorizeBasicRootOperationOrThrow(PolarisAuthorizableOperation.LIST_CATALOGS); return listCatalogsUnsafe(); } /** - * List all catalogs without checking for permission. May contain NULLs due to multiple non-atomic - * API calls to the persistence layer. Specifically, this can happen when a PolarisEntity is - * returned by listCatalogs, but cannot be loaded afterward because it was purged by another - * process before it could be loaded. + * List all catalogs without checking for permission. Nulls due to non-atomic list-then-get are + * filtered out. */ private List listCatalogsUnsafe() { + // loadEntity may return null due to multiple non-atomic + // API calls to the persistence layer. Specifically, this can happen when a PolarisEntity is + // returned by listCatalogs, but cannot be loaded afterward because it was purged by another + // process before it could be loaded. return metaStoreManager .listEntities( getCurrentPolarisContext(), @@ -949,6 +954,7 @@ private List listCatalogsUnsafe() { PolarisEntity.of( metaStoreManager.loadEntity( getCurrentPolarisContext(), 0, nameAndId.getId(), nameAndId.getType()))) + .filter(Objects::nonNull) .toList(); } diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java index 9320c551ed..719e6d44b0 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java @@ -24,7 +24,6 @@ import jakarta.ws.rs.core.SecurityContext; import java.util.List; import java.util.Locale; -import java.util.Objects; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.NotAuthorizedException; @@ -231,7 +230,6 @@ public Response listCatalogs(RealmContext realmContext, SecurityContext security PolarisAdminService adminService = newAdminService(realmContext, securityContext); List catalogList = adminService.listCatalogs().stream() - .filter(Objects::nonNull) .map(CatalogEntity::new) .map(CatalogEntity::asCatalog) .toList(); From 8372f7c4792981d358b2e197177cf996e49c7bc7 Mon Sep 17 00:00:00 2001 From: andrew4699 Date: Thu, 26 Jun 2025 11:31:27 -0700 Subject: [PATCH 3/3] refactor --- .../quarkus/admin/ManagementServiceTest.java | 31 +++++++++++- .../admin/TestPolarisMetaStoreManager.java | 49 ------------------- 2 files changed, 30 insertions(+), 50 deletions(-) delete mode 100644 runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java index 1dc72a154b..3cb8216d87 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java @@ -21,11 +21,13 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import jakarta.annotation.Nonnull; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.SecurityContext; import java.security.Principal; import java.time.Clock; import java.time.Instant; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -52,8 +54,10 @@ import org.apache.polaris.core.entity.PrincipalRoleEntity; import org.apache.polaris.core.persistence.MetaStoreManagerFactory; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.dao.entity.BaseResult; import org.apache.polaris.core.persistence.dao.entity.CreateCatalogResult; import org.apache.polaris.core.persistence.dao.entity.EntityResult; +import org.apache.polaris.core.persistence.transactional.TransactionalMetaStoreManagerImpl; import org.apache.polaris.core.secrets.UnsafeInMemorySecretsManager; import org.apache.polaris.service.TestServices; import org.apache.polaris.service.admin.PolarisAdminService; @@ -314,9 +318,34 @@ public void testCatalogNotReturnedWhenDeletedAfterListBeforeGet() { "my-catalog-2"), List.of()); - metaStoreManager.fakeEntityNotFoundIds.add(catalog1.getCatalog().getId()); + metaStoreManager.setFakeEntityNotFoundIds(Set.of(catalog1.getCatalog().getId())); List catalogs = polarisAdminService.listCatalogs(); assertThat(catalogs.size()).isEqualTo(1); assertThat(catalogs.getFirst().getId()).isEqualTo(catalog2.getCatalog().getId()); } + + /** + * Intended to be a delegate to TransactionalMetaStoreManagerImpl with the ability to inject + * faults. Currently, you can force loadEntity() to return ENTITY_NOT_FOUND for a set of entity + * IDs. + */ + public static class TestPolarisMetaStoreManager extends TransactionalMetaStoreManagerImpl { + private Set fakeEntityNotFoundIds = new HashSet<>(); + + public void setFakeEntityNotFoundIds(Set ids) { + fakeEntityNotFoundIds = new HashSet<>(ids); + } + + @Override + public @Nonnull EntityResult loadEntity( + @Nonnull PolarisCallContext callCtx, + long entityCatalogId, + long entityId, + @Nonnull PolarisEntityType entityType) { + if (fakeEntityNotFoundIds.contains(entityId)) { + return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, ""); + } + return super.loadEntity(callCtx, entityCatalogId, entityId, entityType); + } + } } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java deleted file mode 100644 index aef62747a6..0000000000 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/TestPolarisMetaStoreManager.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.polaris.service.quarkus.admin; - -import jakarta.annotation.Nonnull; -import java.util.HashSet; -import java.util.Set; -import org.apache.polaris.core.PolarisCallContext; -import org.apache.polaris.core.entity.PolarisEntityType; -import org.apache.polaris.core.persistence.dao.entity.BaseResult; -import org.apache.polaris.core.persistence.dao.entity.EntityResult; -import org.apache.polaris.core.persistence.transactional.TransactionalMetaStoreManagerImpl; - -/** - * Intended to be a delegate to TransactionalMetaStoreManagerImpl with the ability to inject faults. - * Currently, you can force loadEntity() to return ENTITY_NOT_FOUND for a set of entity IDs. - */ -public class TestPolarisMetaStoreManager extends TransactionalMetaStoreManagerImpl { - public Set fakeEntityNotFoundIds = new HashSet<>(); - - @Override - public @Nonnull EntityResult loadEntity( - @Nonnull PolarisCallContext callCtx, - long entityCatalogId, - long entityId, - @Nonnull PolarisEntityType entityType) { - if (fakeEntityNotFoundIds.contains(entityId)) { - return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, ""); - } - return super.loadEntity(callCtx, entityCatalogId, entityId, entityType); - } -}