-
Notifications
You must be signed in to change notification settings - Fork 418
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
Constructor injection (was: Annotating Kotlin primary constructor properties) #1195
Comments
@rgoldberg Thank you for raising this. This topic came up earlier in #1107, #1281 and #1358 and while exploring some ideas, we encountered two major issues:
Dependency injectionPicocli currently supports integration with Dependency Injection containers via the I would like to be able to use the However, it is not clear to me how a factory implementation (like the the PicocliSpringFactory) would implement constructor injection. To go into some detail, the interface IFactory {
<K> K create(Class<K> clazz) throws Exception;
} One problem is binary compatibility: this interface does not allow any parameters to be passed other than the class. Would we need to change this interface? That would be bad, but also, what would that new interface look like? Alternatively, perhaps we can avoid changing the IFactory interface, if we have another interface that allows picocli to somehow "register" the constructor parameters in the DI container context, and the DI container will then "pick" these parameters when it calls the constructor. I am very vague on how that would work, and how, for example, the DI container would be able to determine the correct parameter order when there are multiple parameters of the same type. Note also that this means changes not just to picocli, but also to factory implementations, like the Spring Boot one, and the Micronaut, and Quarkus ones. These are not insurmountable blockers, but just to clarify the scope. Potential edge case: what about "hybrid" constructors, that have some picocli-annotated constructor parameters like Without Dependency InjectionAnother alternative is to implement constructor injection of Note that any object that is constructed by picocli via reflection cannot have any services injected by the DI container. So that would be the trade-off. Implementation complexityAnother consideration is that user object creation is already one of the most complex areas of the library. Picocli instantiates subcommands, mixins, argument group classes and various other objects. For reference, some related tickets are [#690] (delayed instantiation), and [#962][#961][#990], about how default values are applied. I am not sure if the complexity is because of how the implementation has grown or whether it is because of the nature of the problem. It is possible that a refactoring can simplify things. However, any new work in this area will need to deal with this and at the very least not cause any regressions. To be honest, at the moment I don't see myself working on this in the near future. |
Thanks for the info. I might look into the DI integrations you mentioned in the future (I can't look at it right now). |
If it proves difficult to implement the hybrid edge case of some constructor / function arguments being supplied by picocli, and others being supplied directly by DI, you could go with a simple MVP that requires all arguments to be supplied by picocli, if any are. I imagine that uses of this hybrid edge case would be exceptionally rare. |
Yes, the hybrid edge case was more for completeness. The minimum version would probably be what I mentioned under "Without Dependency Injection" in my previous comment. |
From a naive perspective, without having looked at the picocli code, if picocli can require Java 8 or newer, could you add the following to default <K> K create(Class<K> clazz, Object... args) throws Exception {
throw new UnsupportedOperationException();
}
default boolean supportsConstructorArguments() {
return false;
} The latter could be used by the annotation processor to determine if the current The arguments will be passed in the order of the Is there anything that this approach is missing? Would picocli be unable to determine which argument corresponds with each parameter? Shouldn't that be able to be determined via reflection? If you must support Java older than 8, then you could just add the overload to a subinterface of Or, if all |
Thanks for helping me think through this. |
Cool. Glad my spitballing was useful. Thanks for making picocli. |
If it makes sense to also allow picocli annotations on method parameters, not just constructor parameters, I don't know if you'd still use |
I might look into implementing constructor injection. A few questions:
interface IFactory {
<K> K create(Class<K> clazz) throws Exception;
} to: interface IFactory {
<K> K create(Class<K> clazz, Object... args) throws Exception;
}
|
No, this is a public interface that is an extension point, meant to be implemented by 3rd parties.
No, we cannot break existing applications. Publishing an extension point and then breaking applications that use it would be a betrayal of trust.
No, I prefer to remain Java 5 compatible. 😬
One idea is to introduce a new Disclosure: I have to warn you that I won't be able to spend much, if any, time on this. Other commitments than picocli are taking more and more of my time recently, so frankly I hesitate to committing to this feature. I worry that it will be a source of follow-up work, while I am personally not yet convinced of the value of this feature. So any PR would have to be picture-perfect, with docs, tests, etc., and the implementation would need to be simple enough that I am confident that I can maintain it, extend it and fix bugs in the future. To be perfectly honest, I would rather not do this. 🤔 |
Thanks for info. I'll put it on the back burner, then. Good luck with other projects. |
Thank you for the discussion. I will close this ticket for now. |
Do you have a backlog issue label? That would be useful to remember what issues weren't actually resolved, and that could possibly be revived later when resources are available. I think this is a good idea that there just aren't resources for right now, rather than being an intrinsically bad idea or a non-issue, which is what I typically associate with wontfix. |
You are probably right. I removed the wontfix label. |
If I implement this nicely with tests, docs, etc., would you have the bandwidth to review and accept it? May I call the new interface |
@rgoldberg Thanks for looking at this again. I have to say, I still have the same reservations as before:
So, while I don't want to be disrespectful of your time, I cannot guarantee when I will review this or whether your PR will ever get merged; upon seeing it I may decide that I still don't like the trade-off between functionality and complexity and choose not to merge this. I realize that this is not very encouraging, but I just want to be honest and clear before you invest a lot of time. |
Thanks for the info. I completely understand about limited bandwidth, and I agree about trying to limit complexity. The functional difference of this feature is to allow Pico CLI users to manage the complexity of their programs without Pico CLI forcing them to use constructs that increase the complexity of the user's codebase. So it's sorta addressing for Pico CLI users the same concerns you have for Pico CLI itself. I'll look into this in some spare time to see if it can be managed without adding much complexity to Pico CLI. I'll let you know if it looks simple, medium, or complex, along with whatever ideas I have. Probably not amazingly soon, but sometime in the future. Then, if & when you have the time to look it over, you can assess if it looks acceptable. |
Great, it sounds like we are on the same wavelength then. |
While looking through the code, I've found some minor issues like:
All changes would be localized, simple, Java 6 compatible & pass all tests. Do you want me to submit PRs? How should I break them down? |
@rgoldberg yes please! Improvements are welcome! How to break them down: the things you mentioned can all go into one single 'improvements' PR. (Feel free to break them up if you want to, either way is fine.) That said, if there are any changes that add to or modify the public API, then let's have a separate PR for those. Anything that impacts the public API needs to go into the next minor release (4.7.0), while things that don't impact the API can go into the next bugfix release (4.6.3), so can be merged sooner. I will probably do a 4.6.3 release first. |
I noticed that annotating a method with Could The main differences that I see are that a class can also be annotated with The simplest solution would be for annotating a class with The simplest solution could disallow picocli injection annotations on fields if any of the constructors are annotated with
|
I had not thought of that point until now: currently, a By the way, it is not yet clear to me what the user object of the
I think it would be confusing to allow multiple constructors to be annotated with
Yes, I think it would also be confusing to allow |
Sorry. Wasn't able to look at this until now. It sounds like you're ok with the general principle of allowing If so, I'll look into this after modifying my existing PR for |
Sorry, I have been extremely busy with Log4j the last few days. I did not want to leave your question unanswered, but perhaps in answering them I sounded more eager than intended. 😅 My stance essentially has not changed. |
Support annotating Kotlin primary constructor properties, like:
This would presumably require supporting applying annotations to
PARAMETER
element types. If implemented, this support should probably support annotating parameters in other use cases, like Java & other language constructor parameters for classes annotated as@Command
, parameters for subcommand methods / functions, etc.This request can be retitled / redescribed as general support for annotating JVM parameters, if the Kotlin primary constrictor property support must indeed be implemented as I outlined above.
The text was updated successfully, but these errors were encountered: