My Approach for "Support Provider instances with Pico Container"

[antagoony]

🤔 What's changed?

This is my approach to solve #2879.

⚡️ What's your motivation?

Just saw the "help wanted" label, plus it sounds challenging ;-)

I read your thoughts and agree most of it. I do not totally follow the argument to "[...] declare a proper factory object". I think if any user decides to use PicoContainter he most likely wants to see it working as expected by this CI framework. Similarly Cucumber does not confuse Spring users. There is no new injection approach different to the Spring way of doing so.

Admittedly Cucumber-Spring does not automatically scan for all Spring components. Instead it scans exactly where the user wants it to do (by providing further Spring annotations on exactly that class that holds the @CucumberContextConfiguration annotation).

Following this idea, my PR comes up with something similar. It provides a @PicoConfiguration annotation that can be used to declare any Provider and/or ProviderAdapter classes that shall be used.

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

Not particular but in general -- what do you think of this approach?

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code I've added new code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[mpkorstanje]

I do not totally follow the argument to "[...] declare a proper factory object". I think if any user decides to use PicoContainter he most likely wants to see it working as expected by this CI framework.

Driving the desire for conceptual simplicity is the fact that the Cucumber project recommends Picocontainer if no other competing default exists (i.e. the user isn't already using Spring). And a good number of users are relatively novices at Java. So we've taken care to sand down the proverbial sharp edges by limiting the supported use-case to constructor dependency injection.

Looking back, I think I was hoping for an equally elegant solution, but none presented itself. But I can see myself explaining that users should use constructor dependency injection for glue code they write and providers for code they don't.

Anyway. The current pull request is definitely in the right spirit! And I think the biggest challenge is design related. Unfortunately, I don't have too much time right now. So below are some quick notes, apologies for the unvalidated train of thought.

1. I don't think that the ProviderAdapter is a good first candidate for this feature. The class is hairy. It might be possible to use without any chance of accidental misuse but I simply don't have the time to consider it. So for now I'd like to leave it out of scope. The Provider is a good candidate though.

2. The @PicoConfiguration could be replaced with a @CucumberPicoProvider (see example below).
   I understand that @PicoConfiguration allows the providers to be anywhere, even outside of the glue path. But I've recently drafted (#3120) and while unfinished I think a @PicoConfiguration could inevitably evolve into a more generic set of @UseGlueClasses, @UseGluePackages annotations that would allow adding arbitrary classes and packages to the glue. We might even have to undeprecate the @Cucumber annotation from JUnit 5 for that.

package com.example.app;

import java.sql.*;
import io.cucumber.picocontainer.CucumberPicoProvider;
import org.picocontainer.injectors.Provider;

public class ExamplePicoConfiguration {

    @CucumberPicoProvider
    public static class DatabaseConnectionProvider implements Provider {
        public Connection provide() {
            return // something
        }
    }

}

3. On reflection, the inner class looks quite similar too what I originally had in mind with. So I think that would be the more elegant way to go, there is much less fluff involved.

public class WebDriverFactory {

   @CucumberPicoProvider
   public static WebDriver webDriver() {
       return // create WebDriver 
   }

    @CucumberPicoProvider
    public static SecuredPage provide(WebDriver driver, Project project) {
       return new SecuredPage(driver, project, "example", "top secret");
    }
}

Which runs into my last point from #2879:

But at point we may as well recommend that people use a fully featured DI such as Spring or Guice. Perhaps what we need instead are some good examples.

I you're willing to make a pull request, I'm sure I can step over that. 😄

So in summary, both the @CucumberPicoProvider annotated Provider class and the @CucumberPicoProvider annotated method are acceptable solutions. I'll leave the choice up to you depending on your desire for a challenge.

If you any have suggestions for improvement, happy to hear them as always!

[antagoony]

Within the last days I did several versions of PR improvements, running into some issues and rash thoughts. The reworked PR can be reviewed now.

Let me reply your thoughts:

1. I don't think that the ProviderAdapter is a good first candidate for this feature. The class is hairy. It might be possible to use without any chance of accidental misuse but I simply don't have the time to consider it. So for now I'd like to leave it out of scope. The Provider is a good candidate though.

On the one hand, Provider should be sufficient enough. On the other hand I cannot ignore ProviderAdapter because it is a sub-class of Provider (and it has to be handled differently). So I decided to not promote the ProviderAdapter at any point, but consider it accordingly within the PicoFactory when calling #addAdapter(ComponentAdapter).

2. The @PicoConfiguration could be replaced with a @CucumberPicoProvider (see example below).
   I understand that @PicoConfiguration allows the providers to be anywhere, even outside of the glue path. But I've recently drafted (Support manual glue class registration #3120) and while unfinished I think a @PicoConfiguration could inevitably evolve into a more generic set of @UseGlueClasses, @UseGluePackages annotations that would allow adding arbitrary classes and packages to the glue. We might even have to undeprecate the @Cucumber annotation from JUnit 5 for that.

Renaming @PicoConfiguration to @CucumberPicoProvider, done.

However, I am not sure if we want to loose the option of re-using existing Pico-Providers. So now, the @CucumberPicoProvider annotation can be used to annotate the Provider class directly, or to refer to one/many existing Provider class(es), or both. (If neither the class implements Provider nor the #providers() attribute refers to at least one class implementing Provider then an according CucumberBackendException will be thrown. I've seen this analogical within SpringFactory when checking the Spring annotations.)

And I totally agree; If, in future, there will be a generic way to add classes (@UseGlueClasses, @UseGluePackages) then the current @CucumberPicoProvider should be replaced by such annotation.

If you still think there should be no #providers() referring to existing/re-used Provider classes then I can remove that code.

3. On reflection, the inner class looks quite similar too what I originally had in mind with. So I think that would be the more elegant way to go, there is much less fluff involved.

public class WebDriverFactory {

   @CucumberPicoProvider
   public static WebDriver webDriver() {
       return // create WebDriver 
   }

    @CucumberPicoProvider
    public static SecuredPage provide(WebDriver driver, Project project) {
       return new SecuredPage(driver, project, "example", "top secret");
    }
}

Which runs into my last point from #2879:

But at point we may as well recommend that people use a fully featured DI such as Spring or Guice. Perhaps what we need instead are some good examples.

I you're willing to make a pull request, I'm sure I can step over that. 😄

So in summary, both the @CucumberPicoProvider annotated Provider class and the @CucumberPicoProvider annotated method are acceptable solutions. I'll leave the choice up to you depending on your desire for a challenge.

I played with both approaches. And at the end, I really prefer the @CucumberPicoProvider annotated Provider class. That is because finding and invoking the default constructor of such class is way more easy than to reflectively create a new Provider instance with a method named "provide" having exactly these method parameters that are equal to the parameters of the @CucumberPicoProvider annotated method.

So, seeing the evolved PR, what do you think of the current approach?

[mpkorstanje]

If you still think there should be no #providers() referring to existing/re-used Provider classes then I can remove that code.

Please do. It will give me a reason to finish #3120 and add a cucumber.glue.classes property which should cover most cases.

[mpkorstanje]

Suggested change

	"PicoContainer Provider classes. Please ensure that at least one the following requirements is satisfied:\n"
	"PicoContainer Provider classes. Please ensure that at least one the following requirements are satisfied:\n"

[antagoony]

The text has changed even more. There are five requirements now, and all must be satisfied.

[mpkorstanje]

It would make sense to split @CucumberPicoProvider annotated and other glue classes here. It would clean up the logic in start and prevent constructor dependencies being added to the container..

[antagoony]

There are two collections now, one for the Provider classes, one for the "normal" component classes.

[mpkorstanje]

The class must also have a zero arg constructor IIRC.

[antagoony]

You are right, because that constructor is used when instantiating the Provider class within PicoFactory#adapterForProviderClass(Class). This requirement (and others) will be checked now (see PicoFactory#checkProperPicoProvider(Class)).

[antagoony]

If you still think there should be no #providers() referring to existing/re-used Provider classes then I can remove that code.

Please do. It will give me a reason to finish #3120 and add a cucumber.glue.classes property which should cover most cases.

Done. And in addition all of your comments has been implemented (see each reply). So, do you think the PR fits the issue now?

[mpkorstanje]

Looks good at a glance. I'll probably merge this sometime next week pending availability!

[antagoony]

Oh, I just have seen that the JUnit tests did not have proper names anymore. So I added another commit fixing this (instead of waiting and sending another PR). I hope, this is OK after you assigned the issue to yourself already. Please let me know if I should have handled this fix differently.

[mpkorstanje]

No problem. The assignment is just so it stays visible as a todo.