#192 Adding more missing error handling

swisspost · Jul 2, 2024 · cd093ff · cd093ff
1 parent 456d8a1
commit cd093ff
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 11 deletions.
diff --git a/src/main/java/org/swisspush/redisques/action/GetLockAction.java b/src/main/java/org/swisspush/redisques/action/GetLockAction.java
@@ -39,9 +39,9 @@ public void execute(Message<JsonObject> event) {
         }
         var p = redisProvider.redis();
         p.onSuccess(redisAPI -> {
-            redisAPI.hget(locksKey, body.getJsonObject(PAYLOAD).getString(QUEUENAME), new GetLockHandler(event));
+            redisAPI.hget(locksKey, body.getJsonObject(PAYLOAD).getString(QUEUENAME), new GetLockHandler(event, exceptionFactory));
         });
-        p.onFailure(ex -> replyErrorMessageHandler(event).handle(ex));
+        p.onFailure(ex -> handleFail(event,"Operation GetLockAction failed", ex));
     }
 
 }
diff --git a/src/main/java/org/swisspush/redisques/handler/GetLockHandler.java b/src/main/java/org/swisspush/redisques/handler/GetLockHandler.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.NO_SUCH_LOCK;
 import static org.swisspush.redisques.util.RedisquesAPI.OK;
 import static org.swisspush.redisques.util.RedisquesAPI.STATUS;
@@ -23,9 +23,11 @@ public class GetLockHandler implements Handler<AsyncResult<Response>> {
 
     private static final Logger log = getLogger(GetLockHandler.class);
     private final Message<JsonObject> event;
+    private final RedisQuesExceptionFactory exceptionFactory;
 
-    public GetLockHandler(Message<JsonObject> event) {
+    public GetLockHandler(Message<JsonObject> event, RedisQuesExceptionFactory exceptionFactory) {
         this.event = event;
+        this.exceptionFactory = exceptionFactory;
     }
 
     @Override
@@ -37,8 +39,8 @@ public void handle(AsyncResult<Response> reply) {
                 event.reply(new JsonObject().put(STATUS, NO_SUCH_LOCK));
             }
         } 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
@@ -358,10 +358,10 @@ private void getSingleLock(RoutingContext ctx) {
                 queue -> eventBus.request(redisquesAddress, buildGetLockOperation(queue), (Handler<AsyncResult<Message<JsonObject>>>) reply -> {
                     final HttpServerResponse response = ctx.response();
                     if (reply.failed()) {
-                        log.warn("Received failed message for getSingleLockOperation. Lets run into NullPointerException now", reply.cause());
-                        // IMO we should respond with 'HTTP 5xx' here. But we don't, to keep backward compatibility.
-                    }
-                    if (OK.equals(reply.result().body().getString(STATUS))) {
+                        String error = "Received failed message for getSingleLockOperation";
+                        log.warn(error, exceptionFactory.newException(reply.cause()));
+                        respondWith(StatusCode.INTERNAL_SERVER_ERROR, error, ctx.request());
+                    } else if (OK.equals(reply.result().body().getString(STATUS))) {
                         response.putHeader(CONTENT_TYPE, APPLICATION_JSON);
                         response.end(reply.result().body().getString(VALUE));
                     } else {

diff --git a/src/test/java/org/swisspush/redisques/action/GetLockActionTest.java b/src/test/java/org/swisspush/redisques/action/GetLockActionTest.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,56 @@ public void testGetLockWhenRedisIsNotReady(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 testGetLock(TestContext context){
+        when(message.body()).thenReturn(buildGetLockOperation("q1"));
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,2);
+            handler.handle(Future.succeededFuture(SimpleStringType.create(new JsonObject().put("requestedBy", "UNKNOWN").put("timestamp", 1719931433522L).encode())));
+            return null;
+        }).when(redisAPI).hget(anyString(), anyString(), any());
+
+        action.execute(message);
+
+        verify(redisAPI, times(1)).hget(anyString(), anyString(), any());
+        verify(message, times(1)).reply(eq(new JsonObject(Buffer.buffer("{\"status\":\"ok\"," +
+                "\"value\":\"{\\\"requestedBy\\\":\\\"UNKNOWN\\\",\\\"timestamp\\\":1719931433522}\"}"))));
+    }
+
+    @Test
+    public void testGetLockNoSuchLock(TestContext context){
+        when(message.body()).thenReturn(buildGetLockOperation("q1"));
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,2);
+            handler.handle(Future.succeededFuture());
+            return null;
+        }).when(redisAPI).hget(anyString(), anyString(), any());
+
+        action.execute(message);
+
+        verify(redisAPI, times(1)).hget(anyString(), anyString(), any());
+        verify(message, times(1)).reply(eq(new JsonObject(Buffer.buffer("{\"status\":\"No such lock\"}"))));
+    }
+
+    @Test
+    public void testGetLockHGETFail(TestContext context){
+        when(message.body()).thenReturn(buildGetLockOperation("q1"));
+
+        doAnswer(invocation -> {
+            var handler = createResponseHandler(invocation,2);
+            handler.handle(Future.failedFuture("booom"));
+            return null;
+        }).when(redisAPI).hget(anyString(), anyString(), any());
+
+        action.execute(message);
+
+        verify(redisAPI, times(1)).hget(anyString(), anyString(), any());
+        verify(message, times(1)).fail(eq(0), eq("booom"));
+    }
 }