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

Spring not used when calling parseWithHandler #658

Closed
dupirefr opened this issue Apr 6, 2019 · 15 comments
Closed

Spring not used when calling parseWithHandler #658

dupirefr opened this issue Apr 6, 2019 · 15 comments
Milestone

Comments

@dupirefr
Copy link

dupirefr commented Apr 6, 2019

Hi!

I'm trying to use the parseWithHandler method of CommandLine to delegate calls to my subcommands automatically, but it doesn't seem to work with SpringBoot (dependencies are not autowired).

Here is my Application class:

@SpringBootApplication
public class Application implements CommandLineRunner {
	@Autowired
	private ForHRMCommand mainCommand;

	public static void main(String[] args) {
		SpringApplication.run(Application.class, args);
	}

	@Override
	public void run(String... args) {
		CommandLine command = new CommandLine(mainCommand);
		command.parseWithHandler(new CommandLine.RunLast(), args);
	}
}

The commands I created:

@Component
@Command(
		name = "forhrm",
		subcommands = {
				ServicesCommand.class
		},
		description = "Command to interact with your ForHRM installation"
)
public class ForHRMCommand implements Callable<Void> {
	@Option(names = "--start", description = "Starts ForHRM FrontOffice") private boolean start;
	@Option(names = "--with-services", needs = "--start", description = "Starts the services as well") private boolean withServices;

	@Autowired
	private ServicesCommandService servicesCommandService;

	@Override
	public Void call() {
		...
	}
}

@Component
@Command(name = "services")
public class ServicesCommand implements Callable<Void> {
	...
	@Autowired
	private ServicesCommandService service;

	...
}

And finally a service I intend to use:

@Service
public class ServicesCommandServiceImpl implements ServicesCommandService {...}

When I try to execute the subcommand, it fails because the service is null. Is it not supported yet?

Thanks a lot!

@remkop
Copy link
Owner

remkop commented Apr 6, 2019

Ouch! It looks like the setup explained in the manual only covers top-level commands, not subcommands. I think this is because picocli instantiates subcommands directly, while it should get subcommand instances from the Spring ApplicationContext.

What is needed is an implementation of picocli's IFactory that takes beans from the Spring ApplicationContext instead of instantiating them directly. This factory can be passed in to the picocli CommandLine constructor. (Something similar to MicronautFactory for the Micronaut dependency injection container.)

Do you think you can create this factory yourself? Picocli 4.0 has plans to improve Spring Boot support, but I cannot give you any time lines.

@dupirefr
Copy link
Author

dupirefr commented Apr 7, 2019 via email

kakawait added a commit to kakawait/picocli-spring-boot-starter that referenced this issue Apr 21, 2019
See `ApplicationContextAwarePicocliFactory`

fixes #9 and remkop/picocli#658
related to #7

---

[x] Impl
[x] Test
[ ] Sample
[ ] Doc
kakawait added a commit to kakawait/picocli-spring-boot-starter that referenced this issue Apr 22, 2019
See `ApplicationContextAwarePicocliFactory`

fixes #9 and remkop/picocli#658
related to #7

---

- [x] Impl
- [x] Test
- [ ] Sample
- [x] Doc
kakawait added a commit to kakawait/picocli-spring-boot-starter that referenced this issue Apr 22, 2019
See `ApplicationContextAwarePicocliFactory`

fixes #9 and remkop/picocli#658
related to #7

---

- [x] Impl
- [x] Test
- [x] Sample
- [x] Doc
kakawait added a commit to kakawait/picocli-spring-boot-starter that referenced this issue Apr 22, 2019
See `ApplicationContextAwarePicocliFactory`

fixes #9 and remkop/picocli#658
related to #7

---

- [x] Impl
- [x] Test
- [x] Sample
- [x] Doc
@remkop remkop added this to the 4.0 milestone May 4, 2019
@dupirefr
Copy link
Author

dupirefr commented May 5, 2019

As I understand there is nothing more to be done here?

@remkop
Copy link
Owner

remkop commented May 5, 2019

@dupirefr There is work in progress in the kakawait/picocli-spring-boot-starter project to update that project to picocli 3.x. My goal is to work together with @kakawait to bring full Spring support into the picocli project as a picocli module (or maybe two modules, one starter (basically just a pom.xml) and one that has a Spring picocli IFactory).
I am hoping the picocli 4.0 release (still some time away) will include full Spring support.

It would be great if you would be able to try this functionality as it matures, so we can get early feedback to help flush out issues before 4.0.

@remkop
Copy link
Owner

remkop commented May 10, 2019

@dupirefr Meanwhile, I believe it should be possible to address this with a custom IFactory that delegates to the Spring ApplicationContext, something like this:

public class PicocliSpringFactory implements CommandLine.IFactory {
    private final ApplicationContext applicationContext;

    public PicocliSpringFactory(ApplicationContext applicationContext) {
        this.applicationContext = applicationContext;
    }

    @Override
    public <K> K create(Class<K> aClass) throws Exception {
        try {
            return applicationContext.getBean(aClass);
        } catch (Exception e) {
            try {
                return aClass.newInstance();
            } catch (Exception ex) {
                Constructor<K> constructor = aClass.getDeclaredConstructor();
                constructor.setAccessible(true);
                return constructor.newInstance();
            }
        }
    }
}

CommandLine cmd = new CommandLine(new MyApp(), new PicocliSpringFactory(appContext));

@dupirefr
Copy link
Author

dupirefr commented May 13, 2019

Hi @remkop !
Unfortunately it doesn't work out of the box. I watched the default factory implementation and, indeed, it is more complex than the part in the first catch-block here. So some components, such as lists, are not instantiable by this factory.

I copied the code from the DefaultFactory in the catch block and now it's working. Of course, copying it is not the long-term solution I think, but as DefaultFactory is private I couldn't do something smarter.

I was thinking maybe some kind of decorator pattern could work if the DefaultFactory was made available (maybe only via the static factory method that I saw in the code). We could then do something like:

public class PicocliSpringFactory implements CommandLine.IFactory {
    private final ApplicationContext applicationContext;
    private final IFactory backupFactory;

    public PicocliSpringFactory(ApplicationContext applicationContext, IFactory backupFactory) {
        this.applicationContext = applicationContext;
        this.backupFactory = backupFactory;
    }

    public PicocliSpringFactory(ApplicationContext applicationContext) {
        this(applicationContext, IFactory.defaultFactory());
    }

    @Override
    public <K> K create(Class<K> aClass) throws Exception {
        try {
            return applicationContext.getBean(aClass);
        } catch (Exception e) {
            return backupFactory.create(aClass)
        }
    }
}

What do you think?

@remkop
Copy link
Owner

remkop commented May 13, 2019

Absolutely agreed. I made the factory method public: #684

This was included in the 4.0-alpha-3 release just now:
https://github.com/remkop/picocli/releases/tag/v4.0.0-alpha-3

@remkop
Copy link
Owner

remkop commented May 14, 2019

I like your idea of putting the defaultFactory factory method on the IFactory interface, that makes it easy to find. I created #692 for this.

@dupirefr
Copy link
Author

Nice, thank you very much :-).

It seems to work for now, though I didn't do extensive tests.

@remkop
Copy link
Owner

remkop commented Jul 10, 2019

@dupirefr, making good progress on the Spring support now, but struggling a bit with Spring Boot auto-configuration. I wonder if you would be able to give any advice on this issue: #766

Never mind; I simply missed that the @EnableAutoConfiguration annotation needed to be added. :-)

remkop added a commit that referenced this issue Jul 10, 2019
remkop added a commit that referenced this issue Jul 10, 2019
…cli-spring-boot-starter

For now I prefer the simplicity of having a single module.
The [docs](https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-developing-auto-configuration.html#boot-features-custom-starter-naming) mention that if you only have one module that combines [the starter and auto-configuration], name it XXX-spring-boot-starter.
@remkop
Copy link
Owner

remkop commented Jul 10, 2019

@dupirefr The picocli-spring-boot-starter module contains a PicocliSpringFactory and an auto-configuration class for much improved Spring integration.

I believe this fixes the issue reported in this ticket. Can you verify?

This will be included in the upcoming picocli 4.0 release.

@dupirefr
Copy link
Author

dupirefr commented Jul 23, 2019 via email

@remkop
Copy link
Owner

remkop commented Jul 24, 2019

@dupirefr Thank you! Since the 4.0 release, I expect that picocli offers much better Spring support, but it would be great if you could confirm that my assessment is correct.

Of course, suggestions for improvements are always very welcome!

@dupirefr
Copy link
Author

Hi @remkop ,

I just tested the feature, it works like a charm! Thanks a lot for fixing that problem :-).

Cheers!

François

@remkop
Copy link
Owner

remkop commented Jul 27, 2019

Great! Thanks for the confirmation.

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

No branches or pull requests

2 participants