ebean: ScalarTypeWrapper doesn't handle "nullValue" correctly

I have a column using a custom type with a converter as:

    @Convert(converter = CustomDateConverter.class)
    CustomDate somedate;

@Converter
public class CustomDateConverter implements ScalarTypeConverter<CustomDate, Date> {
    @Override
    public CustomDate getNullValue() {
        // Return null value or null object here will cause either reading or binding issue
        return CustomDate.nullDate; 
    }

    @Override
    public Date unwrapValue(CustomDate attribute) {
        return attribute != null ? attribute.toDate() : null;
    }

    @Override
    public CustomDate wrapValue(Date dbData) {
        return (dbData != null) ? CustomDate.fromDate(dbData) : CustomDate.nullDate;
    }
}

From my investigation, I can see that ebean can’t wrap / unwrap value correctly due to the following code in ScalaTypeWrapper:

  @Override
  public B read(DataReader reader) throws SQLException {
    S sv = scalarType.read(reader);
    if (sv == null) {
      return nullValue;
    }
    return converter.wrapValue(sv);
  }

  public Object toJdbcType(Object value) {
    Object sv = converter.unwrapValue((B) value);
    if (sv == null) {
      return nullValue;
    }
    return scalarType.toJdbcType(sv);
  }

If the value from database is “null”, the converter doesn’t have chance to wrap it into a “none” at custom type
If the value from the entity property is “none”, it never gets chance to translate into “null” at JDBC type

The fix would be:

  @Override
  public B read(DataReader reader) throws SQLException {
    S sv = scalarType.read(reader);
    return converter.wrapValue(sv);
  }

  public Object toJdbcType(Object value) {
    Object sv = converter.unwrapValue((B) value);
    return scalarType.toJdbcType(sv);
  }

… which leaves the handling of null value / null object to the converter instead.

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 15 (9 by maintainers)

Commits related to this issue

#2199 - ScalarTypeWrapper doesn't handle "nullValue" correctly Handles the case for ScalarTypeConverter with custom null value and binding the custom null value (via query parameter, CallableSql para... — committed to ebean-orm/ebean by rbygrave 3 years ago
#2199 - ScalarTypeWrapper doesn't handle "nullValue" correctly Fix for - standard JPA AttributeConverter which doesn't have the getNullValue method, he/she can't convert null to a custom null object ... — committed to ebean-orm/ebean by rbygrave 3 years ago

Most upvoted comments

Pushed a fix for - “standard JPA AttributeConverter which doesn’t have the getNullValue method, he/she can’t convert null to a custom null object”

The fix is for AttributeConverterAdapter to probe the AttributeConverter for the nullValue rather than just assuming it is null. I’m happy with this approach and keeps it in line with what we have. We still need to ponder if ScalarTypeConverter should be deprecated at some point but no rush on this.

rbygrave on Mar 18, 2021

uses standard JPA AttributeConverter which doesn’t have the getNullValue method, he/she can’t convert null to a custom null object

Yes, I can see that now.

I am pondering if this part of the issue is more a design issue in terms of this subtle difference between ScalarTypeConverter and AttributeConverter.

For Ebean ScalarTypeConverter the javadoc says:

 * Matches the functionality of javax.persistence.AttributeConverter
 * <p>
 * In general AttributeConverter should be used in preference to this
 * ScalarTypeConverter as it is JPA standard and offers the same functionality.
 * </p>
 * <p>
 * For Ebean we will look to deprecate this interface in preference to AttributeConverter.
 * </p>

Edit: The javadoc doesn’t talk about “null treatment”

Ultimately there is a subtle design mismatch between these 2 API’s with Ebean ScalarTypeConverter having the additional getNullValue() method (which is not present on JPA AttributeConverter).

I’m pondering if the correct thing to do is actually remove the ScalarTypeConverter.getNullValue() method with the thought to make these interfaces exactly compatible wrt null treatment. In theory this would make it clear that “null conversion logic” needs to be in the wrapValue() and unwrapValue() methods [rather than outside in ScalarTypeWrapper].

Status:

I’ll push the commit for the “fix the parameter binding in case we use ScalarTypeConverter”
We have an issue for AttributeConverter with a custom null value
Longer term we have design issue in terms of subtle difference between ScalarTypeConverter and AttributeConverter and maybe should look to actually deprecate ScalarTypeConverter (once we sort 2).

Next steps: Failing test case for AttributeConverter with a custom null value.

rbygrave on Mar 17, 2021

Yes, I think the fix is literally just to remove the if (sv == null) { return nullValue; } … as you suggest.

  public Object toJdbcType(Object value) {
    Object sv = converter.unwrapValue((B) value);
    // if (sv == null) {    
    //   return nullValue;  // <-- It returns the "nullValue" which is "CustomDate.nullValue" here but we want "null" instead
    // }
    return scalarType.toJdbcType(sv);
  }

rbygrave on Mar 17, 2021

It doesn’t have to be a CallableSql. It’s reproducible as long as the code runs over the Binder.bindObject on ScalarTypeWrapper.

instcode on Mar 17, 2021

Yeah cool. I probably only need to see approximately how to reproduce it. I’m guessing CallableSql with a bind parameter value of CustomDate.nullValue.

rbygrave on Mar 17, 2021