Add hierarchical Teams API facade to TeamsBotApplication#334
Add hierarchical Teams API facade to TeamsBotApplication#334
Conversation
Introduced a structured API facade (`TeamsBotApplication.Api`) for Teams operations, grouping functionality into Conversations, Users, Teams, Meetings, and Batch sub-APIs. Added new classes for each sub-API, with extensive XML documentation. Updated `TeamsBotApplication` to expose the new facade. Added integration tests for the API hierarchy and error handling. Cleaned up `.csproj` by removing unused RunSettings property. This refactor improves discoverability, organization, and developer experience for Teams API usage.
Added detailed tables to README.md mapping TeamsApi facade methods to their underlying REST endpoints. Documentation covers Conversations, Users (Token), Teams, Meetings, and Batch operations, including HTTP methods, endpoint paths, and relevant base URLs. Added notes on service URL usage and path parameter encoding.
There was a problem hiding this comment.
Pull request overview
This PR introduces a hierarchical Teams API facade surfaced as TeamsBotApplication.Api, organizing Teams operations into sub-APIs (Conversations/Activities/Members, Users/Token, Teams, Meetings, Batch) and adding integration tests to validate the facade structure and delegation.
Changes:
- Added
TeamsApifacade with sub-API classes for common Teams operations (activities, members, users/tokens, teams, meetings, batch). - Updated
TeamsBotApplicationto expose the facade via a newApiproperty. - Added integration tests covering API hierarchy and some error-handling behavior; removed an unused
RunSettingsFilePathproperty from the test project.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/test/Microsoft.Teams.Bot.Core.Tests/TeamsApiFacadeTests.cs | Adds integration tests for the new TeamsBotApplication.Api hierarchy and delegations. |
| core/test/Microsoft.Teams.Bot.Core.Tests/Microsoft.Teams.Bot.Core.Tests.csproj | Removes unused RunSettingsFilePath property group. |
| core/src/Microsoft.Teams.Bot.Apps/TeamsBotApplication.cs | Exposes new Api facade via lazy-initialized property. |
| core/src/Microsoft.Teams.Bot.Apps/Api/TeamsApi.cs | Defines top-level hierarchical facade and wires up sub-APIs. |
| core/src/Microsoft.Teams.Bot.Apps/Api/ConversationsApi.cs | Adds container for conversation-scoped APIs (Activities/Members). |
| core/src/Microsoft.Teams.Bot.Apps/Api/ActivitiesApi.cs | Adds activity operations wrapper around ConversationClient. |
| core/src/Microsoft.Teams.Bot.Apps/Api/MembersApi.cs | Adds conversation member operations wrapper around ConversationClient. |
| core/src/Microsoft.Teams.Bot.Apps/Api/UsersApi.cs | Adds container for user-scoped APIs (Token). |
| core/src/Microsoft.Teams.Bot.Apps/Api/UserTokenApi.cs | Adds user token operations wrapper around UserTokenClient. |
| core/src/Microsoft.Teams.Bot.Apps/Api/TeamsOperationsApi.cs | Adds team/channel operations wrapper around TeamsApiClient. |
| core/src/Microsoft.Teams.Bot.Apps/Api/MeetingsApi.cs | Adds meeting operations wrapper around TeamsApiClient. |
| core/src/Microsoft.Teams.Bot.Apps/Api/BatchApi.cs | Adds batch messaging and batch-operation management wrapper around TeamsApiClient. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ArgumentNullException.ThrowIfNull(activity); | ||
| return _client.GetTokenAsync( | ||
| activity.From.Id!, | ||
| connectionName, | ||
| activity.ChannelId!, | ||
| code, | ||
| cancellationToken); |
There was a problem hiding this comment.
The activity-context overload uses null-forgiving operators for required fields (e.g., activity.From.Id and activity.ChannelId) but doesn’t validate them. If ChannelId/From.Id are missing, null can flow into UserTokenClient and result in confusing downstream failures or invalid requests. Add explicit validation (e.g., ThrowIfNullOrWhiteSpace on From.Id and ChannelId) for all activity-context methods in this API to fail fast with clear exceptions.
| ArgumentNullException.ThrowIfNull(activity); | ||
| return _client.FetchParticipantAsync( | ||
| meetingId, | ||
| participantId, | ||
| activity.ChannelData?.Tenant?.Id ?? throw new InvalidOperationException("Tenant ID not available in activity"), | ||
| activity.ServiceUrl!, | ||
| activity.From.GetAgenticIdentity(), | ||
| customHeaders, | ||
| cancellationToken); |
There was a problem hiding this comment.
This throws InvalidOperationException when tenant ID is missing from the provided activity. Since this is invalid caller input (the argument doesn’t contain required data), prefer ArgumentException/ArgumentNullException (or ThrowIfNull/ThrowIfNullOrWhiteSpace) to keep exception semantics consistent with other argument validation across the clients.
| ArgumentNullException.ThrowIfNull(contextActivity); | ||
| return _client.SendMessageToListOfUsersAsync( | ||
| activity, | ||
| teamsMembers, | ||
| contextActivity.ChannelData?.Tenant?.Id ?? throw new InvalidOperationException("Tenant ID not available in activity"), | ||
| contextActivity.ServiceUrl!, | ||
| contextActivity.From.GetAgenticIdentity(), | ||
| customHeaders, | ||
| cancellationToken); |
There was a problem hiding this comment.
This throws InvalidOperationException when tenant ID is missing from the provided activity. Because the tenant ID is required input derived from the argument, use an argument validation exception (ArgumentException/ArgumentNullException) for consistency and clearer caller feedback.
| public TeamsApi Api => _api ??= new TeamsApi( | ||
| ConversationClient, | ||
| UserTokenClient, | ||
| _teamsApiClient); |
There was a problem hiding this comment.
Api is lazily initialized on a singleton TeamsBotApplication without synchronization. Concurrent calls can create multiple TeamsApi instances and fail the “same instance” guarantee under load. Consider initializing in the constructor, using Lazy, or protecting the initialization with a lock/Interlocked to make it thread-safe.
There was a problem hiding this comment.
@rido-min copilot has a good point here, let's initialize this in the constructor?
| /// <param name="customHeaders">Optional custom headers to include in the request.</param> | ||
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the response with the ID of the updated activity.</returns> | ||
| public Task<UpdateActivityResponse> UpdateAsync( |
There was a problem hiding this comment.
Wondering why we have disparity between send and update in extracting conv id/ activity id from activity itself? maybe we should have an overload for it similar to all other apis?
| /// <param name="customHeaders">Optional custom headers to include in the request.</param> | ||
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation.</returns> | ||
| public Task DeleteAsync( |
There was a problem hiding this comment.
Why do we need this overload for teamsactivity ?
There was a problem hiding this comment.
good point, having the overload defeats the benefits of polymorphism
| /// <item><see cref="Members"/> - Member operations (get, delete)</item> | ||
| /// </list> | ||
| /// </remarks> | ||
| public class ConversationsApi |
There was a problem hiding this comment.
Shouldn't there be a method here for create conversation? intentional omission?
| /// <param name="customHeaders">Optional custom headers to include in the request.</param> | ||
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the conversation member.</returns> | ||
| public Task<T> GetByIdAsync<T>( |
There was a problem hiding this comment.
Should we combine GetByIdAsync<T and GetByIdAsync<ConversationAccount to GetByIdAsync <TeamsConversationAccount
| /// <param name="customHeaders">Optional custom headers to include in the request.</param> | ||
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the team details.</returns> | ||
| public Task<TeamDetails> GetByIdAsync( |
There was a problem hiding this comment.
Should teamId also be extracted from activity here ? from activity.channelData.team.id
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the list of channels.</returns> | ||
| public Task<ChannelList> GetChannelsAsync( | ||
| string teamId, |
There was a problem hiding this comment.
^ same
Should teamId also be extracted from activity here ? from activity.channelData.team.id
| /// <param name="customHeaders">Optional custom headers to include in the request.</param> | ||
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation.</returns> | ||
| public Task DeleteAsync( |
There was a problem hiding this comment.
good point, having the overload defeats the benefits of polymorphism
| /// <param name="customHeaders">Optional custom headers to include in the request.</param> | ||
| /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains a list of members for the activity.</returns> | ||
| public Task<IList<ConversationAccount>> GetMembersAsync( |
There was a problem hiding this comment.
what does "members of a specific activity" even mean? is it members of the conversation in which this activity was posted?
| public TeamsApi Api => _api ??= new TeamsApi( | ||
| ConversationClient, | ||
| UserTokenClient, | ||
| _teamsApiClient); |
There was a problem hiding this comment.
@rido-min copilot has a good point here, let's initialize this in the constructor?
Introduced a structured API facade (
TeamsBotApplication.Api) for Teams operations, grouping functionality into Conversations, Users, Teams, Meetings, and Batch sub-APIs. Added new classes for each sub-API, with extensive XML documentation. UpdatedTeamsBotApplicationto expose the new facade. Added integration tests for the API hierarchy and error handling. Cleaned up.csprojby removing unused RunSettings property. This refactor improves discoverability, organization, and developer experience for Teams API usage.This pull request introduces a new
ActivitiesApiclass to the codebase, which provides a unified interface for performing activity-related operations in conversations, such as sending, updating, deleting activities, handling conversation history, and retrieving activity members. The class wraps existing functionality from theConversationClient, making these operations easier to use and more consistent throughout the application.New API for activity operations:
ActivitiesApiclass incore/src/Microsoft.Teams.Bot.Apps/Api/ActivitiesApi.cs, providing methods for sending, updating, and deleting activities, uploading conversation history, and retrieving activity members.Integration and usability improvements:
ConversationClient, ensuring consistency and reusability of existing logic.