Merge pull request #139 from matthiasblaesing/transcript-middleware-a…

…zure Using TranscriptLoggerMiddleware breaks Bot on Azure Infrastructure
microsoft · Nov 18, 2019 · e5dcc60 · e5dcc60
2 parents 2ad8470 + c417b49
commit e5dcc60
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 32 deletions.
diff --git a/...aries/bot-builder/src/main/java/com/microsoft/bot/builder/TranscriptLoggerMiddleware.java b/...aries/bot-builder/src/main/java/com/microsoft/bot/builder/TranscriptLoggerMiddleware.java
@@ -3,13 +3,10 @@
 
 package com.microsoft.bot.builder;
 
-import com.fasterxml.jackson.databind.JsonNode;
-import com.fasterxml.jackson.databind.ObjectMapper;
-import com.fasterxml.jackson.databind.SerializationFeature;
 import com.microsoft.bot.schema.Activity;
 import com.microsoft.bot.schema.ActivityTypes;
 import com.microsoft.bot.schema.ChannelAccount;
-import org.apache.commons.lang3.StringUtils;
+import com.microsoft.bot.schema.RoleTypes;
 
 import java.time.OffsetDateTime;
 import java.time.ZoneId;
@@ -22,16 +19,6 @@
  * When added, this middleware will log incoming and outgoing activities to a TranscriptStore.
  */
 public class TranscriptLoggerMiddleware implements Middleware {
-    /**
-     * To/From JSON.
-     */
-    private static ObjectMapper mapper;
-
-    static {
-        mapper = new ObjectMapper()
-            .enable(SerializationFeature.INDENT_OUTPUT)
-            .findAndRegisterModules();
-    }
 
     /**
      * The TranscriptLogger to log to.
@@ -68,20 +55,7 @@ public TranscriptLoggerMiddleware(TranscriptLogger withTranscriptLogger) {
     public CompletableFuture<Void> onTurn(TurnContext context, NextDelegate next) {
         // log incoming activity at beginning of turn
         if (context.getActivity() != null) {
-            if (context.getActivity().getFrom() == null) {
-                context.getActivity().setFrom(new ChannelAccount());
-            }
-
-            JsonNode role = null;
-            if (context.getActivity().getFrom().getProperties().containsKey("role")) {
-                role = context.getActivity().getFrom().getProperties().get("role");
-            }
-
-            if (role == null || StringUtils.isBlank(role.asText())) {
-                context.getActivity().getFrom().getProperties().put("role", mapper.createObjectNode().with("user"));
-            }
-
-            logActivity(Activity.clone(context.getActivity()));
+            logActivity(Activity.clone(context.getActivity()), true);
         }
 
         // hook up onSend pipeline
@@ -90,7 +64,7 @@ public CompletableFuture<Void> onTurn(TurnContext context, NextDelegate next) {
             return nextSend.get()
                 .thenApply(responses -> {
                     for (Activity activity : activities) {
-                        logActivity(Activity.clone(activity));
+                        logActivity(Activity.clone(activity), false);
                     }
 
                     return responses;
@@ -105,7 +79,7 @@ public CompletableFuture<Void> onTurn(TurnContext context, NextDelegate next) {
                     // add Message Update activity
                     Activity updateActivity = Activity.clone(activity);
                     updateActivity.setType(ActivityTypes.MESSAGE_UPDATE);
-                    logActivity(updateActivity);
+                    logActivity(updateActivity, false);
 
                     return resourceResponse;
                 });
@@ -123,7 +97,7 @@ public CompletableFuture<Void> onTurn(TurnContext context, NextDelegate next) {
                         applyConversationReference(reference, false);
                     }};
 
-                    logActivity(deleteActivity);
+                    logActivity(deleteActivity, false);
 
                     return null;
                 });
@@ -141,10 +115,19 @@ public CompletableFuture<Void> onTurn(TurnContext context, NextDelegate next) {
             });
     }
 
-    private void logActivity(Activity activity) {
+    private void logActivity(Activity activity, boolean incoming) {
         if (activity.getTimestamp() == null) {
             activity.setTimestamp(OffsetDateTime.now(ZoneId.of("UTC")));
         }
+
+        if (activity.getFrom() == null) {
+            activity.setFrom(new ChannelAccount());
+        }
+
+        if (activity.getFrom().getRole() == null) {
+            activity.getFrom().setRole(incoming ? RoleTypes.USER : RoleTypes.BOT);
+        }
+
         transcript.offer(activity);
     }
 }

diff --git a/libraries/bot-builder/src/test/java/com/microsoft/bot/builder/TranscriptMiddlewareTest.java b/libraries/bot-builder/src/test/java/com/microsoft/bot/builder/TranscriptMiddlewareTest.java
@@ -223,5 +223,34 @@ public void Transcript_TestDateLogUpdateActivities() {
         pagedResult = transcriptStore.getTranscriptActivities("test", conversationId[0], null, OffsetDateTime.MAX).join();
         Assert.assertEquals(0, pagedResult.getItems().size());
     }
+
+    @Test
+    public final void Transcript_RolesAreFilled() {
+        MemoryTranscriptStore transcriptStore = new MemoryTranscriptStore();
+        TestAdapter adapter = (new TestAdapter()).use(new TranscriptLoggerMiddleware(transcriptStore));
+        final String[] conversationId = {null};
+
+        new TestFlow(adapter, (context) -> {
+            // The next assert implicitly tests the immutability of the incoming
+            // message. As demonstrated by the asserts after this TestFlow block
+            // the role attribute is present on the activity as it is passed to
+            // the transcript, but still missing inside the flow
+            Assert.assertNull(context.getActivity().getFrom().getRole());
+            conversationId[0] = context.getActivity().getConversation().getId();
+            context.sendActivity("echo:" + context.getActivity().getText()).join();
+            return CompletableFuture.completedFuture(null);
+        })
+            .send("test")
+            .startTest().join();
+
+        PagedResult<Activity> pagedResult = transcriptStore.getTranscriptActivities("test", conversationId[0]).join();
+        Assert.assertEquals(2, pagedResult.getItems().size());
+        Assert.assertNotNull(pagedResult.getItems().get(0).getFrom());
+        Assert.assertEquals(RoleTypes.USER, pagedResult.getItems().get(0).getFrom().getRole());
+        Assert.assertNotNull(pagedResult.getItems().get(1).getFrom());
+        Assert.assertEquals(RoleTypes.BOT, pagedResult.getItems().get(1).getFrom().getRole());
+
+        System.out.printf("Complete");
+    }
 }