-
Notifications
You must be signed in to change notification settings - Fork 21
fix: handle missing channelData in ConversationUpdateActivity for Direct Line #361
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,9 @@ | |
| Licensed under the MIT License. | ||
| """ | ||
|
|
||
| from typing import List, Literal, Optional | ||
| from typing import Any, List, Literal, Optional | ||
|
|
||
| from pydantic import model_validator | ||
|
|
||
| from ...models import Account, ActivityBase, ActivityInputBase, ChannelData, CustomBaseModel | ||
|
|
||
|
|
@@ -49,10 +51,51 @@ class _ConversationUpdateBase(CustomBaseModel): | |
|
|
||
|
|
||
| class ConversationUpdateActivity(_ConversationUpdateBase, ActivityBase): | ||
| """Output model for received conversation update activities with required fields and read-only properties.""" | ||
| """Output model for received conversation update activities with required fields and read-only properties. | ||
|
|
||
| Design note (channel_data field): | ||
| In Teams, conversationUpdate activities always include channelData with an eventType | ||
| discriminator (e.g. "channelCreated", "teamArchived"). The routing system | ||
| (activity_route_configs.py) relies on channel_data.event_type to dispatch to specific | ||
| handlers like on_channel_created() and on_team_archived(). | ||
|
|
||
| However, non-Teams channels (notably Direct Line) send conversationUpdate activities | ||
| WITHOUT channelData, which would cause a Pydantic ValidationError if the field were | ||
| strictly required. See https://github.com/microsoft/teams.py/issues/239. | ||
|
|
||
| Resolution: We keep channel_data as a REQUIRED field (preserving the type contract for | ||
| the 99% of developers building Teams bots) but add a model_validator that defaults it to | ||
| an empty ConversationChannelData() when missing from the incoming payload. This matches | ||
| the TypeScript SDK pattern, where the type declares channelData as required on | ||
| IConversationUpdateActivity (conversation-update.ts:25) but the router uses optional | ||
| chaining defensively (router.ts:73-87). | ||
|
|
||
| The empty default means: | ||
| - Teams activities: channel_data is populated normally, event_type routes work as before | ||
| - Direct Line activities: channel_data is an empty ConversationChannelData (event_type=None), | ||
| so Teams-specific event handlers don't fire, but the generic on_conversation_update() | ||
| handler still does | ||
| - Type checkers see a non-optional ConversationChannelData — no None-guards needed | ||
| """ | ||
|
|
||
| channel_data: ConversationChannelData # pyright: ignore [reportGeneralTypeIssues, reportIncompatibleVariableOverride] | ||
| """Channel data with event type information.""" | ||
| """Channel data with event type information. Always present — defaulted to empty for non-Teams channels.""" | ||
|
|
||
| @model_validator(mode="before") | ||
| @classmethod | ||
| def _default_channel_data(cls, data: Any) -> Any: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description says "all 577 existing tests pass" but adds none. Add a regression test that parses a Direct Line payload. |
||
| """Supply an empty channelData when absent (e.g. Direct Line). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstring frames this as "incoming Direct Line payload" handling, but |
||
|
|
||
| Without this, Pydantic rejects the payload because channel_data is required. | ||
| The empty default preserves the required-field contract for Teams developers | ||
| while allowing non-Teams channels to send conversationUpdate without channelData. | ||
| See: https://github.com/microsoft/teams.py/issues/239 | ||
| """ | ||
| if isinstance(data, dict): | ||
| # Check both the snake_case field name and the camelCase alias | ||
| if "channel_data" not in data and "channelData" not in data: | ||
| data["channelData"] = {} | ||
| return data | ||
|
|
||
|
|
||
| class ConversationUpdateActivityInput(_ConversationUpdateBase, ActivityInputBase): | ||
|
|
||
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.
MessageUpdateActivity(andMessageDeleteActivity) have the same "requiredchannel_dataon output, optional on base" shape, and the route selectors atactivity_route_configs.py:107, 129, 137rely onactivity.channel_data.event_type. If Direct Line ever sends amessageUpdate/messageDeletewithoutchannelData, the sameValidationErrorfires that this PR is fixing forconversationUpdate. The fix should be symmetric.Note:
MessageDeleteChannelData.event_typeisLiteral["softDeleteMessage"]with no default, so{}won't deserialize for it. A shared_default_channel_datamixin would need to be parameterized by the channel-data class.