Add cdi req

[scottkurz]

Fixes #46

Guide to reviewing:

• 3.Foreword
  • Introduce the change to require CDI
• 9.3 Batch Properties
  • refactored the property stuff to be used for both batch-mgd and CDI dependent scope
  • added a note on producers.. left undefined if a user adds own producer
  • mentioned it was undefined to use properties on non-Dependent scope
  • refactored the no matching property or empty property value discussion (was this only supposed to be about empty
    property values? Did I wrongly add the "no matching property" use case??? Will have to check)
• 9.4 Batch Contexts
  • Tried to leverage the existing scope definitions and say that the batch runtime would provide a similar scope view even for a Dependent-scoped bean
  • Undefined if user adds own producer
• 10.5. Batch Artifact Loading
  • Added the 1-5 sequence : CDI name, CDI batch.xml, CDI FQCN, batch-mgd batch.xml, batch-mgd FQCN
  • Clarified that using CDI with FQCN implied default classifier .. normal ambiguity resolution
  • Added some scope examples to illustrate one instance per JSL ref
  • I don't see the point of saying the impl-specific load should happen first? Isn't this untestable?

Finally, I might have messed up some links , section refs so please keep an eye out for that.

[rmannibucau]

A few wording issues (producer/bean and probably a classloader unification to do) but overall +1 (no need to go back in review if the changes are ok ;))

[rmannibucau]

s/Producer/Bean/

a producer is a field or method with @Producer which enables the container to register a Bean<?> which is a generic available "factory". Consumers only see beans.

Do we want to support a few more primitive/wrappers? Any impl not doing it?

Lastly we must define what @Dependent gives, whicih is likely during component lookup (after thread local will be off in all impl) so it is not dependent on the job execution but on the initialization only, no?

[scottkurz]

a producer is a field or method with @Producer which enables the container to register a Bean<?> which is a generic available "factory". Consumers only see beans.

So we just say the batch runtime should ensure there is a Bean.. and whether it is done via Producer or some other method like an extension is just an implementation detail? I think I got it.

Do we want to support a few more primitive/wrappers? Any impl not doing it?

I was hoping to still do this in #43

Lastly we must define what @dependent gives, whicih is likely during component lookup (after thread local will be off in all impl) so it is not dependent on the job execution but on the initialization only, no?

I was trying to say that the values would be set the same as in the **Properties for Dependent-scoped bean instances
** which implies we potentially have to wait until job execution for the JSL substitution to take place. That's why I separated out Dependent scope from the other scopes.

Seems like we have a different take on this. I'm thinking you do want to take advantage of JSL subsitution but you also want CDI injection, etc. Do you see a use case where this isn't possible?

[scottkurz]

Thx for the review @radcortez .. and I can see you're not a fan of two spaces after a period :) so I cleaned that up. Appreciate it.

[scottkurz]

From ML: clarify that for "===== Properties for Dependent-scoped bean instances" we're referring to the resolution of values, not injection. (If a better wording can be found).

[scottkurz]

Let me respond to @rmannibucau 's comment from the ML: https://www.eclipse.org/lists/jakartabatch-dev/msg00241.html

Since batch properties will be beans you can look them up doing something like CDI.current().select(String.class, new BatchProperty.Literal("myName") /literal implements Batchproperty/). We should just mention when this lookup is valid, ie when the bean is instantiated - means an interceptor with an @AroundConstruct works but not a listener which would get the property of the listener but not the reader for ex.  @chengfang hope to get your input too.

I think what he's getting at is that:

1. The properties are resolved when a bean instance is created based on a ThreadLocal
2. The ThreadLocal is populated based on the current batch artifact being executed within the job lifecycle: listener, batchlet, reader/writer/etc. (Although saying "artifact being executed" is a bit misleading... we might be executing the user method or we might simply be loading it. It's definitely the "current artifact" at a vague, high level but I might struggle to put this in words).

I did try an example using select like Romain mentioned (assuming I understood correctly).

        Instance<String> myBatchProp = CDI.current().select(String.class, new BatchPropertyLiteral("aa"));
        String lazy = myBatchProp.get();

I hit some problems. In both Open Liberty & JBeret we do get a non-null InjectionPoint passed to the property producer method, but then we get an NPE. I think this is because of the logic "try to get the default property name from the field name". So maybe this could be considered a bug in each impl then.. not prioritizing this use case yet. In BatchEE, OTOH, I see a null InjectionPoint passed to the property producer method, so we just get a null back. (Batchee => OWB, OL + JBeret => Weld).

Now... unless I did something wrong, I don't want to get caught up in this issue which these three impls don't seem to work with.

But maybe we can make the same point in the spec using another example: when a batch artifact bean injects some other bean, can that bean have batch properties and contexts injected within? (@follis raised this as: #132).

I think this illustrates the ThreadLocal-based resolution of properties and contexts.

To illustrate this latter case and the select, Instance.get() that I couldn't get to work I forked a BatchEE UT:

1. git clone git@github.com:scottkurz/geronimo-batchee.git
2. cd extensions; mvn -pl cdi test -Dtest=BatchScopesTest#test -Drat.skip=true

[scottkurz]

Do you mean === instead of ====? By the way, you might need to add a couple of sentences to summarise the sub sections.

No I meant to nest all of this under === Batch Properties. Will see if I can summarize better.

[scottkurz]

Hi, thank you for the reviews so far. There have been enough changes that I'm going to dismiss the previous reviews. Hopefully at least some of you can take another look. I'll try to guide you on the most recent changes.

WHY CHANGE?

@rmannibucau made some comments on the ML:
https://www.eclipse.org/lists/jakartabatch-dev/msg00238.html
https://www.eclipse.org/lists/jakartabatch-dev/msg00241.html
and in Slack: https://eclipsefoundationhq.slack.com/archives/C02K9FX2ETA/p1638899851005900

Now, I'm not sure I completely understand Romain's comments and terminology. But trying to think through the issues on my own led me to take a different path.

KEY CHANGE

Basically I tried to say that any context or properties bean instance created on a batch thread of execution gets its values from the batch job: for properties from the artifact being loaded or executed, and for the contexts from the step/job being executed on that thread.

Before had said: "you can inject batch properties into a Dependent-scoped batch artifact bean predictably but injecting into an ApplicationScoped-bean is undefined. Now in this latest revision I reworded to show that the Dependent-scoped batch artifact gives the expected context/property values because the new bean instance is created from the current batch execution thread. But I tried to show that you can ALSO use a more dynamic approach to have a bean created on the current execution thread, with the correct values, e.g. inject a @BatchProperty Instance into an ApplicationScoped-bean and then do an Instance.get() from within the current execution thread.

I didn't go into too much detail about what a batch thread of execution means. Historically we've tried to say as little as possible here and have never gone into too much detail. The basic TCK coverage can hopefully cover us enough.

SUMMARY of GUARANTEED TO WORK USE CASES

As mentioned in 9.3.3.3 and 9.4.3.1, the spec basically guarantees you can do the three use cases:

1. For a @Dependent-scoped batch artifact - statically @Inject the context/property Bean into a , (e.g. via a injection of type: @Inject JobContext within a batch artifact loaded as a CDI Bean). This assumes the artifact loading will occur on the execution thread.
2. For a normal-scoped batch artifact - Inject an Instance wrapping the property/context then dynamically access via jakarta.enterprise.inject.Instance#get() (or javax.enterprise.inject.spi.CDI#select() ) from the batch execution thread.
3. From a non-bean access property/context via javax.enterprise.inject.spi.CDI#select() from the batch execution thread.

Another significant consequence is that you can also access the contexts/properties from a NON-BATCH-ARTIFACT.. running on the batch execution thread. E.g. statically inject the contexts, which could be fairly useful (injecting the properties would be possible though not as obviously useful).

LESSER CHANGES

Finally, a small change is that I think it's an implementation-detail to say the context/property beans need to be @Dependent-scoped. That's probably a good way to implement, but if an impl wanted to use something like @StepScoped then I don't see anything wrong with that.

HOW TO REVIEW THE CHANGE

• It's probably easiest to just fetch the change, mvn install -Dmaven.javadoc.skip=true then read the generated sections 9.3, 9.4 in full.
• However, these changes are all covered in the last commit.
• Note TCK branch: https://github.com/scottkurz/batch-tck/commits/cdi-tests provides some coverage of the above.
  E.g. see the tests in CDITests.java and the backing artifacts. I covered the three styles of bean access (mentioned above).

[rmannibucau]

If we want to be picky there are still some sentences making cdi injection = field injection (whereas it can be methods or constructor too) but I think the goal is reached to clarify the IoC integration and these sentences can be seen as very acceptable "language abuse" so +1

[scottkurz]

Thx @rmannibucau for your re-review and of course the many rounds of feedback to get to this point.

I agree the language is a bit loose w.r.t field vs method vs ctor injection.
I tried to keep most of the references to field injection qualified as only "for example".

Since this issue / PR is somewhat blocking (the primitive wrapper PR is stacked on top of it), let me move that discussion to #50, since maybe we are close to closing this one.

[scottkurz]

I'm planning on merging this today. With EOY approaching, given the reviews that have already been done, and given Romain already re-reviewed.... I think we got most of the comments we're going to get.