Fix NPE in catalogOverlapsWithExistingCatalog() occurring when a list…

…ed catalog could not be loaded correctly (#575) Co-authored-by: Alvin Chen <alvin.chen@snowflake.com>
apache · Dec 19, 2024 · a15ddea · a15ddea
1 parent 5761b53
commit a15ddea
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 8 deletions.
diff --git a/dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/catalog/TestUtil.java b/dropwizard/service/src/test/java/org/apache/polaris/service/dropwizard/catalog/TestUtil.java
@@ -84,7 +84,7 @@ public static RESTCatalog createSnowmanManagedCatalog(
             .header("Authorization", "Bearer " + adminToken.token())
             .header(REALM_PROPERTY_KEY, realm)
             .post(Entity.json(catalog))) {
-      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+      assertStatusCode(response, Response.Status.CREATED.getStatusCode());
     }
 
     // Create a new CatalogRole that has CATALOG_MANAGE_CONTENT and CATALOG_MANAGE_ACCESS
@@ -98,7 +98,7 @@ public static RESTCatalog createSnowmanManagedCatalog(
             .header("Authorization", "Bearer " + adminToken.token())
             .header(REALM_PROPERTY_KEY, realm)
             .post(Entity.json(newRole))) {
-      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+      assertStatusCode(response, Response.Status.CREATED.getStatusCode());
     }
     CatalogGrant grantResource =
         new CatalogGrant(CatalogPrivilege.CATALOG_MANAGE_CONTENT, GrantResource.TypeEnum.CATALOG);
@@ -112,7 +112,7 @@ public static RESTCatalog createSnowmanManagedCatalog(
             .header("Authorization", "Bearer " + adminToken.token())
             .header(REALM_PROPERTY_KEY, realm)
             .put(Entity.json(grantResource))) {
-      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+      assertStatusCode(response, Response.Status.CREATED.getStatusCode());
     }
     CatalogGrant grantAccessResource =
         new CatalogGrant(CatalogPrivilege.CATALOG_MANAGE_ACCESS, GrantResource.TypeEnum.CATALOG);
@@ -126,7 +126,7 @@ public static RESTCatalog createSnowmanManagedCatalog(
             .header("Authorization", "Bearer " + adminToken.token())
             .header(REALM_PROPERTY_KEY, realm)
             .put(Entity.json(grantAccessResource))) {
-      assertThat(response).returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+      assertStatusCode(response, Response.Status.CREATED.getStatusCode());
     }
 
     // Assign this new CatalogRole to the service_admin PrincipalRole
@@ -140,7 +140,8 @@ public static RESTCatalog createSnowmanManagedCatalog(
             .header("Authorization", "Bearer " + adminToken.token())
             .header(REALM_PROPERTY_KEY, realm)
             .get()) {
-      assertThat(response).returns(Response.Status.OK.getStatusCode(), Response::getStatus);
+      assertStatusCode(response, Response.Status.OK.getStatusCode());
+
       CatalogRole catalogRole = response.readEntity(CatalogRole.class);
       try (Response assignResponse =
           client
@@ -154,8 +155,7 @@ public static RESTCatalog createSnowmanManagedCatalog(
               .header("Authorization", "Bearer " + adminToken.token())
               .header(REALM_PROPERTY_KEY, realm)
               .put(Entity.json(catalogRole))) {
-        assertThat(assignResponse)
-            .returns(Response.Status.CREATED.getStatusCode(), Response::getStatus);
+        assertStatusCode(assignResponse, Response.Status.CREATED.getStatusCode());
       }
     }
 
@@ -179,4 +179,22 @@ public static RESTCatalog createSnowmanManagedCatalog(
     restCatalog.initialize("polaris", propertiesBuilder.buildKeepingLast());
     return restCatalog;
   }
+
+  /**
+   * Asserts that the response has the expected status code, with a fail message if the assertion
+   * fails. The response entity is buffered so it can be read multiple times.
+   *
+   * @param response The response to check
+   * @param expectedStatusCode The expected status code
+   */
+  private static void assertStatusCode(Response response, int expectedStatusCode) {
+    // Buffer the entity so we can read it multiple times
+    response.bufferEntity();
+
+    assertThat(response)
+        .withFailMessage(
+            "Expected status code %s but got %s with message: %s",
+            expectedStatusCode, response.getStatus(), response.readEntity(String.class))
+        .returns(expectedStatusCode, Response::getStatus);
+  }
 }
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
@@ -27,6 +27,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
@@ -518,6 +519,7 @@ private boolean catalogOverlapsWithExistingCatalog(CatalogEntity catalogEntity)
 
     Set<String> newCatalogLocations = getCatalogLocations(catalogEntity);
     return listCatalogsUnsafe().stream()
+        .filter(Objects::nonNull)
         .map(CatalogEntity::new)
         .anyMatch(
             existingCatalog -> {
@@ -726,7 +728,12 @@ public List<PolarisEntity> listCatalogs() {
     return listCatalogsUnsafe();
   }
 
-  /** List all catalogs without checking for permission */
+  /**
+   * 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.
+   */
   private List<PolarisEntity> listCatalogsUnsafe() {
     return metaStoreManager
         .listEntities(