-
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
Picocli does not inject options if a bean is annotated with @Transactional #1650
Comments
The picocli-related code you’re showing looks fine. How is this command wired up to the main method of the application? I think that’s where the problem may be… |
Actually, I don’t know what |
@transactional wraps a DB-Transaction around all public methods (if the caller is also a spring bean). @transactional can also be used on selected methods only. I tried this, but had the same problems. When I remove the annotation and user programmatic transaction handling, everything works fine. This is my main Class:
MainCommand.java:
And the CommandLineRunner implementation:
|
Would it be an idea to put the For example: @Command(name="datenlieferantLoeschen")
public class DatenlieferantLoeschen implements Callable<Integer> {
@Inject // or @Autowired
private MyBusinessLogic myBusinessLogic; // injected by Spring
@Option(names = {"-d", "--dlids", "--datenLieferantIds"}, required = true)
long[] datenliefrantIds;
@Option(names = {"-r", "--dryRun"})
boolean dryRun;
@Spec
CommandLine.Model.CommandSpec spec;
@Override
public Integer call() {
myBusinessLogic.doStuff();
}
}
@Service
@Transactional
class MyBusinessLogic {
public void doStuff() {
//...
}
} |
Yes, everything that keeps the spring transaction management away from the command bean works. But there is definitely something wrong. It's likely that any kind of crossfunctional behaviour mixed in by spring will cause the same problem. |
Just a guess: |
Ok, thanks for confirming that that solution works! We can add a note to the picocli user manual Spring section saying something like:
Additionally, it may be an idea to add logic to the picocli annotation processor (the picocli-codegen module) to detect Also, does Spring have a mechanism to declare some annotations incompatible with each other? If so, perhaps that mechanism can be used in the picocli-spring-boot-starter module to declare this incompatibility. Thoughts? |
As long picocli cannot deal with this, any kind of documentation and hints is useful. It would have saved me a full day of nasty debugging. It's the purpose of CDI framworks like spring to allow for any number of cross cutting concerns to be mixed in at runtime. So you can easily have transactioning, security, logging and caching. There are a lot of 3rd party framworks (like picocli :-) ) that provide spring integration like this. It's impossible to maintain a list of incompatible tags. To be fully spring compatible, picocli should use setter methods to inject values in favour of direct field injection. It's likely that this is also true for other CDI frameworks. |
I’m still unclear on how picocli can be made to deal with this. 😅 I can certainly add the documentation mentioned above (any suggestions to improve the wording are welcome).
Oh, would that solve the problem? |
My bad, I searched for something like that but missed this in the docs. Yes, for CDI containers like spring, using setter methods is a must and should always work. If the container wrapps the original object, it is also responsible to delegate to the method on the original object. So this will always work. I'm not an expert on the other CDI containers mentioned in https://picocli.info/#_dependency_injection. But it's likely that they have the same restriction. I suggest to add something like this in the docs: I just tested this, and it definitely solves the problem. |
@pluess Great, thank you for testing and confirming this. Quick question: is just the About the docs phrasing, I'm thinking to keep it limited to the Spring section for now until I can confirm how the same issue would manifest in other frameworks. This is what I thought of so far:
|
Basically every access to spring beans has to go through public or package private setters methods. This means the example of MailCommand is not valid spring code. Depending on the dependency graph of the spring beans you can get proxys even if you only use the basic @component annotation. BTW: All of the above is also true for the @SPEC annotation as well. |
I see your point, although I would not go as far as saying that the example MailCommand is not valid Spring code. (The example was reviewed and updated by @snicoll, so I have some confidence that it is okay as it stands.) But your point is valid: with a different configuration, Spring may use proxies and annotated fields will no longer work as expected. The Spring reference manual examples mostly use constructor and setter method injection. On the other hand, the docs on So I am still pondering the best way to phrase this... 😅 This is my latest attempt:
|
That's perfect. |
I added the above text to the user manual. |
@remkop I hit this same issue on Weld SE CDI following the example in the manual: https://picocli.info/#_cdi_2_0_jsr_365 In that example, those fields having @option annotation are not getting set to the provided values due to picocli setting the fields on a proxy which is retrieved from CDI. I only managed to get the fields populated with correct values after creating public setter methods and annotating them with @option. I believe manual needs update to reflect this. If you agree, I'm happy to raise a PR if you can point me to the source of the manual. |
@Mert-Z The html is generated by the build. (From memory, running |
I have spring bean which is represents a command and uses @transactional for DB access.
Up on entering the
call()
method the options nor spec is set.While debugging I realized, that the object picocli is using to inject the options and the spec and the one finally executing the
call()
method within the spring managed transaction are not identical.As a workaround I can of course use springs TransactionTemplate.
The text was updated successfully, but these errors were encountered: