Repository navigation
Conversation
Map reader, writer, and store interfaces to the same registered singleton regardless of tracing. Preserve existing services and avoid duplicate traced store registrations. Host-based regressions reproduced 16 failures before the fix. All 31 DI tests now pass on net10.0; Stores.cs line coverage is 78/78. Fixes Eventuous#608
Contributor
Author
|
@alexeyzimarev would you please review? |
Contributor
Author
|
/review |
Contributor
Code Review by Qodo
1. Disposable stores can be disposed repeatedly
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+139
to
+141
| services.TryAddSingleton<IEventReader>(sp => sp.GetRequiredService<T>()); | ||
| services.TryAddSingleton<IEventWriter>(sp => sp.GetRequiredService<T>()); | ||
| services.TryAddSingleton<IEventStore>(sp => sp.GetRequiredService<T>()); |
Contributor
There was a problem hiding this comment.
1. Disposable stores can be disposed repeatedly 🐞 Bug ☼ Reliability
When tracing is disabled, AddEventStore<T> registers T and three singleton interface factories that each return that same instance. If a custom store implements IDisposable or IAsyncDisposable and the interfaces are resolved, the container tracks the factory results separately, so host shutdown can dispose the store multiple times; the aliases can also dispose an externally supplied instance.
Agent Prompt
## Issue description
Factory-based interface aliases can make the container track one disposable store several times, including a store supplied as an externally owned instance.
## Fix Focus Areas
- src/Extensions/src/Eventuous.Extensions.DependencyInjection/Registrations/Stores.cs[129-141]
- src/Extensions/src/Eventuous.Extensions.DependencyInjection/Registrations/Stores.cs[153-165]
## Recommended Fix
Revise the alias and ownership strategy so resolving every interface shares the backing store without registering multiple disposable factory results for that object. Cover both container-created disposable stores and pre-registered instances with shutdown tests.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+139
to
+141
| services.TryAddSingleton<IEventReader>(sp => sp.GetRequiredService<T>()); | ||
| services.TryAddSingleton<IEventWriter>(sp => sp.GetRequiredService<T>()); | ||
| services.TryAddSingleton<IEventStore>(sp => sp.GetRequiredService<T>()); |
Contributor
There was a problem hiding this comment.
2. Scoped stores can break singleton resolution 🐞 Bug ☼ Reliability
TryAddSingleton<T>() leaves an existing scoped registration of T in place, but the new singleton interface factories resolve that scoped T. When a host with scope validation resolves an interface, resolution throws; without validation, the singleton can retain a scoped instance beyond its intended lifetime.
Agent Prompt
## Issue description
A pre-registered scoped concrete store can become a dependency of the newly added singleton interface aliases.
## Fix Focus Areas
- src/Extensions/src/Eventuous.Extensions.DependencyInjection/Registrations/Stores.cs[129-141]
- src/Extensions/src/Eventuous.Extensions.DependencyInjection/Registrations/Stores.cs[153-165]
## Recommended Fix
Before adding singleton aliases to an existing concrete registration, detect a non-singleton lifetime and reject it with a clear registration-time error, or choose an explicit lifetime policy that does not capture scoped services. Add a scope-validation regression test.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// <typeparam name="T">Implementation of <see cref="IEventStore"/> </typeparam> | ||
| /// <returns></returns> | ||
| public IServiceCollection AddEventStore<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>() where T : class, IEventStore { | ||
| services.TryAddSingleton<T>(); |
Contributor
There was a problem hiding this comment.
3. Unused store types can prevent host builds 🐞 Bug ≡ Correctness
The generic AddEventStore<T> overload now registers T even when existing reader, writer, and store registrations cause every conditional interface registration to be skipped. With tracing disabled and build-time service validation enabled, an unused T whose constructor has an unresolved dependency can therefore fail host construction, although the existing interfaces need no replacement.
Agent Prompt
## Issue description
An existing registration for every store interface leaves the new concrete registration unused, but build-time validation can still reject its constructor.
## Fix Focus Areas
- src/Extensions/src/Eventuous.Extensions.DependencyInjection/Registrations/Stores.cs[129-141]
## Recommended Fix
When no interface or wrapper will need the concrete store, avoid adding its descriptor. Add a regression test with all three interfaces pre-registered, an unconstructable replacement type, tracing disabled, and build-time validation enabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Map reader, writer, and store interfaces to the same registered singleton regardless of tracing. Preserve existing services and avoid duplicate traced store registrations.
Host-based regressions reproduced 16 failures before the fix. All 31 DI tests now pass on net10.0; Stores.cs line coverage is 78/78.
Fixes #608
Fixes #610
Fixes #611