Surface persistence exceptions to the caller more often

polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java

@@ -190,4 +190,14 @@ protected FeatureConfiguration(
           .description("If true, the generic-tables endpoints are enabled")
           .defaultValue(false)
           .buildFeatureConfiguration();
+
+  public static final FeatureConfiguration<Boolean> DETAILED_PERSISTENCE_EXCEPTIONS =
+      PolarisConfiguration.<Boolean>builder()
+          .key("DETAILED_PERSISTENCE_EXCEPTIONS")
+          .description(
+              "If true, APIs like dropTable and dropGenericTable will report more detailed exceptions"
+                  + " when a call to persistence fails. These exceptions may include corrected error codes and may"
+                  + " result in improved retry behavior.")
+          .defaultValue(true)
+          .buildFeatureConfiguration();
 }

polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java

@@ -22,9 +22,22 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
+import java.util.Optional;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
+import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
+import org.apache.iceberg.exceptions.NotFoundException;
+import org.apache.iceberg.exceptions.UnprocessableEntityException;
+import org.apache.polaris.core.config.FeatureConfiguration;
+import org.apache.polaris.core.context.CallContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /** Base result class for any call to the persistence layer */
 public class BaseResult {
+  private static final Logger LOGGER = LoggerFactory.getLogger(BaseResult.class);
+
   // return code, indicates success or failure
   private final int returnStatusCode;
 
@@ -57,14 +70,90 @@ public String getExtraInformation() {
     return extraInformation;
   }
 
-  public boolean isSuccess() {
+  public final boolean isSuccess() {
     return this.returnStatusCode == ReturnStatus.SUCCESS.getCode();
   }
 
-  public boolean alreadyExists() {
+  public final boolean alreadyExists() {
     return this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode();
   }
 
+  /**
+   * If this result is not a successful one, this builds an exception from the failed result which
+   * the exception mapper can use to provide the caller some useful information about the failure.
+   * The message relies on `extraInformation`. If {@link
+   * org.apache.polaris.core.config.FeatureConfiguration#DETAILED_PERSISTENCE_EXCEPTIONS} is false,
+   * this should return an empty option.
+   */
+  @SuppressWarnings("FormatStringAnnotation")
+  public Optional<RuntimeException> getException(CallContext callContext) {
+    if (!callContext
+        .getPolarisCallContext()
+        .getConfigurationStore()
+        .getConfiguration(
+            callContext.getPolarisCallContext(),
+            FeatureConfiguration.DETAILED_PERSISTENCE_EXCEPTIONS)) {
+      return Optional.empty();
+    }
+
+    String message = this.extraInformation;
+    if (this.extraInformation == null) {
+      // TODO this should ideally never be hit but it's hit often.
+      // We should raise the logging level once it's less common
+      LOGGER.debug(
+          "A {} was discovered with status {} but without a detailed message",
+          this.getClass().getName(),
+          ReturnStatus.getStatus(this.returnStatusCode).name());
+      message = "Error reported by the metastore";
+    }
+    // TODO use a switch expression if the Java version here is ever raised
+    if (this.returnStatusCode == ReturnStatus.SUCCESS.getCode()) {
+      return Optional.empty();
+    } else if (this.returnStatusCode == ReturnStatus.UNEXPECTED_ERROR_SIGNALED.getCode()) {
+      return Optional.of(new RuntimeException(message));
+    } else if (this.returnStatusCode == ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED.getCode()) {
+      return Optional.of(new NotFoundException(message));
+    } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RESOLVED.getCode()) {
+      return Optional.of(new NotFoundException(message));
+    } else if (this.returnStatusCode == ReturnStatus.ENTITY_NOT_FOUND.getCode()) {
+      return Optional.of(new NotFoundException(message));
+    } else if (this.returnStatusCode == ReturnStatus.GRANT_NOT_FOUND.getCode()) {
+      return Optional.of(new NotFoundException(message));
+    } else if (this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode()) {
+      return Optional.of(new AlreadyExistsException(message));
+    } else if (this.returnStatusCode == ReturnStatus.NAMESPACE_NOT_EMPTY.getCode()) {
+      return Optional.of(new NamespaceNotEmptyException(message));
+    } else if (this.returnStatusCode == ReturnStatus.CATALOG_NOT_EMPTY.getCode()) {
+      return Optional.of(new BadRequestException(message));
+    } else if (this.returnStatusCode
+        == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) {
+      return Optional.of(new CommitFailedException(message));
+    } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RENAMED.getCode()) {
+      return Optional.of(new BadRequestException(message));
+    } else if (this.returnStatusCode == ReturnStatus.SUBSCOPE_CREDS_ERROR.getCode()) {
+      return Optional.of(new UnprocessableEntityException(message));
+    } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_NOT_FOUND.getCode()) {
+      return Optional.of(new NotFoundException(message));
+    } else if (this.returnStatusCode
+        == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) {
+      return Optional.of(new AlreadyExistsException(message));
+    } else {
+      return Optional.of(new RuntimeException(message));
+    }
+  }
+
+  /**
+   * If this result is failed, this should throw the appropriate exception which corresponds to the
+   * result status. See {@link BaseResult#getException} for details. If {@link
+   * org.apache.polaris.core.config.FeatureConfiguration#DETAILED_PERSISTENCE_EXCEPTIONS} is false,
+   * nothing should be thrown.
+   */
+  public void maybeThrowException(CallContext callContext) throws RuntimeException {
+    if (this.getException(callContext).isPresent()) {
+      throw this.getException(callContext).get();
+    }
+  }
+
   /** Possible return code for the various API calls. */
   public enum ReturnStatus {
     // all good

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java

@@ -41,6 +41,7 @@
 import org.apache.iceberg.Schema;
 import org.apache.iceberg.catalog.Namespace;
 import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.NotFoundException;
 import org.apache.iceberg.types.Types;
 import org.apache.polaris.core.PolarisCallContext;
 import org.apache.polaris.core.PolarisDiagnostics;
@@ -546,7 +547,8 @@ public void testDropViaIceberg() {
     genericTableCatalog.createGenericTable(
         TableIdentifier.of("ns", "t1"), "format", "doc", Map.of());
 
-    Assertions.assertThat(icebergCatalog.dropTable(TableIdentifier.of("ns", "t1"))).isFalse();
+    Assertions.assertThatCode(() -> icebergCatalog.dropTable(TableIdentifier.of("ns", "t1")))
+        .isInstanceOf(NotFoundException.class);
     Assertions.assertThat(genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", "t1")))
         .isNotNull();
   }
@@ -558,7 +560,8 @@ public void testDropViaIcebergView() {
     genericTableCatalog.createGenericTable(
         TableIdentifier.of("ns", "t1"), "format", "doc", Map.of());
 
-    Assertions.assertThat(icebergCatalog.dropView(TableIdentifier.of("ns", "t1"))).isFalse();
+    Assertions.assertThatCode(() -> icebergCatalog.dropView(TableIdentifier.of("ns", "t1")))
+        .isInstanceOf(NotFoundException.class);
     Assertions.assertThat(genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", "t1")))
         .isNotNull();
   }

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java

@@ -154,7 +154,9 @@ public Map<String, String> getConfigOverrides() {
           "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
           "true",
           "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"",
-          "[\"FILE\"]");
+          "[\"FILE\"]",
+          "polaris.features.defaults.\"DETAILED_PERSISTENCE_EXCEPTIONS\"",
+          "false");
     }
   }
 

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogViewTest.java

@@ -90,7 +90,9 @@ public Map<String, String> getConfigOverrides() {
           "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
           "true",
           "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"",
-          "[\"FILE\"]");
+          "[\"FILE\"]",
+          "polaris.features.defaults.\"DETAILED_PERSISTENCE_EXCEPTIONS\"",
+          "false");
     }
   }
 

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

@@ -603,18 +603,7 @@ public void deleteCatalog(String name) {
         metaStoreManager.dropEntityIfExists(
             getCurrentPolarisContext(), null, entity, Map.of(), cleanup);
 
-    // at least some handling of error
-    if (!dropEntityResult.isSuccess()) {
-      if (dropEntityResult.failedBecauseNotEmpty()) {
-        throw new BadRequestException(
-            "Catalog '%s' cannot be dropped, it is not empty", entity.getName());
-      } else {
-        throw new BadRequestException(
-            "Catalog '%s' cannot be dropped, concurrent modification detected. Please try "
-                + "again",
-            entity.getName());
-      }
-    }
+    dropEntityResult.maybeThrowException(callContext);
   }
 
   public @Nonnull CatalogEntity getCatalog(String name) {
@@ -804,16 +793,7 @@ public void deletePrincipal(String name) {
         metaStoreManager.dropEntityIfExists(
             getCurrentPolarisContext(), null, entity, Map.of(), false);
 
-    // at least some handling of error
-    if (!dropEntityResult.isSuccess()) {
-      if (dropEntityResult.isEntityUnDroppable()) {
-        throw new BadRequestException("Root principal cannot be dropped");
-      } else {
-        throw new BadRequestException(
-            "Root principal cannot be dropped, concurrent modification "
-                + "detected. Please try again");
-      }
-    }
+    dropEntityResult.maybeThrowException(callContext);
   }
 
   public @Nonnull PrincipalEntity getPrincipal(String name) {
@@ -963,21 +943,11 @@ public void deletePrincipalRole(String name) {
     PolarisEntity entity =
         findPrincipalRoleByName(name)
             .orElseThrow(() -> new NotFoundException("PrincipalRole %s not found", name));
-    // TODO: Handle return value in case of concurrent modification
     DropEntityResult dropEntityResult =
         metaStoreManager.dropEntityIfExists(
             getCurrentPolarisContext(), null, entity, Map.of(), true); // cleanup grants
 
-    // at least some handling of error
-    if (!dropEntityResult.isSuccess()) {
-      if (dropEntityResult.isEntityUnDroppable()) {
-        throw new BadRequestException("Polaris service admin principal role cannot be dropped");
-      } else {
-        throw new BadRequestException(
-            "Polaris service admin principal role cannot be dropped, "
-                + "concurrent modification detected. Please try again");
-      }
-    }
+    dropEntityResult.maybeThrowException(callContext);
   }
 
   public @Nonnull PrincipalRoleEntity getPrincipalRole(String name) {
@@ -1088,16 +1058,7 @@ public void deleteCatalogRole(String catalogName, String name) {
             Map.of(),
             true); // cleanup grants
 
-    // at least some handling of error
-    if (!dropEntityResult.isSuccess()) {
-      if (dropEntityResult.isEntityUnDroppable()) {
-        throw new BadRequestException("Catalog admin role cannot be dropped");
-      } else {
-        throw new BadRequestException(
-            "Catalog admin role cannot be dropped, concurrent "
-                + "modification detected. Please try again");
-      }
-    }
+    dropEntityResult.maybeThrowException(callContext);
   }
 
   public @Nonnull CatalogRoleEntity getCatalogRole(String catalogName, String name) {

service/common/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalog.java

@@ -158,6 +158,7 @@ public boolean dropGenericTable(TableIdentifier tableIdentifier) {
             Map.of(),
             false);
 
+    dropEntityResult.maybeThrowException(callContext);
     return dropEntityResult.isSuccess();
   }