PicocliSpringFactory should not fall back default Factory

[a1dutch]

Hi,

Using the picoli-spring-boot-starter the PicocliSpringFactory falls back to CommandLine.defaultFactory().create(clazz) if its not able to create a bean.

This can lead to unintended consequences and dependents not being injected or available. In my case the logging was turned off and there was no indication that this had failed

A better approach may be to just throw the caught exeception and fail.

Happy to create a PR with a feature flag (but this probably should be the default behaviour in the next major version)

Cheers

@a1dutch

[remkop]

Hi @a1dutch, thank you for raising this!

I am not sure yet though. :-)
For background, the factory is used internally to find implementation classes for interfaces, for example, when an option is defined like this:

@Option(names = "-x") List list;
@Option(names = "-y") Map map;

... then the factory is used to create objects for the List class and the Map class when the option is specified on the command line. The default factory knows to create and return an ArrayList and a HashMap object for these classes. That is why the factory falls back to the default factory. So this is kind of by design.

However, all of this is just the background for the current implementation, but that does not solve your use case.
Can you tell be a bit more about what actually happened in your case?
What was the class you were trying to instantiate and what object did the factory actually produce?

[a1dutch]

HI @remkop

Thanks for the quick response.

In my case i had a ConfigurationProperties class that had a binding issue with one of the values, this resulted in my Command class having none of the fields injected.

Running the command just resulted in NPE errors on the autowired fields.

[remkop]

@a1dutch so I am guessing that the factory could not find the ConfigurationProperties in the Spring ApplicationContext, so it ended up delegating to the default factory which instantiated a new instance.

From my point of view, the PicocliSpringFactory is doing its job, since the factory is used for instantiating many kinds of objects used in picocli:

• type converters, as in @Option(names = "-a", converter = CipherConverter.class)
• explicit field types, as in @Option(names = "--big", type = BigDecimal.class) Number n
• default value providers, as in @Command(defaultValueProvider = MyDefaultProvider.class)
• custom parameter consumers, as in @Option(names = "-x", parameterConsumer = CustomConsumer.class)
• custom parameter preprocessors, as in @Option(names = "-x", preprocessor = MyPreprocessor.class)
• subcommands, as in @Command(subcommands = Subcommand.class)
• version providers, as in @Command(versionProvider = ManifestVersionProvider.class)
• completion candidates, as in @Option(names = "-o", completionCandidates = MyAbcCandidates.class)

These objects do not all exist in the Spring ApplicationContext, so it is normal and desirable for the PicocliSpringFactory to fall back to the default factory.

That said, I can see your point, you would like to validate that your application and configuration is correct.
And the PicocliSpringFactory seems like a good place to perform that validation.
So what can we do to facilitate that?

Idea 1: enhance PicocliSpringFactory to provide an explicit validation mechanism

We could do something like add a method to PicocliSpringFactory where we can specify all classes that we expect to be found in the Spring ApplicationContext. If the ApplicationContext does not have this class, then we throw an exception. We can call the method something like expectAppContextToContain and it could be implemented like this:

// PicocliSpringFactory

@Override
public <K> K create(Class<K> clazz) throws Exception {
    try {
        return getBeanOrCreate(clazz);
    } catch (Exception e) {
        return handleNotFound(clazz, e);
    }
}

private Set<Class<?>> noFallback = new HashSet<>();
public void expectAppContextToContain(Class<?>... classes) {
    noFallback.addAll(Arrays.asList(classes));
}

private <K> K handleNotFound(Class<K> clazz, Exception ex) throws Exception {
    if (noFallback.contains(clazz)) {
        throw new IllegalStateException("Missing " + clazz + " in ApplicationContext");
    }
    logger.warn("Unable to get bean of class {}, using default Picocli factory", clazz);
    return CommandLine.defaultFactory().create(clazz);
}

In your application, you would then somehow need to get the factory and register all beans that picocli may look up that you expect should be found in the ApplicationContext, by calling expectAppContextToContain with the list of such classes.

I am not sure how you would do that, and even if you could, it seems a bit fragile. What do you think of this idea?

Idea 2: enhance PicocliSpringFactory to allow a user-specified fallback factory

Your application may not have any subcommands or custom type converters or other, so perhaps in your application, any call to the fallback factory indicates a problem: your application expects Spring's ApplicationContext to contain all classes.

We could enhance the PicocliSpringFactory class to take a second constructor that accepts a factory to be used as the fallback factory. For example:

// PicocliSpringFactory
private final ApplicationContext applicationContext;
private final IFactory fallbackFactory;

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

public PicocliSpringFactory(ApplicationContext applicationContext, IFactory fallbackFactory) {
    this.applicationContext = Objects.requireNonNull(applicationContext, "applicationContext");
    this.fallbackFactory = Objects.requireNonNull(fallbackFactory, "fallbackFactory");
}

@Override
public <K> K create(Class<K> clazz) throws Exception {
    try {
        return getBeanOrCreate(clazz);
    } catch (Exception e) {
        logger.warn("Unable to get bean of class {}, using fallback factory {}", 
                clazz, fallbackFactory.getClass().getName());
        return fallbackFactory.create(clazz);
    }
}

Your application could then supply a custom fallback factory that always throws an exception.

Unsure how you want to pass in such a custom factory, maybe via PicocliAutoConfiguration, perhaps you have ideas around that?

Idea 3: do validation elsewhere

Perhaps the PicocliSpringFactory class is not as attractive a place to do validation as it appeared to be.
If so, then validation should be done in some other way, perhaps in unit tests that validate that all autowired fields have a non-null value.

That is all that I can come up with.
Thoughts?

[remkop]

I am thinking to implement Idea 2 (add PicocliSpringFactory constructor to allow a user-specified fallback factory) for the next minor picocli release (version 4.7).

[a1dutch]

Hi @remkop,

At the minute i am just redeclaring my own Factory like so

    @Bean
    public IFactory factory(ApplicationContext context) {
        return new IFactory() {
            @Override
            public <K> K create(Class<K> clazz) throws Exception {
                try {
                    return context.getBean(clazz);
                } catch (Exception e) {
                    return context.getAutowireCapableBeanFactory().createBean(clazz);
                }
            }
        };
    }

i plan to do a little testing using some of those types and converters you mentioned

[remkop]

FYI, idea number 2 is now implemented and will be available in the next picocli release (4.7.0).

Are there any other tasks outstanding, or are you okay to close this ticket?

[a1dutch]

You can close, thankyou

[ebuildy]

Just upgraded from 3.X to 4.X, using Guice injector as factory, took me a few hours to figure out this BC. I got https://github.com/google/guice/wiki/MISSING_IMPLEMENTATION error, to fix it quickly:

@Override
    public <K> K create(Class<K> cls) throws Exception {
        // Everything go to the factory since v4
        if (cls.equals(Map.class)) {
            return CommandLine.defaultFactory().create(cls);
        }
        return injector.getInstance(cls);
    }

[remkop]

Hi @ebuildy, please look at the Guice example in the manual: https://picocli.info/#_guice_example
That would be the implementation I recommend.

[remkop]

@a1dutch Thanks for the confirmation!

[ebuildy]

woa dont understand how I missed it..... thanks you very much , this is very clear.

[remkop]

@ebuildy Great, glad to hear that!