This is an automated email from the ASF dual-hosted git repository.
emaynard pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 3fea897c0 Fix NPE in listCatalogs (#1949)
3fea897c0 is described below
commit 3fea897c04b51da29eea1a2553822bfd0623b644
Author: Andrew Guterman <andrew.guterm...@gmail.com>
AuthorDate: Thu Jun 26 14:23:38 2025 -0700
Fix NPE in listCatalogs (#1949)
listCatalogs is non-atomic. It first atomically lists all entities and then
iterates through each one and does an individual loadEntity call. This causes
an NPE when calling `CatalogEntity::new`.
I don't think it's ever useful for listCatalogsUnsafe to return null since
the caller isn't expecting a certain length of elements, so I just filtered it
there.
---
.../quarkus/admin/ManagementServiceTest.java | 72 ++++++++++++++++++++++
.../polaris/service/admin/PolarisAdminService.java | 16 +++--
2 files changed, 83 insertions(+), 5 deletions(-)
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 47128c86d..3cb8216d8 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 @@ package org.apache.polaris.service.quarkus.admin;
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;
@@ -43,11 +45,19 @@ import
org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
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.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;
@@ -276,4 +286,66 @@ public class ManagementServiceTest {
() -> 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.setFakeEntityNotFoundIds(Set.of(catalog1.getCatalog().getId()));
+ List<PolarisEntity> 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<Long> fakeEntityNotFoundIds = new HashSet<>();
+
+ public void setFakeEntityNotFoundIds(Set<Long> 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/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 1a1914ad1..164d92d8c 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 @@ public class PolarisAdminService {
Set<String> newCatalogLocations = getCatalogLocations(catalogEntity);
return listCatalogsUnsafe().stream()
- .filter(Objects::nonNull)
.map(CatalogEntity::new)
.anyMatch(
existingCatalog -> {
@@ -923,18 +922,24 @@ public class PolarisAdminService {
return returnedEntity;
}
+ /**
+ * List all catalogs after checking for permission. Nulls due to non-atomic
list-then-get are
+ * filtered out.
+ */
public List<PolarisEntity> 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<PolarisEntity> 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 @@ public class PolarisAdminService {
PolarisEntity.of(
metaStoreManager.loadEntity(
getCurrentPolarisContext(), 0, nameAndId.getId(),
nameAndId.getType())))
+ .filter(Objects::nonNull)
.toList();
}
