From 0d511d51a6eb6ae59a7ca6bdd0334bbf97d097e6 Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Tue, 25 Aug 2020 09:22:15 +1200 Subject: [PATCH] #2044 - Using @AttributeOverrides breaking generated SQL (when override nullable and no column name) --- .../deploy/BeanEmbeddedMetaFactory.java | 27 +++---- .../server/deploy/BeanProperty.java | 5 +- .../server/deploy/BeanPropertyOverride.java | 12 +++- .../deploy/meta/DeployBeanEmbedded.java | 9 ++- .../server/deploy/parse/AnnotationParser.java | 5 +- src/test/ddl-review/h2-create-all.sql | 15 +++- src/test/ddl-review/h2-drop-all.sql | 4 +- .../org/tests/model/embedded/EPerson2.java | 71 +++++++++++++++++++ .../TestEmbeddedNullableOverride.java | 33 +++++++++ 9 files changed, 154 insertions(+), 27 deletions(-) create mode 100644 src/test/java/org/tests/model/embedded/EPerson2.java create mode 100644 src/test/java/org/tests/model/embedded/TestEmbeddedNullableOverride.java diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanEmbeddedMetaFactory.java b/src/main/java/io/ebeaninternal/server/deploy/BeanEmbeddedMetaFactory.java index 2dd07b2a6..58f75838a 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanEmbeddedMetaFactory.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanEmbeddedMetaFactory.java @@ -3,6 +3,7 @@ package io.ebeaninternal.server.deploy; import io.ebean.config.BeanNotRegisteredException; import io.ebeaninternal.server.deploy.meta.DeployBeanPropertyAssocOne; +import javax.persistence.Column; import java.util.Map; /** @@ -15,7 +16,6 @@ class BeanEmbeddedMetaFactory { * Create BeanProperties for embedded beans using the deployment specific DB column name and table alias. */ public static BeanEmbeddedMeta create(BeanDescriptorMap owner, DeployBeanPropertyAssocOne prop) { - // we can get a BeanDescriptor for an Embedded bean // and know that it is NOT recursive, as Embedded beans are // only allow to hold simple scalar types... @@ -28,23 +28,17 @@ class BeanEmbeddedMetaFactory { // deployment override information (column names) String columnPrefix = prop.getColumnPrefix(); - Map propColMap = prop.getDeployEmbedded().getPropertyColumnMap(); + Map propColMap = prop.getDeployEmbedded().getPropertyColumnMap(); BeanProperty[] sourceProperties = targetDesc.propertiesNonTransient(); BeanProperty[] embeddedProperties = new BeanProperty[sourceProperties.length]; for (int i = 0; i < sourceProperties.length; i++) { String propertyName = sourceProperties[i].getName(); - String dbColumn = propColMap.get(propertyName); - if (dbColumn == null) { - // dbColumn not overridden so take original - dbColumn = sourceProperties[i].getDbColumn(); - if (columnPrefix != null) { - dbColumn = columnPrefix + dbColumn; - } - } - - BeanPropertyOverride overrides = new BeanPropertyOverride(dbColumn); + Column column = propColMap.get(propertyName); + String dbColumn = dbColumn(columnPrefix, column, sourceProperties[i]); + boolean dbNullable = dbNullable(column, sourceProperties[i]); + BeanPropertyOverride overrides = new BeanPropertyOverride(dbColumn, dbNullable); if (sourceProperties[i] instanceof BeanPropertyAssocOne) { embeddedProperties[i] = new BeanPropertyAssocOne((BeanPropertyAssocOne)sourceProperties[i], overrides); } else { @@ -54,4 +48,13 @@ class BeanEmbeddedMetaFactory { return new BeanEmbeddedMeta(embeddedProperties); } + + private static String dbColumn(String prefix, Column override, BeanProperty source) { + String dbCol = (override != null && !override.name().isEmpty()) ? override.name() : source.getDbColumn(); + return prefix == null ? dbCol : prefix + dbCol; + } + + private static boolean dbNullable(Column override, BeanProperty source) { + return (override != null && !override.nullable()) ? override.nullable() : source.isNullable(); + } } diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java b/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java index 454d17e22..e422e3090 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java @@ -274,7 +274,6 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty { } public BeanProperty(BeanDescriptor descriptor, DeployBeanProperty deploy) { - this.descriptor = descriptor; this.name = InternString.intern(deploy.getName()); this.propertyIndex = deploy.getPropertyIndex(); @@ -382,9 +381,10 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty { public BeanProperty(BeanProperty source, BeanPropertyOverride override) { this.descriptor = source.descriptor; - this.name = InternString.intern(source.getName()); this.propertyIndex = source.propertyIndex; + this.name = source.getName(); this.dbColumn = InternString.intern(override.getDbColumn()); + this.nullable = override.isDbNullable(); // override with sqlFormula not currently supported this.sqlFormulaJoin = null; this.sqlFormulaSelect = null; @@ -418,7 +418,6 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty { this.dbRead = source.isDbRead(); this.dbInsertable = source.isDbInsertable(); this.dbUpdatable = source.isDbUpdatable(); - this.nullable = source.isNullable(); this.unique = source.isUnique(); this.naturalKey = source.isNaturalKey(); this.dbLength = source.getDbLength(); diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyOverride.java b/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyOverride.java index 2baadd46d..43b0955a9 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyOverride.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyOverride.java @@ -12,16 +12,22 @@ import io.ebeaninternal.server.core.InternString; class BeanPropertyOverride { private final String dbColumn; + private final boolean dbNullable; - BeanPropertyOverride(String dbColumn) { + BeanPropertyOverride(String dbColumn, boolean dbNullable) { this.dbColumn = InternString.intern(dbColumn); + this.dbNullable = dbNullable; } - public String getDbColumn() { + String getDbColumn() { return dbColumn; } - public String replace(String src, String srcDbColumn) { + boolean isDbNullable() { + return dbNullable; + } + + String replace(String src, String srcDbColumn) { return StringHelper.replaceString(src, srcDbColumn, dbColumn); } } diff --git a/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanEmbedded.java b/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanEmbedded.java index 3d2f6b25f..72b541cef 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanEmbedded.java +++ b/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanEmbedded.java @@ -1,5 +1,6 @@ package io.ebeaninternal.server.deploy.meta; +import javax.persistence.Column; import java.util.HashMap; import java.util.Map; @@ -8,28 +9,26 @@ import java.util.Map; *

* Typically collects the overridden column names mapped * to the Embedded bean. - *

*/ public class DeployBeanEmbedded { /** * A map of property names to dbColumns. */ - private final Map propMap = new HashMap<>(); + private final Map propMap = new HashMap<>(); /** * Set a Map of property names to dbColumns. */ - public void putAll(Map propertyColumnMap) { + public void putAll(Map propertyColumnMap) { propMap.putAll(propertyColumnMap); } /** * Return a map of property names to dbColumns. */ - public Map getPropertyColumnMap() { + public Map getPropertyColumnMap() { return propMap; } - } diff --git a/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationParser.java b/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationParser.java index bb8158eb0..8646aa62b 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationParser.java +++ b/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationParser.java @@ -11,6 +11,7 @@ import javax.persistence.CascadeType; import javax.persistence.Column; import javax.validation.groups.Default; import java.util.HashMap; +import java.util.Map; import java.util.Set; import java.util.UUID; @@ -89,9 +90,9 @@ public abstract class AnnotationParser extends AnnotationBase { Set attrOverrides = annotationAttributeOverrides(prop); if (!attrOverrides.isEmpty()) { - HashMap propMap = new HashMap<>(attrOverrides.size()); + Map propMap = new HashMap<>(attrOverrides.size()); for (AttributeOverride attrOverride : attrOverrides) { - propMap.put(attrOverride.name(), attrOverride.column().name()); + propMap.put(attrOverride.name(), attrOverride.column()); } prop.getDeployEmbedded().putAll(propMap); } diff --git a/src/test/ddl-review/h2-create-all.sql b/src/test/ddl-review/h2-create-all.sql index f9d6631ce..a9cb06e29 100644 --- a/src/test/ddl-review/h2-create-all.sql +++ b/src/test/ddl-review/h2-create-all.sql @@ -1,4 +1,4 @@ --- Generated by ebean unknown at 2020-08-24T21:06:38.731290Z +-- Generated by ebean unknown at 2020-08-24T21:11:54.586122Z create table asimple_bean ( id bigint generated by default as identity not null, name varchar(255), @@ -1430,6 +1430,19 @@ create table eperson ( constraint pk_eperson primary key (id) ); +create table eperson2 ( + id bigint generated by default as identity not null, + name varchar(255), + notes varchar(255), + street varchar(255), + suburb varchar(255), + city varchar(255) not null, + status varchar(3) not null, + version bigint not null, + constraint ck_eperson2_status check ( status in ('ONE','TWO')), + constraint pk_eperson2 primary key (id) +); + create table e_person_online ( id bigint generated by default as identity not null, email varchar(127), diff --git a/src/test/ddl-review/h2-drop-all.sql b/src/test/ddl-review/h2-drop-all.sql index b83a78fa6..8aeadc446 100644 --- a/src/test/ddl-review/h2-drop-all.sql +++ b/src/test/ddl-review/h2-drop-all.sql @@ -1,4 +1,4 @@ --- Generated by ebean unknown at 2020-08-24T21:06:38.731290Z +-- Generated by ebean unknown at 2020-08-24T21:11:54.586122Z alter table bar drop constraint if exists fk_bar_foo_id; drop index if exists ix_bar_foo_id; @@ -1189,6 +1189,8 @@ drop table if exists eper_addr; drop table if exists eperson; +drop table if exists eperson2; + drop table if exists e_person_online; drop table if exists esimple; diff --git a/src/test/java/org/tests/model/embedded/EPerson2.java b/src/test/java/org/tests/model/embedded/EPerson2.java new file mode 100644 index 000000000..d955c9864 --- /dev/null +++ b/src/test/java/org/tests/model/embedded/EPerson2.java @@ -0,0 +1,71 @@ +package org.tests.model.embedded; + +import javax.persistence.AttributeOverride; +import javax.persistence.AttributeOverrides; +import javax.persistence.Column; +import javax.persistence.Embedded; +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.Version; + +@Entity +public class EPerson2 { + + @Id + long id; + + @Version + long version; + + String name; + + String notes; + + @Embedded + @AttributeOverrides({ + @AttributeOverride(name = "city", column = @Column(nullable = false)), + @AttributeOverride(name = "status", column = @Column(nullable = false)) + }) + EAddress address; + + public long getId() { + return id; + } + + public void setId(long id) { + this.id = id; + } + + public long getVersion() { + return version; + } + + public void setVersion(long version) { + this.version = version; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public String getNotes() { + return notes; + } + + public void setNotes(String notes) { + this.notes = notes; + } + + public EAddress getAddress() { + return address; + } + + public void setAddress(EAddress address) { + this.address = address; + } + +} diff --git a/src/test/java/org/tests/model/embedded/TestEmbeddedNullableOverride.java b/src/test/java/org/tests/model/embedded/TestEmbeddedNullableOverride.java new file mode 100644 index 000000000..2b19bc612 --- /dev/null +++ b/src/test/java/org/tests/model/embedded/TestEmbeddedNullableOverride.java @@ -0,0 +1,33 @@ +package org.tests.model.embedded; + +import io.ebean.BaseTestCase; +import io.ebean.DB; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertNotNull; + +public class TestEmbeddedNullableOverride extends BaseTestCase { + + @Test + public void test() { + + EPerson2 person = new EPerson2(); + person.setName("foo"); + + EAddress address = new EAddress(); + address.setCity("myCity"); + address.setStatus(EAddressStatus.TWO); + person.setAddress(address); + + DB.save(person); + + final EPerson2 found = DB.find(EPerson2.class, person.getId()); + + assertNotNull(found); + assertNotNull(found.getAddress()); + assertThat(found.getAddress().getCity()).isEqualTo("myCity"); + assertThat(found.getAddress().getStatus()).isEqualTo(EAddressStatus.TWO); + assertThat(found.getAddress().getStreet()).isNull(); + } +}