Support composition of multiple parameterResolvers

[TimvdLippe]

Overview

In Mockito, we want to support injection of multiple parameters. To that end, we have to check for a multitude of acceptable annotations and then later act on that annotation. This issue appeared when working on mockito/mockito#1503 which had to add a new annotation (@Captor) besides our @Mock injection-capabilities.

The author of the PR wrote a CompositeParameterResolver which would support this usecase. However, it feels like this kind of solution should live in JUnit, rather than being Mockito-specific. I would suspect that other projects will run into this problem (eventually).

Therefore, I would like to request a JUnit-official implementation for the use-case of composing various resolvers into a single parameter resolver, that then can be used in the extension.

Related Issues

• Support composability for ParameterResolvers #1802
• Support fallback parameter resolution extension #1945

Deliverables

• A means to compose multiple parameter resolvers together, such that an extension can provide a list of resolvers and pass that automatically in resolveParameter.

[marcphilipp]

IIUC, the linked CompositeParameterResolver delegates to the first ParameterResolver that can resolve a parameter, while the Jupiter engine currently requires that there's exactly one ParameterResolver to resolve any given parameter.

Looking closer at the linked PR it seems to me that of the two implementations only one will ever match as long as it's not a @Mock ArgumentCaptor parameter. I hope that never happens... 😅

So, if there was a way for an extension to register other extensions, the MockitoExtension could have registered both ArgumentCaptorParameterResolver and MockParameterResolver without needing a CompositeParameterResolver.

For example, we could add an apply(ExtensionRegistry) method to Extension where ExtensionRegistry would have a register(Extension) method.

Do you think that would be useful?

[TimvdLippe]

I think the issue is that there is no link between the parameter in supportsParameter and resolveParameter. E.g. it results in duplicate code where you first check whether a parameter is accepted and then when resolving you have to again get the parameter. Instead, I would expect the API to give a parameter and you can optionally return a resolution. E.g. Optional<Object> rather than Object.

I would rather not create multiple extensions, but instead have a method that allows me to iterate through multiple possibilities with their corresponding resolution strategy.

[marcphilipp]

Our intention was to make it possible to resolve all kinds of values, including null. Thus using an Optional was not an option (no pun intended).

You could create the missing link using ExtensionContext:

public class MultiSourceParameterResolver implements ParameterResolver {

    public static final String KEY = "resolvedParameter";

    @Override
    public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
        Optional<Object> resolvedParameter = resolve(parameterContext);
        resolvedParameter.ifPresent(value -> getStore(extensionContext, parameterContext).put(KEY, value));
        return resolvedParameter.isPresent();
    }

    private Optional<Object> resolve(ParameterContext parameterContext) {
        // just some dummy checks ;-)
        if (parameterContext.isAnnotated(Deprecated.class)) {
            return Optional.of("deprecated");
        }
        if (Integer.class.equals(parameterContext.getParameter().getType())) {
            return Optional.of(42);
        }
        return Optional.empty();
    }

    @Override
    public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
        return getStore(extensionContext, parameterContext).get(KEY);
    }

    private Store getStore(ExtensionContext extensionContext, ParameterContext parameterContext) {
        return extensionContext.getStore(Namespace.create(MultiSourceParameterResolver.class, parameterContext));
    }
}

[TimvdLippe]

While technically possible, I am not really a fan of that solution. If you would then add both @Mock and @Captor on a variable, depending on the order of the resolution mechanism, you could end up with different results. E.g. you are now relying on multiple extensions agreeing that what they are resolving to is the single truth.

After writing the above paragraph, I checked out the internal JUnit implementation, which takes care of any clashes. However, that handles clashes between extensions, but not any potential clashes within a single extension.

In the above snippet, you are using the context to store the interim state. However, to me that feels very similar to doing the following:

public class MultiSourceParameterResolver implements ParameterResolver {

    private Object parameterValue;

    @Override
    public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
        Optional<Object> resolvedParameter = resolve(parameterContext);
        resolvedParameter.ifPresent(value -> this.parameterValue = value);
        return resolvedParameter.isPresent();
    }

    private Optional<Object> resolve(ParameterContext parameterContext) {
        // just some dummy checks ;-)
        if (parameterContext.isAnnotated(Deprecated.class)) {
            return Optional.of("deprecated");
        }
        if (Integer.class.equals(parameterContext.getParameter().getType())) {
            return Optional.of(42);
        }
        return Optional.empty();
    }

    @Override
    public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
        return this.parameterValue;
    }
}

(Instead of storing the value in a context, it now stores it in the "class' context", aka an attribute)

However, I don't think that supportsParameter should do any resolution at all. That should happen in resolveParameter. If you are pre-emptively resolving in supportsParameter, that feels backwards to me.

All in all, I realize this is mostly about "feels" rather than concrete evidence. I also realize we can just use the CompositeParameterResolver and be done with it. However, I don't feel comfortable with the design as a whole, but sadly I have no concrete improvement proposal atm.

[marcphilipp]

Well, storing it in an instance variable is error-prone when tests are executed in parallel. Thus, we always recommend storing state in Store.

I agree that resolution should happen in resolveParameter rather than supportsParameter. However, in simple cases (like this dummy example), triggering it from the latter and checking for isPresent() and triggering it again from the former would also be a viable solution.

Let's keep this issue open for the time being and see if someone comes up with a more convincing proposal.

[nsmolenskii]

For example, we could add an apply(ExtensionRegistry) method to Extension where ExtensionRegistry would have a register(Extension) method.

@marcphilipp I voting up for something like that, but probably separated trait will make a bit more sense.

[rmannibucau]

+1, I want to impl a parameter resolver which only handles the parameter if it is not handled by another parameter resolver and it is quite hard ATM without doing a lot of reflection and depending on the engine to get the ExtensionRegistry

[Bukama]

Would also love to see this.

My scenario: I use the selenium-jupiter extension which has a ParameterResolver to pass all configured browsers (ChromDriver, FireFoxDriver, etc.) to the @testtemplatemethod (which only has theWebDriver` interface as an parameter.

I know wanted to add another ParameterResolver to wrap the WebDriver with an EventFiringWebDriver to register an EventReporter to log interactions of the driver with the browser. This failed, because JUnit discovered multiple ParameterResolvers for WebDriver.

I'll get in reach with the maintainer, because I think the feature will be useful for many others, but I wanted to leave a note here, that there the current state limits the great flexibility of JUnit Jupiter - of course, I'm sure you have reasons to do so at the moment.

[rmannibucau]

A trick to implement it is to integrate with main engine this way: https://github.com/apache/karaf-winegrower/blob/master/winegrower-extension/winegrower-testing/winegrower-testing-junit5/src/main/java/org/apache/winegrower/extension/testing/junit5/internal/engine/CaptureExtensionRegistry.java. Then you can test if another resolve supports it (https://github.com/apache/karaf-winegrower/blob/master/winegrower-extension/winegrower-testing/winegrower-testing-junit5/src/main/java/org/apache/winegrower/extension/testing/junit5/internal/BaseInjection.java#L40). It is way less elegant than priorities at engine level but works/unblock today.

[Michael1993]

What about classic composition?

If you have a ParameterResolver that gets registered with @ExtendWith but you want to modify the value - why not just wrap the original ParameterResolver with your own? Like so:

public class MyParameterResolver implements ParameterResolver {
    private final ParameterResolver alien = new AlienParameterResolver();

    @Override
    public boolean supportsParameter(/*omitted*/) {
        return alien.supportsParameter(/*omitted*/);
    }

    @Override
    public Object resolveParameter(/*omitted*/) {
        Object value = alien.resolveParameter(/*omitted*/);
        if (/* some optional condition */) {
            // do your wrapping/modifying of the value here
        }
        return value;
    }
}

This, of course, presumes that you can change your annotation from @ExtendWith(AlienParameterResolver.class) to @ExtendWith(MyParameterResolver.class).

If you can't change the @ExtendWith annotation for some reason, you can still use InvocationInterceptor to modify the state of the argument object, like so:

@Override
	public void interceptTestTemplateMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) throws Throwable {
	Optional<Alien> alien = invocationContext.getArguments().stream()
			.filter(any -> any instanceof Alien)
			.map(a -> (Alien) a)
			.findFirst();
	alien.ifPresent(a -> /* change state here, e.g.: a.setSomeStringValue("some string"); */);
	invocation.proceed();
}

All in all, I like this idea, will see if we can implement it in an extension in JUnit Pioneer - and if it looks promising/good enough JUnit team can move it into core.

[rmannibucau]

@Michael1993 composition is almost immediately blocked by the fact you can't know all potential other extensions and integrate with all others - if it would be true then you would end up having all extensons un jupiter and then drop the extension concept. I think it is saner to have a kind of ranking per parameter (which could have a default value in ParameterResolver). This way extensions can evaluate their "priority" and jupiter can sort it to find the one to use. Note that it can also be dynamic if "getPriority" takes the same parameters than "supportsParamter", enabling the user to use a @priority or @MyExtensionOrder to select himself which one he wants without having to use a custom extension qualifier filtered in supports hook.

[Michael1993]

Yeah, if you look at it from JUnit side, execution order is a big question mark. But my solution is on the client/user side - you control how you wrap the ParameterResolver, meaning priority is controlled.

The second solution does have a problem with execution order but it's a last resort hack anyways.

[rmannibucau]

@Michael1993 hmm, your solution works if the user disables all extensions which are sometimes already composed so - to have tested it before the hack I shared before - it is very complex for end users to use when having multiple extensions - it is doable for simple cases, I agree. There is no issue except the dependency on the engine but I suspect it can be pushed in api module - with the priority option - it is controlled from both the extension and therefore user perspective where it works OOTB.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

[stale]

This issue has been automatically closed due to inactivity. If you have a good use case for this feature, please feel free to reopen the issue.

[TimvdLippe]

As part of mockito/mockito#3133 we will introduce a CompositeParameterResolver to be able to compose multiple of these resolvers into one and find the appropriate one in a list of resolvers.

[sbrannen]

• related to Support fallback parameter resolution extension #1945