Netflix / astyanax

Cassandra Java Client
Apache License 2.0
1.04k stars 357 forks source link

Blocker: @Column fields do not get reset to null #358

Open jacek99 opened 11 years ago

jacek99 commented 11 years ago

We found this while doing regression testing from old persister to new JPA annotations.

We have a lot of columns that are being reset to null before the row gets updated. This means they were not null before, but they are now.

It looks like the code in LeafColumnMapper.fillMutationBatch() is skipping those columns altogether, i.e. it leaves them in Cassandra with the previous non-null value.

public boolean fillMutationBatch(Object entity, ColumnListMutation<String> clm, String prefix) throws Exception {
    Object value = field.get(entity);
    if(value == null) {
        if(columnAnnotation.nullable())
            return false; // skip

This is a regression compared to the previous persister framework. It is also not in line with how JPA annotations work.

If a nullable column is reset to null, that should be reflected in the DB instead of being skipped altogether in the mutation.

This is currently a total blocker for us as there is no way to customize this behavior. We are not able to move from the old ORM API to the new JPA one...

jacek99 commented 11 years ago

I apologize. I realize looking at our own code that we developed a custom MappingUtil that overrode this default functionality to behave as we expected (i.e. we delete the column in the mutation).

Is there any way to implement the same functionality in the new JPA annotations?

elandau commented 11 years ago

Part of the issue with the lightweight implementation in astyanax is that it doesn't deal with deleted, or modified, data very well. Ideally the POJOs should be proxied so that changes can be properly detected. Otherwise, it's difficult to discern between an update to a column in a row as apposed to wanting to delete all columns with NULL values.

Two possible ways to handle this could be,

  1. Make this behavior a configuration parameter
  2. Add a new method that is call something like putAndDeleteNullFields.

I'm open to other suggestions.

jacek99 commented 11 years ago

maybe put(T entity, MutationParameters params)

where MutationParameters is a POJO with.

private boolean deleteNullFields;

That way if something changes in the future you only need to add fields to this POJO instead of create new function calls with different names.

sverrehu commented 10 years ago

We were bitten by this one as well, as we expected any change we did to the entities (including setting fields to null) to be reflected in the database.

I changed the fillMutationBatch method of LeafColumnMapper to the following, and now it appears to do what we want. (Not yet rigorously tested.)

public boolean fillMutationBatch(Object entity, ColumnListMutation<String> clm, String prefix) throws Exception {
    Object value = field.get(entity);
    if(value == null) {
        if(!columnAnnotation.nullable())
            throw new IllegalArgumentException("cannot write non-nullable column with null value: " + columnName);
    }
    @SuppressWarnings("rawtypes")
    final Serializer valueSerializer = serializer;
    // TODO: suppress the unchecked raw type now.
    // we have to use the raw type to avoid compiling error
    if (value == null) {
        clm.deleteColumn(prefix + columnName);
    } else {
        clm.putColumn(prefix + columnName, value, valueSerializer, null);
    }
    return true;
}

(I do not really see why an entity mapper should interpret null values in the entity as "leave old value intact". If it does, it's more like a "selected-fields-of-an-entity mapper" to me. :-) )

blop commented 9 years ago

+1

We also call deleteColumn if value is null and column is nullable : clm.deleteColumn(prefix + columnName);

antoinebaudoux commented 9 years ago

+1