(gravitino) branch main updated: [#8661] fix(server): Correct exception handling in setPolicy and corresponding unit test (#8681)

[mchades]

This is an automated email from the ASF dual-hosted git repository.

mchades pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git


The following commit(s) were added to refs/heads/main by this push:
     new 0b63aa9a70 [#8661] fix(server): Correct exception handling in 
setPolicy and corresponding unit test (#8681)
0b63aa9a70 is described below

commit 0b63aa9a70f5c184746228e2597360c149d1f756
Author: Samyak Jain <115939022+kaymas2...@users.noreply.github.com>
AuthorDate: Thu Sep 25 14:57:16 2025 +0530

    [#8661] fix(server): Correct exception handling in setPolicy and 
corresponding unit test (#8681)
    
    ### What changes were proposed in this pull request?
    
    This PR fixes a bug in the exception handling of the `setPolicy` method
    in `PolicyOperations.java`.
    
    The `OperationType` variable is now determined from the request
    **before** the `try` block to ensure it's available in the `catch`
    block. The `catch` block now uses this dynamic variable instead of a
    hardcoded `OperationType.ENABLE`.
    
    ### Why are the changes needed?
    
    Previously, if an exception occurred while a user was trying to
    **disable** a policy via the PATCH API endpoint, the error handler would
    incorrectly report it as a failure of an **enable** operation. This was
    misleading and made debugging difficult.
    
    Fix: #8661
    
    ### Does this PR introduce _any_ user-facing change?
    
    Yes. The error response returned to an API user when a `disable` policy
    operation fails is now correctly identified. There are no changes to the
    API's structure or success paths.
    
    ### How was this patch tested?
    
    - Added a new unit test, `testSetPolicyDisableFailure`, to
    `TestPolicyOperations.java`.
    - This test specifically mocks a `RuntimeException` during a
    `disablePolicy()` call.
    - It verifies that the API returns the expected `500 Internal Server
    Error` and that the error payload is correct.
    - Confirmed that all existing tests in the `:server` module continue to
    pass after the change.
---
 .../server/web/rest/PolicyOperations.java          |  6 ++++--
 .../server/web/rest/TestPolicyOperations.java      | 23 ++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git 
a/server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java
 
b/server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java
index 634545e349..dd5d61fc55 100644
--- 
a/server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java
+++ 
b/server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java
@@ -208,11 +208,13 @@ public class PolicyOperations {
       PolicySetRequest request) {
     LOG.info("Received set policy request for policy: {} under metalake: {}", 
name, metalake);
 
+    OperationType op = request.isEnable() ? OperationType.ENABLE : 
OperationType.DISABLE;
+
     try {
       return Utils.doAs(
           httpRequest,
           () -> {
-            if (request.isEnable()) {
+            if (op == OperationType.ENABLE) {
               policyDispatcher.enablePolicy(metalake, name);
             } else {
               policyDispatcher.disablePolicy(metalake, name);
@@ -227,7 +229,7 @@ public class PolicyOperations {
             return response;
           });
     } catch (Exception e) {
-      return ExceptionHandlers.handlePolicyException(OperationType.ENABLE, 
name, metalake, e);
+      return ExceptionHandlers.handlePolicyException(op, name, metalake, e);
     }
   }
 
diff --git 
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestPolicyOperations.java
 
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestPolicyOperations.java
index 892bc7c785..a9899aecac 100644
--- 
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestPolicyOperations.java
+++ 
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestPolicyOperations.java
@@ -514,6 +514,29 @@ public class TestPolicyOperations extends JerseyTest {
     Assertions.assertEquals(0, baseResponse1.getCode());
   }
 
+  // Test to check exception on failure in set policy is dynamic based on 
enable/disable
+  @Test
+  public void testSetPolicyDisableFailure() {
+    PolicySetRequest req = new PolicySetRequest(false);
+    doThrow(new RuntimeException("mock disable exception"))
+        .when(policyManager)
+        .disablePolicy(any(), any());
+    Response resp =
+        target(policyPath(metalake))
+            .path("policy1")
+            .property(HttpUrlConnectorProvider.SET_METHOD_WORKAROUND, true)
+            .request(MediaType.APPLICATION_JSON_TYPE)
+            .accept("application/vnd.gravitino.v1+json")
+            .method("PATCH", Entity.entity(req, 
MediaType.APPLICATION_JSON_TYPE));
+
+    Assertions.assertEquals(
+        Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), 
resp.getStatus());
+
+    ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+    Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, 
errorResp.getCode());
+    Assertions.assertEquals(RuntimeException.class.getSimpleName(), 
errorResp.getType());
+  }
+
   @Test
   public void testDeletePolicy() {
     when(policyManager.deletePolicy(metalake, "policy1")).thenReturn(true);