Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support common java types in @BatchProperty injection #43

Closed
follis opened this issue Jul 15, 2020 · 9 comments · Fixed by #182
Closed

Support common java types in @BatchProperty injection #43

follis opened this issue Jul 15, 2020 · 9 comments · Fixed by #182

Comments

@follis
Copy link
Contributor

follis commented Jul 15, 2020

Originally opened as bug 5382 by cf126330

--------------Original Comment History----------------------------
Comment from = cf126330 on 2013-09-11 14:56:48 +0000

Currently only java.lang.String is supported in @BatchProperty field injection. For other types of data, applications have to convert from String. I propose we support all common java types for @BatchProperty field injections, to make batch api easier to use. A related bug was Bug 4353.

We don't need to change the job xml schema, where properties are still declared as string values. The batch container is responsible for data conversion behind the scene. For example,

@BatchProperty
private int count;

A list of common java types pertaining to batch API:

all primitive types (int, short, byte, long, double, float, boolean, char)
array of all primitive types
wrapper type of all primitive types
array of all wrapper types of primitive types
List
Map<String, String>
Set
BigInteger
BitDecimal
java.util.Date
java.io.File
java.util.jar.JarFile
java.util.regex.Pattern
URL
URI
Inet4Address
Inet6Address
java.util.logging.Logger
java.lang.Class


Comment from = ScottKurz on 2015-09-10 17:44:34 +0000

Getting back to this.

I like the idea.. just wondering
A) precisely which ones to include/exclude
B) How to reference the specifics of each type of conversion (without having to define it in the batch spec itself). Not that it would be that hard to do so in our spec..just wondering if this was already defined.

E.g. it seems BatchEE accomplishes this using
org.apache.xbean.propertyeditor.PropertyEditors

http://geronimo.apache.org/maven/xbean/3.6/xbean-reflect/apidocs/org/apache/xbean/propertyeditor/package-tree.html


I wonder where that would leave JSONObject ? Not as easy as I'm naively assuming for some reason or just didn't happen to be in that list?


Comment from = rmannibucau on 2015-09-24 21:51:11 +0000

Most probably something like github.com/tomitribe/sabot mecanism (same idea as JAXRS with its fromString or constructor handling) would match very well, I agree.


Comment from = jrperkinsjr on 2015-09-29 20:53:30 +0000

I think the spec should only guarantee that primitives (and their Object types), String, BigDecimal and BigInteger the allowed injection types. Maybe primitive arrays too. Implementations can implement more providers for other types if they'd like.


Comment from = ScottKurz on 2015-10-08 13:54:02 +0000

Seems kind of arbitrary to pick the set beyond the primitives, I agree...

I guess another direction to take this in is to model after JSF Converter, and define some built-in converters and the opportunity for custom converters.

Since JSF and JPA each have a bit different converters, seems like the precedent would be set for batch to define its own.

Would need to be clear if this is part of CDI integration or DI-neutral.

@mminella
Copy link

mminella commented Apr 5, 2021

I agree on the take around adding a converter abstraction. I'd think that the spec would keep it very bare bones with implementations being allowed to provide more as they see fit. I do think that the list as provided is probably too broad (Logger?).

@scottkurz
Copy link
Contributor

scottkurz commented Apr 6, 2021

We should probably break this apart and use this current issue for a relative few types. If we wanted to add a converter abstraction that could be done later/separately (or just left to impls).

What about starting from the list of JSF converters: https://docs.oracle.com/javaee/7/tutorial/jsf-page-core001.htm

BigDecimalConverter | javax.faces.BigDecimal
BigIntegerConverter | javax.faces.BigInteger
BooleanConverter | javax.faces.Boolean
ByteConverter | javax.faces.Byte
CharacterConverter | javax.faces.Character
DateTimeConverter | javax.faces.DateTime
DoubleConverter | javax.faces.Double
EnumConverter | javax.faces.Enum
FloatConverter | javax.faces.Float
IntegerConverter | javax.faces.Integer
LongConverter | javax.faces.Long
NumberConverter | javax.faces.Number
ShortConverter | javax.faces.Short

The JSF package would not be visible in the batch API in any way... it's just a useful set of primitive/almost-primitive types.

@scottkurz
Copy link
Contributor

scottkurz commented Nov 28, 2021

Reading the comments we have had, the majority opinion seems to err on the side of specifying less rather than more (and leaving it up to the implementation).

So I'll propose we specify support for the java.lang types: (Boolean, Byte, Character, Double, Float, Integer, Long, Short) and the corresponding primitives, using the constructors w/ String parms.

I'm also going to raise the issue in the Config spec going forward.

@scottkurz
Copy link
Contributor

For reference, noting JBeret's function implemented here: https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/creation/ValueConverter.java

@rmannibucau
Copy link

using the constructors w/ String parms

Why not parseXXX as preferred since several versions?

otherwise +1

@scottkurz
Copy link
Contributor

Why not parseXXX as preferred since several versions?

I was looking at the Java 8 doc and didn't realize that so agree, when these are deprecated in newer versions we should use better ones like valueOf, parseXXX, etc.

@scottkurz
Copy link
Contributor

I removed Byte and Character since I didn't want to have to think about conversion to/from String.

@rmannibucau
Copy link

Hmm, Byte has valueOf no? And Character is mainly string.charAt(0) or fail. No need of the to conversion which is a toString btw ;).
But agree it is not a big deal to not handle it.

@scottkurz
Copy link
Contributor

Though it'd be trivial to come up with a sensible scheme... to take the time to research whatever precedent there is around these tiny details is just not worth it given that you can easily use another type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants