[proposal] Split ObjectFactory in Container and Lookup

[rmannibucau]

Summary

ObjectFactory has 2 concerns:

1. container lifecycle (start/stop)
2. instance lookup

Context & Motivation

In CukeSpace the container is handled by Arquillian "by design" so reusing ObjectFactory is weird and it makes hard to reuse cucumber-jvm goodness.

Your Environment

CukeSpace

Proposal

Make ObjectFactory being:

public interface ObjectFactory extends Container, Lookup {}

This will keep current code working and allow to split the way the container is handled from the way instances are retrieved. This is quite common for CDI, Spring, ... so think it can benefit users.

[mpkorstanje]

I'm not too familiar with the design of CDI frameworks, specifically Arquillian, and from the implementations in cucumber-jvm I can't quite see what benefits separating the these interface methods would provide. It all appears to be tightly coupled.

Can you describe which methods each interface would have and how the Lookup would interact with the Container? Can you describe why this is needed with Arquillian? I.e. is there a reason you can't wrap the Arquillian parts in an ObjectFactory?

[rmannibucau]

Yes, arquillian is a runner you cant easily change to make a lifecycle manager. Also cdi, spring etc.. separate the lookup from the container lifecycle in term of api. Also you can combine container lookups but start a single one (spring in ee). For these 3 reasons splitting both concerns would be good for cucumber and help cukespace

[mpkorstanje]

I understand your desire and I understand that other frameworks also make the same separation but I don't understand the technical reasoning behind that separation. Nor do I understand how such a separation would be implemented.

Perhaps I am asking to many questions at the same time. Lets start with the most important one. Can you describe which methods each interface would have and how the Lookup would interact with the Container?

[rmannibucau]

Sure, just before sharing some API example keep in mind cucumber-jvm assumes it manages the container which is not always the case and can require to implement a full object factory where only a Lookup or Container would work. In CukeSpace case, original idea is to integrate cucumber-jvm with arquillian. Main advantage of arquillian is a deep integration with containers (startup/deployment/undeployment/shutdown) so in this case Container part of the API is pointless and in the integration pollutes the understanding.

In term of split i would just define lookup as:

public interface Lookup {
    <T> T getInstance(Class<T> type);
}

And the Container as ObjectFactory minus Lookup.

[mpkorstanje]

Okay. This would let you use the Java(Hook|Step)Defintions with a lookup provided by Arquillian.

Cucumber requires that each Scenario is executed with a clean dependency injection context. So some form of container management is needed. Do you intended to create your own back-end that uses Backend#buildWorld and Backend#disposeWorld to manage the dependency injection context directly?

[rmannibucau]

This is more or less what is done being said most of the time container = context/lifecycle management. World only makes sense for cucumber managed instance which is less true/a constraint in a container.

[mpkorstanje]

What keeps you from reusing the existing JavaBackend? From a design perspective I'd rather see that you reuse that then some specific parts. The JavaBackend and parts aren't really designed for reuse (for example look at the way java 8 step defintions are loaded).

[rmannibucau]

This is what is done actually. All the scanning is replaced cause in the container you don't know if you can scan but the blocking part is really how to get step instances in a container friendly way portably. Users asked to not just rely on injections to handle the context but also to make steps beans themself. Naturally we made configurable the object factory which owns it but then we have all the container API part which is ignores and can even prevent to reuse existing object factories. That is why from a design perspective, splitting the objectfactory to made obvious its 2 concerns would be beneficial.

[mpkorstanje]

Sorry. I should have stated that more clearly. Currently cuckespace is extending the JavaBackend and overriding several methods. The JavaBackend is not designed for this. So I'd rather see that cuckespace didn't extend it.

I'm thinking you'd be better served by abstracting the class path scanning and let you instantiate the JavaBackend with that instead.

[rmannibucau]

Think that's what is done if I get you right, or at least it was the intent. What was not an option was to scan in the container (ie from the backend) since we can not have the right filesystem handling (jndi://, vfs://, ...)

IIRC the inheritance is there just for the snippets and not for any other handling.

[mpkorstanje]

I think that if you were to implement your own backend rather then extend the JavaBackend we can cover your needs by extracting a StepDefinitionRegistry interface from the JavaBackend that contains the INSTANCE, addStepDefintion, addBeforeHookDefintion and addAfterHookDefintion.

[rmannibucau]

It is a bit more complicated because cukespace must also ensure the test instance is registered properly so handle the "lookup" part but yes, being able to provide a manually built registry can help. Not sure INSTANCE is the way to go (AFAIK it is mainly a workaround for Java 8 which also has the issue to use the constructor -> see Lambda in cukespace to workaround it) .

Rephrased the needs can be split as such:

1. being able to register custom steps/hooks
2. being able to register custom instances without forcing the user to change of container (Lookup abstraction in this issue)
3. being able to use any container (Container abstraction in this issue)
4. not use constructor for java 8 construction but a hook to be able to use managed instances (constructor are broken with weld, openwebbeans, spring, .... if you add into play any proxy or equivalent technics)

[mpkorstanje]

Okay. We can at least extract the StepDefinitionRegistry interface then.

That said I still don't understand the need to separate the lookup from the container or even how you'd implement this. They appear to be intrinsically linked - for each call made getInstance, at the very least the stop method would need to do some cleanup and as such reference a common data structure somewhere. Please make this concrete. How would your and our code end up looking?

Not sure INSTANCE is the way to go (AFAIK it is mainly a workaround for Java 8 which also has the issue to use the constructor -> see Lambda in cukespace to workaround it) .

Given the current syntax there is no way around registering lambda step definitions to a static instance. The current syntax requires the use of either static methods or default methods. The former can only access a static context, the latter can access a static context and its own methods.

(constructor are broken with weld, openwebbeans, spring, .... if you add into play any proxy or equivalent technics)

Can you provide a reproducer?

[rmannibucau]

@mpkorstanje are you familiar with Arquillian? It is basically the same kind of split: the container will just handle the lifecycle of the beans and the lookup the way they are retrieved. It is indeed linked but splitting both allows to reuse them but also enrich them. Typically I can reuse weld-container (or spring-container) and enrich the weld-lookup (spring-lookup) with some custom lookups instead of having to delegate the container lifecycle too. In the case of cukespace it is worse: the container is managed by arquillian so only the lookup part is important for cucumber integration.

The INSTANCE issue is more about the fact it is not really an internal at the moment but it should, so I'm not very tempted to rely on it a lot - I didn't say it was wrong since I agree there is no real alternative to hide it to end users, just that using it in cukespace is not that great. However the fact to use constructor instead of a define() lifecycle hook is quite bad because it prevent some usage in during the step definition. For instance compare these two step def style (old vs j8 one):

public class BellyStepdefs {

    @Inject
    private Belly belly;

    private boolean inTheBelly = false;

    @Given("^I have (\\d+) cukes in my belly")
    public void haveCukes(int n) {
        belly.setCukes(n);
        inTheBelly = true;
    }

    @Then("^there are (\\d+) cukes in my belly")
    public void checkCukes(int n) {
        assertEquals(n, belly.getCukes());
        assertTrue(inTheBelly);
    }
}


public class BellyStepdefs implements En {

    @Inject
    private Belly belly;

    private boolean inTheBelly = false;

    public BellyStepdefs() {
        assertNotNull(belly); // fails but should be valid and allow to precompute/define stuff for steps

        Given("^I have (\\d+) cukes in my belly", (Integer n) -> {
            belly.setCukes(n);
            inTheBelly = true;
        });
        Then("^there are (\\d+) cukes in my belly", (Integer n) -> {
            assertEquals(n, belly.getCukes(), 0);
            assertTrue(inTheBelly);
        });
    }
}

Simple solution is to just do:

public class BellyStepdefs implements En, DefinitionHook /*or whatever name*/ {

    @Inject
    private Belly belly;

    private boolean inTheBelly = false;

    @Override
    public void define() {
        assertNotNull(belly); // success :)

        Given("^I have (\\d+) cukes in my belly", (Integer n) -> {
            belly.setCukes(n);
            inTheBelly = true;
        });
        Then("^there are (\\d+) cukes in my belly", (Integer n) -> {
            assertEquals(n, belly.getCukes(), 0);
            assertTrue(inTheBelly);
        });
    }
}

[mpkorstanje]

Typically I can reuse weld-container (or spring-container) and enrich the weld-lookup (spring-lookup) with some custom lookups instead of having to delegate the container lifecycle too.

I was looking for an example of this, but the explanation is now sufficiently clear.

Unfortunately this not a concept used by cucumber and provides no benefit to our code base so I am hesitant to introduce it. By appearing to be without use it might be re-factored out again in the future and break things you've come to rely upon. As such I'd recommend wrapping the ObjectFactory provided through the constructor with a decorator that implements Container and Lookup and proceed with that decorator as if it was the ObjectFactory.

However the fact to use constructor instead of a define() lifecycle hook is quite bad because it prevent some usage in during the step definition. For instance compare these two step def style (old vs j8 one):

I see the problem. Though using precomputation in step definitions is a bad practice. If done at all, this should be done in the before hook.

[rmannibucau]

As such I'd recommend wrapping the ObjectFactory provided through the constructor with a decorator that implements Container and Lookup and proceed with that decorator as if it was the ObjectFactory.

This is what has been done but half of the API is ignored and therefore it is weird for end users - that's why i opened this issue.

Though using precomputation in step definitions is a bad practice.

Yes and no, I get it for the "old" way but for the java 8 way it is easier and more natural than using only hooks to set instance variables which is not natural. Also note that some impl will just bypass the constructor (completely, using Unsafe) and being lazy you will never register any step. Does the define() solution hurt that much?

[mpkorstanje]

I would rather not add anything to cucumber that is not also used by cucumber. I think that the burden of dealing and maintaining special cases should lie with the those who use and benefit from them. As such the Arquillian backend gets to solve the problems of dealing with Arquillian. Can you provide an example or documentation reference of a DI container used by cucumber that routinely bypasses the constructor? On Oct 3, 2017 10:13, "Romain Manni-Bucau" <[email protected]> wrote: As such I'd recommend wrapping the ObjectFactory provided through the constructor with a decorator that implements Container and Lookup and proceed with that decorator as if it was the ObjectFactory. This is what has been done but half of the API is ignored and therefore it is weird for end users - that's why i opened this issue. Though using precomputation in step definitions is a bad practice. Yes and no, I get it for the "old" way but for the java 8 way it is easier and more natural than using only hooks to set instance variables which is not natural. Also note that some impl will just bypass the constructor (completely, using Unsafe) and being lazy you will never register any step. Does the define() solution hurts that much? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1117 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGoAZ5lvaSOcuCpQCmUYqnGnHsAC-cwTks5soezFgaJpZM4NEYhn> .

[rmannibucau]

Can you provide an example or documentation reference of a DI container used by cucumber that routinely bypasses the constructor?

If you use a step definition which is an EJB with openejb (which is perfectly fine for cucumber and would allow to execute steps in transactions) then the constructor will not be called.
If you use an openwebbeans (should be the case for weld but didnt check) @ApplicationScoped bean then the constructor shouldn't be called until you use the bean which will not happen cause the steps will not be found before

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[rmannibucau]

Please dont close

[mpkorstanje]

Okay. It took me a long time to understand this. But now that I am looking into sharing the object factory between different back ends it starts to make sense.

[mpkorstanje]

Implemented by aa76421. It will be available in v5.

[rmannibucau]

Hi @mpkorstanje

Is the inheritance right?

Container should have start/addClass/stop and Lookup just getInstance

ObjectFactory would extend both but lookup and container have no inheritance between them, they are used by composition in cucumber.

Also objectfactory shouldnt be used in the code anymore, it would just be an api for lib integration - api sugar, cucumber itself would just see a container and a lookup with the shortcut that if both are using the same class then it leads to the same instance.

Wdyt?

[mpkorstanje]

Ah right. No I didn't set it up like that.

In the long run I'm looking at reusing a lot of concepts from JUnit5 and introduce an ExecutionContext. The execution context will contain the step definitions, type registry and glue classes.

At this point the Backend will only be used to discover step definitions, instantiate them and add them to the glue. The Lookup only exists to allow an instance of a StepDefinition to get an instance of a glue class to execute on. This means that the Container won't be exposed any more at all and I think that at this point Lookup will only be provided via the ExecutionContext.

[rmannibucau]

Well with junit 5 - and thanks platform abstraction - you should be able to have N>1 containers with M>1 corresponding lookups and be able to compose them at need instead of rewriting a lookup each time. So think it would make sense to enable to configure the container on steps. Would enable more composition and reuse of steps for advanced envs.

[mpkorstanje]

That will mostly be up to us. The junit platform doesn't put any restrictions on the shape of the exectuion context.

[rmannibucau]

Agree but then the commit does not solve this issue which was about breaking that inheritance and the api is not that smooth with junit5

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.