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

Allow disabling resolving concrete types #377

Closed
dotnetjunkie opened this Issue Feb 14, 2017 · 22 comments

Comments

Projects
None yet
7 participants
@dotnetjunkie
Copy link
Collaborator

commented Feb 14, 2017

Simple Injector currently resolves unregistered concrete types by default. Although this is convenient in a number of cases, it can also lead to errors. Because of this, Simple Injector has several Diagnostic warnings that shield the user from making errors when using concrete unregistered types, such as:

Some improvements in v4 will even improve the likelihood that configuration mistakes are spotted.

Still however it is possible for the user to shoot himself in the foot, for instance by accidentally resolving a concrete type (when doing dispatching for instance) instead of the interface. This would prevent Simple Injector from being able to apply decorators.

Perhaps we should disable the creation of unregistered concrete types by default, to prevent these errors and supply a configuration switch that allows the user to re-enable this behaviour, for legacy scenarios:

container.Options.ResolveUnregisteredConcreteTypes = true;

Let's discuss.

@TheBigRic

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2017

I completely vote for disabling this by default. I shot myself in my foot multiple times because concrete types where autowired.

One scenario that comes to mind is where some concrete service is used in a transient component at one part of the solution, but this same component is scoped or singleton in an other part of the solution.

Although Simple Injector will prevent this other application from running when this happens and the exception message is super clear it is most of the times still a surprise why the container won't verify for this application because in my mind nothing has really changed in this application, while in fact something has changed.

And I think it is just sane practice the configure all parts of the application.

@trailmax

This comment has been minimized.

Copy link

commented Feb 20, 2017

I completely agree with disabling this by default. With option to enable when needed. Never had to use this function because CQRS.

@AppiePau

This comment has been minimized.

Copy link

commented Feb 21, 2017

Interesting, so far setting the default registration lifetime to for example scoped resolved the issue for me. If resolving by default is disabled, would the diagnostic show a list of services which are not resolvable?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2017

@AppiePau, showing a list won't be needed. Verify will simply fail on the first unresolvable type in the graph and this type will be prompted in an exception message - as always. But even withou calling Verify, a call to GetInstance would fail for a graph with such a type.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2017

I'm really interested to see some examples from developers who are actually currently depending on this behavior.

@alexfoxgill

This comment has been minimized.

Copy link

commented Mar 11, 2017

Perhaps concrete types could be resolvable if they implement no interfaces? I have in the past used simple 'record' types for providing strongly-typed access to config settings, for example

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 11, 2017

@alexfoxgrill, but wouldn't that require those records to be registered as singleton?

@alexfoxgill

This comment has been minimized.

Copy link

commented Mar 11, 2017

I don't see how the lifestyle is impacted by this..? You could just as easily have transient instances if you wanted?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 11, 2017

When you have objects that provide strongly typed access to configuration values, these configuration values should typically be loaded at app startup to ensure the application fails fast in case of a misconfiguration. In case config values are read once at startup, it means that these strongly typed wrappers just wrap the values and are created at startup as well. This means they will be singletons as well.

What you are describing seems an object that contains behavior and forwards the request to something like ConfigurationManager.AppSettings which is sub optional.

The case where this does make a lot of sense is when the comfig values are volatile and can change during the lifetime of the application.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 11, 2017

I think there are different options here when it comes to resolving unregistered concrete types:

  • Simple Injector can try to resolve all unregistered concrete types (current v3 behavior)
  • It can resolve unregistered concrete types only if they are a dependency of some other type. The rational behind this is that root types should always be registered, since omiting them make the Diagnostic Services blind, while dependencies are already analyzed by the container and the improvements in the Diagnostic Services of v4 should already find any misconfigurations of unregistered concrete types that are dependencies. To me, unregistered root types will be the biggest problem (as long as users actually call Verify() ofcourse).
  • It can resolve unregistered concrete types only if it implements no interfaces, as @alexfoxgill suggests.
  • Some other criteria?
  • A combination of one of the previous three.
  • None. No unregistered concrete type should be resolved.

The dilemma is here that although we want to improve Simple Injector, we also want to prevent introducing breaking changes that will impact and confuse too many users, and potentionally cost a lot of effort to fix, while they might have a valid use case for this.

I'm currently thinking about making the "don't resolve unregistere concrete root types" the default for v4 (although that allone could be quite a breaking change already), where the user might be able to switch to "Resolve All" and "Resolve None" using the Container.Options.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 13, 2017

After building a proof of concept, a massive amount of (218) tests broke. Main reason for this is that a lot of tests depend on the behavior to be able to resolve unregistered concrete types. But also some of the current code samples in the documentation depend on this behavior (like the interception extensions).

This makes me realize, that although this change might be a noble one, it might impact more users than I first envisions and it is quite some work to get this feature in correctly.

I am therefore going to postpone adding this configuration switch till after v4, and if the default behavior (which is to resolve all unregistered concrete types) is changed, this will be done in the following major release, v5.

Thanks for everyone responding.

@dotnetjunkie dotnetjunkie added this to the Backlog milestone Mar 13, 2017

dotnetjunkie added a commit that referenced this issue Mar 13, 2017

Passed along InjectionConsumerInfo more often.
Passed along InjectionConsumerInfo more often during the resolving
phase. This makes it easier to add a feature like #377 in the future.
@davidroth

This comment has been minimized.

Copy link

commented Mar 20, 2017

@dotnetjunkie An example which comes to my mind where this feature causes confusion is when using Entity Framework. If I don't want to register my custom DbContext via the DbContext type this can lead to stupid mistakes.

Say for example you have a context named MyContext and you did not register it as DbContext because you you want to explicitly inject MyContext:

public MyClass(MyContext ctx) // Injects MyContext => everything ok
{ }

public void SampleMethod() => ctx.Set<MyEntity>(); // ok

However, in another class, you accidentally used DbContext instead of MyContext

public MyClass2(DbContext ctx) // Injects DbContext which does not makes sense and fails later on
{ }

// fails because DbContext has no registrations
public void SampleMethod() => ctx.Set<MyEntity>(); 

Of course, you can easily fix this by registering DbContext as MyDbContext via the registration api:

container.Register<DbContext, MyContext>()

but this only works as long as there is a single DbContext. If you have multiple DbContexts this will fail.

=> I`d prefer to disable resolving unregistered concrete types per default. A switch to enable it would make sense though.

@dotnetjunkie dotnetjunkie changed the title Disable resolving concrete types by default Allow dDisable resolving concrete types by default Apr 5, 2017

@dotnetjunkie dotnetjunkie changed the title Allow dDisable resolving concrete types by default Allow disabling resolving concrete types Apr 5, 2017

@dotnetjunkie dotnetjunkie modified the milestones: v4.1, Backlog Apr 5, 2017

@aKzenT

This comment has been minimized.

Copy link

commented Aug 24, 2017

Is there any way currently to disable the automatic resolution of concrete types? In my environment this can easily lead to bugs, so it would be nice if there is a way to force registration of these types.

How about introducing the property:
container.Options.ResolveUnregisteredConcreteTypes = false;
with a default value of true and then change the default to false in a later milestone?

Is there any other way to disable the behavior in the current release?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 25, 2017

@aKzenT,

This feature is planned for v4.1. The way to disable this in the current release is by adding the following code to the container:

container.ResolveUnregisteredType += (s, e) =>
{
    if (!e.Handled && !e.UnregisteredServiceType.IsAbstract)
    {
        throw new InvalidOperationException(
            e.UnregisteredServiceType.ToFriendlyName() + " has not been registered.");
    }
};

@dotnetjunkie dotnetjunkie modified the milestones: v4.1, v4.2 Feb 28, 2018

@dotnetjunkie dotnetjunkie modified the milestones: v4.2, Backlog Apr 26, 2018

@dotnetjunkie dotnetjunkie removed this from the Backlog milestone Mar 18, 2019

@dotnetjunkie dotnetjunkie added this to the v4.5 milestone Mar 18, 2019

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 18, 2019

Branch feature-377 created.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 19, 2019

I decided to add an Container.Options.ResolveUnregisteredConcreteTypes property that allows influencing Simple Injector's behavior. The property exposes the ConcreteTypeResolutionOption enum that defines three options, namely:

  • Never
  • OnlyDependencies
  • Always

For v4.x, the default option will be Always, which is identical to Simple Injector's current behavior.

An explanation of what the different options entail, can be seen in the definion of the enum:

/// <summary>
/// This enumeration defines the container's behavior for resolving concrete unregistered types.
/// </summary>
public enum ConcreteTypeResolutionOption
{
    /// <summary>
    /// Specifies that the container will never try to resolve an unregistered concrete type and
    /// instead throw an exception when such concrete type is requested, either directly or
    /// when injected as dependency of a consuming type.
    /// </summary>
    Never = 0,

    /// <summary>
    /// Specifies that the container will only try to resolve an unregistered concrete type in case
    /// it is requested as dependency of a consuming type, but never when requested directly
    /// (i.e. as root type) by calling GetInstance or similar methods.
    /// </summary>
    OnlyDependencies = 1,

    /// <summary>
    /// Specifies that the container will always try to resolve an unregistered concrete type
    /// when it is requested.
    /// </summary>
    Always = 2,
}

This feature is planned to be released in v4.5.

Any feedback on this is welcome.

@aKzenT

This comment has been minimized.

Copy link

commented Mar 19, 2019

I'm not sure if the option "OnlyDependencies" is a good idea. Is there precedence for configuring different behavior for root types vs. dependencies and is this desired? GetInstance is often used as a Service-Locator in places where frameworks do not provide proper dependency injection. Technically they would classify as resolving root types but conceptually they are regular dependencies of the class.

What are the use cases for this option?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 19, 2019

The idea of the OnlyDependencies option is to only block the resolution of unregistered root types. The reasoning to introduce this is as follow:

My experience is that roughly half of Simple Injector's users depend on Simple Injector's ability to create unregistered types. Usage can be divided into two categories, namely: developers who resolve unregistered root types (like MVC controllers, etc), and developers who let components depend on concrete types that they, for the sake of simplicity, don't register. (and of course there will be developers that do both things, both knowingly and unknowingly).

Because this many developers are depending on this exact behavior, changing this is a huge breaking change. It will impact many users and understand why this change was made and making the changes can potentially take a considerable amount of time.

Unregistered root types are the biggest risk as they disallow Simple Injector's diagnostic system from doing a complete analysis on the complete graph (as the root types are unknown during verification). This problem does not exist with unregistered dependencies as Simple Injector will find them anyway when it analysis the consuming types. Simple Injector also contains several verification checks to check the sanity of these unregistered types. This makes unregistered dependencies much less risky.

So the idea here is to spread this breaking change out over time:

  • Step 1: Introduce this feature in v4.5 where the default configuration is Always (old behavior, non-breaking)
  • Step 2: Introduce a breaking change in v5.0 by switching to OnlyDependencies. This will fix the most imminent problem of missing root types. [ETA 2020]
  • Step 3: Introduce a breaking change in v6.0 by switching to Never. [ETA 2021]. We might, however, chose to never do this breaking change, but we have to do an analysis to see how big the risk is and how many developers are impacted.
@TheBigRic

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

@dotnetjunkie This last comment convinces me. Let's do this in the proposed time schedule.

This sounds like a good path to follow which will trip the most lazy users at first (from v5). Thereby tremendously increasing the safety net, because now almost any type is in the container to be verified.

I guess Never option, if this becomes the default or not, won't add much because the types missed by OnlyDependencies are verified anyway. So for verification purposes the big win is v5.

Can you share a case where Never would help a user?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 19, 2019

Can you share a case where Never would help a user?

I think, eventually, Never will be the most sensible default. Users still trip over this from time to time as a configuration that at one time seems to work, suddenly stops later on after Simple Injector starts throwing diagnostic errors. Even though reading those diagnostic errors will allow the user to fix those problems, I often see that users get confused because the given diagnostic error has seemingly nothing to do with the change they made (or the type they added in the system). Blocking unregistered concrete types by default, therefore, prevents this confusion. Besides, there might be other subtle misconfigurations that can lurk behind bushes that might get unnoticed because of this behavior.

@aKzenT

This comment has been minimized.

Copy link

commented Mar 19, 2019

I agree that Never is the most sensible default.

@TheBigRic one example:
I like to inject Configuration-Objects into my services which I fill for example from a configuration file and then register with the container. These are simple POCOs and I don't see any value in creating an interface for them. When I forget to register the configuration object, I would not want the container to simply create an empty object for me.

@dotnetjunkie maybe it's worth considering making 'Never' the default for v5 already. Not all users will check the roadmap and understand that OnlyDependencies is only a first step towards Never. They might spend time and effort only to have a breaking change again in v6. On the other hand, it's easy enough for them to configure the property manually if they want to have the "light" version or just keep the original behavior completely.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 21, 2019

@aKzenT, you bring up a fair point. The main reason for me to introduce the third OnlyDependencies option, is as an intermediate step, but when we're introduce a breaking change in v5, it would be best to go directly to Never. Even if developers only resolve types with unregistered dependencies, they can still easily switch the configuration switch to Always to get the old behavior. This makes the OnlyDependencies option redundant and perhaps even confusing, while internally causing extra complexity as well.

Because of that, I reverted my decision to go with the three-state solution and the ConcreteTypeResolutionOption option. Instead I will implement the option as a boolean flag.

dotnetjunkie added a commit that referenced this issue Mar 21, 2019

Container.Options.ResolveUnregisteredConcreteTypes setting added to d…
…isable creation of unregistered concrete types. Fixes #377.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.