From 1837c3443d3e8a0b0fb8528d7d1168ef1c22d69b Mon Sep 17 00:00:00 2001 From: Robin Bygrave Date: Wed, 17 Mar 2021 21:02:59 +1300 Subject: [PATCH] #2186 - Invalid generated table and column names of @ManyToMany relation with allQuotedIdentifiers=true --- .../config/AbstractNamingConvention.java | 47 ++++++++++++---- .../config/MatchingNamingConvention.java | 5 -- .../io/ebean/config/NamingConvention.java | 22 ++++---- .../main/java/io/ebean/config/TableName.java | 24 +++----- .../config/UnderscoreNamingConvention.java | 12 ---- .../deploy/parse/AnnotationAssocManys.java | 29 ++++------ .../config/MatchingNamingConventionTest.java | 55 ++++++++++++++++++- .../java/org/tests/config/TestTableName.java | 20 +++++++ 8 files changed, 138 insertions(+), 76 deletions(-) diff --git a/ebean-api/src/main/java/io/ebean/config/AbstractNamingConvention.java b/ebean-api/src/main/java/io/ebean/config/AbstractNamingConvention.java index 9bb65ccce..33d607e97 100644 --- a/ebean-api/src/main/java/io/ebean/config/AbstractNamingConvention.java +++ b/ebean-api/src/main/java/io/ebean/config/AbstractNamingConvention.java @@ -7,6 +7,8 @@ import javax.persistence.DiscriminatorValue; import javax.persistence.Inheritance; import javax.persistence.Table; +import static io.ebean.util.StringHelper.isNull; + /** * Provides some base implementation for NamingConventions. * @@ -78,10 +80,16 @@ public abstract class AbstractNamingConvention implements NamingConvention { @Override public String getSequenceName(String rawTableName, String pkColumn) { - final String tableNameUnquoted = databasePlatform.unQuote(rawTableName); + TableName tableName = new TableName(rawTableName); + String seqName = seqName(pkColumn, tableName.getName()); + return tableName.withCatalogAndSchema(seqName); + } + + private String seqName(String pkColumn, String tableName) { + final String tableNameUnquoted = unQuote(tableName); String seqName = sequenceFormat.replace("{table}", tableNameUnquoted); - pkColumn = (pkColumn == null) ? "" : databasePlatform.unQuote(pkColumn); - return seqName.replace("{column}", pkColumn); + pkColumn = (pkColumn == null) ? "" : unQuote(pkColumn); + return quoteIdentifiers(seqName.replace("{column}", pkColumn)); } /** @@ -216,15 +224,13 @@ public abstract class AbstractNamingConvention implements NamingConvention { || AnnotationUtil.has(supCls, DiscriminatorValue.class); } - @Override public TableName getM2MJoinTableName(TableName lhsTable, TableName rhsTable) { - StringBuilder buffer = new StringBuilder(); - buffer.append(lhsTable.getName()); + buffer.append(unQuote(lhsTable.getName())); buffer.append("_"); - String rhsTableName = rhsTable.getName(); + String rhsTableName = unQuote(rhsTable.getName()); if (rhsTableName.indexOf('_') < rhsPrefixLength) { // trim off a xx_ prefix if there is one rhsTableName = rhsTableName.substring(rhsTableName.indexOf('_') + 1); @@ -238,7 +244,13 @@ public abstract class AbstractNamingConvention implements NamingConvention { buffer.setLength(maxTableNameLength); } - return new TableName(lhsTable.getCatalog(), lhsTable.getSchema(), buffer.toString()); + String tableName = quoteIdentifiers(buffer.toString()); + return new TableName(lhsTable.getCatalog(), lhsTable.getSchema(), tableName); + } + + @Override + public String deriveM2MColumn(String tableName, String dbColumn) { + return quoteIdentifiers(unQuote(tableName) +"_" + unQuote(dbColumn)); } /** @@ -255,14 +267,29 @@ public abstract class AbstractNamingConvention implements NamingConvention { return null; } + @Override + public String getTableName(String catalog, String schema, String name) { + StringBuilder sb = new StringBuilder(); + if (!isNull(catalog)) { + sb.append(quoteIdentifiers(catalog)).append("."); + } + if (!isNull(schema)) { + sb.append(quoteIdentifiers(schema)).append("."); + } + return sb.append(quoteIdentifiers(name)).toString(); + } + /** - * Replace back ticks (if they are used) with database platform specific - * quoted identifiers. + * Replace back ticks (if they are used) with database platform specific quoted identifiers. */ protected String quoteIdentifiers(String s) { return databasePlatform.convertQuotedIdentifiers(s); } + private String unQuote(String val) { + return databasePlatform.unQuote(val); + } + /** * Checks string is null or empty . */ diff --git a/ebean-api/src/main/java/io/ebean/config/MatchingNamingConvention.java b/ebean-api/src/main/java/io/ebean/config/MatchingNamingConvention.java index 0b741994a..d12cb2e80 100644 --- a/ebean-api/src/main/java/io/ebean/config/MatchingNamingConvention.java +++ b/ebean-api/src/main/java/io/ebean/config/MatchingNamingConvention.java @@ -39,11 +39,6 @@ public class MatchingNamingConvention extends AbstractNamingConvention { return new TableName(quoteIdentifiers(getCatalog()), quoteIdentifiers(getSchema()), quoteIdentifiers(beanClass.getSimpleName())); } - @Override - public String getPropertyFromColumn(Class beanClass, String dbColumnName) { - return dbColumnName; - } - @Override public String getForeignKey(String prefix, String fkProperty) { prefix = databasePlatform.unQuote(prefix); diff --git a/ebean-api/src/main/java/io/ebean/config/NamingConvention.java b/ebean-api/src/main/java/io/ebean/config/NamingConvention.java index c5a0a2b86..470822e5b 100644 --- a/ebean-api/src/main/java/io/ebean/config/NamingConvention.java +++ b/ebean-api/src/main/java/io/ebean/config/NamingConvention.java @@ -53,6 +53,16 @@ public interface NamingConvention { */ TableName getM2MJoinTableName(TableName lhsTable, TableName rhsTable); + /** + * Derive a DB Column from a FK table and column. + */ + String deriveM2MColumn(String tableName, String dbColumn); + + /** + * Return the full table name taking into account quoted identifiers. + */ + String getTableName(String catalog, String schema, String name); + /** * Return the column name given the property name. * @@ -60,18 +70,6 @@ public interface NamingConvention { */ String getColumnFromProperty(Class beanClass, String propertyName); - /** - * Return the property name from the column name. - *

- * This is used to help mapping of raw SQL queries onto bean properties. - *

- * - * @param beanClass the bean class - * @param dbColumnName the db column name - * @return the property name from the column name - */ - String getPropertyFromColumn(Class beanClass, String dbColumnName); - /** * Return the sequence name given the table name (for DB's that use sequences). *

diff --git a/ebean-api/src/main/java/io/ebean/config/TableName.java b/ebean-api/src/main/java/io/ebean/config/TableName.java index dba8b1d0d..9f55577b3 100644 --- a/ebean-api/src/main/java/io/ebean/config/TableName.java +++ b/ebean-api/src/main/java/io/ebean/config/TableName.java @@ -20,7 +20,7 @@ public final class TableName { /** * The name. */ - private String name; + private final String name; /** * Construct with the given catalog schema and table name. @@ -29,7 +29,6 @@ public final class TableName { *

*/ public TableName(String catalog, String schema, String name) { - super(); this.catalog = catalog != null ? catalog.trim() : null; this.schema = schema != null ? schema.trim() : null; this.name = name != null ? name.trim() : null; @@ -110,14 +109,11 @@ public final class TableName { * @return the qualified name */ public String getQualifiedName() { - StringBuilder buffer = new StringBuilder(); - // Add catalog if (catalog != null) { buffer.append(catalog); } - // Add schema if (schema != null) { if (buffer.length() > 0) { @@ -125,31 +121,27 @@ public final class TableName { } buffer.append(schema); } - if (buffer.length() > 0) { buffer.append("."); } - buffer.append(name); - - return buffer.toString(); + return buffer.append(name).toString(); } /** * Append a catalog and schema prefix if they exist to the string builder. */ - public void appendCatalogAndSchema(StringBuilder buffer) { - if (catalog != null) { - buffer.append(catalog).append("."); - } + public String withCatalogAndSchema(String name) { if (schema != null) { - buffer.append(schema).append("."); + name = schema + "." + name; } + if (catalog != null) { + name = catalog + "." + name; + } + return name; } /** * Checks if is table name is valid i.e. it has at least a name. - * - * @return true, if is valid */ public boolean isValid() { return name != null && !name.isEmpty(); diff --git a/ebean-api/src/main/java/io/ebean/config/UnderscoreNamingConvention.java b/ebean-api/src/main/java/io/ebean/config/UnderscoreNamingConvention.java index 16b685227..603e4e76c 100644 --- a/ebean-api/src/main/java/io/ebean/config/UnderscoreNamingConvention.java +++ b/ebean-api/src/main/java/io/ebean/config/UnderscoreNamingConvention.java @@ -60,18 +60,6 @@ public class UnderscoreNamingConvention extends AbstractNamingConvention { return toUnderscoreFromCamel(propertyName); } - /** - * Converts underscore based column name to Camel case property name. - * - * @param beanClass the bean class - * @param dbColumnName the db column name - * @return the property from column - */ - @Override - public String getPropertyFromColumn(Class beanClass, String dbColumnName) { - return toCamelFromUnderscore(dbColumnName); - } - /** * Return true if the result will be upper case. *

diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationAssocManys.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationAssocManys.java index b3346562b..b47612d65 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationAssocManys.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/parse/AnnotationAssocManys.java @@ -38,8 +38,6 @@ import javax.persistence.OrderBy; import javax.persistence.OrderColumn; import java.util.Set; -import static io.ebean.util.StringHelper.isNull; - /** * Read the deployment annotation for Assoc Many beans. */ @@ -346,18 +344,6 @@ class AnnotationAssocManys extends AnnotationAssoc { return append(joinTable.catalog(), joinTable.schema(), joinTable.name()); } - private String append(String catalog, String schema, String name) { - StringBuilder sb = new StringBuilder(); - if (!isNull(catalog)) { - sb.append(catalog).append("."); - } - if (!isNull(schema)) { - sb.append(schema).append("."); - } - sb.append(name); - return sb.toString(); - } - /** * Return the full table name */ @@ -368,6 +354,13 @@ class AnnotationAssocManys extends AnnotationAssoc { return append(collectionTable.catalog(), collectionTable.schema(), collectionTable.name()); } + /** + * Return the full table name taking into account quoted identifiers. + */ + private String append(String catalog, String schema, String name) { + return namingConvention.getTableName(catalog, schema, name); + } + /** * Define intersection table and foreign key columns for ManyToMany. *

@@ -414,8 +407,8 @@ class AnnotationAssocManys extends AnnotationAssoc { BeanProperty localId = localTable.getIdProperty(); if (localId != null) { // add the source to intersection join columns - String fkCol = localTableName + "_" + localId.getDbColumn(); - intJoin.addJoinColumn(new DeployTableJoinColumn(localId.getDbColumn(), namingConvention.getColumnFromProperty(null, fkCol))); + String fkCol = namingConvention.deriveM2MColumn(localTableName, localId.getDbColumn()); + intJoin.addJoinColumn(new DeployTableJoinColumn(localId.getDbColumn(), fkCol)); } } @@ -424,8 +417,8 @@ class AnnotationAssocManys extends AnnotationAssoc { BeanProperty otherId = otherTable.getIdProperty(); if (otherId != null) { // set the intersection to dest table join columns - final String fkCol = otherTableName + "_" + otherId.getDbColumn(); - destJoin.addJoinColumn(new DeployTableJoinColumn(namingConvention.getColumnFromProperty(null, fkCol), otherId.getDbColumn())); + String fkCol = namingConvention.deriveM2MColumn(otherTableName, otherId.getDbColumn()); + destJoin.addJoinColumn(new DeployTableJoinColumn(fkCol, otherId.getDbColumn())); } } diff --git a/ebean-core/src/test/java/io/ebean/config/MatchingNamingConventionTest.java b/ebean-core/src/test/java/io/ebean/config/MatchingNamingConventionTest.java index c9a2fe030..2db6b95ba 100644 --- a/ebean-core/src/test/java/io/ebean/config/MatchingNamingConventionTest.java +++ b/ebean-core/src/test/java/io/ebean/config/MatchingNamingConventionTest.java @@ -11,7 +11,7 @@ import static org.junit.Assert.assertNull; public class MatchingNamingConventionTest { - private MatchingNamingConvention namingConvention; + private final MatchingNamingConvention namingConvention; public MatchingNamingConventionTest() { this.namingConvention = new MatchingNamingConvention(); @@ -30,6 +30,50 @@ public class MatchingNamingConventionTest { return nc; } + @Test + public void getTableName() { + assertThat(namingConvention.getTableName("a", "b", "c")).isEqualTo("a.b.c"); + assertThat(namingConvention.getTableName("", "b", "c")).isEqualTo("b.c"); + assertThat(namingConvention.getTableName("", "", "c")).isEqualTo("c"); + assertThat(namingConvention.getTableName("a", "", "c")).isEqualTo("a.c"); + } + + @Test + public void getTableName_when_allQuoted() { + MatchingNamingConvention nc = createMatchingNamingConventionAllQuoted(); + + assertThat(nc.getTableName("a", "b", "c")).isEqualTo("[a].[b].[c]"); + assertThat(nc.getTableName("", "b", "c")).isEqualTo("[b].[c]"); + assertThat(nc.getTableName("", "", "c")).isEqualTo("[c]"); + assertThat(nc.getTableName("a", "", "c")).isEqualTo("[a].[c]"); + } + + @Test + public void getM2MJoinTableName() { + TableName t0 = new TableName("One"); + TableName t1 = new TableName("Two"); + assertThat(namingConvention.getM2MJoinTableName(t0, t1).toString()).isEqualTo("One_Two"); + } + + @Test + public void getM2MJoinTableName_when_allQuoted() { + MatchingNamingConvention nc = createMatchingNamingConventionAllQuoted(); + TableName t0 = new TableName("[One]"); + TableName t1 = new TableName("[Two]"); + assertThat(nc.getM2MJoinTableName(t0, t1).toString()).isEqualTo("[One_Two]"); + } + + @Test + public void deriveM2MColumn() { + assertThat(namingConvention.deriveM2MColumn("One", "Two")).isEqualTo("One_Two"); + } + + @Test + public void deriveM2MColumn_when_allQuoted() { + MatchingNamingConvention nc = createMatchingNamingConventionAllQuoted(); + assertThat(nc.deriveM2MColumn("[One]", "[Two]")).isEqualTo("[One_Two]"); + } + @Test public void getColumnFromProperty_when_allQuoted() { @@ -50,11 +94,16 @@ public class MatchingNamingConventionTest { assertNull(tableName.getSchema()); } - @Test public void getSequenceName() { MatchingNamingConvention nc = createMatchingNamingConventionAllQuoted(); - assertEquals("Customer_seq", nc.getSequenceName("[Customer]", null)); + assertThat(nc.getSequenceName("[Customer]", null)).isEqualTo("[Customer_seq]"); + } + + @Test + public void getSequenceName_when_quotedSchema() { + MatchingNamingConvention nc = createMatchingNamingConventionAllQuoted(); + assertThat(nc.getSequenceName("[dbo].[Customer]", null)).isEqualTo("[dbo].[Customer_seq]"); } @Test diff --git a/ebean-core/src/test/java/org/tests/config/TestTableName.java b/ebean-core/src/test/java/org/tests/config/TestTableName.java index f42e98c09..83fefa08b 100644 --- a/ebean-core/src/test/java/org/tests/config/TestTableName.java +++ b/ebean-core/src/test/java/org/tests/config/TestTableName.java @@ -5,8 +5,28 @@ import io.ebean.config.TableName; import org.junit.Assert; import org.junit.Test; +import static org.assertj.core.api.Assertions.assertThat; + public class TestTableName extends BaseTestCase { + @Test + public void withCatalogAndSchema() { + TableName t = new TableName("a.b.c"); + assertThat(t.withCatalogAndSchema("foo")).isEqualTo("a.b.foo"); + } + + @Test + public void withCatalogAndSchema_when_quoted() { + TableName t = new TableName("[a].[b].[c]"); + assertThat(t.withCatalogAndSchema("foo")).isEqualTo("[a].[b].foo"); + + TableName noCat = new TableName("[b].[c]"); + assertThat(noCat.withCatalogAndSchema("foo")).isEqualTo("[b].foo"); + + TableName tabOnly = new TableName("[c]"); + assertThat(tabOnly.withCatalogAndSchema("foo")).isEqualTo("foo"); + } + @Test public void test() {