You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@polaris.apache.org by yu...@apache.org on 2026/06/02 17:38:00 UTC
(polaris) branch main updated: Make granting catalog roles idempotent for JDBC persistence (#4554)
This is an automated email from the ASF dual-hosted git repository.
flyrain 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 3bf3898ff7 Make granting catalog roles idempotent for JDBC persistence (#4554)
3bf3898ff7 is described below
commit 3bf3898ff790cbef3bc2fe755f983362e689000f
Author: Nicholas Barton <12...@users.noreply.github.com>
AuthorDate: Tue Jun 2 13:37:55 2026 -0400
Make granting catalog roles idempotent for JDBC persistence (#4554)
---
.../PolarisManagementServiceIntegrationTest.java | 32 +++++
.../relational/jdbc/DatasourceOperations.java | 8 +-
.../relational/jdbc/JdbcBasePersistenceImpl.java | 8 +-
.../RelationalJdbcIdempotencyStore.java | 2 +-
.../jdbc/JdbcGrantRecordsIdempotencyTest.java | 158 +++++++++++++++++++++
.../polaris/core/persistence/BasePersistence.java | 7 +-
6 files changed, 206 insertions(+), 9 deletions(-)
diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java
index dd1afa0170..8505d34249 100644
--- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java
+++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java
@@ -2001,6 +2001,38 @@ public class PolarisManagementServiceIntegrationTest {
}
}
+ @Test
+ public void testAddGrantIsIdempotent() {
+ String catalogName = client.newEntityName("mycatalog_addgrant_idempotent");
+ Catalog catalog =
+ PolarisCatalog.builder()
+ .setType(Catalog.TypeEnum.INTERNAL)
+ .setName(catalogName)
+ .setStorageConfigInfo(new AwsStorageConfigInfo(StorageConfigInfo.StorageTypeEnum.S3))
+ .setProperties(new CatalogProperties("s3://bucket1/"))
+ .build();
+ managementApi.createCatalog(catalog);
+
+ String catalogRoleName = client.newEntityName("mycr_addgrant_idempotent");
+ managementApi.createCatalogRole(catalogName, catalogRoleName);
+
+ CatalogGrant grant =
+ new CatalogGrant(CatalogPrivilege.TABLE_WRITE_DATA, GrantResource.TypeEnum.CATALOG);
+
+ // First PUT creates the grant.
+ managementApi.addGrant(catalogName, catalogRoleName, grant);
+ // Second PUT of the same grant must succeed idempotently.
+ managementApi.addGrant(catalogName, catalogRoleName, grant);
+
+ // The grant should appear exactly once.
+ assertThat(managementApi.listGrants(catalogName, catalogRoleName).getGrants())
+ .filteredOn(
+ g ->
+ g instanceof CatalogGrant cg
+ && cg.getPrivilege() == CatalogPrivilege.TABLE_WRITE_DATA)
+ .hasSize(1);
+ }
+
@Test
public void testServiceAdminCanTransferCatalogAdmin() {
// Create a PrincipalRole and a new catalog. Grant the catalog_admin role to the new principal
diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java
index 7967c11954..cea7f58b95 100644
--- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java
+++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java
@@ -54,7 +54,9 @@ public class DatasourceOperations {
private static final Logger LOGGER = LoggerFactory.getLogger(DatasourceOperations.class);
// PG STATUS CODES
- private static final String CONSTRAINT_VIOLATION_SQL_CODE = "23505";
+ // 23505 = unique key violation, consistent across PG/Cockroach/H2; other checks (FK, NOT NULL,
+ // CHECK) all propagate
+ private static final String UNIQUENESS_CONSTRAINT_VIOLATION_SQL_CODE = "23505";
private static final String RELATION_DOES_NOT_EXIST = "42P01";
// H2 STATUS CODES
@@ -407,8 +409,8 @@ public class DatasourceOperations {
boolean execute(Connection connection) throws SQLException;
}
- public boolean isConstraintViolation(SQLException e) {
- return CONSTRAINT_VIOLATION_SQL_CODE.equals(e.getSQLState());
+ public boolean isUniquenessConstraintViolation(SQLException e) {
+ return UNIQUENESS_CONSTRAINT_VIOLATION_SQL_CODE.equals(e.getSQLState());
}
public boolean isRelationDoesNotExist(SQLException e) {
diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
index bbc917bc0b..ea1f211edc 100644
--- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
+++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
@@ -193,7 +193,7 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
values,
realmId));
} catch (SQLException e) {
- if (datasourceOperations.isConstraintViolation(e)) {
+ if (datasourceOperations.isUniquenessConstraintViolation(e)) {
PolarisBaseEntity existingEntity =
lookupEntityByName(
callCtx,
@@ -265,6 +265,10 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
QueryGenerator.generateInsertQuery(
ModelGrantRecord.ALL_COLUMNS, ModelGrantRecord.TABLE_NAME, values, realmId));
} catch (SQLException e) {
+ if (datasourceOperations.isUniquenessConstraintViolation(e)) {
+ LOGGER.debug("Grant record already exists; treating as no-op: {}", grantRec);
+ return;
+ }
throw new RuntimeException(
String.format("Failed to write to grant records due to %s", e.getMessage()), e);
}
@@ -926,7 +930,7 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
.toList(),
realmId));
} catch (SQLException e) {
- if (datasourceOperations.isConstraintViolation(e)) {
+ if (datasourceOperations.isUniquenessConstraintViolation(e)) {
throw new AlreadyExistsException(e.getMessage(), e);
}
LOGGER.error(
diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java
index 7b8d82f819..11be0b3813 100644
--- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java
+++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStore.java
@@ -83,7 +83,7 @@ public class RelationalJdbcIdempotencyStore implements IdempotencyStore {
datasourceOperations.executeUpdate(insert);
return new ReserveResult(ReserveResultType.OWNED, Optional.empty());
} catch (SQLException e) {
- if (datasourceOperations.isConstraintViolation(e)) {
+ if (datasourceOperations.isUniquenessConstraintViolation(e)) {
return new ReserveResult(ReserveResultType.DUPLICATE, load(realmId, idempotencyKey));
}
throw new IdempotencyPersistenceException("Failed to reserve idempotency key", e);
diff --git a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcGrantRecordsIdempotencyTest.java b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcGrantRecordsIdempotencyTest.java
new file mode 100644
index 0000000000..41e9182296
--- /dev/null
+++ b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcGrantRecordsIdempotencyTest.java
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.relational.jdbc;
+
+import static org.apache.polaris.core.persistence.PrincipalSecretsGenerator.RANDOM_SECRETS;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.sql.SQLException;
+import java.util.Optional;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.PolarisDefaultDiagServiceImpl;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.entity.PolarisGrantRecord;
+import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
+import org.h2.jdbcx.JdbcConnectionPool;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.Mockito;
+
+class JdbcGrantRecordsIdempotencyTest {
+
+ private static final RealmContext REALM_CONTEXT = () -> "REALM";
+
+ private static final long SECURABLE_CATALOG_ID = 1L;
+ private static final long SECURABLE_ID = 2L;
+ private static final long GRANTEE_CATALOG_ID = 3L;
+ private static final long GRANTEE_ID = 4L;
+ private static final int PRIVILEGE_CODE = 21;
+
+ @ParameterizedTest
+ @ValueSource(ints = {1, 2, 3, 4})
+ void writeToGrantRecordsIsIdempotent(int schemaVersion) throws SQLException {
+ JdbcConnectionPool dataSource =
+ JdbcConnectionPool.create(
+ "jdbc:h2:mem:grant_idempotency_v"
+ + schemaVersion
+ + "_"
+ + System.nanoTime()
+ + ";DB_CLOSE_DELAY=-1",
+ "sa",
+ "");
+ DatasourceOperations datasourceOperations =
+ new DatasourceOperations(dataSource, new TestJdbcConfiguration());
+ try (InputStream scriptStream = DatabaseType.H2.openInitScriptResource(schemaVersion)) {
+ datasourceOperations.executeScript(scriptStream);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
+ JdbcBasePersistenceImpl basePersistence =
+ new JdbcBasePersistenceImpl(
+ new PolarisDefaultDiagServiceImpl(),
+ datasourceOperations,
+ RANDOM_SECRETS,
+ mock(PolarisStorageIntegrationProvider.class),
+ REALM_CONTEXT.getRealmIdentifier(),
+ schemaVersion);
+ PolarisCallContext callCtx = new PolarisCallContext(REALM_CONTEXT, basePersistence);
+
+ PolarisGrantRecord grant =
+ new PolarisGrantRecord(
+ SECURABLE_CATALOG_ID, SECURABLE_ID, GRANTEE_CATALOG_ID, GRANTEE_ID, PRIVILEGE_CODE);
+
+ assertThatCode(() -> basePersistence.writeToGrantRecords(callCtx, grant))
+ .doesNotThrowAnyException();
+ assertThatCode(() -> basePersistence.writeToGrantRecords(callCtx, grant))
+ .doesNotThrowAnyException();
+ }
+
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "23502", // NOT NULL violation
+ "23503", // FK violation
+ "23513", // CHECK violation
+ "08006", // connection failure
+ "40001" // serialization failure
+ })
+ void writeToGrantRecords_IdempotencyCheck_OnlyCatchesPKUniqueness(String sqlStateCode)
+ throws SQLException {
+ SQLException nonUniqueViolation = new SQLException(sqlStateCode, sqlStateCode);
+
+ DatasourceOperations datasourceOperations = Mockito.mock(DatasourceOperations.class);
+ when(datasourceOperations.getDatabaseType()).thenReturn(DatabaseType.H2);
+ doThrow(nonUniqueViolation)
+ .when(datasourceOperations)
+ .executeUpdate(any(QueryGenerator.PreparedQuery.class));
+ doCallRealMethod()
+ .when(datasourceOperations)
+ .isUniquenessConstraintViolation(any(SQLException.class));
+
+ int schemaVersion = 4;
+ JdbcBasePersistenceImpl basePersistence =
+ new JdbcBasePersistenceImpl(
+ new PolarisDefaultDiagServiceImpl(),
+ datasourceOperations,
+ RANDOM_SECRETS,
+ mock(PolarisStorageIntegrationProvider.class),
+ REALM_CONTEXT.getRealmIdentifier(),
+ schemaVersion);
+ PolarisCallContext callCtx = new PolarisCallContext(REALM_CONTEXT, basePersistence);
+
+ PolarisGrantRecord grant =
+ new PolarisGrantRecord(
+ SECURABLE_CATALOG_ID, SECURABLE_ID, GRANTEE_CATALOG_ID, GRANTEE_ID, PRIVILEGE_CODE);
+
+ assertThatExceptionOfType(RuntimeException.class)
+ .isThrownBy(() -> basePersistence.writeToGrantRecords(callCtx, grant))
+ .withMessageContaining("Failed to write to grant records")
+ .withCause(nonUniqueViolation);
+ }
+
+ private static final class TestJdbcConfiguration implements RelationalJdbcConfiguration {
+ @Override
+ public Optional<Integer> maxRetries() {
+ return Optional.of(2);
+ }
+
+ @Override
+ public Optional<Long> maxDurationInMs() {
+ return Optional.of(100L);
+ }
+
+ @Override
+ public Optional<Long> initialDelayInMs() {
+ return Optional.of(100L);
+ }
+
+ @Override
+ public Optional<String> databaseType() {
+ return Optional.of("h2");
+ }
+ }
+}
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
index 60e4f9de08..6415cc0d42 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
@@ -133,11 +133,12 @@ public interface BasePersistence extends PolicyMappingPersistence, MetricsPersis
/**
* Write the specified grantRecord to the grant_records table. If there is a conflict (existing
- * record with the same PK), all attributes of the new record will replace the existing one.
+ * record with the same PK), this is a no-op, because currently all fields of the grantRecord are
+ * part of the PK. If additional non-PK attributes are added this might change.
*
* @param callCtx call context
- * @param grantRec entity record to write, potentially replacing an existing entity record with
- * the same key
+ * @param grantRec entity record to write; no update occurs if identical to an already-persisted
+ * grantRecord
*/
void writeToGrantRecords(
@NonNull PolarisCallContext callCtx, @NonNull PolarisGrantRecord grantRec);