-
Notifications
You must be signed in to change notification settings - Fork 15
Return Activity Id From api/messages #261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Refactored the handling of SendActivityAsync responses by removing the null check and warning log. Now, the response ID is always assigned (even if null), and an informational log records the response ID. This streamlines the code and improves logging clarity.
Introduce granular status codes for InvokeResponse across the bot, add optional Id property, and improve conditional serialization of Type and Body. Refactor proactive message logic in EchoBot, add message reaction handler, and update response handling in Program.cs, CompatAdapter, and TeamsBotApplication to include activity Ids and new status codes.
Refactored the bot and Teams application layers to handle invoke responses by writing directly to the HTTP response stream, removing the need to return InvokeResponse objects through the pipeline. Updated delegate and middleware signatures to return Task instead of Task<InvokeResponse?>. Introduced CoreInvokeResponse as the standard invoke response type. Updated adapters and application builders to use IHttpContextAccessor for response handling. Adjusted tests and removed legacy code to align with the new response model, simplifying and modernizing the activity processing flow.
- Re-enabled echo and proactive messaging in EchoBot - Refactored invokeResponse handling in CompatBotAdapter - Replaced ArgumentNullException with ArgumentException for string validation - Moved CoreInvokeResponse to InvokeHandler.cs for consolidation - Removed ActivityType.Invoke; use hardcoded "invoke" in TeamsActivityType - Cleaned up middleware and removed unused usings - Allowed route path in UseBotApplication for flexible routing - Updated tests for new handler signatures and validation - General code cleanup and improved maintainability
Refactored CompatBotAdapter to use StreamWriter and JsonTextWriter with BotMessageSerializer.Serialize (Newtonsoft.Json) for writing InvokeResponse to HTTP response. Removed System.Text.Json-based serialization and added commented alternatives for reference. This improves compatibility with Bot Framework serialization expectations.
| [JsonPropertyName("value")] | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public object? Body { get; set; } = body; | ||
|
|
||
| // TODO: Get confirmation that this should be "Type" | ||
| // This particular type should be for AC responses | ||
| /// <summary> | ||
| /// Gets or Sets the Type | ||
| /// </summary> | ||
| [JsonPropertyName("type")] | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public string? Type { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this is not exactly correct since "value" etc is for adaptive cards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe InvokeResponse should be abstract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC Value is for any invoke.
Keep in mind the goal is the compat layer, once we get into the new FE we will look into the concrete invoke types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, so let's add a TODO here so we fix this for the new FE?
heyitsaamir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! 1 comment about a todo

BF Adapter base expects an Id in the SendActivity return value.
Without this fix, the compat layer throws NRE
This pull request introduces several updates to the bot framework sample applications and core libraries, mainly focusing on improving the handling and propagation of
InvokeResponseobjects, refining status codes, and enhancing logging and code structure. The changes help standardize response formats, improve traceability, and clarify code intent.InvokeResponse Handling and Status Codes
Idproperty to theInvokeResponseclass, ensuring responses can be correlated to originating activities, and updated serialization attributes forBodyandTypeto ignore nulls for cleaner JSON output. [1] [2] [3]InvokeResponseacross sample bots and core libraries (EchoBot,CoreBot,TeamsBot, and core adapter), improving clarity in response semantics. [1] [2] [3] [4] [5]Refactoring and Code Structure
EchoBotto a dedicated helper method (SendUpdateDeleteActivityAsync), improving code readability and maintainability. [1] [2]EchoBot.cs, such as removing unusedusingstatements.Logging and Diagnostics
CompatBotAdapterto provide better traceability of activity responses, replacing a warning for null responses with information logs and always assigning the response ID.Other Improvements
These updates collectively improve the reliability, maintainability, and clarity of bot applications and supporting libraries.