Reduce the number of fields sent to the service #288
Conversation
This file contains hidden or 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
This pull request refactors the `CompatAdapter` and related middleware to use dependency injection for service resolution, improving maintainability and aligning with modern .NET practices. The main changes involve replacing constructor parameters with `IServiceProvider`, updating middleware to resolve dependencies, and enhancing logging in the `CompatBotAdapter`. **Dependency Injection Refactoring:** * `CompatAdapter` now receives an `IServiceProvider` in its constructor, and uses it to resolve `TeamsBotApplication` and `CompatBotAdapter` instead of receiving them directly as parameters. All usages of these services within the class have been updated accordingly. * `CompatAdapterMiddleware` now receives both the Bot Framework middleware and an `IServiceProvider`, enabling it to resolve required services such as `IHttpContextAccessor` and `ILogger`. **Middleware and Adapter Updates:** * Middleware instantiation in `CompatAdapter` is updated to pass the `IServiceProvider` to each `CompatAdapterMiddleware`, ensuring dependencies are available during middleware execution. * Within `CompatAdapterMiddleware`, the `CompatBotAdapter` is now constructed with resolved `IHttpContextAccessor` and `ILogger` instances, improving logging and context-awareness. **Logging Improvements:** * Enhanced logging in `CompatBotAdapter` to include HTTP status codes when sending invoke responses, and to avoid serialization if the response body is null. **General Code Modernization:** * Added missing `using` statements for dependency injection and logging namespaces in affected files. [[1]](diffhunk://#diff-e6d701ce121dda6651c0dea00498d45e2cc0b0cec446a578f3058be691c7c0f5R8) [[2]](diffhunk://#diff-80a974c881b9ff3a697fe5b07985c8f55edb2e2896384d6d437a3e8dfa5a2817R5-R6)
The CompatMiddlware Adapter was creating a new TurnContext, and that created problems with Middleware. Instead of trying to adapt the middlware, we can just run the BF Middleware pipeline
Enhance null handling for From/Recipient properties in activities and related classes. Make these properties nullable, add null checks before assignment, and update method calls to use null-conditional operators. Adjust JSON array deserialization to return null instead of empty lists when input is null. These changes increase robustness when handling incomplete or missing activity data.
- Update CoreActivityBuilder and TeamsActivityBuilder to set From to Recipient and leave Recipient null when From is missing in WithConversationReference. - Remove default assertions that From/Recipient are non-null in builder tests; add explicit null checks where needed. - WithConversationReference no longer throws for null ChannelId; throws for null Recipient. - WithChannelData(null) no longer overwrites existing ChannelData. - Add null-conditional access for ca.From?.Properties in EchoBot. - Explicitly set Recipient in test activities to prevent null reference errors. - Add settings.local.json with Bash command permissions.
singhk97
reviewed
Jan 27, 2026
Collaborator
singhk97
left a comment
singhk97
left a comment
There was a problem hiding this comment.
added a few comments the understand implication of this
| WithChannelId(activity.ChannelId); | ||
| SetConversation(activity.Conversation); | ||
| SetFrom(activity.Recipient); | ||
| SetRecipient(activity.From); |
Collaborator
There was a problem hiding this comment.
What is the main motivation behind these changes? If activity.From is set shouldn't it be mapped?
Member
Author
There was a problem hiding this comment.
APX does not require the Recipient, so I was thinking of using it only for TM
| string convId = conversationId.Length > 325 ? conversationId[..325] : conversationId; | ||
| url = $"{activity.ServiceUrl.ToString().TrimEnd('/')}/v3/conversations/{convId}/activities"; | ||
| } | ||
| activity.ServiceUrl = null; // do not serialize in the outgoing payload |
Collaborator
There was a problem hiding this comment.
doing this means it will always be null and not overridable by the end user. What's the tradeoff here?
Member
Author
There was a problem hiding this comment.
this means that we do not serialize the ServiceURL when sending the response to APX
MehakBindra
force-pushed
the
next/core
branch
from
9b16656 to
3d480b9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request refactors how sender (
From) and recipient (Recipient) accounts are handled throughout the Teams Bot SDK, making them nullable and updating related code to handle null values safely. It also updates builder methods and tests to reflect these changes, and improves robustness by removing unnecessary null checks and updating collection initialization logic.The most important changes are:
Core SDK and Schema Updates:
Made
FromandRecipientproperties nullable inCoreActivity,TeamsActivity, and related classes, and updated their usage throughout the codebase to handle null values safely. This includes replacing direct property access with null-safe navigation and updating serialization/deserialization logic. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Updated builder methods (
WithFrom,WithChannelData, etc.) to accept nullable arguments and only set properties if the value is not null. Also removed unnecessary null checks for properties that are now allowed to be null. [1] [2] [3]Collection Handling:
EntityList.FromJsonArray,TeamsAttachment.FromJArray) to returnnullinstead of empty collections when the input is null, aligning with the new nullable approach. [1] [2]Test Suite Updates:
FromandRecipient, ensuring tests check for null as appropriate and updating test names and assertions for clarity and correctness. [1] [2] [3] [4] [5] [6] [7] [8]Payload and Serialization Improvements:
ServiceUrlis not serialized in outgoing payloads by explicitly setting it to null before serialization inSendActivityAsync.Configuration:
core/.claude/settings.local.json) to specify permissions for running certain dotnet CLI commands.