Make com.ibm.jbatch.tck.tests.jslxml.CDITests#testCDILookup(...) portable by using a bean indirection instead of jbatch annotations directly

[rzo1]

What does this PR do?

We are currently working on updating Geronimo BatchEE to be compliant with Jakarta Batch 2.1 via a PR.

While working on TCK compliance, we encountered an issue with com.ibm.jbatch.tck.tests.jslxml.CDITests#testCDILookup(...).

This test makes use of dynamic injections via CDI.current() of @BatchProperty via an AnnotationLiteral.

In case of the reference impl, Weld will create a "dynamic" (mock) injection point, so the BatchProducerBean will return the correct properties. OWB doesn't create such a "mock" injection point, so it isn't possible to obtain the qualifier as InjectionPoint#getMember cannot be null (see the javadoc and the logger example). Overall, this is most likely a thing to discuss on the CDI side.

Therefore, I would like to propose to adjust the actual test on the batch tck to be portable by adding an indirection on a bean which gets jbatch injections instead of using CDI.current() on the jbatch injections directly. This will still test the same thing but should be (more) portable.

[scottkurz]

Hi @rzo1, thanks for bringing up this issue.

I think the first thing we should clarify is whether you are raising a challenge to the current 2.1 TCK, because you are trying to certify Batch EE against it, like you said, or if you are just raising this for a future release.

It sounds like more of a challenge, in which case there is a template to use , see "Filing A Challenge" in the TCK process doc. Don't worry too much at first about all the info requested there.. you've already noted the important aspects and we can start by just adding the label if so.

So let me just ask you to confirm.

FIRST THOUGHTS

I appreciate the explanation about the difference between Weld vs. OWB.

Unfortunately that makes it clear that this isn't some trivial bug fix. The change does impact a hypothetical application using the same pattern as this test. (It's not "trivial" then in that the programming model is impacted...which is NOT to say that I'm arguing this use case is particularly important in real world usage).

Though we could fix it in the test we should consider if instead the implementation should change (somehow) and/or if the specification wording is correct (the description here:
https://jakarta.ee/specifications/batch/2.1/jakarta-batch-spec-2.1#consequences-and-suggested-patterns
and in the parent section).

We may need to get some thoughts from CDI experts here.

So, sorry this is not going to be simple, but will await for your response for the next step.

[rmannibucau]

Hi @scottkurz

The change does impact a hypothetical application using the same pattern as this test.

Yes, as any application using a not portable code relying on a vendor instead of a spec so this is not a big deal to change.
Note that this will NOT break the application, it just makes the test using CDI instead of Weld and therefore EE compliant.

We may need to get some thoughts from CDI experts here.

CDI is explicitly forbidding what Weld does - note that weld is allowed to do it as an additional feature on top of CDI but it is out of spec scope and vendor specific - with the reference Richard pointed out (mainly Instance documentation).
There is no big ambiguity there and on a pure JBatch standpoint this does not change the spec coverage strictly speaking - CDI injections are stil there and tested.

It sounds like more of a challenge

Fully agree, blame on me cause I proposed to Richard to start there since the fix was trivial and easy to do then to move to a challenge to be allowed to exclude the test in the suite until the new release is done.
Think there is no big deal to do both concurrently.

[rzo1]

Hey @scottkurz, thanks for the fast response and thanks @rmannibucau for following up.

Disclaimer

I think, that we are in a patt situation here, because the TCK challenge process states:

Any implementor may submit a challenge to one or more tests in the TCK as it relates to their implementation. Implementor means the entity as a whole in charge of producing the final certified release. Challenges filed MUST represent the consensus of that entity.

As I am not part of the Geronimo PMC, I am not a representative of BatchEE. So from a formal point of view, a potential challenge would need to be opened by @jeanouii, @rmannibucau or @struberg., I guess? ;-)

Thoughts

The major difference between the reference impl and Geronimo BatchEE is the CDI impl used. Major parts are actually a fork of the reference impl with some tweaks.

So boiling down the issue, it seems that it is actually a discussion around CDI or more specificially about the JavaDoc of InjectionPoint.

The JavaDoc of InjectionPoint ( https://jakarta.ee/specifications/cdi/4.0/apidocs/jakarta.cdi/jakarta/enterprise/inject/spi/injectionpoint ) or more specifically of InjectionPoint#getMember() does not tell us, if the return value can be NULL.

However, we find a code example in the interface docs:

 @Produces
 Logger createLogger(InjectionPoint injectionPoint) {
     return Logger.getLogger(injectionPoint.getMember().getDeclaringClass().getName());
 }

If InjectionPoint#getMember() can be NULL, this code example would throw a NullPointerException.

Weld will create an injection point with a NULL member for the following call (from the test):

cdi.select(String.class, new BatchPropertyLiteral("prop2")).get();

This would violate the current JavaDoc of InjectionPoint.

So the actual question would be:

• Is it allowed behaviour of InjectionPoint#getMember() to return NULL? If so, the Javadoc of InjectionPoint needs to be updated or needs to be clarified by the CDI spec people.
• If it is not allowed, the Batch TCK would rely on undefined vendor-specific (Weld) behaviour.

[rmannibucau]

If it helps InjectionPoint#getBean can return null and it is stated, any other method can't - intentionally cause an Instance - so CDI - only defines an InjectionPoint when there is one: @Inject Instance<X> i;.

[scottkurz]

So, @rmannibucau, @struberg and team, please open a challenge issue, when ready, (assuming that's what you want to do).

A concern I have here is that, while I don't understand the API details mentioned in the PR comments here very well, it is sounding to me like you're implying that the spec is wrong.

In the section: https://jakarta.ee/specifications/batch/2.1/jakarta-batch-spec-2.1#consequences-and-suggested-patterns it says:

As a consequence of the previous section, an application must be able to get a CDI Bean with a correct view of a batch property by either:
...
Dynamically accessing the batch property bean via jakarta.enterprise.inject.Instance#get() or javax.enterprise.inject.spi.CDI#select() from the batch execution thread.

I feel like the implication of your point is that we should not have said that.

Should we have said it differently, aligning more with your reworked test? Should we just try to remove this text in a future release?

I'm not so concerned with real-world usage... if someone noticed this issue we could just say "you bumped up against an ambiguity in CDI and this is actually only a Weld behavior".

But ideally we can wrap this up with the correct text.

And, having decided what that text should be, we can figure out whether to simply exclude the test or rewrite it.

[rmannibucau]

I feel like the implication of your point is that we should not have said that.

As written it is not wrong but ambiguous, or javax.enterprise.inject.spi.CDI#select() part should be rewritten or javax.enterprise.inject.spi.CDI#select() using a wrapper bean injecting batch properties but this is often implicit, rest is valid (since Instance can be injected itself).
Long story short on the JBatch side the spec should never explain how and delegate that to the CDI knowledge IMHO, just rely on something like using CDI should be sufficient.

simply exclude the test or rewrite it

think the feature tested is great and should be covered, Richard's proposal is quite great and the way to go on a CDI standpoint IMHO so I'm for keeping it but reworked as in the PR.

[rzo1]

@scottkurz Just a short update. I just started a discussion to get consensus (according to the TCK challenge process) on the TomEE side to fill a challenge from our side. Might take some time.

[rmannibucau]

@scottkurz would this impl in the TCK be more acceptable - speaking outside the challenge:

@Override
public String process() throws Exception {
    CDI<Object> cdi = CDI.current();
    
    JobContext jobCtx = cdi.select(JobContext.class).get();
    StepContext stepCtx = cdi.select(StepContext.class).get();
    BeanManager bm = cdi.select(BeanManager.class).get(); // or cdi.getBeanManager()
    String prop1Val = lookupProperty(bm, "prop1");
    String prop2Val = lookupProperty(bm, "prop2");


    appendExitStatus(jobCtx, jobCtx.getExecutionId() + ":" + stepCtx.getStepName() + ":" + prop1Val + ":" + prop2Val);

    return "OK";
}

private String lookupProperty(final BeanManager beanManager, final String key) {
    final var creationalContext = beanManager.createCreationalContext(null);
    try {
        return (String) beanManager.getInjectableReference(new InjectionPoint() {
            private final Set<Annotation> qualifiers = Set.of(
                    new BatchPropertyLiteral(key),
                    Any.Literal.INSTANCE
            );

            @Override
            public Type getType() {
                return String.class;
            }

            @Override
            public Set<Annotation> getQualifiers() {
                return qualifiers;
            }

            @Override
            public Bean<?> getBean() {
                return null;
            }

            @Override
            public Member getMember() {
                return null;
            }

            @Override
            public Annotated getAnnotated() {
                return null;
            }

            @Override
            public boolean isDelegate() {
                return false;
            }

            @Override
            public boolean isTransient() {
                return false;
            }
        }, creationalContext);
    } finally {
        creationalContext.release();
    }
}

It is stil somehow violating CDI but will not be visible from a vendor standpoint short term and will keep the test at the same functional level.

Side note: indeed, it enforces my point about the fact JBatch should rely on CDI and not explicitly reference CDI or any other solution since the lookup ways are CDI hosted.

Side note 2: this is more relevant technically since it enables to call CreationalContext#release - theorically after appendExitStatus in process but for String it can be earlier since the instance is immutable and without dependency - which avoids leak in a real world impl - Instance release call is uncontrolled and should be reserved for normal scoped instances which is not the case for JBatch instances so this can leak depending the implementation using CDI or Instance.

[scottkurz]

Thanks all for the detailed input and discussion.

After talking about this issue some more internally with the IBM team, I have some thoughts on how to proceed. Since Richard mentioned it might take awhile to decide whether to formally submit a TCK challenge, let me share them now.

First, I think the test should simply be excluded, rather than rewritten. Yes, it is unfortunate to lose any amount of coverage. But what if an already-certified implementation doesn't pass the new, reworked test? I don't think it's worth the risk unless it is a trivial, straightforward change, which this is not.

I think instead of fixing the test we should think in terms of future enhancements. I opened up #70 to gather related ideas for how we might improve/re-establish coverage in this area. I don't think we have to have a 1-1 replacement necessarily, but rather can decide what we want to do.

Next, it seems we need to do a Service Release to fix what looks like a "spec bug".

I like Romain's general thought that we just need to be more general and less specific, knowing that this behavior is fundamentally governed by the CDI specification. I will work on this text. At the same time, I think we could offer a simple example, to make it a bit more concrete.

E.g. maybe this is non-controversial:

      // Inject into a CDI bean
     @Inject @BatchProperty(name="prop1") Instance<String> p1;

I opened jakartaee/batch#209 for this.

Conclusion

If we agree on that and stop there, I think we can proceed without even needing to know how the CDI discussions, e.g. jakartaee/cdi#779, will play out.

That all said, I'd still like to wait on the challenge, to get a feel for the priority here.

[rzo1]

Thanks @scottkurz for the update and the ideas. I think creating the challenge (now) should be pretty straightforward, but I just put a call on our list discussion to raise any concerns within the next few days, and then I will assume lazy consensus and open the formal challenge issue with the arguments from the list + already raised in this discussion. I guess, that we are fine, if the test can be excluded (for now) until there is consensus on the CDI side.

[rzo1]

@scottkurz Here it is: #71

[scottkurz]

We can close this PR.