Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infrastructure: Add parameter objects and service rewriting code to avoid breaking base class D.I. constructors in patch/point releases #7465

Closed
ajcvickers opened this issue Jan 24, 2017 · 4 comments
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Contributor

When a service is overridden by a provider, the provider must call the base service constructor. This means it becomes a breaking change when base service gets new dependencies such that its constructor needs to change.

We previously implemented a mechanism whereby additional services could be injected as part of the provider service resolution mechanism. This worked because provider services were always registered through a delegate, and so we could add code to that delegate as needed to inject services. Following #7457 services are registered by the provider explicitly in the container, which is much preferable for perf, but means that there is no hook to inject services.

@jnm2
Copy link

jnm2 commented Jan 24, 2017

I'm super curious what you come up with. I've hit this in my own unrelated architectures.
I've been thinking about the fact that the BCL best practice for adding parameters to event handlers in a back-compatible way is to define an EventArgs from the start.
Compared to straight up constructor injection it feels like quite some overhead to write a DependencyArgs for each class, yet we've done it for every event for years so maybe it's not as bad is it first seems.

The DependencyArgs subclass could itself be recognized and constructor injected by the IoC container and the subclass could provide getter-only autoproperties which could be added to at any time. The understanding would be that this class is only to be instantiated by the IoC container, so perhaps making the constructor internal or private or obsolete would help prevent library consumers' code from misunderstanding and calling the constructor.

If you don't see any value to inheritance, or even want to make it impossible, DependencyArgs could be an immutable struct and be recognized by the IoC container by name convention or by attribute or by the parameter name used to inject it. For example, it could always be a nested struct within the service named DependencyArgs. It should probably be nested in any case.

Just thinking out loud.

@popcatalin81
Copy link

popcatalin81 commented Jan 25, 2017

How about just passing a single IServiceProvider parameter to all base services, and to allow customization of per provider create a wrapper IServiceProvider for each provider.

Edit: I've meant IServiceProvider not IServiceCollection

@ajcvickers
Copy link
Contributor Author

@jnm2 @popcatalin81 Passing an IServiceProvider is probably what we will end up doing, but also what we hope to avoid doing because it results in use of the service locator pattern, which then obscures dependencies. I suspect we will use this pattern along with adding [Obsolete] to the existing constructor while at the same time introducing a new constructor with the new dependency added explicitly. Then at next major version rev we will remove the obsolete constructor.

@ajcvickers
Copy link
Contributor Author

Discussed in design meeting and decided to try the parameter object approach for services, using sealed classes. Considered composition, but this would be a big change to the architecture and design of certain services.

@ajcvickers ajcvickers self-assigned this Jan 26, 2017
@ajcvickers ajcvickers added this to the 2.0.0 milestone Jan 26, 2017
ajcvickers added a commit that referenced this issue Jan 26, 2017
This is a proof-of-concept implementation of parameter objects for service dependencies. Issue #7465.

Notes:
- Parameter class is sealed.
- Ended up going with Clone on the parameters object--cleaner than other things I tried, and general purpose. We can add overloads of Clone as needed when doing point releases without breaking existing code. (May want to consider whether parameter should be options.)
- Registration is not try-add and is of concrete class
ajcvickers added a commit that referenced this issue Jan 26, 2017
This is a proof-of-concept implementation of parameter objects for service dependencies. Issue #7465.

Notes:
- Parameter class is sealed.
- Ended up going with With methods on the parameters object--cleaner than other things I tried, and general purpose.
- Registration is back to try-add again because apparently Add results in multiple services registered in the ServiceCollection...
ajcvickers added a commit that referenced this issue Jan 27, 2017
This is a proof-of-concept implementation of parameter objects for service dependencies. Issue #7465.

Notes:
- Parameter class is sealed.
- Ended up going with With methods on the parameters object--cleaner than other things I tried, and general purpose.
- Registration is back to try-add again because apparently Add results in multiple services registered in the ServiceCollection...
ajcvickers added a commit that referenced this issue Feb 3, 2017
Issue #7465

This change introduces a new class ServiceCollectionMap that is used by providers and by our service-adding methods. The new class builds a map from service type to indexes of the ServiceDescriptors in the list for the given type. This allows all the TryAdd calls that we make to work without scanning the list every time.

In addition, this type has a method for re-writing services to do property injection when we need to add a service dependency without breaking a constructor in a patch or point release. This is used by making a call at the end of our service registrations like so:

```C#
   ...
   .TryAddScoped<IResultOperatorHandler, ResultOperatorHandler>()
   .DoPatchInjection<IValueGeneratorSelector>();
```

Also, ReplaceService code has been updated to ensure that service-injection is also run on any replaced services, since these services may also inherit from our base classes.
@ajcvickers ajcvickers changed the title Find new solution to not break base class D.I. constructors Add service-re-writing code to avoid breaking base class D.I. constructors in patch/point releases Feb 3, 2017
@ajcvickers ajcvickers changed the title Add service-re-writing code to avoid breaking base class D.I. constructors in patch/point releases Add service rewriting code to avoid breaking base class D.I. constructors in patch/point releases Feb 3, 2017
ajcvickers added a commit that referenced this issue Feb 3, 2017
Issue #7465

This change introduces a new class ServiceCollectionMap that is used by providers and by our service-adding methods. The new class builds a map from service type to indexes of the ServiceDescriptors in the list for the given type. This allows all the TryAdd calls that we make to work without scanning the list every time.

In addition, this type has a method for re-writing services to do property injection when we need to add a service dependency without breaking a constructor in a patch or point release. This is used by making a call at the end of our service registrations like so:

```C#
   ...
   .TryAddScoped<IResultOperatorHandler, ResultOperatorHandler>()
   .DoPatchInjection<IValueGeneratorSelector>();
```

Also, ReplaceService code has been updated to ensure that service-injection is also run on any replaced services, since these services may also inherit from our base classes.
roji added a commit to npgsql/efcore.pg that referenced this issue Feb 5, 2017
roji added a commit to npgsql/efcore.pg that referenced this issue Feb 5, 2017
@ajcvickers ajcvickers changed the title Add service rewriting code to avoid breaking base class D.I. constructors in patch/point releases Add paramter objects and service rewriting code to avoid breaking base class D.I. constructors in patch/point releases Feb 16, 2017
ajcvickers added a commit that referenced this issue Feb 16, 2017
Issue #7465

This change introduces parameter objects for the service dependencies of all public services. This allows new dependencies to be added without breaking derived classes that call the base class constructor. For factories, these dependency objects are passed down into the services created by the factory. This means that we also don't break by adding parameters to the constructors called by the factory. This is the main reason for using this approach rather than just the service rewriting approach.

The pattern is not used for internal services since no code should be inheriting from these classes. However, the service re-writer code is still in place, which means that we have some flexibility to add dependencies to internal services even in patch releases where we are being very strict about binary compatibility.

Some work remains still to do:
- Add With methods to all ...Dependencies objects to allow for service replacement
- Add sugar for registration
- Re-visit service scopes
ajcvickers added a commit that referenced this issue Feb 23, 2017
Issue #7465

This change introduces parameter objects for the service dependencies of all public services. This allows new dependencies to be added without breaking derived classes that call the base class constructor. For factories, these dependency objects are passed down into the services created by the factory. This means that we also don't break by adding parameters to the constructors called by the factory. This is the main reason for using this approach rather than just the service rewriting approach.

The pattern is not used for internal services since no code should be inheriting from these classes. However, the service re-writer code is still in place, which means that we have some flexibility to add dependencies to internal services even in patch releases where we are being very strict about binary compatibility.

Some work remains still to do:
- Add With methods to all ...Dependencies objects to allow for service replacement
- Add sugar for registration
- Re-visit service scopes
roji added a commit to roji/efcore.pg that referenced this issue Feb 28, 2017
roji added a commit to roji/efcore.pg that referenced this issue Mar 1, 2017
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Apr 13, 2017
@ajcvickers ajcvickers changed the title Add paramter objects and service rewriting code to avoid breaking base class D.I. constructors in patch/point releases Infrastructure: Add parameter objects and service rewriting code to avoid breaking base class D.I. constructors in patch/point releases May 9, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview1, 2.0.0 Oct 15, 2022
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants