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

KeyFilterAttribute behavior different from AutowiringParameter #1030

Closed
bignoncedric opened this issue Oct 10, 2019 · 5 comments
Closed

KeyFilterAttribute behavior different from AutowiringParameter #1030

bignoncedric opened this issue Oct 10, 2019 · 5 comments
Labels
bug

Comments

@bignoncedric
Copy link

@bignoncedric bignoncedric commented Oct 10, 2019

When there the parameter type is not registered in the DI, the KeyFilterAttribute behavior is different from the AutowiringParameter behavior.

Examples

"Normal" required parameter

class Test
{
    public Test(int parameter) => Console.WriteLine($"Test(parameter = {parameter})");
}

var cb = new ContainerBuilder();
cb.RegisterType<Test>()
  .WithAttributeFiltering()
  .As<Test>();
cb.Build().Resolve<Test>();

Output

Throws:

Autofac.Core.DependencyResolutionException: An error occurred during the activation of a particular registration. See the inner exception for details. Registration: Activator = Test (ReflectionActivator), Services = [Submission#46+Test], Lifetime = Autofac.Core.Lifetime.CurrentScopeLifetime, Sharing = None, Ownership = OwnedByLifetimeScope ---> None of the constructors found with 'Autofac.Core.Activators.Reflection.DefaultConstructorFinder' on type 'Submission#46+Test' can be invoked with the available services and parameters:
Cannot resolve parameter 'Int32 parameter' of constructor 'Void .ctor(Int32)'. (See inner exception for details.)

"Normal" optional parameter

class Test
{
    public Test(int parameter = 1) => Console.WriteLine($"Test(parameter = {parameter})");
}

var cb = new ContainerBuilder();
cb.RegisterType<Test>()
  .WithAttributeFiltering()
  .As<Test>();
cb.Build().Resolve<Test>();

Output

Prints:

Test(parameter = 1)

Required parameter with KeyFilter

class Test
{
    public Test([KeyFilter(0)] int parameter) => Console.WriteLine($"Test(parameter = {parameter})");
}

var cb = new ContainerBuilder();
cb.RegisterType<Test>()
  .WithAttributeFiltering()
  .As<Test>();
cb.Build().Resolve<Test>();

Output

Prints:

Test(parameter = 0)

Expected Output?

Throws:

Autofac.Core.DependencyResolutionException: ...

Optional parameter with KeyFilter

class Test
{
    public Test([KeyFilter(0)] int parameter = 1) => Console.WriteLine($"Test(parameter = {parameter})");
}

var cb = new ContainerBuilder();
cb.RegisterType<Test>()
  .WithAttributeFiltering()
  .As<Test>();
cb.Build().Resolve<Test>();

Output

Prints:

Test(parameter = 0)

Expected Output?

Prints:

Test(parameter = 1)

Detailed analysis

The AutowiringParameter checks whether the parameter type is registered or not. If not, the the AutowiringParameter returns false in the CanSupplyValue method.

The WithAttributeFiltering method will allows return true in the CanSupplyValue if a ParameterFilterAttribute is found.

Here is a table summarizing the returned value of Parameter.CanSupplyValue method:

AutowiringParameter KeyFilterAttribute
Registered true true
Not registered, Required parameter false true
Not registered, Optional parameter false true

Question

Is it the expected behavior?
If so, why is it preferred over consistency with "standard" parameters?

@tillig tillig added the bug label Oct 10, 2019
@tillig

This comment has been minimized.

Copy link
Contributor

@tillig tillig commented Oct 10, 2019

Sounds like a bug! Nice find, thanks for the analysis.

@bignoncedric

This comment has been minimized.

Copy link
Author

@bignoncedric bignoncedric commented Oct 13, 2019

Fixing this issue will be a breaking change.
What if some users of the package rely on (intentionally or unintentionally) on this behavior?

Additionally, to fix this bug, I think we will need to change the ParameterFilterAttribute.ResolveParameter method signature. Is it acceptable? (changing public method signature of an abstract class).

To limit the breaking changes, a new class "NewParameterFilterAttribute" could be created and the other ParameterFilterAttribute marked as obsolete.

What is the best approach to solve this?

@tillig

This comment has been minimized.

Copy link
Contributor

@tillig tillig commented Oct 13, 2019

We have a 5.0 in the works that changes containers to be immutable and changes several other method signatures. If something has to break to fix this, this would be the time.

@bignoncedric

This comment has been minimized.

Copy link
Author

@bignoncedric bignoncedric commented Oct 14, 2019

@tillig, What is the branch for this new version? Is it pr-immutable-container?

@tillig

This comment has been minimized.

Copy link
Contributor

@tillig tillig commented Oct 14, 2019

Go ahead and target develop - there are some breaking changes in there already as we prepare. The pr-immutable-container bit is under testing right now but will make it into develop before 5.0 rolls out. We bring develop changes into pr-immutable-container as we test. Thanks for asking!

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
2 participants
You can’t perform that action at this time.