fix(filetransfer): register a default no-op IFileProvider - #23
Merged
Conversation
AddEntglDbFileTransfer() unconditionally registers FileQueryHandler and FileDownloadHandler as INetworkMessageHandler, and both require IFileProvider in their constructor with no default. A node that only ever downloads files (never serves any) had to hand-write and register its own no-op IFileProvider just to satisfy that dependency - otherwise resolving IEnumerable<INetworkMessageHandler> during IEntglDbNode.Start() throws. That failure is especially dangerous because Start() is commonly invoked as a fire-and-forget task (`_ = node.Start()`) during app startup: the exception goes unobserved, and the node never finishes starting - no TCP listener, no logs, no crash, nothing to grep for. Added NullFileProvider, registered via TryAddSingleton inside AddEntglDbFileTransfer() itself. A download-only consumer now needs nothing further. A node that does want to serve files still registers its own provider afterward via plain AddSingleton; since FileQueryHandler/FileDownloadHandler depend on a single (non- enumerable) IFileProvider, the last registration wins. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A couple of newly added behaviors/docs are misleading/incomplete (notably cancellation-token handling and the rationale/documentation around TryAddSingleton), which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a DI startup failure in EntglDb.Services.FileTransfer by providing a default IFileProvider so download-only consumers can call AddEntglDbFileTransfer() without needing to implement server-side file serving.
Changes:
- Added
NullFileProvider, a defaultIFileProviderimplementation that reports all files as not found. - Registered
IFileProviderviaTryAddSingleton<IFileProvider, NullFileProvider>()insideAddEntglDbFileTransfer()to avoid forcing consumers to register a provider.
File summaries
| File | Description |
|---|---|
| src/EntglDb.Services.FileTransfer/NullFileProvider.cs | Introduces a default no-op provider implementation for download-only nodes. |
| src/EntglDb.Services.FileTransfer/FileTransferExtensions.cs | Registers NullFileProvider by default and updates XML docs to describe the behavior. |
Review details
Suppressed comments (1)
src/EntglDb.Services.FileTransfer/FileTransferExtensions.cs:57
- The inline comment explains TryAddSingleton as necessary so a later AddSingleton becomes the “last registration”, but that would also be true if this were AddSingleton. The key reason for TryAddSingleton is to avoid overriding a caller-provided IFileProvider that was registered before AddEntglDbFileTransfer(). Updating the comment will prevent future refactors from reintroducing the issue.
// Server-side default so a download-only consumer doesn't have to implement IFileProvider itself
// just to satisfy FileQueryHandler/FileDownloadHandler's constructor dependency - see remarks
// above. Must be TryAddSingleton (not AddSingleton) so a real provider the caller registers
// afterward via plain AddSingleton becomes the winning "last registration" for the single-instance
// resolution FileQueryHandler/FileDownloadHandler actually use.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+17
to
+21
| public Task<FileTransferInfo?> GetInfoAsync(string fileId, CancellationToken ct = default) => | ||
| Task.FromResult<FileTransferInfo?>(null); | ||
|
|
||
| public Task<Stream?> OpenReadAsync(string fileId, CancellationToken ct = default) => | ||
| Task.FromResult<Stream?>(null); |
Comment on lines
+30
to
+39
| /// A node that only ever downloads files (never serves any) needs nothing further - it uses the | ||
| /// registered <see cref="NullFileProvider"/> default, reporting every file as not found. To make files | ||
| /// available for remote download instead, register a real provider <em>after</em> this call: | ||
| /// <code> | ||
| /// services.AddEntglDbFileTransfer(); | ||
| /// services.AddSingleton<IFileProvider, MyFileProvider>(); | ||
| /// </code> | ||
| /// <see cref="FileQueryHandler"/>/<see cref="FileDownloadHandler"/> take a single (non-enumerable) | ||
| /// <see cref="IFileProvider"/>, so the last registration wins - the call above's own provider overrides | ||
| /// the default even though both end up registered. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
AddEntglDbFileTransfer()forced every consumer to implementIFileProvider, even a pure download-only client, or hit an unobservedInvalidOperationExceptioninside a fire-and-forgetnode.Start()call - silently never starting the node (no listener, no logs, no crash).NullFileProvider(reports every file as not found), registered viaTryAddSingleton<IFileProvider, NullFileProvider>()insideAddEntglDbFileTransfer().AddSingleton<IFileProvider, MyProvider>()- since the handlers depend on a single (non-enumerable)IFileProvider, the last registration wins over the library's default.Test plan
dotnet build/dotnet testonEntglDb.Services.FileTransferIFileProviderafterAddEntglDbFileTransfer()still resolves to that provider, notNullFileProviderIFileProviderat all now starts cleanly and reports every file query as not found instead of throwing🤖 Generated with Claude Code