fix: Filter null entities in listPrincipals and listPrincipalRoles to prevent NPE

Author: HotSushi

runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java

@@ -315,4 +315,81 @@ public void testCatalogNotReturnedWhenDeletedAfterListBeforeGet() {
     assertThat(catalogs.size()).isEqualTo(1);
     assertThat(catalogs.getFirst().getId()).isEqualTo(catalog2.getCatalog().getId());
   }
+
+  /**
+   * Simulates the case when a principal is dropped after being found while listing all principals.
+   */
+  @Test
+  public void testPrincipalNotReturnedWhenDeletedAfterListBeforeGet() {
+    PolarisMetaStoreManager metaStoreManager = Mockito.spy(setupMetaStoreManager());
+    PolarisCallContext callContext = setupCallContext();
+    PolarisAdminService polarisAdminService =
+        setupPolarisAdminService(metaStoreManager, callContext);
+
+    int initialPrincipalCount = polarisAdminService.listPrincipals().size();
+
+    PrincipalEntity principal1 = createPrincipal(metaStoreManager, callContext, "my-principal-1");
+    metaStoreManager.createPrincipal(callContext, principal1);
+
+    PrincipalEntity principal2 = createPrincipal(metaStoreManager, callContext, "my-principal-2");
+    metaStoreManager.createPrincipal(callContext, principal2);
+
+    Mockito.doAnswer(
+            invocation -> {
+              Object entityId = invocation.getArgument(2);
+              if (entityId.equals(principal1.getId())) {
+                return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, "");
+              }
+              return invocation.callRealMethod();
+            })
+        .when(metaStoreManager)
+        .loadEntity(Mockito.any(), Mockito.anyLong(), Mockito.anyLong(), Mockito.any());
+
+    List<PolarisEntity> principals = polarisAdminService.listPrincipals();
+
+    assertThat(principals.size())
+        .isEqualTo(initialPrincipalCount + 1); // initial + 2 created - 1 filtered
+    assertThat(principals.stream().anyMatch(p -> p.getId() == principal2.getId()))
+        .isTrue(); // principal2 survived
+    assertThat(principals.stream().noneMatch(p -> p.getId() == principal1.getId()))
+        .isTrue(); // principal1 filtered out
+  }
+
+  /**
+   * Simulates the case when a principal role is dropped after being found while listing all
+   * principal roles.
+   */
+  @Test
+  public void testPrincipalRoleNotReturnedWhenDeletedAfterListBeforeGet() {
+    PolarisMetaStoreManager metaStoreManager = Mockito.spy(setupMetaStoreManager());
+    PolarisCallContext callContext = setupCallContext();
+    PolarisAdminService polarisAdminService =
+        setupPolarisAdminService(metaStoreManager, callContext);
+
+    int initialRoleCount = polarisAdminService.listPrincipalRoles().size();
+
+    PrincipalRoleEntity role1 = createRole(metaStoreManager, callContext, "my-role-1", false);
+    EntityResult createResult1 = metaStoreManager.createEntityIfNotExists(callContext, null, role1);
+    assertThat(createResult1.isSuccess()).isTrue();
+
+    PrincipalRoleEntity role2 = createRole(metaStoreManager, callContext, "my-role-2", false);
+    EntityResult createResult2 = metaStoreManager.createEntityIfNotExists(callContext, null, role2);
+    assertThat(createResult2.isSuccess()).isTrue();
+
+    Mockito.doAnswer(
+            invocation -> {
+              Object entityId = invocation.getArgument(2);
+              if (entityId.equals(role1.getId())) {
+                return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, "");
+              }
+              return invocation.callRealMethod();
+            })
+        .when(metaStoreManager)
+        .loadEntity(Mockito.any(), Mockito.anyLong(), Mockito.anyLong(), Mockito.any());
+
+    List<PolarisEntity> principalRoles = polarisAdminService.listPrincipalRoles();
+    assertThat(principalRoles.size()).isEqualTo(initialRoleCount + 1);
+    assertThat(principalRoles.stream().anyMatch(r -> r.getId() == role2.getId())).isTrue();
+    assertThat(principalRoles.stream().noneMatch(r -> r.getId() == role1.getId())).isTrue();
+  }
 }

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

@@ -1141,10 +1141,18 @@ public void deletePrincipal(String name) {
     return rotateOrResetCredentialsHelper(principalName, true);
   }
 
+  /**
+   * List all principals after checking for permission. Nulls due to non-atomic list-then-get are
+   * filtered out.
+   */
   public List<PolarisEntity> listPrincipals() {
     PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_PRINCIPALS;
     authorizeBasicRootOperationOrThrow(op);
 
+    // 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 listEntities, but cannot be loaded afterward because it was purged by another
+    // process before it could be loaded.
     return metaStoreManager
         .listEntities(
             getCurrentPolarisContext(),
@@ -1159,6 +1167,7 @@ public List<PolarisEntity> listPrincipals() {
                 PolarisEntity.of(
                     metaStoreManager.loadEntity(
                         getCurrentPolarisContext(), 0, nameAndId.getId(), nameAndId.getType())))
+        .filter(Objects::nonNull)
         .toList();
   }
 
@@ -1254,10 +1263,18 @@ public void deletePrincipalRole(String name) {
     return returnedEntity;
   }
 
+  /**
+   * List all principal roles after checking for permission. Nulls due to non-atomic list-then-get
+   * are filtered out.
+   */
   public List<PolarisEntity> listPrincipalRoles() {
     PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_PRINCIPAL_ROLES;
     authorizeBasicRootOperationOrThrow(op);
 
+    // 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 listEntities, but cannot be loaded afterward because it was purged by another
+    // process before it could be loaded.
     return metaStoreManager
         .listEntities(
             getCurrentPolarisContext(),
@@ -1272,6 +1289,7 @@ public List<PolarisEntity> listPrincipalRoles() {
                 PolarisEntity.of(
                     metaStoreManager.loadEntity(
                         getCurrentPolarisContext(), 0, nameAndId.getId(), nameAndId.getType())))
+        .filter(Objects::nonNull)
         .toList();
   }