Repository navigation
Conversation
The RegisterDecorator overloads taking a decorator type or factory returned void and built the decorator registration internally, calling CreateRegistration() - which clones the pipeline - before returning. There was nothing to chain configuration onto, and the pipeline was frozen by the time the caller got control back. They now return IDecoratorRegistrationBuilder<TService>, exposing ConfigurePipeline, WithMetadata, WithCondition and the registration metadata. This is deliberately a new, narrow builder rather than IRegistrationBuilder: As, Named, Keyed and the lifetime methods are declared directly on IRegistrationBuilder and are meaningless or harmful for a decorator, whose service is fixed by DecoratorService and whose lifetime is governed by the decoration machinery. Creating the component registration is deferred to a single container build callback, so configuration applied any time before Build() is picked up. RegisterGenericDecorator is unchanged: open generic decorators build their closed registration lazily at resolve time, so there is no registration-time builder to hand back. Middleware added through ConfigurePipeline runs on the decorator's own registration pipeline, which is what lets an integration wrap the outermost decorator instead of the component being decorated. Source-compatible for existing callers, but binary-breaking on the strong-named assembly, so this needs to ship in a major release. The version in default.proj is left alone. Refs autofac#1416
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1501 +/- ##
===========================================
- Coverage 78.23% 77.55% -0.69%
===========================================
Files 217 218 +1
Lines 5928 6005 +77
Branches 1269 1286 +17
===========================================
+ Hits 4638 4657 +19
- Misses 753 755 +2
- Partials 537 593 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
.editorconfig sets charset = utf-8-bom for C# sources. The two files added in the previous commit were written without the byte order mark, which the CI build rejects with "error CHARSET: Fix file encoding". A Release build of the solution on Windows does not run that check, so it passed locally.
|
Since this is a breaking API change, I don't know that I'll accept this immediately. I just released a major version and avoided this (though it is a valuable change) so I may need to "save this for later." That said, without doing a deep review - one of the things I've been trying to get to in the repo for a while is the separation of "tests through the public API" from "tests that require use of an internal API." The "Autofac.Specification.Test" project is the "test through the public API" set of tests - these are the ones that we shouldn't change as we modify internals. "Autofac.Test" is continued testing that requires internal access. This is admittedly a work in progress - we started this separation before AI agents were really a thing and it would have caused a lot of churn. It may be something I'll look at finishing later. Anyway - we do have a decorator testing fixture here and I think the tests here that verify how one might access pipeline configurations and such through this new feature would go in there rather than Autofac.Test, ideally reusing types and things in there. |
All three overloads changed from returning void, so the type argument each one closes over is now part of the public API. Asserting only that the returned builder is non-null left that unpinned. The non-generic overload deliberately returns IDecoratorRegistrationBuilder<object>, matching the precedent set by RegisterType(Type) returning IRegistrationBuilder<object, ...>, and the other two close over the decorated service type.
test/README.md reserves Autofac.Specification.Test for the public API as an average user would exercise it, so the 90% case stays pinned down across refactors and breaking changes. The tests for the new builder lived only in Autofac.Test and drive the resolve pipeline directly, which left the public WithCondition surface uncovered by the suite meant to protect it. Adds the public-API twin of the existing conditional decoration test, a test that WithCondition replaces a condition passed to RegisterDecorator, and a test that configuration applied after the RegisterDecorator call still takes effect - the user-visible guarantee of deferring the registration snapshot to container build time. Also covers the two overloads whose returned builder was previously discarded. Both were already called here, so only the generic overload had its return value exercised at all.
Autofac.Test leaves <Nullable> unset, and SDK versions disagree about which nullable diagnostics that produces: CI reported CS8600 for locals initialised to null and CS8620 for a Dictionary<string, object> passed to WithMetadata, while a clean Release build on 10.0.401 reported nothing at all. TreatWarningsAsErrors is on for Release, so the build failed on diagnostics that could not be reproduced locally. Opting the file into #nullable enable removes that dependence and makes a local build meaningful. The captured locals become nullable, since they genuinely are null until the middleware or condition runs, the metadata dictionary matches the IEnumerable<KeyValuePair<string, object?>> the interface declares, and the null-forgiving operator that Assert.NotNull makes redundant is dropped.
Per review feedback: Autofac.Specification.Test is the "test through the public API" project and Autofac.Test is for tests that need internal access. Nothing in DecoratorRegistrationBuilderTests touched an internal API - Service, IComponentRegistration, DecoratorService, IResolvePipelineBuilder, PipelinePhase, MiddlewareInsertionMode and ResolveRequestContext are all public - so the whole file belonged in the specification fixture. The tests now reuse the types already in that fixture, ImplementorA, DecoratorA and DecoratorB, rather than redeclaring their own near-identical copies, and assert on the decorated chain the way the surrounding tests do. The one type added is ProxyStandIn, which derives from the existing Decorator base and stands in for the proxy an interception integration would apply. Five tests were dropped as redundant with coverage already in the fixture: decorator nesting order is DecoratorCanBeAppliedTwice, the WithCondition replacement test was an exact duplicate, and the two overload tests and the configure-after-registration test overlapped CanConfigureADecoratorRegisteredByType, CanConfigureADecoratorRegisteredByLambda and DecoratorCanBeConfiguredAfterItIsRegistered. Captured values use list accumulators instead of locals initialised to null, and the metadata sequence uses a collection expression, so none of the moved tests need nullable annotations. That keeps the fixture's nullable context as it was - enabling it for the whole file would have meant changing an unrelated pre-existing test.
No rush at all — please take whatever time you need on the review, and I completely understand if this has to wait for the next major. Thanks for the pointer on test placement. You're right, and I'd got it wrong: nothing in that file actually needed internal access — Service, IComponentRegistration, DecoratorService, IResolvePipelineBuilder, PipelinePhase and ResolveRequestContext are all public — so I've moved the whole thing into Autofac.Specification.Test/Features/DecoratorTests.cs and deleted the Autofac.Test file. The tests now reuse ImplementorA, DecoratorA and DecoratorB instead of redeclaring near-identical copies, and I dropped five that duplicated coverage already in that fixture. Any other suggestions are very welcome — happy to keep iterating. |
2 participants
The RegisterDecorator overloads taking a decorator type or factory returned void and built the decorator registration internally, calling CreateRegistration() - which clones the pipeline - before returning. There was nothing to chain configuration onto, and the pipeline was frozen by the time the caller got control back.
They now return IDecoratorRegistrationBuilder, exposing ConfigurePipeline, WithMetadata, WithCondition and the registration metadata. This is deliberately a new, narrow builder rather than IRegistrationBuilder: As, Named, Keyed and the lifetime methods are declared directly on IRegistrationBuilder and are meaningless or harmful for a decorator, whose service is fixed by DecoratorService and whose lifetime is governed by the decoration machinery.
Creating the component registration is deferred to a single container build callback, so configuration applied any time before Build() is picked up. RegisterGenericDecorator is unchanged: open generic decorators build their closed registration lazily at resolve time, so there is no registration-time builder to hand back.
Middleware added through ConfigurePipeline runs on the decorator's own registration pipeline, which is what lets an integration wrap the outermost decorator instead of the component being decorated.
Source-compatible for existing callers, but binary-breaking on the strong-named assembly, so this needs to ship in a major release. The version in default.proj is left alone.
Refs #1416