This is an automated email from the ASF dual-hosted git repository.
adutra 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 a15ddea1 Fix NPE in catalogOverlapsWithExistingCatalog() occurring
when a listed catalog could not be loaded correctly (#575)
a15ddea1 is described below
commit a15ddea1e5c0a25df6a5a2b498cc3d3221559efe
Author: Hung-An (Alvin) Chen <[email protected]>
AuthorDate: Thu Dec 19 18:09:32 2024 +0800
Fix NPE in catalogOverlapsWithExistingCatalog() occurring when a listed
catalog could not be loaded correctly (#575)
Co-authored-by: Alvin Chen <[email protected]>
---
.../service/dropwizard/catalog/TestUtil.java | 32 +++++++++++++++++-----
.../polaris/service/admin/PolarisAdminService.java | 9 +++++-
2 files changed, 33 insertions(+), 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
index 5bc8a0e9..c2342d8b 100644
---
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 class TestUtil {
.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 class TestUtil {
.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 class TestUtil {
.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 class TestUtil {
.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 class TestUtil {
.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 class TestUtil {
.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 class TestUtil {
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
index 7ad5cd88..d6d2c697 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
@@ -27,6 +27,7 @@ import java.util.Arrays;
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 @@ public class PolarisAdminService {
Set<String> newCatalogLocations = getCatalogLocations(catalogEntity);
return listCatalogsUnsafe().stream()
+ .filter(Objects::nonNull)
.map(CatalogEntity::new)
.anyMatch(
existingCatalog -> {
@@ -726,7 +728,12 @@ public class PolarisAdminService {
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(
