From 4e088a40d9d6fb6df72bbe45e944dc6611c6ea0b Mon Sep 17 00:00:00 2001 From: rbygrave Date: Fri, 11 Jun 2021 00:08:03 +1200 Subject: [PATCH] Refactor DbJson Jackson handling adding DatabaseConfig.setJsonDirtyByDefault() Adds the ability to change the default "jsonDirtyByDefault" configuration setting used with DbJson Jackson properties. Currently these default to being assumed dirty and this allows us to change that to be assumed not dirty (which is very likely the better default). --- .../java/io/ebean/config/DatabaseConfig.java | 29 +++++- .../server/type/CheckMarkedDirty.java | 26 ----- .../server/type/DefaultTypeManager.java | 7 +- .../server/type/ScalarTypeJsonCollection.java | 2 +- .../server/type/ScalarTypeJsonMap.java | 2 +- .../type/ScalarTypeJsonObjectMapper.java | 67 +++++-------- .../server/type/ScalarTypePostgresHstore.java | 2 +- .../server/type/TypeJsonManager.java | 98 +++++++++++++++++++ .../io/ebean/config/ServerConfigTest.java | 5 + .../org/tests/json/TestDbJson_Jackson3.java | 3 + 10 files changed, 166 insertions(+), 75 deletions(-) delete mode 100644 ebean-core/src/main/java/io/ebeaninternal/server/type/CheckMarkedDirty.java create mode 100644 ebean-core/src/main/java/io/ebeaninternal/server/type/TypeJsonManager.java diff --git a/ebean-api/src/main/java/io/ebean/config/DatabaseConfig.java b/ebean-api/src/main/java/io/ebean/config/DatabaseConfig.java index be2ac6c1f..bc3ae587f 100644 --- a/ebean-api/src/main/java/io/ebean/config/DatabaseConfig.java +++ b/ebean-api/src/main/java/io/ebean/config/DatabaseConfig.java @@ -193,6 +193,12 @@ public class DatabaseConfig { */ private JsonConfig.Include jsonInclude = JsonConfig.Include.ALL; + /** + * When true then by default DbJson beans are assumed to be dirty. + * I believe we want to change this default to false in the future. + */ + private boolean jsonDirtyByDefault = true; + /** * The database platform name. Used to imply a DatabasePlatform to use. */ @@ -737,6 +743,26 @@ public class DatabaseConfig { this.jsonInclude = jsonInclude; } + /** + * Return true if DbJson beans are assumed dirty by default. + *

+ * That is, when true beans that do not implement ModifyAwareType are by + * default assumed to be dirty and included in updates. + */ + public boolean isJsonDirtyByDefault() { + return jsonDirtyByDefault; + } + + /** + * Set to false if we want DbJson beans to not be assumed to be dirty. + *

+ * That is, when true beans that do not implement ModifyAwareType are by + * default assumed to be dirty and included in updates. + */ + public void setJsonDirtyByDefault(boolean jsonDirtyByDefault) { + this.jsonDirtyByDefault = jsonDirtyByDefault; + } + /** * Return the name of the Database. */ @@ -2909,6 +2935,7 @@ public class DatabaseConfig { jsonInclude = p.getEnum(JsonConfig.Include.class, "jsonInclude", jsonInclude); jsonDateTime = p.getEnum(JsonConfig.DateTime.class, "jsonDateTime", jsonDateTime); jsonDate = p.getEnum(JsonConfig.Date.class, "jsonDate", jsonDate); + jsonDirtyByDefault = p.getBoolean("jsonDirtyByDefault", jsonDirtyByDefault); runMigration = p.getBoolean("migration.run", runMigration); ddlGenerate = p.getBoolean("ddl.generate", ddlGenerate); @@ -3369,7 +3396,7 @@ public class DatabaseConfig { this.loadModuleInfo = loadModuleInfo; } - public enum UuidVersion { + public enum UuidVersion { VERSION4, VERSION1, VERSION1RND diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/CheckMarkedDirty.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/CheckMarkedDirty.java deleted file mode 100644 index 321a35fec..000000000 --- a/ebean-core/src/main/java/io/ebeaninternal/server/type/CheckMarkedDirty.java +++ /dev/null @@ -1,26 +0,0 @@ -package io.ebeaninternal.server.type; - -import io.ebean.ModifyAwareType; - -/** - * Check dirty state of json value which might be modify aware. - */ -class CheckMarkedDirty { - - /** - * Return true if the value should be considered dirty (and included in an update). - */ - static boolean isDirty(Object value) { - if (value instanceof ModifyAwareType) { - ModifyAwareType modifyAware = (ModifyAwareType) value; - if (modifyAware.isMarkedDirty()) { - // reset the dirty state (consider not dirty after update) - modifyAware.setMarkedDirty(false); - return true; - } else { - return false; - } - } - return true; - } -} diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/DefaultTypeManager.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/DefaultTypeManager.java index 5f8a64699..82c3ff46a 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/type/DefaultTypeManager.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/type/DefaultTypeManager.java @@ -92,6 +92,7 @@ public final class DefaultTypeManager implements TypeManager { private final Object objectMapper; private final boolean objectMapperPresent; private final boolean postgres; + private final TypeJsonManager jsonManager; private final boolean offlineMigrationGeneration; private final EnumType defaultEnumType; @@ -131,11 +132,11 @@ public final class DefaultTypeManager implements TypeManager { this.typeMap = new ConcurrentHashMap<>(); this.nativeMap = new ConcurrentHashMap<>(); this.logicalMap = new ConcurrentHashMap<>(); + this.postgres = isPostgres(config.getDatabasePlatform()); this.objectMapperPresent = config.getClassLoadConfig().isJacksonObjectMapperPresent(); this.objectMapper = (objectMapperPresent) ? initObjectMapper(config) : null; - + this.jsonManager = (objectMapperPresent) ? new TypeJsonManager(postgres, objectMapper, config.isJsonDirtyByDefault()) : null; this.extraTypeFactory = new DefaultTypeFactory(config); - this.postgres = isPostgres(config.getDatabasePlatform()); this.arrayTypeListFactory = arrayTypeListFactory(config.getDatabasePlatform()); this.arrayTypeSetFactory = arrayTypeSetFactory(config.getDatabasePlatform()); this.offlineMigrationGeneration = DbOffline.isGenerateMigration(); @@ -425,7 +426,7 @@ public final class DefaultTypeManager implements TypeManager { if (objectMapper == null) { throw new IllegalArgumentException("Type [" + type + "] unsupported for @DbJson mapping - Jackson ObjectMapper not present"); } - return ScalarTypeJsonObjectMapper.createTypeFor(postgres, (AnnotatedField) prop.getJacksonField(), (ObjectMapper) objectMapper, dbType, docType); + return ScalarTypeJsonObjectMapper.createTypeFor(jsonManager, (AnnotatedField) prop.getJacksonField(), dbType, docType); } /** diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonCollection.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonCollection.java index a6c51ff05..e40e1a9ae 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonCollection.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonCollection.java @@ -61,7 +61,7 @@ abstract class ScalarTypeJsonCollection extends ScalarTypeBase implements */ @Override public boolean isDirty(Object value) { - return CheckMarkedDirty.isDirty(value); + return TypeJsonManager.checkIsDirty(value); } @Override diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonMap.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonMap.java index f0714c467..6dd4f900b 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonMap.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonMap.java @@ -123,7 +123,7 @@ public abstract class ScalarTypeJsonMap extends ScalarTypeBase { */ @Override public boolean isDirty(Object value) { - return CheckMarkedDirty.isDirty(value); + return TypeJsonManager.checkIsDirty(value); } @Override diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonObjectMapper.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonObjectMapper.java index 3cf05d9c2..5e24412b5 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonObjectMapper.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypeJsonObjectMapper.java @@ -7,7 +7,6 @@ import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.ObjectWriter; import com.fasterxml.jackson.databind.introspect.AnnotatedField; -import io.ebean.config.dbplatform.DbPlatformType; import io.ebean.core.type.DataBinder; import io.ebean.core.type.DataReader; import io.ebean.core.type.DocPropertyType; @@ -30,38 +29,23 @@ import java.util.Set; /** * Supports @DbJson properties using Jackson ObjectMapper. */ -public class ScalarTypeJsonObjectMapper { +class ScalarTypeJsonObjectMapper { /** * Create and return the appropriate ScalarType. */ - public static ScalarType createTypeFor(boolean postgres, AnnotatedField field, ObjectMapper objectMapper, - int dbType, DocPropertyType docType) { - + static ScalarType createTypeFor(TypeJsonManager jsonManager, AnnotatedField field, int dbType, DocPropertyType docType) { Class type = field.getRawType(); - String pgType = getPostgresType(postgres, dbType); if (Set.class.equals(type)) { - return new OmSet(objectMapper, field, dbType, pgType, docType); + return new OmSet(jsonManager, field, dbType, docType); } if (List.class.equals(type)) { - return new OmList(objectMapper, field, dbType, pgType, docType); + return new OmList(jsonManager, field, dbType, docType); } if (Map.class.equals(type)) { - return new OmMap(objectMapper, field, dbType, pgType); + return new OmMap(jsonManager, field, dbType); } - return new GenericObject(objectMapper, field, dbType, pgType); - } - - private static String getPostgresType(boolean postgres, int dbType) { - if (postgres) { - switch (dbType) { - case DbPlatformType.JSON: - return PostgresHelper.JSON_TYPE; - case DbPlatformType.JSONB: - return PostgresHelper.JSONB_TYPE; - } - } - return null; + return new GenericObject(jsonManager, field, dbType, type); } /** @@ -69,8 +53,8 @@ public class ScalarTypeJsonObjectMapper { */ private static class GenericObject extends Base { - public GenericObject(ObjectMapper objectMapper, AnnotatedField field, int dbType, String pgType) { - super(Object.class, objectMapper, field, dbType, pgType, DocPropertyType.OBJECT); + GenericObject(TypeJsonManager jsonManager, AnnotatedField field, int dbType, Class rawType) { + super(Object.class, jsonManager, field, dbType, DocPropertyType.OBJECT, rawType); } } @@ -80,8 +64,8 @@ public class ScalarTypeJsonObjectMapper { @SuppressWarnings("rawtypes") private static class OmSet extends Base { - public OmSet(ObjectMapper objectMapper, AnnotatedField field, int dbType, String pgType, DocPropertyType docType) { - super(Set.class, objectMapper, field, dbType, pgType, docType); + OmSet(TypeJsonManager jsonManager, AnnotatedField field, int dbType, DocPropertyType docType) { + super(Set.class, jsonManager, field, dbType, docType); } @Override @@ -98,8 +82,8 @@ public class ScalarTypeJsonObjectMapper { @SuppressWarnings("rawtypes") private static class OmList extends Base { - public OmList(ObjectMapper objectMapper, AnnotatedField field, int dbType, String pgType, DocPropertyType docType) { - super(List.class, objectMapper, field, dbType, pgType, docType); + OmList(TypeJsonManager jsonManager, AnnotatedField field, int dbType, DocPropertyType docType) { + super(List.class, jsonManager, field, dbType, docType); } @Override @@ -116,8 +100,8 @@ public class ScalarTypeJsonObjectMapper { @SuppressWarnings("rawtypes") private static class OmMap extends Base { - public OmMap(ObjectMapper objectMapper, AnnotatedField field, int dbType, String pgType) { - super(Map.class, objectMapper, field, dbType, pgType, DocPropertyType.OBJECT); + OmMap(TypeJsonManager jsonManager, AnnotatedField field, int dbType) { + super(Map.class, jsonManager, field, dbType, DocPropertyType.OBJECT); } @Override @@ -135,24 +119,23 @@ public class ScalarTypeJsonObjectMapper { private static abstract class Base extends ScalarTypeBase { private final ObjectWriter objectWriter; - private final ObjectMapper objectReader; - private final JavaType deserType; - private final String pgType; - private final DocPropertyType docType; + private final TypeJsonManager.DirtyHandler dirtyHandler; - /** - * Construct given the object mapper, property type and DB type for storage. - */ - public Base(Class cls, ObjectMapper objectMapper, AnnotatedField field, int dbType, String pgType, DocPropertyType docType) { + Base(Class cls, TypeJsonManager jsonManager, AnnotatedField field, int dbType, DocPropertyType docType) { + this(cls, jsonManager, field, dbType, docType, cls); + } + + Base(Class cls, TypeJsonManager jsonManager, AnnotatedField field, int dbType, DocPropertyType docType, Class rawType) { super(cls, false, dbType); - this.pgType = pgType; + this.objectReader = jsonManager.objectMapper(); + this.pgType = jsonManager.postgresType(dbType); this.docType = docType; - this.objectReader = objectMapper; - final JacksonTypeHelper helper = new JacksonTypeHelper(field, objectMapper); + this.dirtyHandler = jsonManager.dirtyHandler(cls, rawType); + final JacksonTypeHelper helper = new JacksonTypeHelper(field, objectReader); this.deserType = helper.type(); this.objectWriter = helper.objectWriter(); } @@ -170,7 +153,7 @@ public class ScalarTypeJsonObjectMapper { */ @Override public boolean isDirty(Object value) { - return CheckMarkedDirty.isDirty(value); + return dirtyHandler.isDirty(value); } @Override diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypePostgresHstore.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypePostgresHstore.java index 810504411..14d2c50d3 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypePostgresHstore.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/type/ScalarTypePostgresHstore.java @@ -33,7 +33,7 @@ public class ScalarTypePostgresHstore extends ScalarTypeBase { @Override public boolean isDirty(Object value) { - return CheckMarkedDirty.isDirty(value); + return TypeJsonManager.checkIsDirty(value); } @SuppressWarnings("unchecked") diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/type/TypeJsonManager.java b/ebean-core/src/main/java/io/ebeaninternal/server/type/TypeJsonManager.java new file mode 100644 index 000000000..baea26dec --- /dev/null +++ b/ebean-core/src/main/java/io/ebeaninternal/server/type/TypeJsonManager.java @@ -0,0 +1,98 @@ +package io.ebeaninternal.server.type; + +import com.fasterxml.jackson.databind.ObjectMapper; +import io.ebean.ModifyAwareType; +import io.ebean.config.DatabaseConfig; +import io.ebean.config.dbplatform.DbPlatformType; + +class TypeJsonManager { + + interface DirtyHandler { + boolean isDirty(Object value); + } + + private final boolean postgres; + private final ObjectMapper objectMapper; + private final DirtyHandler defaultHandler; + private final DirtyHandler modifyAwareHandler; + + TypeJsonManager(boolean postgres, Object objectMapper, boolean defaultDirty) { + this.postgres = postgres; + this.objectMapper = (ObjectMapper) objectMapper; + this.defaultHandler = new DefaultHandler(defaultDirty); + this.modifyAwareHandler = new ModifyAwareHandler(); + } + + ObjectMapper objectMapper() { + return objectMapper; + } + + String postgresType(int dbType) { + if (postgres) { + switch (dbType) { + case DbPlatformType.JSON: + return PostgresHelper.JSON_TYPE; + case DbPlatformType.JSONB: + return PostgresHelper.JSONB_TYPE; + } + } + return null; + } + + /** + * Return the DirtyHandler to use. + */ + DirtyHandler dirtyHandler(Class cls, Class rawType) { + if (!Object.class.equals(cls) || ModifyAwareType.class.isAssignableFrom(rawType)) { + // Set, List and Map are modify aware + return modifyAwareHandler; + } + return defaultHandler; + } + + /** + * Return true if the value should be considered dirty (and included in an update). + */ + static boolean checkIsDirty(Object value) { + if (value instanceof ModifyAwareType) { + return checkModifyAware(value); + } + return true; + } + + private static boolean checkModifyAware(Object value) { + ModifyAwareType modifyAware = (ModifyAwareType) value; + if (modifyAware.isMarkedDirty()) { + // reset the dirty state (consider not dirty after update) + modifyAware.setMarkedDirty(false); + return true; + } else { + return false; + } + } + + static final class ModifyAwareHandler implements DirtyHandler { + @Override + public boolean isDirty(Object value) { + return checkModifyAware(value); + } + } + + /** + * Effectively constant based on {@link DatabaseConfig#isJsonDirtyByDefault()} + */ + static final class DefaultHandler implements DirtyHandler { + + private final boolean dirty; + + DefaultHandler(boolean dirty) { + this.dirty = dirty; + } + + @Override + public boolean isDirty(Object value) { + return dirty; + } + } + +} diff --git a/ebean-core/src/test/java/io/ebean/config/ServerConfigTest.java b/ebean-core/src/test/java/io/ebean/config/ServerConfigTest.java index 4e92798d6..dde2cb216 100644 --- a/ebean-core/src/test/java/io/ebean/config/ServerConfigTest.java +++ b/ebean-core/src/test/java/io/ebean/config/ServerConfigTest.java @@ -62,6 +62,7 @@ public class ServerConfigTest { props.setProperty("dbOffline", "true"); props.setProperty("jsonDateTime", "MILLIS"); props.setProperty("jsonDate", "MILLIS"); + props.setProperty("jsonDirtyByDefault", "false"); props.setProperty("autoReadOnlyDataSource", "true"); props.setProperty("disableL2Cache", "true"); props.setProperty("notifyL2CacheInForeground", "true"); @@ -103,6 +104,9 @@ public class ServerConfigTest { assertEquals(PlatformConfig.DbUuid.BINARY, serverConfig.getPlatformConfig().getDbUuid()); assertEquals(JsonConfig.DateTime.MILLIS, serverConfig.getJsonDateTime()); assertEquals(JsonConfig.Date.MILLIS, serverConfig.getJsonDate()); + assertFalse(serverConfig.isJsonDirtyByDefault()); + serverConfig.setJsonDirtyByDefault(true); + assertTrue(serverConfig.isJsonDirtyByDefault()); assertEquals("r0,users,orgs", serverConfig.getEnabledL2Regions()); @@ -155,6 +159,7 @@ public class ServerConfigTest { assertFalse(serverConfig.isIdGeneratorAutomatic()); assertEquals(JsonConfig.DateTime.ISO8601, serverConfig.getJsonDateTime()); assertEquals(JsonConfig.Date.ISO8601, serverConfig.getJsonDate()); + assertTrue(serverConfig.isJsonDirtyByDefault()); assertTrue(serverConfig.getPlatformConfig().isCaseSensitiveCollation()); assertTrue(serverConfig.isAutoLoadModuleInfo()); diff --git a/ebean-core/src/test/java/org/tests/json/TestDbJson_Jackson3.java b/ebean-core/src/test/java/org/tests/json/TestDbJson_Jackson3.java index 74f004bca..747f01426 100644 --- a/ebean-core/src/test/java/org/tests/json/TestDbJson_Jackson3.java +++ b/ebean-core/src/test/java/org/tests/json/TestDbJson_Jackson3.java @@ -9,6 +9,7 @@ import org.tests.model.json.EBasicJsonList; import org.tests.model.json.PlainBean; import org.tests.model.json.PlainBeanDirtyAware; +import java.util.Arrays; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -59,12 +60,14 @@ public class TestDbJson_Jackson3 extends BaseTestCase { EBasicJsonList bean = new EBasicJsonList(); bean.setName("p1"); bean.setPlainBean(contentBean); + bean.setBeanList(Arrays.asList(contentBean)); DB.save(bean); final EBasicJsonList found = DB.find(EBasicJsonList.class, bean.getId()); // json bean not modified but not aware // ideally don't load the json content if we are not going to modify it found.setName("p1-mod"); + found.setBeanList(null); LoggedSql.start(); DB.save(found);