search

j-easy / easy-batch

The simple, stupid batch framework for Java
https://github.com/j-easy/easy-batch/wiki
MIT License
611 stars 199 forks source link

Fix NPE if value is null #376

Closed seseso closed 4 years ago

seseso commented 4 years ago

This is a FIX suggestion for a NPE when a value is null on a POJO.

fmbenhassine commented 4 years ago

Thank you for the PR. Since we are getting the value by calling a getter method, we can use the return type of that getter to get the sql type instead of the value's class, something like:

-- Object value = propertyDescriptor.getReadMethod().invoke(record);
-- Integer sqlType = javaTypesToSqlTypes.get(value.getClass());
++ Method readMethod = propertyDescriptor.getReadMethod();
++ Integer sqlType = javaTypesToSqlTypes.get(readMethod.getReturnType());
++ Object value = readMethod.invoke(record);

The getter method should be there (by definition of a javabean). This approach avoids an additional check in the prepareStatement method which starts to become a bit complex already..

Do you agree? If you agree, please update the PR accordingly and it will be good to merge.

seseso commented 4 years ago

I've changed per your suggestion. Now I have a problem because my Use Case is a little bit "akward". I generate my beans at runtime an don't know the type of the objects my field will have so all my field have the type java.lang.Object. The way I had it, the SQL type would be inferred by the value of the object and that would work. This way it won't (for my use case) recognize the SQL type. Maybe I'll have to stick with a forked version after all...

seseso commented 4 years ago

For my side I would always have to re-implement the prepareStatement method. Would it be okay to move private properties to protected, so I could extend this class, instead of having to make a copy of it to change the method prepareStatement?

fmbenhassine commented 4 years ago

LGTM. Rebased, squashed and merged as 0babc6ff643d0fd1ee495ce524520170f1983476.

If you need to fork the project (or one of its classes), that means there is something wrong with Easy Batch! The BeanPropertiesPreparedStatementProvider is designed for regular JavaBeans. Your use case is very specific, so I would keep the implementation targeted at the majority of use cases, and make it possible to extend the default behaviour for specific cases (I made all fields protected as you suggested, see 5e541f8ba550a441da35fd15e63c54d0699c9116).

You can already try that in 6.1.0-SNAPSHOT.

Thank you for your PR!