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.

Author: andrew4699

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;
@@ -43,11 +45,19 @@
 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 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.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);
+    }
+  }
 }

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

@@ -614,7 +614,6 @@ private boolean catalogOverlapsWithExistingCatalog(CatalogEntity catalogEntity)
 
     Set<String> 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<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 @@ private List<PolarisEntity> listCatalogsUnsafe() {
                 PolarisEntity.of(
                     metaStoreManager.loadEntity(
                         getCurrentPolarisContext(), 0, nameAndId.getId(), nameAndId.getType())))
+        .filter(Objects::nonNull)
         .toList();
   }