doanduyhai / Achilles

An advanced Java Object Mapper/Query DSL generator for Cassandra
http://achilles.archinnov.info
Apache License 2.0
241 stars 92 forks source link

Null not handled #290

Closed rmannibucau closed 7 years ago

rmannibucau commented 7 years ago

Use case: reset to null a field of an entity Issue: info.archinnov.achilles.internals.statements.PreparedStatementGenerator#generateUpdate filter null values instead of allowing to set Unset.Value so there is no way to reset the value until you set a custom "null" value and info.archinnov.achilles.internals.statements.BoundValuesWrapper#bindForUpdate should not filter null values but use UNSET probably?

Edit: confirming than removing both filter(value != null) (BTW Objects::nonNull is usable ;)) then as a user you can reset a value to null instead of keeping the old value.

doanduyhai commented 7 years ago

Indeed manager().crud().update() is not meant to be used to update values with null. Updating values with null means delete them.

If you want to delete some columns, why don't you use 👍

When using InsertStrategy.ALL_FIELDS, Achilles with bind all values, null included so you have the desired behavior

rmannibucau commented 7 years ago

Hmm, if I use delete then an "update" (standard CRUD semantic) means delete+insert which sounds not really usable (and cassandra-driver doesn't imply that, achilles wrapping does), if I use insert I will get a duplicate (relatively to the "key"), so none of these solutions actually work.

Patching achilles I got it working not ignoring null fields so I think it makes sense to have it as a feature (think it means having an UpdateStrategy or just use a single CrudStrategy enum for that).

Here is the pseudo java code matching this ticket to ensure we speak of the same use case:

Model m = new Model();
m.setId(...);
m.setDisplayName("foo");
achillesDto.insert(m); // crud.insert().execute()

m.setDisplayName(null);
achillesDto.update(m); // crud.update().execute()
doanduyhai commented 7 years ago

if I use insert I will get a duplicate (relatively to the "key"), so none of these solutions actually work --> wrong, new values will override old value

You don't understand insert semantics of Cassandra

Model m = new Model();
m.setId(...);
m.setDisplayName("foo");
m.setXXX("xxx");
achillesDto.insert(m); // crud.insert().execute()

m.setDisplayName(null);
achillesDto.insert(m); // crud.insert()..withInsertStrategy(ALL_FIELDS).execute()

The 2nd insert will:

So after the 2nd insert, you'll have (id, null, xxx) and that's what you want ...

rmannibucau commented 7 years ago

Thought so but if i do:

@Test
public void tmp() {
    final Role role = new Role();
    role.setName("1");
    role.setUuid("1");
    repository.insert(role);
    role.setName("2");
    repository.insert(role);
    System.out.println(repository.count()); // select count(*) from role
}

count == 2 so the delete/insert is not reliable, or did i miss a flag?

doanduyhai commented 7 years ago

What's the schema ? And what does repository.insert() ? It's not Achilles API here

rmannibucau commented 7 years ago

schema:

CREATE TABLE IF NOT EXISTS test.role(
    uuid text,
    name text,
    PRIMARY KEY(uuid));

the repo just does:

roleManager.crud().insert(role).execute()
doanduyhai commented 7 years ago

Normally the select count(*) from role should return 1 because the uuid column has not been changed between the 2 inserts. Can you do just a truncate role just before executing the test to be sure there is not side-effects from other tests ?

rmannibucau commented 7 years ago

the test setup creates a dedicated cassandra instance in a temporary folder so it really starts from a plain empty cassandra

doanduyhai commented 7 years ago

Just enable DML logging to see the CQL statements generated by Achilles and the bound values

rmannibucau commented 7 years ago

forget it, very no luck case. Just found we have a default role so the 1+1 is not insert+insert of the test but (default)+(test).

Sorry for the noise. We can close this one and a big thanks for the help and patience.