@DbJsonB Map Still Perform ModifyAware Dirty Checking When MutationCheck is NONE

[raphaelNguyen]

Expected behavior

A Map<String, Object> field only annotated with @DbJsonB(mutationCheck = MutationCheck.NONE) in an @Entity bean should not invoke modify aware dirty checking logic when saved. This correct behaviour can be observed if the annotated field is typed Object.

Actual behavior

Modify aware logic is always on regardless of MutationCheck.NONE being set as the json mutation check strategy on @DbJsonB annotation or as the server default on DatabaseConfig.

Steps to reproduce

The code below can be found in sample project test-ebean.

@Entity
public class TestObject {
  @Id
  private String id;

  @DbJson(mutationDetection = MutationDetection.NONE)
  private Map<String, Object> testMapJson;

  @DbJson(mutationDetection = MutationDetection.NONE)
  private Object testObjectJson;
  
  // constructor, getter, setter etc...
}

public class Main {
  public static void main(String[] args) {
    // setup
    // table "test_object" has 3 columns id, test_map_json, test_object_json
    TestObject testObject = new TestObject(
        "me",
        Map.of("field", List.of("a", "b", "c")),
        Map.of("field", List.of("a", "b", "c")));

    Database database = DatabaseConfiguration.getDatabase();

    try (Transaction transaction = database.beginTransaction()) {
      database.delete(TestObject.class, "me");
      database.save(testObject);
      transaction.commit();
    }

    // set test_map_json to a map of exactly the same value
    testObject = database.find(TestObject.class, "me");
    testObject.setTestMapJson(Map.of("field", List.of("a", "b", "c")));

    try (Transaction transaction = database.beginTransaction()) {
      database.save(testObject);
      transaction.commit();
    }

    // set test_object_json to a map of exactly the same value
    testObject = database.find(TestObject.class, "me");
    testObject.setTestObjectJson(Map.of("field", List.of("a", "b", "c")));

    try (Transaction transaction = database.beginTransaction()) {
      database.save(testObject);
      transaction.commit();
    }

    database.shutdown();
  }
}

Log:

11:17:01.154 [main] INFO io.ebean - ebean version: 13.8.1
11:17:01.169 [main] INFO io.avaje.config - Loaded properties from [] 
11:17:01.208 [main] DEBUG io.ebean.internal - Classpath search entities[1] searchTime[27] in packages[[]]
11:17:01.869 [main] INFO io.ebean.datasource - DataSourcePool [db] autoCommit[false] transIsolation[READ_COMMITTED] min[2] max[200] in[646ms]
11:17:01.874 [main] DEBUG io.ebean.core - platform for productName[postgresql] version[10.18]
11:17:01.887 [main] DEBUG io.ebean.BackgroundExecutor - Created backgroundExecutor ebean-db (schedulePoolSize=1, shutdownWaitSeconds=30)
11:17:02.045 [main] DEBUG io.ebean.internal - BeanPersistControllers[0] BeanFinders[0] BeanPersistListeners[0] BeanQueryAdapters[0] BeanPostLoaders[0] BeanPostConstructors[0]
11:17:02.093 [main] DEBUG io.ebean.internal - Entities[1]
11:17:02.118 [main] INFO io.ebean.core - Started database[db] platform[POSTGRES] in 944ms
11:17:02.127 [main] DEBUG io.ebean.SUM - txn[1001] -- Deleting TestObject Id: me
11:17:02.133 [main] DEBUG io.ebean.SQL - txn[1001] delete from test_object where id = ?; -- bind(me) rows(1)
11:17:02.148 [main] DEBUG io.ebean.SQL - txn[1001] insert into test_object (id, test_map_json, test_object_json) values (?,?,?); -- bind(me,{field=[a, b, c]},{field=[a, b, c]})
11:17:02.150 [main] DEBUG io.ebean.SUM - txn[1001] Inserted [TestObject] [me]
11:17:02.153 [main] DEBUG io.ebean.TXN - txn[1001] Commit
11:17:02.176 [main] DEBUG io.ebean.SQL - txn[1002] select t0.id, t0.test_map_json, t0.test_object_json from test_object t0 where t0.id = ?; --bind(me, ) --micros(1564)
11:17:02.192 [main] DEBUG io.ebean.SUM - txn[1002] FindBean type[TestObject] origin[CKTX3U.A.A] exeMicros[17929] rows[1] bind[me, ]
11:17:02.194 [main] DEBUG io.ebean.SQL - txn[1003] update test_object set test_map_json=? where id=?; -- bind({field=[a, b, c]},me)
11:17:02.195 [main] DEBUG io.ebean.SUM - txn[1003] Updated [TestObject] [me]
11:17:02.196 [main] DEBUG io.ebean.TXN - txn[1003] Commit
11:17:02.196 [main] DEBUG io.ebean.SQL - txn[1004] select t0.id, t0.test_map_json, t0.test_object_json from test_object t0 where t0.id = ?; --bind(me, ) --micros(669)
11:17:02.197 [main] DEBUG io.ebean.SUM - txn[1004] FindBean type[TestObject] origin[CKTX3U.A.A] exeMicros[808] rows[1] bind[me, ]
11:17:02.197 [main] DEBUG io.ebean.internal - Update skipped as bean is unchanged: TestObject@0(id:me, testMapJson:{field:[a, b, c]}, testObjectJson:{field:[a, b, c]})
11:17:02.197 [EbeanHook] DEBUG io.ebean - Ebean shutting down
11:17:02.198 [EbeanHook] DEBUG io.ebeaninternal.server.executor.DaemonExecutorService - DaemonExecutorService[ebean-db] shutting down...
11:17:02.198 [EbeanHook] DEBUG io.ebean.BackgroundExecutor - BackgroundExecutor stopped
11:17:02.204 [EbeanHook] INFO io.ebean.datasource - DataSourcePool [db] shutdown min[2] max[200] free[2] busy[0] waiting[0] highWaterMark[1] waitCount[0] hitCount[7]  psc[hit:1 miss:5 put:6 rem:0]

Expected transaction 1003 and 1004 to behave the same, i.e. not perform update.
However, transaction 1003 actually performs an update (see log at 11:17:02.195).

[raphaelNguyen]

A little debugging showed that in io.ebeaninternal.server.type.DefaultTypeManager.getJsonScalarType(), Map type fields has some special handling that in case of (!hasJacksonAnnotations && isMapValueTypeObject(genericType)), ScalarTypeJsonMap.typeFor is used to create ScalarType for the field instead of createJsonObjectMapperType().

ebean/ebean-core/src/main/java/io/ebeaninternal/server/type/DefaultTypeManager.java

Lines 345 to 351 in 7affd20

	if (type.equals(Map.class)) {
	if (!hasJacksonAnnotations && isMapValueTypeObject(genericType)) {
	return ScalarTypeJsonMap.typeFor(postgres, dbType, jsonManager.keepSource(prop));
	} else {
	return createJsonObjectMapperType(prop, dbType, DocPropertyType.OBJECT);
	}
	}

Since ScalarTypeJsonMap.typeFor does not take into account mutationDetection strategy at all, unlike in createJsonObjectMapperType(), it seems to be the reason why Map field is behaving this way. Can you advise if this behaviour is intentional?

[rbygrave]

Since ScalarTypeJsonMap.typeFor does not take into account mutationDetection strategy at all, unlike in createJsonObjectMapperType(), it seems to be the reason why Map field is behaving this way.

Yes.

Can you advise if this behaviour is intentional?

Well I can explain the background as to why it is like that and then we can see what we think from there.

The Map<String,Object> gets special treatment because

• (A) we can actually implement dirty checking on it [without using the mutation detection feature]
• (B) we are not using ObjectMapper for it (jackson-core only)
• (C) this case got conceptually missed. The MutationDetection.NONE on Map<String,Object> case.

The Map implementation returned for this case is going to be a ModifyAwareMap. There is similarly a ModifyAwareList, ModifyAwareSet etc. Now this stuff all existed before we added the MutationDetection functionality, and the MutationDetection functionality was primarily all aimed at the "JSON types that we use ObjectMapper for".

So internally the Map<String,Object> is only using jackson-core and not actually using jackson-databind and we were dealing with it prior to MutationDetection by using ModifyAwareMap etc.

EXCEPT ... if we want MutationDetection.NONE here we can't get it (which is what you have discovered here).

I think it is fair to say this is a case we didn't consider / missed and we should treat this as a bug.

I think the next step is to consider if we fix this / treat it as a bug without resorting to a workaround. The potential workaround being that if the field has a Jackson annotation it then skips that and gets handled via ObjectMapper etc [but this isn't ideal, I don't like this workaround as we don't want to rely on that / bit of a hack etc ... but maybe ok for short term workaround].

Edit: Note there are 2 PRs around refactoring out modules for ebean-jackson-jsonnode and ebean-jackson-mapper so I'll almost certainly desire to pull those in first.

[raphaelNguyen]

Thanks for the explanation, @rbygrave. That makes a lot of sense. While looking into this, I found that there are a lot of inconsistency between how a Map<String, Object> works compare to an Object field.

A few other things I found:

	Map<String, Object>	Object
mutationCheck=MutationCheck.HASH	still doing modify aware checking instead of hash mutation check (probably same reason as the NONE case)	do hash mutation check
mutationCheck=MutationCheck.SOURCE	do source mutation check but for @DbJsonB does not do JsonUtils.trim on the original raw json string before storing leading to the mutation check always returning dirty (the json string from db can have spaces while the formatted one from the field value usually does not)	does JsonUtils.trim so the mutation check work as documented

I think it would be a great improvement to make these cases work consistently.

If you decided that the current behaviour is what it should be or it can't be fixed in the short term, it would still be great to clarify this in the documentation at https://github.com/ebean-orm/ebean-annotation/blob/master/src/main/java/io/ebean/annotation/MutationDetection.java. While the DEFAULT option's documentation specifies different behaviour for Map, the rest of the options did not mention it, leading to the possible assumption that it only works differently in DEFAULT. I can document my findings in MutationDetection.java javadoc with an ebean-annotation PR if you find this agreeable.

Thank you for taking the time to look into this issue.