#192 Adding more missing error handling

swisspost · Jul 3, 2024 · ffae879 · ffae879
1 parent cd093ff
commit ffae879
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 12 deletions.
diff --git a/src/main/java/org/swisspush/redisques/action/DeleteLockAction.java b/src/main/java/org/swisspush/redisques/action/DeleteLockAction.java
@@ -34,14 +34,18 @@ public void execute(Message<JsonObject> event) {
         var p = redisProvider.redis();
         p.onSuccess(redisAPI -> {
             redisAPI.exists(Collections.singletonList(queuesPrefix + queueName), event1 -> {
-                if( event1.failed() ) log.warn("Concealed error", new Exception(event1.cause()));
+                if (event1.failed()) {
+                    log.warn("Concealed error", exceptionFactory.newException(event1.cause()));
+                }
+
                 if (event1.succeeded() && event1.result() != null && event1.result().toInteger() == 1) {
                     notifyConsumer(queueName);
                 }
-                redisAPI.hdel(Arrays.asList(locksKey, queueName), new DeleteLockHandler(event));
+
+                redisAPI.hdel(Arrays.asList(locksKey, queueName), new DeleteLockHandler(event, exceptionFactory));
             });
         });
-        p.onFailure(ex -> replyErrorMessageHandler(event).handle(ex));
+        p.onFailure(ex -> handleFail(event, "Operation DeleteLockAction failed", ex));
     }
 
 }
diff --git a/src/main/java/org/swisspush/redisques/handler/DeleteLockHandler.java b/src/main/java/org/swisspush/redisques/handler/DeleteLockHandler.java
@@ -6,9 +6,9 @@
 import io.vertx.core.json.JsonObject;
 import io.vertx.redis.client.Response;
 import org.slf4j.Logger;
+import org.swisspush.redisques.exception.RedisQuesExceptionFactory;
 
 import static org.slf4j.LoggerFactory.getLogger;
-import static org.swisspush.redisques.util.RedisquesAPI.ERROR;
 import static org.swisspush.redisques.util.RedisquesAPI.OK;
 import static org.swisspush.redisques.util.RedisquesAPI.STATUS;
 
@@ -21,18 +21,20 @@ public class DeleteLockHandler implements Handler<AsyncResult<Response>> {
 
     private static final Logger log = getLogger(DeleteLockHandler.class);
     private final Message<JsonObject> event;
+    private final RedisQuesExceptionFactory exceptionFactory;
 
-    public DeleteLockHandler(Message<JsonObject> event) {
+    public DeleteLockHandler(Message<JsonObject> event, RedisQuesExceptionFactory exceptionFactory) {
         this.event = event;
+        this.exceptionFactory = exceptionFactory;
     }
 
     @Override
     public void handle(AsyncResult<Response> reply) {
         if (reply.succeeded()) {
             event.reply(new JsonObject().put(STATUS, OK));
         } else {
-            log.warn("Concealed error", new Exception(reply.cause()));
-            event.reply(new JsonObject().put(STATUS, ERROR));
+            log.warn("Concealed error", exceptionFactory.newException(reply.cause()));
+            event.fail(0, reply.cause().getMessage());
         }
     }
 }
diff --git a/src/main/java/org/swisspush/redisques/handler/RedisquesHttpRequestHandler.java b/src/main/java/org/swisspush/redisques/handler/RedisquesHttpRequestHandler.java
@@ -392,11 +392,12 @@ private void deleteSingleLock(RoutingContext ctx) {
                 queue -> eventBus.request(redisquesAddress, buildDeleteLockOperation(queue),
                         (Handler<AsyncResult<Message<JsonObject>>>) reply -> {
                             if (reply.failed()) {
-                                log.warn("Received failed message for deleteSingleLockOperation. Lets run into NullPointerException now", reply.cause());
-                                // IMO we should respond with 'HTTP 5xx'. But we don't, to keep backward compatibility.
-                                // Nevertheless. Lets run into NullPointerException by calling below method.
+                                String error = "Received failed message for deleteSingleLockOperation";
+                                log.warn(error, exceptionFactory.newException(reply.cause()));
+                                respondWith(StatusCode.INTERNAL_SERVER_ERROR, error, ctx.request());
+                            } else {
+                                checkReply(reply.result(), ctx.request(), StatusCode.INTERNAL_SERVER_ERROR);
                             }
-                            checkReply(reply.result(), ctx.request(), StatusCode.INTERNAL_SERVER_ERROR);
                         }));
     }
 

diff --git a/src/test/java/org/swisspush/redisques/action/DeleteLockActionTest.java b/src/test/java/org/swisspush/redisques/action/DeleteLockActionTest.java
@@ -5,6 +5,7 @@
 import io.vertx.core.json.JsonObject;
 import io.vertx.ext.unit.TestContext;
 import io.vertx.ext.unit.junit.VertxUnitRunner;
+import io.vertx.redis.client.impl.types.SimpleStringType;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -41,7 +42,76 @@ public void testDeleteLockWhenRedisIsNotReady(TestContext context){
 
         action.execute(message);
 
-        verify(message, times(1)).reply(eq(new JsonObject(Buffer.buffer("{\"status\":\"error\"}"))));
+        verify(message, times(1)).fail(eq(0), eq("not ready"));
         verifyNoInteractions(redisAPI);
     }
+
+    @Test
+    public void testDeleteLock(TestContext context){
+        when(message.body()).thenReturn(buildDeleteLockOperation("testLock1"));
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,1);
+            handler.handle(Future.succeededFuture(SimpleStringType.create("0")));
+            return null;
+        }).when(redisAPI).exists(anyList(), any());
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,1);
+            handler.handle(Future.succeededFuture());
+            return null;
+        }).when(redisAPI).hdel(anyList(), any());
+
+        action.execute(message);
+
+        verify(redisAPI, times(1)).exists(anyList(), any());
+        verify(redisAPI, times(1)).hdel(anyList(), any());
+        verify(message, times(1)).reply(eq(new JsonObject(Buffer.buffer("{\"status\":\"ok\"}"))));
+    }
+
+    @Test
+    public void testDeleteLockExistsFail(TestContext context){
+        when(message.body()).thenReturn(buildDeleteLockOperation("testLock1"));
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,1);
+            handler.handle(Future.failedFuture("booom"));
+            return null;
+        }).when(redisAPI).exists(anyList(), any());
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,1);
+            handler.handle(Future.succeededFuture());
+            return null;
+        }).when(redisAPI).hdel(anyList(), any());
+
+        action.execute(message);
+
+        verify(redisAPI, times(1)).exists(anyList(), any());
+        verify(redisAPI, times(1)).hdel(anyList(), any());
+        verify(message, times(1)).reply(eq(new JsonObject(Buffer.buffer("{\"status\":\"ok\"}"))));
+    }
+
+    @Test
+    public void testDeleteLockHDELFail(TestContext context){
+        when(message.body()).thenReturn(buildDeleteLockOperation("testLock1"));
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,1);
+            handler.handle(Future.succeededFuture(SimpleStringType.create("0")));
+            return null;
+        }).when(redisAPI).exists(anyList(), any());
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,1);
+            handler.handle(Future.failedFuture("booom"));
+            return null;
+        }).when(redisAPI).hdel(anyList(), any());
+
+        action.execute(message);
+
+        verify(redisAPI, times(1)).exists(anyList(), any());
+        verify(redisAPI, times(1)).hdel(anyList(), any());
+        verify(message, times(1)).fail(eq(0), eq("booom"));
+    }
 }