From 89f4c795ac96e55098a4d8dc8e3db29d063d2cf1 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Tue, 22 Apr 2025 19:29:06 -0400 Subject: [PATCH 1/3] Propagate SQLException as "caused by" * Improves debuggability (full exception info will be available whenever the wrapper exception is logged) * Improves observability (e.g. Open Telemetry exception info will have full details and (manual) correlation with log messages will not be necessary) --- .../jdbc/JdbcBasePersistenceImpl.java | 57 +++++++++++-------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java index 4317ba1095..655a5140f7 100644 --- a/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java +++ b/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java @@ -95,10 +95,12 @@ public void writeEntity( } catch (SQLException e) { if ((datasourceOperations.isConstraintViolation(e) || datasourceOperations.isAlreadyExistsException(e))) { - throw new EntityAlreadyExistsException(entity); + EntityAlreadyExistsException ee = new EntityAlreadyExistsException(entity); + ee.initCause(e); + throw ee; } else { throw new RuntimeException( - String.format("Failed to write entity due to %s", e.getMessage())); + String.format("Failed to write entity due to %s", e.getMessage()), e); } } } else { @@ -122,7 +124,7 @@ public void writeEntity( } } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to write entity due to %s", e.getMessage())); + String.format("Failed to write entity due to %s", e.getMessage()), e); } } } @@ -169,10 +171,12 @@ public void writeEntities( } catch (SQLException e) { if ((datasourceOperations.isConstraintViolation(e) || datasourceOperations.isAlreadyExistsException(e))) { - throw new EntityAlreadyExistsException(entity); + EntityAlreadyExistsException ee = new EntityAlreadyExistsException(entity); + ee.initCause(e); + throw ee; } else { throw new RuntimeException( - String.format("Failed to write entity due to %s", e.getMessage())); + String.format("Failed to write entity due to %s", e.getMessage()), e); } } } else { @@ -198,7 +202,7 @@ public void writeEntities( } } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to write entity due to %s", e.getMessage())); + String.format("Failed to write entity due to %s", e.getMessage()), e); } } } @@ -207,7 +211,8 @@ public void writeEntities( } catch (SQLException e) { throw new RuntimeException( String.format( - "Error executing the transaction for writing entities due to %s", e.getMessage())); + "Error executing the transaction for writing entities due to %s", e.getMessage()), + e); } } @@ -220,7 +225,7 @@ public void writeToGrantRecords( datasourceOperations.executeUpdate(query); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to write to grant records due to %s", e.getMessage())); + String.format("Failed to write to grant records due to %s", e.getMessage()), e); } } @@ -239,7 +244,7 @@ public void deleteEntity(@Nonnull PolarisCallContext callCtx, @Nonnull PolarisBa datasourceOperations.executeUpdate(generateDeleteQuery(ModelEntity.class, params)); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to delete entity due to %s", e.getMessage())); + String.format("Failed to delete entity due to %s", e.getMessage()), e); } } @@ -252,7 +257,7 @@ public void deleteFromGrantRecords( datasourceOperations.executeUpdate(query); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to delete from grant records due to %s", e.getMessage())); + String.format("Failed to delete from grant records due to %s", e.getMessage()), e); } } @@ -266,7 +271,7 @@ public void deleteAllEntityGrantRecords( datasourceOperations.executeUpdate(generateDeleteQueryForEntityGrantRecords(entity, realmId)); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to delete grant records due to %s", e.getMessage())); + String.format("Failed to delete grant records due to %s", e.getMessage()), e); } } @@ -277,7 +282,8 @@ public void deleteAll(@Nonnull PolarisCallContext callCtx) { datasourceOperations.executeUpdate(generateDeleteAll(ModelGrantRecord.class, realmId)); datasourceOperations.executeUpdate(generateDeleteAll(ModelEntity.class, realmId)); } catch (SQLException e) { - throw new RuntimeException(String.format("Failed to delete all due to %s", e.getMessage())); + throw new RuntimeException( + String.format("Failed to delete all due to %s", e.getMessage()), e); } } @@ -331,7 +337,7 @@ private PolarisBaseEntity getPolarisBaseEntity(String query) { } } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to retrieve polaris entity due to %s", e.getMessage())); + String.format("Failed to retrieve polaris entity due to %s", e.getMessage()), e); } } @@ -346,7 +352,7 @@ public List lookupEntities( query, ModelEntity.class, ModelEntity::toEntity, null, Integer.MAX_VALUE); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to retrieve polaris entities due to %s", e.getMessage())); + String.format("Failed to retrieve polaris entities due to %s", e.getMessage()), e); } } @@ -440,7 +446,7 @@ public List listEntities( : results.stream().filter(entityFilter).map(transformer).collect(Collectors.toList()); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to retrieve polaris entities due to %s", e.getMessage())); + String.format("Failed to retrieve polaris entities due to %s", e.getMessage()), e); } } @@ -494,7 +500,7 @@ public PolarisGrantRecord lookupGrantRecord( return results.getFirst(); } catch (SQLException e) { throw new RuntimeException( - String.format("Failed to retrieve grant record due to %s", e.getMessage())); + String.format("Failed to retrieve grant record due to %s", e.getMessage()), e); } } @@ -524,7 +530,8 @@ public List loadAllGrantRecordsOnSecurable( throw new RuntimeException( String.format( "Failed to retrieve grant records for securableCatalogId: %s securableId: %s due to %s", - securableCatalogId, securableId, e.getMessage())); + securableCatalogId, securableId, e.getMessage()), + e); } } @@ -549,7 +556,8 @@ public List loadAllGrantRecordsOnGrantee( throw new RuntimeException( String.format( "Failed to retrieve grant records for granteeCatalogId: %s granteeId: %s due to %s", - granteeCatalogId, granteeId, e.getMessage())); + granteeCatalogId, granteeId, e.getMessage()), + e); } } @@ -575,8 +583,8 @@ public boolean hasChildren( } catch (SQLException e) { throw new RuntimeException( String.format( - "Failed to retrieve entities for catalogId: %s due to %s", - catalogId, e.getMessage())); + "Failed to retrieve entities for catalogId: %s due to %s", catalogId, e.getMessage()), + e); } } @@ -602,7 +610,7 @@ public PolarisPrincipalSecrets loadPrincipalSecrets( e.getMessage(), e); throw new RuntimeException( - String.format("Failed to retrieve principal secrets for clientId: %s", clientId)); + String.format("Failed to retrieve principal secrets for clientId: %s", clientId), e); } } @@ -638,7 +646,8 @@ public PolarisPrincipalSecrets generateNewPrincipalSecrets( e); throw new RuntimeException( String.format( - "Failed to generate new principal secrets for principalId: %s", principalId)); + "Failed to generate new principal secrets for principalId: %s", principalId), + e); } // if not found, return null return principalSecrets; @@ -696,7 +705,7 @@ public PolarisPrincipalSecrets rotatePrincipalSecrets( e.getMessage(), e); throw new RuntimeException( - String.format("Failed to rotatePrincipalSecrets for clientId: %s", clientId)); + String.format("Failed to rotatePrincipalSecrets for clientId: %s", clientId), e); } // return those @@ -718,7 +727,7 @@ public void deletePrincipalSecrets( e.getMessage(), e); throw new RuntimeException( - String.format("Failed to delete principalSecrets for clientId: %s", clientId)); + String.format("Failed to delete principalSecrets for clientId: %s", clientId), e); } } From 7f4a546cc7f14f6367ae4339ad320139b276face Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Wed, 23 Apr 2025 11:56:41 -0400 Subject: [PATCH 2/3] review: new ctor for EntityAlreadyExistsException --- .../relational/jdbc/JdbcBasePersistenceImpl.java | 8 ++------ .../core/persistence/EntityAlreadyExistsException.java | 10 +++++++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java index 655a5140f7..24bb927027 100644 --- a/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java +++ b/extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java @@ -95,9 +95,7 @@ public void writeEntity( } catch (SQLException e) { if ((datasourceOperations.isConstraintViolation(e) || datasourceOperations.isAlreadyExistsException(e))) { - EntityAlreadyExistsException ee = new EntityAlreadyExistsException(entity); - ee.initCause(e); - throw ee; + throw new EntityAlreadyExistsException(entity, e); } else { throw new RuntimeException( String.format("Failed to write entity due to %s", e.getMessage()), e); @@ -171,9 +169,7 @@ public void writeEntities( } catch (SQLException e) { if ((datasourceOperations.isConstraintViolation(e) || datasourceOperations.isAlreadyExistsException(e))) { - EntityAlreadyExistsException ee = new EntityAlreadyExistsException(entity); - ee.initCause(e); - throw ee; + throw new EntityAlreadyExistsException(entity, e); } else { throw new RuntimeException( String.format("Failed to write entity due to %s", e.getMessage()), e); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java index e31e2ae02c..0802349154 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java @@ -31,10 +31,18 @@ public class EntityAlreadyExistsException extends RuntimeException { * @param existingEntity The conflicting entity that caused creation to fail. */ public EntityAlreadyExistsException(PolarisBaseEntity existingEntity) { - super(existingEntity.getName() + ":" + existingEntity.getId()); + super(message(existingEntity)); + } + + public EntityAlreadyExistsException(PolarisBaseEntity existingEntity, Throwable cause) { + super(message(existingEntity), cause); this.existingEntity = existingEntity; } + private static String message(PolarisBaseEntity existingEntity) { + return existingEntity.getName() + ":" + existingEntity.getId(); + } + public PolarisBaseEntity getExistingEntity() { return this.existingEntity; } From 21c3af975a5233388c9e70de60e0b0163f4abc12 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Wed, 23 Apr 2025 14:56:11 -0400 Subject: [PATCH 3/3] fix CI --- .../polaris/core/persistence/EntityAlreadyExistsException.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java index 0802349154..e421bf8f3e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java @@ -25,13 +25,14 @@ * creation of a new entity; provides a member holding the conflicting entity. */ public class EntityAlreadyExistsException extends RuntimeException { - private PolarisBaseEntity existingEntity; + private final PolarisBaseEntity existingEntity; /** * @param existingEntity The conflicting entity that caused creation to fail. */ public EntityAlreadyExistsException(PolarisBaseEntity existingEntity) { super(message(existingEntity)); + this.existingEntity = existingEntity; } public EntityAlreadyExistsException(PolarisBaseEntity existingEntity, Throwable cause) {