microsoft / microsoft/teams.net
A subclassed TeamsBotApplication loses the file downloader's logger, hiding which retrieval route ran
- Dominant language
- C#
- Stars
- 46
- Forks
- 21
- Avg merge
- 22h 25m
- Merged PRs (30d)
- 16
Description
### Bug Description
`TeamsBotApplication` takes three optional constructor parameters that are supplied by DI: `stateLoader`, `fileDownloader`, and `tokenProvider`. The subclass shape the SDK documents on that same constructor forwards only four arguments, so a subclass reaches none of them.
`tokenProvider` was already fixed. `AddTeamsBotApplication` re-registers `TApp` and back-fills it after construction, because without that every content-URL-only file reported no Graph credential and the whole Agentic User path was silently dead for any subclassing app.
`fileDownloader` has the same gap and was deliberately left alone, because changing it changes existing behavior. This issue is about that remaining case.
`TeamsBotApplication.cs:177` reads `FileDownloader = fileDownloader ?? Files.FileDownloader.CreateDefault();`, and `CreateDefault()` is `new(SharedClient)`, passing only the HTTP client. The `logger` parameter therefore defaults to null and becomes `NullLogger`.
Most of what that costs is not serious, and it is worth saying so explicitly so this is not read as larger than it is:
- **Not** a socket exhaustion bug. `SharedClient` is a `private static readonly HttpClient`, which is the supported lifetime for a long-lived client.
- **Not** a credential leak. That client carries no default headers, so there is nothing to leak.
- It does lose `IHttpClientFactory` handler rotation, which is minor.
The real cost is the logger. The downloader's own parameter documentation states what is lost: "Which route ran is otherwise invisible from outside, and the two routes fail in different ways, so a developer diagnosing a download has no way to tell them apart without it."
That matters more now than when it was first noticed. An inbound file resolves either through a pre-authorized URL or through Graph `/shares`, an expired pre-authorized URL is terminal with no fallback, and the two routes fail in different ways. "Why did my download fail" is a question developers have to answer, and a subclassing app is exactly where the signal that answers it is missing.
### Steps to Reproduce
1. Subclass `TeamsBotApplication` using the four-argument constructor shape the SDK documents.
2. Register it with `AddTeamsBotApplication` and send the bot a file in a 1:1 chat.
3. Download the file and watch the logs at `Debug` level.
### Expected Behavior
The downloader logs which retrieval route produced the bytes, the same as it does for a non-subclassed `TeamsBotApplication`.
### Actual Behavior
Nothing is logged. The downloader was constructed by `CreateDefault()` with no logger, so it holds a `NullLogger` and the route is invisible.
There is no compiler signal either way, which is the wider lesson worth recording: adding an optional constructor parameter to `TeamsBotApplication` looks purely additive and is silently not wired for subclasses.
### SDK Version
2.1.1-preview
### .NET Version
8.0 and 10.0
### Additional Context
A fix needs to distinguish "defaulted" from "explicitly supplied" before back-filling. The `??=` approach used for `tokenProvider` cannot simply be extended here, because `FileDownloader` is never null: line 177 has already substituted the default by the time anything could test it. Recording whether the constructor argument was null, and only overwriting when it was, keeps a caller who deliberately passed their own downloader from being clobbered.
`stateLoader` has the same structural gap and should be left as it is. `Context.cs:85-86` already throws an `InvalidOperationException` from `ctx.State` whose message names the subclass case directly: "if using a custom TeamsBotApplication make sure you pass a TurnStateLoader instance." That is loud, self-explanatory, and points at the fix, so re-homing it would change existing behavior for no gain.
Found while porting the Agentic User file retrieval work to .NET (#669).
Contributor guide
Research direction
Start in TeamsBotApplication.cs:177 and trace the documented four-argument subclass constructor through AddTeamsBotApplication. Check how CreateDefault() supplies the file downloader and logger, then verify that subclassed apps log the retrieval route while an explicitly supplied downloader remains unchanged. Confirm the behavior with a 1:1 file download at Debug level.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100