Junit5: Introduce ParameterContext for ParameterResolver extension API

[baev]

Original Title

ParameterResolver has Optional parameter in supports and resolve methods

Original Description

Optional was designed to provide a limited mechanism for library method return types where there needed to be a clear way to represent "no result".

[sbrannen]

Are you suggesting that we replace Optional<Object> target with Object target in both cases and simply document that the reference may be null?

[marcphilipp]

In the end, it's a matter of personal preference. IMHO an Optional is a much clearer way to signal that target may be null whether or not it was designed for this purpose. Interestingly, the Javadoc for Optional does not mention its intended usage at all. Brian Goetz' advice can be found on Stack Overflow.

[jbduncan]

@marcphilipp kinda beat me to it, but here's my take on it.

I personally agree with @baev. In my opinion, it's significantly more tedious to write Optional.ofNullable(target) than just target or null as a user of such an API.

If a ParameterResolver impl were to use Optional internally, I'd expect it to be an implementation detail only, and for the ParameterResolver impl to have in its constructor a line like this.target = Optional.ofNullable(target), so that it deals with the optional for me.

[marcphilipp]

In this case, a user of this API would _implement_ ParameterResolver, not call it. Therefore, there's no need to instantiate an Optional. We cannot pass the target in the constructor, because a ParameterResolver is potentially used for multiple, different target.

[baev]

The good article about Optionals http://blog.jhades.org/java-8-how-to-use-optional/

[marcphilipp]

Well, if we would instead pass a ParameterContext with a method getTarget() that returns Optional<Object>... how would that be different?

[jbduncan]

@marcphilipp Oh oops! Looks like I didn't take a good look at the source code for ParameterResolver. :stuck_out_tongue_closed_eyes: ...

[marcphilipp]

How so?

[jbduncan]

@marcphilipp I didn't fully realise that ParameterResolver was an _interface_ rather than a class, so my constructor example didn't too make much sense in my 2nd last comment.

[jbduncan]

@marcphilipp Is ParameterContext an existing class, or is it just a hypothetical example?

[marcphilipp]

Just a hypothetical example. However, we have similar classes, e.g. ExtensionContext, that are used as extension parameters for other Extension subinterfaces.

What I'm trying to convey: In my point of view this usage of Optional is like returning Optional from an API method because ParameterResolver is an interface to be implemented by users.

[sbrannen]

I agree with @marcphilipp:

this usage of Optional is like returning Optional from an API method because ParameterResolver is an interface to be implemented by users.

It was a conscious decision on my part when I introduced Optional<Object> target in the ParameterResolver methods.

The rationale is that it doesn't provide any added benefit to the user to introduce a ParameterInvocationContext (that contains a single Optional<Object> getTarget() method) since that would only force the user (i.e., implementer of ParameterResolver) to jump through yet another level of indirection. Plus, we want to convey very explicitly to the user that the target may not be present (and in fact will not be present for static methods or constructors).

IMHO, the choice to supply the target via an Optional argument serves these design goals well, regardless of the fact that Brian Goetz suggests that one should _almost never_ do so.

[baev]

@marcphilipp @sbrannen I don't know the codebase of Junit 5 yet, just started to make a first extension. As far as I know it is kinda bad practice to use Optional as method parameters or fields. So I decide to notice you about it. From my point of view Optional parameters are more complex then nulls. If you like to call such API you need to wrap value to Optional, if you like use it - you need to deal with optionals:

if (optionalValue.isPresent()) {
    return;
}
value = optionalValue.get()
...

instead of

if (Objects.nonNull(value)) {
    return;
}
//use value

[baev]

BTW I know about ifPresent =)

[jbduncan]

Again, someone kind of beat me to it, but I think it's worth me asking/saying:

@marcphilipp @sbrannen Can you clarify for me if ParameterResolver implementations would be used either _directly by users_ of JUnit 5 or _by the JUnit framework_ itself?

If they are used by the framework itself, then I agree that using Optional this way is reasonable (even it's not what Brian et al. intended), because after they've implemented the ParameterResolver themselves, users don't need to write code like Optional.ofNullable(target) again, and it also makes the purpose of target clearer.

[sbrannen]

@jbduncan,

ParameterResolver is an extension API to be implemented by third parties.

Please consult the Javadoc and user guide:

http://junit.org/junit5/docs/current/user-guide/#parameter-resolution

[jbduncan]

Thank you @sbrannen.

So as far as I can tell, ParameterResolvers are implemented by users and then passed to JUnit 5 for JUnit itself to manage. JUnit then uses these resolver(s) to fill in or "inject" values into the parameters of test methods, test class constructors, etc. Is my understanding correct?

[sbrannen]

In an nutshell, yes, your understanding is correct.

A user or third-party framework (like Spring or Mockito) implements a ParameterResolver.

Users register such resolvers via @ExtendWith(...). For example, to get dependency injection support from Spring, you annotate a test class with @ExtendWith(SpringExtension.class) to register the extension (which implements ParameterResolver).

JUnit instantiates registered extensions and invokes the supports(...) and resolve(...) methods.

In addition, we don't expect average users of JUnit to implement ParameterResolver. This is more of an advanced extension API that will typically be implemented by third-party frameworks.

[jbduncan]

Okay great, that matches my understanding perfectly! In that case, I'm happy for the target params to be Optionals since JUnit would be instantiating the Optionals itself, and AFAIKT that'd make implementers less likely to use target in such a way as to throw NullPointerExceptions by accident.

[jbduncan]

@sbrannen @marcphilipp Thank you both for taking the time to answer my questions, I really appreciate it. :smile:

[sbrannen]

that'd make implementers less likely to use target in such a way as to throw NullPointerExceptions by accident.

That's the goal!

[sbrannen]

@smoyer64,

The contracts in the ParameterResolver API guarantee that arguments passed to the supports() and resolve() methods will never be null.

Details are visible in the Javadoc:

https://github.com/junit-team/junit5/blob/master/junit5-api/src/main/java/org/junit/gen5/api/extension/ParameterResolver.java

Furthermore, the implementation of org.junit.gen5.engine.junit5.execution.ExecutableInvoker (which invokes ParameterResolvers) ensures that the non-null contracts are upheld.

[smoyer64]

@sbrannen

I've seen (and implemented) the ParameterResolver interface quite a few times (for upREST and for an attempt at a JUnitParams extension). The API does indeed state that target won't be null, but (and this may just be semantics) it's not a contract since the engine's implementation could in fact return a null.

I trust this team enough (having dug through the code) to expect that the implementation will in fact honor the API. And with the defensive style of coding adopted by the team, I have every confidence that you don't intend to violate the API as documented. After all, what could go wrong with:

    public Object invoke(Method method, Object target, ExtensionContext extensionContext,
            ExtensionRegistry extensionRegistry) {

        @SuppressWarnings("unchecked")
        Optional<Object> optionalTarget = (target instanceof Optional ? (Optional<Object>) target
                : Optional.ofNullable(target));
        return ReflectionUtils.invokeMethod(method, target,
            resolveParameters(method, optionalTarget, extensionContext, extensionRegistry));
    }

And it would be even harder here:

    public Object invoke(Method method, ExtensionContext extensionContext, ExtensionRegistry extensionRegistry) {
        return ReflectionUtils.invokeMethod(method, null,
            resolveParameters(method, Optional.empty(), extensionContext, extensionRegistry));
    }

So I certainly meant no offense when I said "there's no guarantee the Optional won't itself be null" but was just trying to describe the fact that the compiler would allow it. Even with FindBugs annotations [1], it's the static code analysis that warns you if you've violated a contract instead of the compiler failing to hand you a class. If by contract you meant between the JUnit5 team and the extension developer, then I apologize for misinterpreting your comment.

[1] http://findbugs.sourceforge.net/manual/annotations.html

[sbrannen]

If by contract you meant between the JUnit5 team and the extension developer, then I apologize for misinterpreting your comment.

Yes, that's what I meant by _contract_ in this context. 😉

But no need to apologize, and certainly no offense taken. I just wanted to ensure that others reading this issue get the full picture.

[rliesenfeld]

Personally, I feel that using Optional in the extension API (which I now implement in the JMockit library) may not be the best idea.

I have been using JSR 305 annotations (from the javax.annotations package). In practice, this should be more useful as there is good tool support (IntelliJ, Eclipse, FindBugs), and it keeps the API simpler. Having @Nonnull and @Nullable annotations also documents the API contracts a lot better than using or not using Optional for return and parameter types.

[sbrannen]

@rliesenfeld, that's awesome that you're building JUnit 5 support into JMockit! 👍

Please let us know how it goes and if you run into any issues.

Regarding the use of Optional for a method argument in an extension API, we will keep everyone's input in mind.

In this particular case, however, @NonNull is not applicable since the target can in fact be null. The target is therefore @Nullable, but I'm not convinced that that would be any better than using Optional here. Anyway... we'll ponder it.

FWIW, I truly don't foresee many extension developers even accessing the target when resolving a parameter. For example, the JMockitExtension and SpringExtension completely ignore the target. 😉

[marcphilipp]

Team decision:

We will change both methods by removing parameter and target and instead pass a new class ParameterContext that holds the Parameter, Optional<Object> target, and int index. Therefore Optional will not be as a method parameter and we'll be able to add additional information without breaking existing implementations in the future.

[marcphilipp]

I'll start to work on this issue.

[marcphilipp]

@baev Thanks for getting this discussion going! I think the new ParameterContext is definitely an improvement!

[nipafx]

FWIW, I think using Optional everywhere is a valid approach. But the ParameterContext is an improvement, nonetheless.