Ensured all Activities stored in transcript have Ids #3701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…vity() is returning
…lf overrides null Activity Ids
… activity with null id)
…le still assigning ids to inbound to TestFlow (the user messages) an id
mrivera-ms
reviewed
Apr 14, 2020
| @@ -134,7 +134,27 @@ public async Task OnTurnAsync(ITurnContext turnContext, NextDelegate nextTurn, C | |||
| private static IActivity CloneActivity(IActivity activity) | |||
| { | |||
| activity = JsonConvert.DeserializeObject<Activity>(JsonConvert.SerializeObject(activity, _jsonSettings)); | |||
| return activity; | |||
| var activityWithId = EnsureActivityHasId(activity); | |||
|
|
|||
There was a problem hiding this comment.
Alternatively, you can just: return EnsureActivityHasId(activity);
| { | ||
| var activityWithId = activity; | ||
|
|
||
| if (activity == null) |
There was a problem hiding this comment.
Better to have the check for null before creating activityWithId
| /// </summary> | ||
| public class AllowNullIdTestAdapter : TestAdapter | ||
| { | ||
| private bool _sendTraceActivity; |
| private readonly object _conversationLock = new object(); | ||
| private readonly object _activeQueueLock = new object(); | ||
| private Queue<TaskCompletionSource<IActivity>> _queuedRequests = new Queue<TaskCompletionSource<IActivity>>(); | ||
| private int _nextId = 0; |
| /// </summary> | ||
| /// <returns>The next activity in the queue; or null, if the queue is empty.</returns> | ||
| /// <remarks>A <see cref="TestFlow"/> object calls this to get the next response from the bot.</remarks> | ||
| public new IActivity GetNextReply() |
| /// </summary> | ||
| /// <param name="cancellationToken">cancellation Token.</param> | ||
| /// <returns>activity when it's available or canceled task if it is canceled.</returns> | ||
| public new Task<IActivity> GetNextReplyAsync(CancellationToken cancellationToken = default) |
| /// <returns>An appropriate message activity.</returns> | ||
| /// <remarks>A <see cref="TestFlow"/> object calls this to get a message activity | ||
| /// appropriate to the current conversation.</remarks> | ||
| public new Activity MakeActivity(string text = null) |
This was referenced Apr 15, 2020
|
Well then... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3665
As, done in JS's
TranscriptMiddlewareLoggeralready, ensure that all cloned activities in logger have Ids, so that when logged intoMemoryTranscriptStore, eachItemalso has an Id.If an activity doesn't have an id,
MemoryTranscriptStorewill generate an Id for that activity/Itemfor the transcript storage, prefixing it with "g_" for "generated", as done in JS.Context
ContinuationTokenwithin the MemoryTranscriptStore.GetTranscriptActivitiesAsync relies on lastActivity.Idof loadedPagedResultPagedResult, we may end up infinitely loading the samePagedResultover and over again.Testing
In order to test that activities with null ids are assigned ids properly in transcript storage, I had to create a custom
TestAdapter, since the one "off the shelf" automatically fills in an activity id, never allowing for null ids to pass through to theTranscriptMiddlewareLoggerlayer.TestFlowis tightly coupled toTestAdapter(it requires an adapter of specifically typeTestAdapterinstead of an interface)