Skip to content

Propagate SQLException as "caused by" #1430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 23, 2025
Merged

Propagate SQLException as "caused by" #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89f4c79
Propagate SQLException as "caused by"
dimas-b Apr 22, 2025
7f4a546
review: new ctor for EntityAlreadyExistsException
dimas-b Apr 23, 2025
21c3af9
fix CI
dimas-b Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 29 additions & 24 deletions ...ava/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,10 @@ public void writeEntity(
} catch (SQLException e) {
if ((datasourceOperations.isConstraintViolation(e)
|| datasourceOperations.isAlreadyExistsException(e))) {
throw new EntityAlreadyExistsException(entity);
throw new EntityAlreadyExistsException(entity, e);
} 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 {
Expand All @@ -122,7 +122,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);
Copy link
Contributor

@flyrain flyrain Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend to introduce a new exception for JDBC implementation instead of using the general one RuntimeException. We may name it like JdbcPersistenceException, which extend RuntimeException. Not a blocker though.

Copy link
Contributor Author

@dimas-b dimas-b Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I do not see any immediate benefit of using a new exception type unless we catch it specifically.

Let's merge this PR and if we have to catch (or do instanceof) JDBC specific exceptions in the future, then we'll add those sub-types.

}
}
}
Expand Down Expand Up @@ -169,10 +169,10 @@ public void writeEntities(
} catch (SQLException e) {
if ((datasourceOperations.isConstraintViolation(e)
|| datasourceOperations.isAlreadyExistsException(e))) {
throw new EntityAlreadyExistsException(entity);
throw new EntityAlreadyExistsException(entity, e);
} 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 {
Expand All @@ -198,7 +198,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);
}
}
}
Expand All @@ -207,7 +207,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);
}
}

Expand All @@ -220,7 +221,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);
}
}

Expand All @@ -239,7 +240,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);
}
}

Expand All @@ -252,7 +253,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);
}
}

Expand All @@ -266,7 +267,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);
}
}

Expand All @@ -277,7 +278,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);
}
}

Expand Down Expand Up @@ -331,7 +333,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);
}
}

Expand All @@ -346,7 +348,7 @@ public List<PolarisBaseEntity> 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);
}
}

Expand Down Expand Up @@ -440,7 +442,7 @@ public <T> List<T> 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);
}
}

Expand Down Expand Up @@ -494,7 +496,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);
}
}

Expand Down Expand Up @@ -524,7 +526,8 @@ public List<PolarisGrantRecord> 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);
}
}

Expand All @@ -549,7 +552,8 @@ public List<PolarisGrantRecord> 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);
}
}

Expand All @@ -575,8 +579,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);
}
}

Expand All @@ -602,7 +606,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);
}
}

Expand Down Expand Up @@ -638,7 +642,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;
Expand Down Expand Up @@ -696,7 +701,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
Expand All @@ -718,7 +723,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);
}
}

Expand Down
13 changes: 11 additions & 2 deletions ...-core/src/main/java/org/apache/polaris/core/persistence/EntityAlreadyExistsException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,25 @@
* 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(existingEntity.getName() + ":" + existingEntity.getId());
super(message(existingEntity));
this.existingEntity = 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;
}
Expand Down