Fix #1084 by enabling app.message method to manage multiple handlers (#1086)

Author: seratch

bolt-aws-lambda/src/test/java/test_locally/Issue419UseCaseTest.java

@@ -79,7 +79,6 @@ public void enableAdditionalListenersRuntime_issue_419() throws Exception {
                 headers.put("additional-header", Arrays.asList("foo"));
                 return Response.builder().statusCode(200).headers(headers).build();
             });
-            app.message("Hello", (req, ctx) -> ctx.ack());
 
             SampleHandler handler = new SampleHandler(app);
 
@@ -95,7 +94,7 @@ public void enableAdditionalListenersRuntime_issue_419() throws Exception {
             // message event
             ApiGatewayRequest message1 = buildMessageEvent();
             ApiGatewayResponse message1Response = handler.handleRequest(message1, context);
-            assertEquals(200, message1Response.getStatusCode());
+            assertEquals(404, message1Response.getStatusCode());
             assertEquals(0, handler.messageCount.get());
 
             // enable other listeners

bolt/src/main/java/com/slack/api/bolt/App.java

@@ -176,8 +176,10 @@ public enum Status {
 
     /**
      * Registered Event API handlers.
+     * Note that only limited set of events allow having multiple handlers.
+     * As of v1.27.1, only MessageEvent.class ones can be multiple.
      */
-    private final Map<String, BoltEventHandler<Event>> eventHandlers = new HashMap<>();
+    private final Map<String, List<BoltEventHandler<Event>>> eventHandlers = new HashMap<>();
 
     /**
      * Primitive Event API handler.
@@ -612,15 +614,32 @@ public App use(Middleware middleware) {
     // Events API
     // https://api.slack.com/events-api
 
-    public <E extends Event> App event(Class<E> eventClass, BoltEventHandler<E> handler) {
+    public <E extends Event> App event(
+            Class<E> eventClass,BoltEventHandler<E> handler) {
+        // Note that having multiple handlers is allowed only for message event handlers.
+        // If we revisit this to unlock the option to all event types, it should work well.
+        // We didn't decide to do so in 2022 in respect of better backward compatibility.
+        boolean allowMultipleEventHandlers = getEventTypeAndSubtype(eventClass) == MessageEvent.TYPE_NAME;
+        return event(eventClass, allowMultipleEventHandlers, handler);
+    }
+
+    protected <E extends Event> App event(
+            Class<E> eventClass,
+            boolean allowMultipleEventHandlers,
+            BoltEventHandler<E> handler) {
         String eventTypeAndSubtype = getEventTypeAndSubtype(eventClass);
         if (eventTypeAndSubtype == null) {
             throw new IllegalArgumentException("Unexpectedly failed to register the handler");
         }
-        if (eventHandlers.get(eventTypeAndSubtype) != null) {
+        List<BoltEventHandler<Event>> handlers = eventHandlers.get(eventTypeAndSubtype);
+        if (handlers == null) {
+            handlers = new ArrayList<>();
+        } else if (!allowMultipleEventHandlers) {
             log.warn("Replaced the handler for {}", eventTypeAndSubtype);
+            handlers = new ArrayList<>();
         }
-        eventHandlers.put(eventTypeAndSubtype, (BoltEventHandler<Event>) handler);
+        handlers.add((BoltEventHandler<Event>) handler);
+        eventHandlers.put(eventTypeAndSubtype, handlers);
         return this;
     }
 
@@ -634,15 +653,15 @@ public App message(String pattern, BoltEventHandler<MessageEvent> messageHandler
     }
 
     public App message(Pattern pattern, BoltEventHandler<MessageEvent> messageHandler) {
-        event(MessageEvent.class, (event, ctx) -> {
+        event(MessageEvent.class, true, (event, ctx) -> {
             String text = event.getEvent().getText();
             if (log.isDebugEnabled()) {
                 log.debug("Run a message event handler (pattern: {}, text: {})", pattern, text);
             }
             if (text != null && pattern.matcher(text).matches()) {
                 return messageHandler.apply(event, ctx);
             } else {
-                return ctx.ack();
+                return null;
             }
         });
         return this;
@@ -1103,10 +1122,15 @@ protected Response runHandler(Request slackRequest) throws IOException, SlackApi
                         return Response.ok();
                     }
                     EventRequest request = (EventRequest) slackRequest;
-                    BoltEventHandler<Event> handler = eventHandlers.get(request.getEventTypeAndSubtype());
-                    if (handler != null) {
-                        EventsApiPayload<Event> payload = buildEventPayload(request);
-                        return handler.apply(payload, request.getContext());
+                    List<BoltEventHandler<Event>> handlers = eventHandlers.get(request.getEventTypeAndSubtype());
+                    if (handlers != null) {
+                        for (BoltEventHandler<Event> handler : handlers) {
+                            EventsApiPayload<Event> payload = buildEventPayload(request);
+                            Response result = handler.apply(payload, request.getContext());
+                            if (result != null && result.getStatusCode() == 200) {
+                                return result;
+                            }
+                        }
                     }
                     if (config().isAllEventsApiAutoAckEnabled()) {
                         // If the flag is true, Bolt acknowledges all the events anyway
@@ -1346,10 +1370,15 @@ protected Response runHandler(Request slackRequest) throws IOException, SlackApi
                         return Response.ok();
                     }
                     EventRequest request = new EventRequest(stepRequest.getRequestBodyAsString(), stepRequest.getHeaders());
-                    BoltEventHandler<Event> handler = eventHandlers.get(request.getEventTypeAndSubtype());
-                    if (handler != null) {
-                        EventsApiPayload<Event> payload = buildEventPayload(request);
-                        return handler.apply(payload, request.getContext());
+                    List<BoltEventHandler<Event>> handlers = eventHandlers.get(request.getEventTypeAndSubtype());
+                    if (handlers != null) {
+                        for (BoltEventHandler<Event> handler : handlers) {
+                            EventsApiPayload<Event> payload = buildEventPayload(request);
+                            Response result = handler.apply(payload, request.getContext());
+                            if (result != null && result.getStatusCode() == 200) {
+                                return result;
+                            }
+                        }
                     }
                     log.warn("No BoltEventHandler registered for event: {}\n{}",
                             request.getEventTypeAndSubtype(), ListenerCodeSuggestion.WORKFLOW_STEP);

bolt/src/test/java/test_locally/app/MessageTest.java

@@ -29,6 +29,9 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.regex.Pattern;
 
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.*;
 
 @Slf4j
@@ -80,7 +83,7 @@ public void withPayload_skipped() throws Exception {
 
         EventRequest req = buildRequest(messagePayload);
         Response response = app.run(req);
-        assertEquals(200L, response.getStatusCode().longValue());
+        assertEquals(404L, response.getStatusCode().longValue());
         assertFalse(userMessageReceived.get());
     }
 
@@ -133,7 +136,7 @@ public void user_word_not_matching() throws Exception {
 
         EventRequest req = buildRequest(gson.toJson(payload));
         Response response = app.run(req);
-        assertEquals(200L, response.getStatusCode().longValue());
+        assertEquals(404L, response.getStatusCode().longValue());
         assertFalse(userMessageReceived.get());
     }
 
@@ -171,7 +174,7 @@ public void user_regexp_not_matching() throws Exception {
 
         EventRequest req = buildRequest(gson.toJson(payload));
         Response response = app.run(req);
-        assertEquals(200L, response.getStatusCode().longValue());
+        assertEquals(404L, response.getStatusCode().longValue());
         assertFalse(userMessageReceived.get());
     }
 
@@ -188,10 +191,54 @@ public void user_skipped() throws Exception {
 
         EventRequest req = buildRequest(gson.toJson(payload));
         Response response = app.run(req);
-        assertEquals(200L, response.getStatusCode().longValue());
+        assertEquals(404L, response.getStatusCode().longValue());
         assertFalse(userMessageReceived.get());
     }
 
+    @Test
+    public void allowMultipleEventHandlers() throws Exception {
+        App app = buildApp();
+        AtomicBoolean pattern1Called = new AtomicBoolean(false);
+        AtomicBoolean pattern2Called = new AtomicBoolean(false);
+        app.message("pattern1", (req, ctx) -> {
+            pattern1Called.set(true);
+            return ctx.ack();
+        });
+        app.message("pattern2", (req, ctx) -> {
+            pattern2Called.set(true);
+            return ctx.ack();
+        });
+
+        EventsApiPayload<MessageEvent> payload1 = buildPatternMessagePayload("This is pattern1");
+        EventRequest req1 = buildRequest(gson.toJson(payload1));
+        Response response1 = app.run(req1);
+        assertEquals(200L, response1.getStatusCode().longValue());
+        assertThat(response1.getBody(), is(nullValue()));
+
+        EventsApiPayload<MessageEvent> payload2 = buildPatternMessagePayload("This is pattern2");
+        EventRequest req2 = buildRequest(gson.toJson(payload2));
+        Response response2 = app.run(req2);
+        assertEquals(200L, response2.getStatusCode().longValue());
+        assertThat(response2.getBody(), is(nullValue()));
+
+        EventsApiPayload<MessageEvent> payload3 = buildPatternMessagePayload("This is pattern3");
+        EventRequest req3 = buildRequest(gson.toJson(payload3));
+        Response response3 = app.run(req3);
+        assertEquals(404L, response3.getStatusCode().longValue());
+        assertThat(response3.getBody(), is("{\"error\":\"no handler found\"}"));
+
+        assertThat("pattern1 message listener called", pattern1Called.get(), is(true));
+        assertThat("pattern2 message listener called", pattern2Called.get(), is(true));
+
+        // test for ensuring the app.event(MessageEvent.class, ..) never overwrites above handlers
+        pattern1Called.set(false); // this needs to be true again
+        app.event(MessageEvent.class, (req, ctx) -> ctx.ack());
+        Response response4 = app.run(req1);
+        assertEquals(200L, response4.getStatusCode().longValue());
+
+        assertThat("pattern1 message listener called", pattern1Called.get(), is(true));
+    }
+
     @Test
     public void bot_subtype() throws Exception {
         App app = buildApp();
@@ -274,4 +321,14 @@ EventsApiPayload<MessageBotEvent> buildUserBotMessagePayload() {
         return payload;
     }
 
+    EventsApiPayload<MessageEvent> buildPatternMessagePayload(String text) {
+        EventsApiPayload<MessageEvent> payload = new MessagePayload();
+        MessageEvent event = new MessageEvent();
+        event.setUser("U123");
+        event.setText(text);
+        payload.setEvent(event);
+        payload.setTeamId("T123");
+        return payload;
+    }
+
 }