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

Coupling introduced by change to add missing required services in AddEntityFramework #956

Closed
divega opened this issue Oct 24, 2014 · 4 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-unknown
Milestone

Comments

@divega
Copy link
Contributor

divega commented Oct 24, 2014

The fix for #890 introduces coupling between EF and the implementation of other framework libraries such as Logging, DI and Options, e.g. EF code now contains hardcoded knowledge about what exact services are needed and how they should be registered. These details which could change in the future if e.g. TypeActivator could take a dependency on some other service, Options could replace TypeActivator, etc and hence EF could be broken.

One way to reduce the coupling would be to abstract the services that are added, e.g. those libraries could implement the DI idiom we have implemented in some of the upper-level frameworks and then you could so something like:

services.AddDependencyInjection();
services.AddLogging();
services.AddOptions();

Furthermore, it might be that the coupling cannot be ever completely addressed with this approach, e.g. what happens if Logging acquires a dependency on a completely different library? Unless Logging itself starts adding its missing required services in a similar fashion as EF does by a consequence of this change, things will still be broken for EF.

For the latter reason I think we should make a decision: either adding missing required services implicitly is a good idea and all libraries should do it, or we should stay away from it and push instead for all required services to be super easy to add explicitly to the service collection in application and test code.

@rowanmiller rowanmiller added this to the 1.0.0 milestone Oct 27, 2014
@divega
Copy link
Contributor Author

divega commented Oct 30, 2014

The current implementation is more broken than we though: ASP.NET vNext applications using EF7 only log certain messages because we are registering a separate ILoggerFactory which hides the ILoggerFactory already registered by Hosting. We cannot assume that looking into the IServiceCollection passed to AddEntityFramework() is enough to know whether an ILoggerFactory is already registered.

We are currently considering two possible options:

  1. Abstract EF7 required external dependencies into our own service types: We would provide wrappers for the services. At runtime the wrappers would try to resolve the original services types and if no instance is returned by DI we can simply new the default implementation of the services up. Take note that we cannot register the wrappers as the original service type because DI does not support cycles in dependencies. Instead we can use the concrete type.
  2. Require applications that are not ASP.NET vNext Web applications but that are built using DI to register the dependencies explicitly into their application containers, e.g.:
services.AddOptions();
services.AddDependencyInjection();
services.AddLogging();
services.AddEntityFramework()
    .AddSqlServer()
    .AddDbContext<MyDbContext>();

(I have filed bugs in different repositories to add the missing "sugar" methods because it seems to be good to add them independently of what we decide to do in EF.)

As a variation of the second option, we can add our own extension method in EF to make this shorter:

services.AddEntityFramework()
    .AddInfraestructure()
    .AddSqlServer()
    .AddDbContext<MyDbContext>();

@divega divega modified the milestones: 1.0.0-beta2, 1.0.0 Oct 30, 2014
@divega
Copy link
Contributor Author

divega commented Oct 30, 2014

Moved to beta2 as this is currently impacting the behavior of logging on ASP.NET vNext applications.

@divega
Copy link
Contributor Author

divega commented Nov 14, 2014

Talked to @lodejard and @ajcvickers. Since this is currently breaking logging on Web apps and a solution for adding "ensure service" functionality for DI may take longer or not happen, we need to revert the change that adds ITypeActivator, IOptions<> and ILoggerFactory inside AddEntityFramework() now.

We should discuss with @rowanmiller whether we want to add a logger factory wrapper or something like the AddInfraestructure() method as I proposed above. My vote if for the latter.

@HaoK
Copy link
Member

HaoK commented Nov 14, 2014

If the changes in this PR aspnet/DependencyInjection#133 are made to DI, and we exposed an over load of the new sugar methods to specify adding services as fallback only, EF should be able to just always add them without getting in in the way if someone else adds them later for real. (isFallback basically means last ditch default)

services.AddOptions(isFallback: true);
services.AddDependencyInjection(isFallback: true);
services.AddLogging(isFallback: true);


@rowanmiller rowanmiller modified the milestones: 7.0.0-beta2, 7.0.0 Nov 25, 2014
ajcvickers added a commit that referenced this issue Nov 27, 2014
…ent.

Issue #956 and also see pull request #1129.

This change updates all of our AddXxx methods to use TryAdd when adding to the service collection such that only services that are not already registered will be registered. This also means that the code we had to check the service collection for "hosting" services is no longer needed.
ajcvickers added a commit that referenced this issue Nov 27, 2014
…ent.

Issue #956 and also see pull request #1129.

This change updates all of our AddXxx methods to use TryAdd when adding to the service collection such that only services that are not already registered will be registered. This also means that the code we had to check the service collection for "hosting" services is no longer needed.

Note that services for which it is expected that multiple instances can be registered (e.g. listeners and data sources) must not use TryAdd since otherwise only the first registered will be used.
ajcvickers added a commit that referenced this issue Dec 2, 2014
…ent.

Issue #956 and also see pull request #1129.

This change updates all of our AddXxx methods to use TryAdd when adding to the service collection such that only services that are not already registered will be registered. This also means that the code we had to check the service collection for "hosting" services is no longer needed.

Note that services for which it is expected that multiple instances can be registered (e.g. listeners and data sources) must not use TryAdd since otherwise only the first registered will be used.
@ajcvickers ajcvickers modified the milestones: 7.0.0-beta2, 7.0.0 Dec 2, 2014
@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 Oct 15, 2022
@ajcvickers ajcvickers modified the milestones: 1.0.0-beta2, 1.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
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-unknown
Projects
None yet
Development

No branches or pull requests

4 participants