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

Bug: base and child class with the same parameter name #123

Closed
kentsday opened this issue Jun 23, 2024 · 8 comments
Closed

Bug: base and child class with the same parameter name #123

kentsday opened this issue Jun 23, 2024 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@kentsday
Copy link

kentsday commented Jun 23, 2024

since version 2.5.1

if base class is injected ILogger logger and child class is injected ILogger<T> logger.
it will create two parameter for child class, but as we all know, it will fail to inject ILogger from MSDI.

in version 2.5.0, it will be ok to handle the issue a show above, because I can ajust the parameter order by myself to make it works.

@k94ll13nn3
Copy link
Owner

Hello, what version of this package are you using ? There is no version 2.5.1 or 2.5.0. Did you mean 5.2.1 and 5.2.0 ?

I've done some fixes yersterday about this specific case, so maybe you are talking about version 5.4.1 ?

Can you give a sample of what was working before and what is not working now ?

@kentsday
Copy link
Author

kentsday commented Jun 23, 2024

Hello, what version of this package are you using ? There is no version 2.5.1 or 2.5.0. Did you mean 5.2.1 and 5.2.0 ?

I've done some fixes yersterday about this specific case, so maybe you are talking about version 5.4.1 ?

Can you give a sample of what was working before and what is not working now ?

Sorry, I don't know why I typed 2.5.1.
It is 5.4.1 and 5.4.0. The new bug is raised due to the new fix.

classes:

[AutoConstructor]
public abstract partial class BaseClas
{
    private readonly ILogger _logger;
}

[AutoConstructor]
public sealed partial class ChildClass : BaseClas
{
    private readonly ILogger<ChildClass> _logger;
}

with 5.4.0, it works well as I expected.

    partial class ChildClass
    {
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("AutoConstructor", "5.0.0.0")]
        public ChildClass(Microsoft.Extensions.Logging.ILogger<Ex3.CryptoProviders.Solana.Tests.ChildClass> logger) : base(logger)
        {
            this._logger = logger;
        }
    }

with 5.4.1, it require a new ILogger, it is no ok.

    partial class ChildClass
    {
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("AutoConstructor", "5.0.0.0")]
        public ChildClass(Microsoft.Extensions.Logging.ILogger<Ex3.CryptoProviders.Solana.Tests.ChildClass> logger, Microsoft.Extensions.Logging.ILogger b0__logger) : base(b0__logger)
        {
            this._logger = logger;
        }
    }

@k94ll13nn3
Copy link
Owner

It is what I was thinking you experienced. And it is not actually a bug, it is was it always should have been and it is what is expected since 5.4.1. I didn't think it would cause many problems because for the most parts, the code that have been fixed did not compile.

It only worked in your case because ILogger<ChildClass> inherits ILogger, but if your BaseClass had, let's say, a ILogger<BaseClass>, the code generated in 5.4.0 would have not compiled, whereas it does now.

The fact that it worked was only because it was the same parameter name and by chance, the child type injected was inheriting the other, there is no type inheritance checking in this library.

Can you change the ILogger in your base class to ILogger<BaseClass> ?

@kentsday
Copy link
Author

@k94ll13nn3
it could be ILogger<BaseClass>, it works. but in in 5.4.1 it would inject two loggers into a class, and making logger is hard to filter. the T in ILogger<T> is used to identify the logger and will append to logging system.
so even using ILogger<BaseClass> would compile success, but to inject two loggers into a service it not a good practice this in case.
FYI: https://stackoverflow.com/questions/60344757/base-class-type-for-iloggert-using-dependency-injection

@kentsday
Copy link
Author

However, it is indeed difficult to judge because it seems impossible to determine the inheritance relationship between ILoggers at the stage of SG.
If it's okay, I think you can add some attributes. Yes, the development can freely control whether to inject new ones or use the same ones. In that case, there may be no problem.

@k94ll13nn3
Copy link
Owner

I will see what I can do to allow you to override the current behavior in order to have only one logger.

@k94ll13nn3 k94ll13nn3 added the enhancement New feature or request label Jun 24, 2024
@k94ll13nn3 k94ll13nn3 self-assigned this Jun 24, 2024
@k94ll13nn3
Copy link
Owner

k94ll13nn3 commented Jun 24, 2024

Version 5.5.0-beta.1 is available with a new parameter matchBaseParameterOnName on AutoConstructor.

You can replace you child code with this and it should be back as it was.

[AutoConstructor(matchBaseParameterOnName: true)]
public sealed partial class ChildClass : BaseClas
{
    private readonly ILogger<ChildClass> _logger;
}

Let me know what you think of this solution before I release a new version.

@kentsday
Copy link
Author

Thanks, you are really helpful!
5.5.0-beta.1 works as I expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants