feat(core)!: Improve DI robustness by removing unsafe casts #42
Conversation
|
Hey @aminafra , thanks man! I’ll review it soon. Could you rebase your branch with main first? |
This commit addresses the fragility of using `Unsafe.As` for handling collections resolved from the service provider. This was causing potential runtime exceptions when using DI containers (like Autofac) that resolve `IEnumerable<T>` to `List<T>` instead of an array. The unsafe casts have been replaced with safer, more idiomatic C# patterns that provide a zero-allocation fast path when the underlying collection is an array, while gracefully handling any other `IEnumerable<T>` implementation by converting it to an array. This change significantly improves the library's robustness and compatibility without a meaningful performance regression for the common path. BREAKING CHANGE: While unlikely to affect users, this changes the internal handling of collections and is a fundamental fix to the library's core resolution logic, warranting a major version bump for safety. Resolves hasanxdev#29
aminafra
force-pushed
the
afra/fix/replace-unsafe-as-with-type-checks
branch
from
4965e01 to
1759c89
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Done! I've rebased the branch with the latest main. ✅ |
|
@aminafra The test coverage has dropped, probably because no tests were written for your changes. Could you please add tests so that the coverage goes back up? Also, I noticed a major issue in your code: you’re using ToArray, which isn’t ideal. It’s better to use is, as I mentioned in the PR. And in the else block, when there are no handled cases, then you can use ToArray. Since we’re working on a library where memory usage is critical, these micro-optimizations really do make a difference |
…viceRegistrator changes - Add 6 new tests for IMediator.Publish method covering: * Single handler scenario (array with one element) * Async handlers requiring await (IsCompletedSuccessfully = false) * Sync handlers already completed (IsCompletedSuccessfully = true) * Non-array IEnumerable path (lines 73-78) * Mixed async/sync handler scenarios * ProcessHandlerAsync local function with both code paths - Add 2 new tests for ServiceRegistrator single handler optimization: * Handler without pipelines * Single handler with pipelines enabled (lines 151-153) Coverage improvements: - IMediator.Publish: All new lines (60-90) now covered with 100% branch coverage - ServiceRegistrator: Lines 144-154 fully covered including single handler path - Overall line coverage increased to 97.9% - Branch coverage at 94% Resolves codecov patch coverage issue (was 70.37%, now 97.9%)
- Introduced two new tests to validate the behavior of the mediator when handling non-array collections returned from the service provider. - The first test checks the synchronous handling of a non-array enumerable without pipelines. - The second test verifies asynchronous handling with pipelines enabled. - Added a helper class to wrap the service provider, ensuring it returns non-array collections correctly. These additions enhance test coverage and ensure robustness in service resolution.
…ut code - Removed unnecessary commented-out test methods and whitespace from RequestHandlerTests.cs to improve readability and maintainability. - This cleanup enhances the clarity of the test suite without altering any test functionality.
|
@hasanxdev thanks again for the detailed review! I've pushed new tests as you requested, which brought the patch coverage up to ~96%. The final uncovered line appears to be the Regarding your feedback on I'm still happy to refactor it to the |
|
@aminafra We’re adding support to this library so that developers using alternative DI containers can still work seamlessly with it. Because of that, we need test coverage for this specific case, in the future, if we modify the code and forget about this feature, those tests should fail and remind us of the missing support. Remember, 50% of testing is to ensure our code currently works, and the other 50% is to make sure future changes don’t break existing features. So please add this test case and raise the coverage to around 99–100%. |
|
@aminafra Also, regarding the use of as for casting, it causes a performance drop and will make the results inconsistent with the benchmark outcomes. |
2 participants
This commit addresses the fragility of using
Unsafe.Asfor handling collections resolved from the service provider. This was causing potential runtime exceptions when using DI containers (like Autofac) that resolveIEnumerable<T>toList<T>instead of an array.The unsafe casts have been replaced with safer, more idiomatic C# patterns that provide a zero-allocation fast path when the underlying collection is an array, while gracefully handling any other
IEnumerable<T>implementation by converting it to an array.This change significantly improves the library's robustness and compatibility without a meaningful performance regression for the common path.
BREAKING CHANGE: While unlikely to affect users, this changes the internal handling of collections and is a fundamental fix to the library's core resolution logic, warranting a major version bump for safety.
Resolves #29