From c417b493839d2b6c2f6849346300963eeff0ea83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Bl=C3=A4sing?= Date: Thu, 31 Oct 2019 21:12:01 +0100 Subject: [PATCH] Using TranscriptLoggerMiddleware breaks Bot on Azure Infrastructure The TranscriptLoggerMiddleware works on the Emulator, but fails when run on Azure Infrastructure. This is caused by the invalid modification of the incoming Activity. While outgoing activities are cloned before they are modified, the incoming activity is modified in place. The modifications then create invalid data leading to this JSON fragment for reply activities: { // ... "recipient":{ "properties":{ "role":{} }, "id":"8d51e790-8bd3-4450-af3d-1741838d1354", "role":{} }, // ... } The role key in the properties subobject should not be there and while the second role entry should indeed be set, it should be set to a string, not an object. This results in a http 400 status from the bot framework: { "error": { "code": "BadSyntax", "message": "Invalid or missing Activity in request" } } While adding the role information to the from property of the activity is convenient, it should not modify the incoming activity. To fix this, the incoming activity is cloned before it is modified, as the outgoing activity also lacks the role information, the update of the role attribute is applied to both types. In addition the correct atttribute is updated for the role information. --- .../builder/TranscriptLoggerMiddleware.java | 47 ++++++------------- .../bot/builder/TranscriptMiddlewareTest.java | 29 ++++++++++++ 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/libraries/bot-builder/src/main/java/com/microsoft/bot/builder/TranscriptLoggerMiddleware.java b/libraries/bot-builder/src/main/java/com/microsoft/bot/builder/TranscriptLoggerMiddleware.java index 634e34c03..81f841f71 100644 --- a/libraries/bot-builder/src/main/java/com/microsoft/bot/builder/TranscriptLoggerMiddleware.java +++ b/libraries/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 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 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 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 onTurn(TurnContext context, NextDelegate next) { applyConversationReference(reference, false); }}; - logActivity(deleteActivity); + logActivity(deleteActivity, false); return null; }); @@ -141,10 +115,19 @@ public CompletableFuture 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 index 00fb40a20..7435dc62d 100644 --- 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 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"); + } }