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

Prevent PredicateContext.Consumer from returning null #694

Closed
dotnetjunkie opened this issue Apr 26, 2019 · 3 comments
Closed

Prevent PredicateContext.Consumer from returning null #694

dotnetjunkie opened this issue Apr 26, 2019 · 3 comments

Comments

@dotnetjunkie
Copy link
Collaborator

@dotnetjunkie dotnetjunkie commented Apr 26, 2019

The PredicateContext.Consumer property will be null when a conditional registration is resolved as root type, e.g.:

container.RegisterConditional<ILogger, NullLogger>(
    c => c.Consumer.ImplementationType == typeof(HomeController));

// Resolves ILogger as root type, causing c.Consumer to be null
container.GetInstance<ILogger>();

With C# 8.0, however (and when #649 is implemented), the possibility of Consumer being null, will cause a warning (or compile error) in the user's code. This forces the user to do explicit null checks to prevent the compiler from tripping. Changing PredicateContext.Consumer to never return null, however, is a breaking change, because users might currently explicitly check for null when something actually is a root type.

I see the following options here:

  1. [Non-breaking] Leave the API the way it is, but do possibly internally use a Null Object implementation of InjectionConsumerInfo object to prevent from passing null throughout the code base.
  2. [Breaking] Prevent PredicateContext.Consumer from returning null, and instead throw an InvalidOperationException when the property is used on a root object. Additionally add a IsRootObject, IsDirectResolve, or similar that allows the user to check, without forcing him to catch an exception, where the earlier InvalidOperationException explains the use of that property.
  3. [Breaking] Let PredicateContext.Consumer return a Null Object InjectionConsumerInfo instance in case there is no consumer. Downside of this approach is that it forces InjectionConsumerInfo.ImplementationType and InjectionConsumerInfo.Target to become null values which basically means we're moving the problem. Or we use default values for Target and ImplementationType, but now the question becomes: what should those values be? System.Object or SimpleInjector.Container for ImplementationType? Both awkward. And what about Target? A Target is an InjectionTargetInfo which either wraps a PropertyInfo or a ParameterInfo. Should it return an empty Target?
@dotnetjunkie
Copy link
Collaborator Author

@dotnetjunkie dotnetjunkie commented May 21, 2020

feature-694 branch created.

Loading

@dotnetjunkie
Copy link
Collaborator Author

@dotnetjunkie dotnetjunkie commented May 21, 2020

I decided to go with option 2:

[Breaking] Prevent PredicateContext.Consumer from returning null, and instead throw an InvalidOperationException when the property is used on a root object. Additionally add a IsRootObject, IsDirectResolve, or similar that allows the user to check, without forcing him to catch an exception, where the earlier InvalidOperationException explains the use of that property

Loading

@dotnetjunkie
Copy link
Collaborator Author

@dotnetjunkie dotnetjunkie commented May 22, 2020

API Changes:

  • PredicateContext.Consumer never returns null, but throws an exception instead.
  • PredicateContext.HasConsumer boolean property added.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant