From cef626b951fa90ab3b6e5dab81fc9f42da392d64 Mon Sep 17 00:00:00 2001 From: Evgenii Koba Date: Sat, 30 Sep 2023 13:51:19 +0400 Subject: [PATCH 1/4] Add support for forein keys in schema generation within aggregates. Closes #1599 Related tickets #756, #1600 --- .../jdbc/core/mapping/schema/ForeignKey.java | 27 ++++++ .../schema/LiquibaseChangeSetWriter.java | 65 ++++++++++++- .../jdbc/core/mapping/schema/SchemaDiff.java | 46 +++++---- .../data/jdbc/core/mapping/schema/Table.java | 2 +- .../jdbc/core/mapping/schema/TableDiff.java | 5 +- .../data/jdbc/core/mapping/schema/Tables.java | 79 +++++++++++++-- ...uibaseChangeSetWriterIntegrationTests.java | 96 +++++++++++++++++++ .../LiquibaseChangeSetWriterUnitTests.java | 66 +++++++++++++ .../mapping/schema/create-fk-with-field.sql | 11 +++ .../mapping/schema/drop-and-create-fk.sql | 14 +++ .../schema/drop-and-create-table-with-fk.sql | 11 +++ 11 files changed, 386 insertions(+), 36 deletions(-) create mode 100644 spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java create mode 100644 spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/create-fk-with-field.sql create mode 100644 spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-fk.sql create mode 100644 spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-table-with-fk.sql diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java new file mode 100644 index 0000000000..d431e1529e --- /dev/null +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java @@ -0,0 +1,27 @@ +package org.springframework.data.jdbc.core.mapping.schema; + +import java.util.Objects; + +/** + * Models a Foreign Key for generating SQL for Schema generation. + * + * @author Evgenii Koba + * @since 3.2 + */ +record ForeignKey(String name, String tableName, String columnName, String referencedTableName, + String referencedColumnName) { + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + ForeignKey that = (ForeignKey) o; + return Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + return Objects.hash(name); + } +} diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java index b935127547..ac0afc567b 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java @@ -21,8 +21,10 @@ import liquibase.change.ColumnConfig; import liquibase.change.ConstraintsConfig; import liquibase.change.core.AddColumnChange; +import liquibase.change.core.AddForeignKeyConstraintChange; import liquibase.change.core.CreateTableChange; import liquibase.change.core.DropColumnChange; +import liquibase.change.core.DropForeignKeyConstraintChange; import liquibase.change.core.DropTableChange; import liquibase.changelog.ChangeLogChild; import liquibase.changelog.ChangeLogParameters; @@ -52,6 +54,7 @@ import java.util.Set; import java.util.function.BiPredicate; import java.util.function.Predicate; +import java.util.stream.Collectors; import org.springframework.core.io.Resource; import org.springframework.data.mapping.context.MappingContext; @@ -321,7 +324,7 @@ private ChangeSet createChangeSet(ChangeSetMetadata metadata, SchemaDiff differe private SchemaDiff initial() { Tables mappedEntities = Tables.from(mappingContext.getPersistentEntities().stream().filter(schemaFilter), - sqlTypeMapping, null); + sqlTypeMapping, null, mappingContext); return SchemaDiff.diff(mappedEntities, Tables.empty(), nameComparator); } @@ -329,7 +332,7 @@ private SchemaDiff differenceOf(Database database) throws LiquibaseException { Tables existingTables = getLiquibaseModel(database); Tables mappedEntities = Tables.from(mappingContext.getPersistentEntities().stream().filter(schemaFilter), - sqlTypeMapping, database.getDefaultCatalogName()); + sqlTypeMapping, database.getDefaultCatalogName(), mappingContext); return SchemaDiff.diff(mappedEntities, existingTables, nameComparator); } @@ -362,6 +365,13 @@ private DatabaseChangeLog getDatabaseChangeLog(File changeLogFile, @Nullable Dat private void generateTableAdditionsDeletions(ChangeSet changeSet, SchemaDiff difference) { + for (Table table : difference.tableDeletions()) { + for (ForeignKey foreignKey : table.foreignKeys()) { + DropForeignKeyConstraintChange dropForeignKey = dropForeignKey(foreignKey); + changeSet.addChange(dropForeignKey); + } + } + for (Table table : difference.tableAdditions()) { CreateTableChange newTable = changeTable(table); changeSet.addChange(newTable); @@ -373,12 +383,24 @@ private void generateTableAdditionsDeletions(ChangeSet changeSet, SchemaDiff dif changeSet.addChange(dropTable(table)); } } + + for (Table table : difference.tableAdditions()) { + for (ForeignKey foreignKey : table.foreignKeys()) { + AddForeignKeyConstraintChange addForeignKey = addForeignKey(foreignKey); + changeSet.addChange(addForeignKey); + } + } } private void generateTableModifications(ChangeSet changeSet, SchemaDiff difference) { for (TableDiff table : difference.tableDiffs()) { + for (ForeignKey foreignKey : table.fkToDrop()) { + DropForeignKeyConstraintChange dropForeignKey = dropForeignKey(foreignKey); + changeSet.addChange(dropForeignKey); + } + if (!table.columnsToAdd().isEmpty()) { changeSet.addChange(addColumns(table)); } @@ -388,6 +410,11 @@ private void generateTableModifications(ChangeSet changeSet, SchemaDiff differen if (!deletedColumns.isEmpty()) { changeSet.addChange(dropColumns(table, deletedColumns)); } + + for (ForeignKey foreignKey : table.fkToAdd()) { + AddForeignKeyConstraintChange addForeignKey = addForeignKey(foreignKey); + changeSet.addChange(addForeignKey); + } } } @@ -444,12 +471,27 @@ private Tables getLiquibaseModel(Database targetDatabase) throws LiquibaseExcept tableModel.columns().add(columnModel); } + tableModel.foreignKeys().addAll(extractForeignKeys(table)); + existingTables.add(tableModel); } return new Tables(existingTables); } + private static List extractForeignKeys(liquibase.structure.core.Table table) { + + return table.getOutgoingForeignKeys().stream().map(foreignKey -> { + String tableName = foreignKey.getForeignKeyTable().getName(); + String columnName = foreignKey.getForeignKeyColumns().stream().findFirst() + .map(liquibase.structure.core.Column::getName).get(); + String referencedTableName = foreignKey.getPrimaryKeyTable().getName(); + String referencedColumnName = foreignKey.getPrimaryKeyColumns().stream().findFirst() + .map(liquibase.structure.core.Column::getName).get(); + return new ForeignKey(foreignKey.getName(), tableName, columnName, referencedTableName, referencedColumnName); + }).collect(Collectors.toList()); + } + private static AddColumnChange addColumns(TableDiff table) { AddColumnChange addColumnChange = new AddColumnChange(); @@ -532,6 +574,25 @@ private static DropTableChange dropTable(Table table) { return change; } + private static AddForeignKeyConstraintChange addForeignKey(ForeignKey foreignKey) { + + AddForeignKeyConstraintChange change = new AddForeignKeyConstraintChange(); + change.setConstraintName(foreignKey.name()); + change.setBaseTableName(foreignKey.tableName()); + change.setBaseColumnNames(foreignKey.columnName()); + change.setReferencedTableName(foreignKey.referencedTableName()); + change.setReferencedColumnNames(foreignKey.referencedColumnName()); + return change; + } + + private static DropForeignKeyConstraintChange dropForeignKey(ForeignKey foreignKey) { + + DropForeignKeyConstraintChange change = new DropForeignKeyConstraintChange(); + change.setConstraintName(foreignKey.name()); + change.setBaseTableName(foreignKey.tableName()); + return change; + } + /** * Metadata for a ChangeSet. */ diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java index 079c40dde1..576badd92d 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java @@ -16,6 +16,7 @@ package org.springframework.data.jdbc.core.mapping.schema; import java.util.ArrayList; +import java.util.Collection; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -91,43 +92,40 @@ private static List diffTable(Tables mappedEntities, Map mappedColumns = createMapping(mappedEntity.columns(), Column::name, nameComparator); - mappedEntity.keyColumns().forEach(it -> mappedColumns.put(it.name(), it)); - Map existingColumns = createMapping(existingTable.columns(), Column::name, nameComparator); - existingTable.keyColumns().forEach(it -> existingColumns.put(it.name(), it)); - // Identify deleted columns - Map toDelete = new TreeMap<>(nameComparator); - toDelete.putAll(existingColumns); - mappedColumns.keySet().forEach(toDelete::remove); - - tableDiff.columnsToDrop().addAll(toDelete.values()); - - // Identify added columns - Map addedColumns = new TreeMap<>(nameComparator); - addedColumns.putAll(mappedColumns); - - existingColumns.keySet().forEach(addedColumns::remove); - - // Add columns in order. This order can interleave with existing columns. - for (Column column : mappedEntity.keyColumns()) { - if (addedColumns.containsKey(column.name())) { - tableDiff.columnsToAdd().add(column); - } - } - + tableDiff.columnsToDrop().addAll(findDiffs(mappedColumns, existingColumns, nameComparator)); + // Identify added columns and add columns in order. This order can interleave with existing columns. + List addedColumns = new ArrayList<>(findDiffs(existingColumns, mappedColumns, nameComparator)); for (Column column : mappedEntity.columns()) { - if (addedColumns.containsKey(column.name())) { + if (addedColumns.contains(column)) { tableDiff.columnsToAdd().add(column); } } + Map mappedForeignKeys = createMapping(mappedEntity.foreignKeys(), ForeignKey::name, + nameComparator); + Map existingForeignKeys = createMapping(existingTable.foreignKeys(), ForeignKey::name, + nameComparator); + // Identify deleted columns + tableDiff.fkToDrop().addAll(findDiffs(mappedForeignKeys, existingForeignKeys, nameComparator)); + // Identify added columns + tableDiff.fkToAdd().addAll(findDiffs(existingForeignKeys, mappedForeignKeys, nameComparator)); + tableDiffs.add(tableDiff); } return tableDiffs; } + private static Collection findDiffs(Map baseMapping, Map toCompareMapping, + Comparator nameComparator) { + Map diff = new TreeMap<>(nameComparator); + diff.putAll(toCompareMapping); + baseMapping.keySet().forEach(diff::remove); + return diff.values(); + } + private static SortedMap createMapping(List items, Function keyFunction, Comparator nameComparator) { diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java index 43b465d9a7..bd31afebf4 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java @@ -27,7 +27,7 @@ * @author Kurt Niemi * @since 3.2 */ -record Table(@Nullable String schema, String name, List keyColumns, List columns) { +record Table(@Nullable String schema, String name, List columns, List foreignKeys) { public Table(@Nullable String schema, String name) { this(schema, name, new ArrayList<>(), new ArrayList<>()); diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/TableDiff.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/TableDiff.java index 5ff5e01e71..189a9edd9d 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/TableDiff.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/TableDiff.java @@ -25,10 +25,11 @@ * @author Kurt Niemi * @since 3.2 */ -record TableDiff(Table table, List columnsToAdd, List columnsToDrop) { +record TableDiff(Table table, List columnsToAdd, List columnsToDrop, List fkToAdd, + List fkToDrop) { public TableDiff(Table table) { - this(table, new ArrayList<>(), new ArrayList<>()); + this(table, new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); } } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java index 12a35ce535..01a3da3fe0 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java @@ -15,14 +15,19 @@ */ package org.springframework.data.jdbc.core.mapping.schema; +import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import org.springframework.data.annotation.Id; +import org.springframework.data.mapping.context.MappingContext; +import org.springframework.data.relational.core.mapping.MappedCollection; import org.springframework.data.relational.core.mapping.RelationalMappingContext; import org.springframework.data.relational.core.mapping.RelationalPersistentEntity; import org.springframework.data.relational.core.mapping.RelationalPersistentProperty; @@ -37,15 +42,16 @@ record Tables(List tables) { public static Tables from(RelationalMappingContext context) { - return from(context.getPersistentEntities().stream(), new DefaultSqlTypeMapping(), null); + return from(context.getPersistentEntities().stream(), new DefaultSqlTypeMapping(), null, context); } - // TODO: Add support (i.e. create tickets) to support mapped collections, entities, embedded properties, and aggregate - // references. + // TODO: Add support (i.e. create tickets) to support entities, embedded properties, and aggregate references. public static Tables from(Stream> persistentEntities, - SqlTypeMapping sqlTypeMapping, @Nullable String defaultSchema) { + SqlTypeMapping sqlTypeMapping, @Nullable String defaultSchema, + MappingContext, ? extends RelationalPersistentProperty> context) { + Map> colAndFKByTableName = new HashMap<>(); List
tables = persistentEntities .filter(it -> it.isAnnotationPresent(org.springframework.data.relational.core.mapping.Table.class)) // .map(entity -> { @@ -54,6 +60,7 @@ public static Tables from(Stream> persis Set identifierColumns = new LinkedHashSet<>(); entity.getPersistentProperties(Id.class).forEach(identifierColumns::add); + collectForeignKeysInfo(entity, context, colAndFKByTableName, sqlTypeMapping); for (RelationalPersistentProperty property : entity) { @@ -61,19 +68,77 @@ public static Tables from(Stream> persis continue; } - String columnType = sqlTypeMapping.getRequiredColumnType(property); - Column column = new Column(property.getColumnName().getReference(), sqlTypeMapping.getColumnType(property), - sqlTypeMapping.isNullable(property), identifierColumns.contains(property)); + sqlTypeMapping.isNullable(property), identifierColumns.contains(property)); table.columns().add(column); } return table; }).collect(Collectors.toList()); + applyForeignKeys(tables, colAndFKByTableName); + return new Tables(tables); } public static Tables empty() { return new Tables(Collections.emptyList()); } + + private static void applyForeignKeys(List
tables, + Map> colAndFKByTableName) { + + colAndFKByTableName.forEach( + (tableName, colsAndFK) -> tables.stream().filter(table -> table.name().equals(tableName)).forEach(table -> { + + colsAndFK.forEach(colAndFK -> { + if (!table.columns().contains(colAndFK.column())) { + table.columns().add(colAndFK.column()); + } + }); + + colsAndFK.forEach(colAndFK -> table.foreignKeys().add(colAndFK.foreignKey())); + })); + } + + private static void collectForeignKeysInfo(RelationalPersistentEntity entity, + MappingContext, ? extends RelationalPersistentProperty> context, + Map> keyColumnsByTableName, SqlTypeMapping sqlTypeMapping) { + + RelationalPersistentProperty identifierColumn = entity.getPersistentProperty(Id.class); + + entity.getPersistentProperties(MappedCollection.class).forEach(property -> { + if (property.isEntity()) { + property.getPersistentEntityTypeInformation().forEach(typeInformation -> { + + String tableName = context.getRequiredPersistentEntity(typeInformation).getTableName().getReference(); + String columnName = property.getReverseColumnName(entity).getReference(); + String referencedTableName = entity.getTableName().getReference(); + String referencedColumnName = identifierColumn.getColumnName().getReference(); + + ForeignKey foreignKey = new ForeignKey(getForeignKeyName(referencedTableName, referencedColumnName), + tableName, columnName, referencedTableName, referencedColumnName); + Column column = new Column(columnName, sqlTypeMapping.getColumnType(identifierColumn), true, false); + + ColumnWithForeignKey columnWithForeignKey = new ColumnWithForeignKey(column, foreignKey); + keyColumnsByTableName.compute( + context.getRequiredPersistentEntity(typeInformation).getTableName().getReference(), (key, value) -> { + if (value == null) { + return new ArrayList<>(List.of(columnWithForeignKey)); + } else { + value.add(columnWithForeignKey); + return value; + } + }); + }); + } + }); + } + + //TODO should we place it in BasicRelationalPersistentProperty/BasicRelationalPersistentEntity and generate using NamingStrategy? + private static String getForeignKeyName(String referencedTableName, String referencedColumnName) { + return String.format("%s_%s_fk", referencedTableName, referencedColumnName); + } + + private record ColumnWithForeignKey(Column column, ForeignKey foreignKey) { + } } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterIntegrationTests.java index d27e59a37e..805cf85256 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterIntegrationTests.java @@ -20,7 +20,9 @@ import liquibase.change.AddColumnConfig; import liquibase.change.ColumnConfig; import liquibase.change.core.AddColumnChange; +import liquibase.change.core.AddForeignKeyConstraintChange; import liquibase.change.core.DropColumnChange; +import liquibase.change.core.DropForeignKeyConstraintChange; import liquibase.change.core.DropTableChange; import liquibase.changelog.ChangeSet; import liquibase.changelog.DatabaseChangeLog; @@ -41,6 +43,7 @@ import org.springframework.core.io.FileSystemResource; import org.springframework.data.annotation.Id; import org.springframework.data.jdbc.core.mapping.schema.LiquibaseChangeSetWriter.ChangeSetMetadata; +import org.springframework.data.relational.core.mapping.MappedCollection; import org.springframework.data.relational.core.mapping.RelationalMappingContext; import org.springframework.data.relational.core.mapping.Table; import org.springframework.data.util.Predicates; @@ -202,6 +205,93 @@ void shouldAppendToChangeLog(@TempDir File tempDir) { }); } + @Test // GH-1599 + void dropAndCreateTableWithRightOrderOfFkChanges() { + + withEmbeddedDatabase("drop-and-create-table-with-fk.sql", c -> { + + H2Database h2Database = new H2Database(); + h2Database.setConnection(new JdbcConnection(c)); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(contextOf(GroupOfPersons.class)); + writer.setDropTableFilter(Predicates.isTrue()); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), h2Database, new DatabaseChangeLog()); + + assertThat(changeSet.getChanges()).hasSize(4); + assertThat(changeSet.getChanges().get(0)).isInstanceOf(DropForeignKeyConstraintChange.class); + assertThat(changeSet.getChanges().get(3)).isInstanceOf(AddForeignKeyConstraintChange.class); + + DropForeignKeyConstraintChange dropForeignKey = (DropForeignKeyConstraintChange) changeSet.getChanges().get(0); + assertThat(dropForeignKey.getConstraintName()).isEqualToIgnoringCase("fk_to_drop"); + assertThat(dropForeignKey.getBaseTableName()).isEqualToIgnoringCase("table_to_drop"); + + AddForeignKeyConstraintChange addForeignKey = (AddForeignKeyConstraintChange) changeSet.getChanges().get(3); + assertThat(addForeignKey.getBaseTableName()).isEqualToIgnoringCase("person"); + assertThat(addForeignKey.getBaseColumnNames()).isEqualToIgnoringCase("group_id"); + assertThat(addForeignKey.getReferencedTableName()).isEqualToIgnoringCase("group_of_persons"); + assertThat(addForeignKey.getReferencedColumnNames()).isEqualToIgnoringCase("id"); + }); + } + + @Test // GH-1599 + void dropAndCreateFkInRightOrder() { + + withEmbeddedDatabase("drop-and-create-fk.sql", c -> { + + H2Database h2Database = new H2Database(); + h2Database.setConnection(new JdbcConnection(c)); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(contextOf(GroupOfPersons.class)); + writer.setDropColumnFilter((s, s2) -> true); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), h2Database, new DatabaseChangeLog()); + + assertThat(changeSet.getChanges()).hasSize(3); + assertThat(changeSet.getChanges().get(0)).isInstanceOf(DropForeignKeyConstraintChange.class); + assertThat(changeSet.getChanges().get(2)).isInstanceOf(AddForeignKeyConstraintChange.class); + + DropForeignKeyConstraintChange dropForeignKey = (DropForeignKeyConstraintChange) changeSet.getChanges().get(0); + assertThat(dropForeignKey.getConstraintName()).isEqualToIgnoringCase("fk_to_drop"); + assertThat(dropForeignKey.getBaseTableName()).isEqualToIgnoringCase("person"); + + AddForeignKeyConstraintChange addForeignKey = (AddForeignKeyConstraintChange) changeSet.getChanges().get(2); + assertThat(addForeignKey.getBaseTableName()).isEqualToIgnoringCase("person"); + assertThat(addForeignKey.getBaseColumnNames()).isEqualToIgnoringCase("group_id"); + assertThat(addForeignKey.getReferencedTableName()).isEqualToIgnoringCase("group_of_persons"); + assertThat(addForeignKey.getReferencedColumnNames()).isEqualToIgnoringCase("id"); + }); + } + + @Test // GH-1599 + void fieldForFkWillBeCreated() { + + withEmbeddedDatabase("create-fk-with-field.sql", c -> { + + H2Database h2Database = new H2Database(); + h2Database.setConnection(new JdbcConnection(c)); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(contextOf(GroupOfPersons.class)); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), h2Database, new DatabaseChangeLog()); + + assertThat(changeSet.getChanges()).hasSize(2); + assertThat(changeSet.getChanges().get(0)).isInstanceOf(AddColumnChange.class); + assertThat(changeSet.getChanges().get(1)).isInstanceOf(AddForeignKeyConstraintChange.class); + + AddColumnChange addColumn = (AddColumnChange) changeSet.getChanges().get(0); + assertThat(addColumn.getTableName()).isEqualToIgnoringCase("person"); + assertThat(addColumn.getColumns()).hasSize(1); + assertThat(addColumn.getColumns()).extracting(AddColumnConfig::getName).containsExactly("group_id"); + + AddForeignKeyConstraintChange addForeignKey = (AddForeignKeyConstraintChange) changeSet.getChanges().get(1); + assertThat(addForeignKey.getBaseTableName()).isEqualToIgnoringCase("person"); + assertThat(addForeignKey.getBaseColumnNames()).isEqualToIgnoringCase("group_id"); + assertThat(addForeignKey.getReferencedTableName()).isEqualToIgnoringCase("group_of_persons"); + assertThat(addForeignKey.getReferencedColumnNames()).isEqualToIgnoringCase("id"); + }); + } + RelationalMappingContext contextOf(Class... classes) { RelationalMappingContext context = new RelationalMappingContext(); @@ -246,4 +336,10 @@ static class DifferentPerson { String hello; } + @Table + static class GroupOfPersons { + @Id int id; + @MappedCollection(idColumn = "group_id") Set persons; + } + } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java index 314bbea8f4..73f377625e 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java @@ -15,9 +15,15 @@ */ package org.springframework.data.jdbc.core.mapping.schema; +import java.util.List; +import java.util.Optional; +import java.util.Set; + import static org.assertj.core.api.Assertions.*; +import liquibase.change.Change; import liquibase.change.ColumnConfig; +import liquibase.change.core.AddForeignKeyConstraintChange; import liquibase.change.core.CreateTableChange; import liquibase.changelog.ChangeSet; import liquibase.changelog.DatabaseChangeLog; @@ -25,6 +31,7 @@ import org.junit.jupiter.api.Test; import org.springframework.data.annotation.Id; import org.springframework.data.jdbc.core.mapping.schema.LiquibaseChangeSetWriter.ChangeSetMetadata; +import org.springframework.data.relational.core.mapping.MappedCollection; import org.springframework.data.relational.core.mapping.RelationalMappingContext; /** @@ -73,6 +80,45 @@ void shouldApplySchemaFilter() { assertThat(createTable.getTableName()).isEqualTo("other_table"); } + @Test // GH-1599 + void createForeignKeyWithNewTable() { + + RelationalMappingContext context = new RelationalMappingContext(); + context.getRequiredPersistentEntity(Tables.class); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); + + AddForeignKeyConstraintChange addForeignKey = (AddForeignKeyConstraintChange) changeSet.getChanges().get(2); + + assertThat(addForeignKey.getBaseTableName()).isEqualTo("other_table"); + assertThat(addForeignKey.getBaseColumnNames()).isEqualTo("tables"); + assertThat(addForeignKey.getReferencedTableName()).isEqualTo("tables"); + assertThat(addForeignKey.getReferencedColumnNames()).isEqualTo("id"); + + } + + @Test // GH-1599 + void fieldForFkShouldNotBeCreatedTwice() { + + RelationalMappingContext context = new RelationalMappingContext(); + context.getRequiredPersistentEntity(DifferentTables.class); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); + + Optional tableWithFk = changeSet.getChanges().stream().filter(change -> { + return change instanceof CreateTableChange && ((CreateTableChange) change).getTableName() + .equals("table_with_fk_field"); + }).findFirst(); + assertThat(tableWithFk.isPresent()).isEqualTo(true); + + List columns = ((CreateTableChange) tableWithFk.get()).getColumns(); + assertThat(columns).extracting(ColumnConfig::getName).containsExactly("id", "tables_id"); + } + @org.springframework.data.relational.core.mapping.Table static class VariousTypes { @Id long id; @@ -88,4 +134,24 @@ static class OtherTable { @Id long id; } + @org.springframework.data.relational.core.mapping.Table + static class Tables { + @Id int id; + @MappedCollection + Set tables; + } + + @org.springframework.data.relational.core.mapping.Table + static class DifferentTables { + @Id int id; + @MappedCollection(idColumn = "tables_id") + Set tables; + } + + @org.springframework.data.relational.core.mapping.Table + static class TableWithFkField { + @Id int id; + int tablesId; + } + } diff --git a/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/create-fk-with-field.sql b/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/create-fk-with-field.sql new file mode 100644 index 0000000000..15b912bed9 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/create-fk-with-field.sql @@ -0,0 +1,11 @@ +CREATE TABLE group_of_persons +( + id int primary key +); + +CREATE TABLE person +( + id int, + first_name varchar(255), + last_name varchar(255) +); diff --git a/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-fk.sql b/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-fk.sql new file mode 100644 index 0000000000..030599f566 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-fk.sql @@ -0,0 +1,14 @@ +CREATE TABLE group_of_persons +( + id int primary key +); + +CREATE TABLE person +( + id int, + first_name varchar(255), + last_name varchar(255), + group_id int, + group_id_to_drop int, + constraint fk_to_drop foreign key (group_id_to_drop) references group_of_persons(id) +); diff --git a/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-table-with-fk.sql b/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-table-with-fk.sql new file mode 100644 index 0000000000..9a81131180 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org/springframework/data/jdbc/core/mapping/schema/drop-and-create-table-with-fk.sql @@ -0,0 +1,11 @@ +CREATE TABLE group_of_persons +( + id int primary key +); + +CREATE TABLE table_to_drop +( + id int primary key, + persons int, + constraint fk_to_drop foreign key (persons) references group_of_persons(id) +); From cf9769b751ce992419d5d8de5181bd43a6dd5b32 Mon Sep 17 00:00:00 2001 From: Evgenii Koba Date: Sat, 14 Oct 2023 18:10:52 +0400 Subject: [PATCH 2/4] Add test. Polishing. Original pull request #1629 --- .../jdbc/core/mapping/schema/ForeignKey.java | 16 -------- .../jdbc/core/mapping/schema/SchemaDiff.java | 6 +-- .../LiquibaseChangeSetWriterUnitTests.java | 40 +++++++++++++++++++ 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java index d431e1529e..0a330bd214 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java @@ -1,7 +1,5 @@ package org.springframework.data.jdbc.core.mapping.schema; -import java.util.Objects; - /** * Models a Foreign Key for generating SQL for Schema generation. * @@ -10,18 +8,4 @@ */ record ForeignKey(String name, String tableName, String columnName, String referencedTableName, String referencedColumnName) { - @Override - public boolean equals(Object o) { - if (this == o) - return true; - if (o == null || getClass() != o.getClass()) - return false; - ForeignKey that = (ForeignKey) o; - return Objects.equals(name, that.name); - } - - @Override - public int hashCode() { - return Objects.hash(name); - } } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java index 576badd92d..ffa7032af5 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java @@ -96,7 +96,7 @@ private static List diffTable(Tables mappedEntities, Map addedColumns = new ArrayList<>(findDiffs(existingColumns, mappedColumns, nameComparator)); + Collection addedColumns = findDiffs(existingColumns, mappedColumns, nameComparator); for (Column column : mappedEntity.columns()) { if (addedColumns.contains(column)) { tableDiff.columnsToAdd().add(column); @@ -107,9 +107,9 @@ private static List diffTable(Tables mappedEntities, Map existingForeignKeys = createMapping(existingTable.foreignKeys(), ForeignKey::name, nameComparator); - // Identify deleted columns + // Identify deleted foreign keys tableDiff.fkToDrop().addAll(findDiffs(mappedForeignKeys, existingForeignKeys, nameComparator)); - // Identify added columns + // Identify added foreign keys tableDiff.fkToAdd().addAll(findDiffs(existingForeignKeys, mappedForeignKeys, nameComparator)); tableDiffs.add(tableDiff); diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java index 73f377625e..518abe4b4b 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java @@ -119,6 +119,39 @@ void fieldForFkShouldNotBeCreatedTwice() { assertThat(columns).extracting(ColumnConfig::getName).containsExactly("id", "tables_id"); } + @Test // GH-1599 + void createForeignKeyForNestedEntities() { + + RelationalMappingContext context = new RelationalMappingContext(); + context.getRequiredPersistentEntity(SetOfTables.class); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); + + Optional tablesFkOptional = changeSet.getChanges().stream().filter(change -> { + return change instanceof AddForeignKeyConstraintChange + && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("other_table"); + }).findFirst(); + assertThat(tablesFkOptional.isPresent()).isTrue(); + AddForeignKeyConstraintChange tableFk = (AddForeignKeyConstraintChange) tablesFkOptional.get(); + assertThat(tableFk.getBaseTableName()).isEqualTo("other_table"); + assertThat(tableFk.getBaseColumnNames()).isEqualTo("tables"); + assertThat(tableFk.getReferencedTableName()).isEqualTo("tables"); + assertThat(tableFk.getReferencedColumnNames()).isEqualTo("id"); + + Optional setOfTablesFkOptional = changeSet.getChanges().stream().filter(change -> { + return change instanceof AddForeignKeyConstraintChange + && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("tables"); + }).findFirst(); + assertThat(setOfTablesFkOptional.isPresent()).isTrue(); + AddForeignKeyConstraintChange setOfTablesFk = (AddForeignKeyConstraintChange) setOfTablesFkOptional.get(); + assertThat(setOfTablesFk.getBaseTableName()).isEqualTo("tables"); + assertThat(setOfTablesFk.getBaseColumnNames()).isEqualTo("set_id"); + assertThat(setOfTablesFk.getReferencedTableName()).isEqualTo("set_of_tables"); + assertThat(setOfTablesFk.getReferencedColumnNames()).isEqualTo("id"); + } + @org.springframework.data.relational.core.mapping.Table static class VariousTypes { @Id long id; @@ -141,6 +174,13 @@ static class Tables { Set tables; } + @org.springframework.data.relational.core.mapping.Table + static class SetOfTables { + @Id int id; + @MappedCollection(idColumn = "set_id") + Set setOfTables; + } + @org.springframework.data.relational.core.mapping.Table static class DifferentTables { @Id int id; From 893cd09cc13ccab2cc492e2a6a9f282f41f744fe Mon Sep 17 00:00:00 2001 From: Evgenii Koba Date: Fri, 20 Oct 2023 18:13:51 +0400 Subject: [PATCH 3/4] Fix fk creation for Map and List. Original pull request #1629 --- .../mapping/schema/DefaultSqlTypeMapping.java | 5 + .../jdbc/core/mapping/schema/ForeignKey.java | 6 +- .../schema/LiquibaseChangeSetWriter.java | 14 +- .../core/mapping/schema/SqlTypeMapping.java | 12 + .../data/jdbc/core/mapping/schema/Tables.java | 218 ++++++++++++++---- .../LiquibaseChangeSetWriterUnitTests.java | 91 ++++++-- 6 files changed, 278 insertions(+), 68 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/DefaultSqlTypeMapping.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/DefaultSqlTypeMapping.java index 526ad5a5fa..cda9811f51 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/DefaultSqlTypeMapping.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/DefaultSqlTypeMapping.java @@ -63,4 +63,9 @@ public DefaultSqlTypeMapping() { public String getColumnType(RelationalPersistentProperty property) { return typeMap.get(ClassUtils.resolvePrimitiveIfNecessary(property.getActualType())); } + + @Override + public String getColumnTypeByClass(Class clazz) { + return typeMap.get(clazz); + } } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java index 0a330bd214..16a7764e96 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java @@ -1,11 +1,13 @@ package org.springframework.data.jdbc.core.mapping.schema; +import java.util.List; + /** * Models a Foreign Key for generating SQL for Schema generation. * * @author Evgenii Koba * @since 3.2 */ -record ForeignKey(String name, String tableName, String columnName, String referencedTableName, - String referencedColumnName) { +record ForeignKey(String name, String tableName, List columnNames, String referencedTableName, + List referencedColumnNames) { } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java index ac0afc567b..d835c4f186 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java @@ -483,12 +483,12 @@ private static List extractForeignKeys(liquibase.structure.core.Tabl return table.getOutgoingForeignKeys().stream().map(foreignKey -> { String tableName = foreignKey.getForeignKeyTable().getName(); - String columnName = foreignKey.getForeignKeyColumns().stream().findFirst() - .map(liquibase.structure.core.Column::getName).get(); + List columnNames = foreignKey.getForeignKeyColumns().stream() + .map(liquibase.structure.core.Column::getName).toList(); String referencedTableName = foreignKey.getPrimaryKeyTable().getName(); - String referencedColumnName = foreignKey.getPrimaryKeyColumns().stream().findFirst() - .map(liquibase.structure.core.Column::getName).get(); - return new ForeignKey(foreignKey.getName(), tableName, columnName, referencedTableName, referencedColumnName); + List referencedColumnNames = foreignKey.getPrimaryKeyColumns().stream() + .map(liquibase.structure.core.Column::getName).toList(); + return new ForeignKey(foreignKey.getName(), tableName, columnNames, referencedTableName, referencedColumnNames); }).collect(Collectors.toList()); } @@ -579,9 +579,9 @@ private static AddForeignKeyConstraintChange addForeignKey(ForeignKey foreignKey AddForeignKeyConstraintChange change = new AddForeignKeyConstraintChange(); change.setConstraintName(foreignKey.name()); change.setBaseTableName(foreignKey.tableName()); - change.setBaseColumnNames(foreignKey.columnName()); + change.setBaseColumnNames(String.join(",", foreignKey.columnNames())); change.setReferencedTableName(foreignKey.referencedTableName()); - change.setReferencedColumnNames(foreignKey.referencedColumnName()); + change.setReferencedColumnNames(String.join(",", foreignKey.referencedColumnNames())); return change; } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SqlTypeMapping.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SqlTypeMapping.java index d66f932ca4..982d739945 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SqlTypeMapping.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SqlTypeMapping.java @@ -40,6 +40,18 @@ public interface SqlTypeMapping { @Nullable String getColumnType(RelationalPersistentProperty property); + /** + * Determines a column type for Class. + * + * @param clazz class for which the type should be determined. + * @return the SQL type to use, such as {@code VARCHAR} or {@code NUMERIC}. Can be {@literal null} if the strategy + * cannot provide a column type. + */ + @Nullable + default String getColumnTypeByClass(Class clazz) { + return null; + } + /** * Returns the required column type for a persistent property or throws {@link IllegalArgumentException} if the type * cannot be determined. diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java index 01a3da3fe0..e9b4e2a66a 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java @@ -17,20 +17,21 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; - import org.springframework.data.annotation.Id; import org.springframework.data.mapping.context.MappingContext; import org.springframework.data.relational.core.mapping.MappedCollection; import org.springframework.data.relational.core.mapping.RelationalMappingContext; import org.springframework.data.relational.core.mapping.RelationalPersistentEntity; import org.springframework.data.relational.core.mapping.RelationalPersistentProperty; +import org.springframework.data.relational.core.sql.SqlIdentifier; import org.springframework.lang.Nullable; /** @@ -51,7 +52,7 @@ public static Tables from(Stream> persis SqlTypeMapping sqlTypeMapping, @Nullable String defaultSchema, MappingContext, ? extends RelationalPersistentProperty> context) { - Map> colAndFKByTableName = new HashMap<>(); + List nestedEntityMetadataList = new ArrayList<>(); List
tables = persistentEntities .filter(it -> it.isAnnotationPresent(org.springframework.data.relational.core.mapping.Table.class)) // .map(entity -> { @@ -60,7 +61,7 @@ public static Tables from(Stream> persis Set identifierColumns = new LinkedHashSet<>(); entity.getPersistentProperties(Id.class).forEach(identifierColumns::add); - collectForeignKeysInfo(entity, context, colAndFKByTableName, sqlTypeMapping); + collectNestedEntityMetadata(entity, context, nestedEntityMetadataList, sqlTypeMapping); for (RelationalPersistentProperty property : entity) { @@ -75,7 +76,7 @@ public static Tables from(Stream> persis return table; }).collect(Collectors.toList()); - applyForeignKeys(tables, colAndFKByTableName); + applyNestedEntityMetadata(tables, nestedEntityMetadataList); return new Tables(tables); } @@ -84,61 +85,196 @@ public static Tables empty() { return new Tables(Collections.emptyList()); } - private static void applyForeignKeys(List
tables, - Map> colAndFKByTableName) { + /** + * Apply all information we know about nested entities to correctly create foreign and primary keys + */ + private static void applyNestedEntityMetadata(List
tables, List nestedEntityMetadataList) { + nestedEntityMetadataList.forEach(nestedEntityMetadata -> { + while (nestedEntityMetadata.parentMetadata != null) { + NestedEntityMetadata current = nestedEntityMetadata; + NestedEntityMetadata parentMetadata = current.parentMetadata; - colAndFKByTableName.forEach( - (tableName, colsAndFK) -> tables.stream().filter(table -> table.name().equals(tableName)).forEach(table -> { + Table table = tables.stream().filter(t -> t.name().equals(current.tableName)).findAny().get(); - colsAndFK.forEach(colAndFK -> { - if (!table.columns().contains(colAndFK.column())) { - table.columns().add(colAndFK.column()); - } - }); + List parentIdColumns = new ArrayList<>(); + collectIdentityColumns(parentMetadata, parentIdColumns); + List parentIdColumnNames = parentIdColumns.stream().map(Column::name).toList(); + + String foreignKeyName = getForeignKeyName(parentMetadata.tableName, parentIdColumnNames); + if(parentIdColumnNames.size() == 1) { + addIfAbsent(table.columns(), new Column(parentMetadata.referencedIdColumnName, parentMetadata.idColumnType, + false, current.idColumnName == null)); + if(parentMetadata.referencedKeyColumnName != null) { + addIfAbsent(table.columns(), new Column(parentMetadata.referencedKeyColumnName, parentMetadata.referencedKeyColumnType, + false, true)); + } + table.foreignKeys().add(new ForeignKey(foreignKeyName, current.tableName, + List.of(parentMetadata.referencedIdColumnName), parentMetadata.tableName, parentIdColumnNames)); + } else { + addIfAbsent(table.columns(), parentIdColumns.toArray(new Column[0])); + addIfAbsent(table.columns(), new Column(parentMetadata.referencedKeyColumnName, parentMetadata.referencedKeyColumnType, + false, true)); + table.foreignKeys().add(new ForeignKey(foreignKeyName, current.tableName, parentIdColumnNames, + parentMetadata.tableName, parentIdColumnNames)); + } + + nestedEntityMetadata = nestedEntityMetadata.parentMetadata; + } + }); + } - colsAndFK.forEach(colAndFK -> table.foreignKeys().add(colAndFK.foreignKey())); - })); + private static void addIfAbsent(List list, E... elements) { + for(E element : elements) { + if (!list.contains(element)) { + list.add(element); + } + } + } + + private static void collectIdentityColumns(NestedEntityMetadata nestedEntityMetadata, List identityColumns) { + if(nestedEntityMetadata.idColumnName != null) { + if(identityColumns.isEmpty()) { + identityColumns.add(new Column(nestedEntityMetadata.idColumnName, nestedEntityMetadata.idColumnType, + false, true)); + } else { + identityColumns.add(new Column(nestedEntityMetadata.referencedIdColumnName, nestedEntityMetadata.idColumnType, + false, true)); + } + Collections.reverse(identityColumns); + } else { + NestedEntityMetadata parentMetadata = nestedEntityMetadata.parentMetadata; + identityColumns.add(new Column(parentMetadata.referencedKeyColumnName, parentMetadata.referencedKeyColumnType, + false, true)); + collectIdentityColumns(parentMetadata, identityColumns); + } } - private static void collectForeignKeysInfo(RelationalPersistentEntity entity, + private static void collectNestedEntityMetadata(RelationalPersistentEntity entity, MappingContext, ? extends RelationalPersistentProperty> context, - Map> keyColumnsByTableName, SqlTypeMapping sqlTypeMapping) { + List nestedEntityMetadataList, SqlTypeMapping sqlTypeMapping) { - RelationalPersistentProperty identifierColumn = entity.getPersistentProperty(Id.class); + Optional idColumn = Optional.ofNullable(entity.getPersistentProperty(Id.class)); entity.getPersistentProperties(MappedCollection.class).forEach(property -> { if (property.isEntity()) { property.getPersistentEntityTypeInformation().forEach(typeInformation -> { - String tableName = context.getRequiredPersistentEntity(typeInformation).getTableName().getReference(); - String columnName = property.getReverseColumnName(entity).getReference(); - String referencedTableName = entity.getTableName().getReference(); - String referencedColumnName = identifierColumn.getColumnName().getReference(); - - ForeignKey foreignKey = new ForeignKey(getForeignKeyName(referencedTableName, referencedColumnName), - tableName, columnName, referencedTableName, referencedColumnName); - Column column = new Column(columnName, sqlTypeMapping.getColumnType(identifierColumn), true, false); - - ColumnWithForeignKey columnWithForeignKey = new ColumnWithForeignKey(column, foreignKey); - keyColumnsByTableName.compute( - context.getRequiredPersistentEntity(typeInformation).getTableName().getReference(), (key, value) -> { - if (value == null) { - return new ArrayList<>(List.of(columnWithForeignKey)); - } else { - value.add(columnWithForeignKey); - return value; - } - }); + String referencedKeyColumnType = null; + if(property.getType() == List.class) { + referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(Integer.class); + } else if (property.getType() == Map.class) { + referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(property.getComponentType()); + } + + NestedEntityMetadata parent = new NestedEntityMetadata( + idColumn.map(column -> column.getColumnName().getReference()).orElse(null), + idColumn.map(column -> sqlTypeMapping.getColumnType(column)).orElse(null), + property.getReverseColumnName(entity).getReference(), + Optional.ofNullable(property.getKeyColumn()).map(SqlIdentifier::getReference).orElse(null), + referencedKeyColumnType, + entity.getTableName().getReference(), + null); + + RelationalPersistentEntity childEntity = context.getRequiredPersistentEntity(typeInformation); + Optional childIdColumn = Optional.ofNullable(entity.getPersistentProperty(Id.class)); + NestedEntityMetadata child = new NestedEntityMetadata( + childIdColumn.map(column -> column.getColumnName().getReference()).orElse(null), + childIdColumn.map(column -> sqlTypeMapping.getColumnType(column)).orElse(null), + null, + null, + null, + childEntity.getTableName().getReference(), + parent); + + boolean added = attachNewChild(nestedEntityMetadataList, child); + added = added || attachNewParent(nestedEntityMetadataList, child); + if(!added) { + nestedEntityMetadataList.add(child); + } + }); } }); } //TODO should we place it in BasicRelationalPersistentProperty/BasicRelationalPersistentEntity and generate using NamingStrategy? - private static String getForeignKeyName(String referencedTableName, String referencedColumnName) { - return String.format("%s_%s_fk", referencedTableName, referencedColumnName); + private static String getForeignKeyName(String referencedTableName, List referencedColumnNames) { + return String.format("%s_%s_fk", referencedTableName, String.join("_", referencedColumnNames)); + } + + private static boolean attachNewParent(List nestedEntityMetadataList, NestedEntityMetadata newParent) { + + Optional oldParent + = nestedEntityMetadataList.stream().filter(elem -> elem.getLastParent().equals(newParent)).findAny(); + if(oldParent.isEmpty()) { + return false; + } else { + oldParent.get().parentMetadata.parentMetadata = newParent.parentMetadata; + return true; + } + } + + private static boolean attachNewChild(List nestedEntityMetadataList, NestedEntityMetadata newChild) { + + int index = nestedEntityMetadataList.indexOf(newChild.parentMetadata); + if(index == -1) { + return false; + } else { + NestedEntityMetadata oldChild = nestedEntityMetadataList.remove(index); + newChild.parentMetadata.parentMetadata = oldChild.parentMetadata; + nestedEntityMetadataList.add(newChild); + return true; + } } - private record ColumnWithForeignKey(Column column, ForeignKey foreignKey) { + private static class NestedEntityMetadata { + + public NestedEntityMetadata(String idColumnName, String idColumnType, String referencedIdColumnName, + String referencedKeyColumnName, String referencedKeyColumnType, String tableName, + NestedEntityMetadata parentMetadata) { + this.idColumnName = idColumnName; + this.idColumnType = idColumnType; + this.referencedIdColumnName = referencedIdColumnName; + this.referencedKeyColumnName = referencedKeyColumnName; + this.referencedKeyColumnType = referencedKeyColumnType; + this.tableName = tableName; + this.parentMetadata = parentMetadata; + } + + private String idColumnName; + private String idColumnType; + //column name for nested entity set by 'idColumn' of MappedCollection + private String referencedIdColumnName; + //column name for nested entity set by 'keyColumn' of MappedCollection + private String referencedKeyColumnName; + private String referencedKeyColumnType; + private String tableName; + private NestedEntityMetadata parentMetadata; + + public NestedEntityMetadata getLastParent() { + if(parentMetadata == null) { + return null; + } + NestedEntityMetadata lastParent = parentMetadata; + while (lastParent.parentMetadata != null) { + lastParent = lastParent.parentMetadata; + } + return lastParent; + } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + NestedEntityMetadata that = (NestedEntityMetadata) o; + return Objects.equals(tableName, that.tableName); + } + + @Override + public int hashCode() { + return Objects.hash(tableName); + } } } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java index 518abe4b4b..b9f773b3ca 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java @@ -16,6 +16,7 @@ package org.springframework.data.jdbc.core.mapping.schema; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -28,6 +29,7 @@ import liquibase.changelog.ChangeSet; import liquibase.changelog.DatabaseChangeLog; +import org.assertj.core.groups.Tuple; import org.junit.jupiter.api.Test; import org.springframework.data.annotation.Id; import org.springframework.data.jdbc.core.mapping.schema.LiquibaseChangeSetWriter.ChangeSetMetadata; @@ -123,33 +125,68 @@ void fieldForFkShouldNotBeCreatedTwice() { void createForeignKeyForNestedEntities() { RelationalMappingContext context = new RelationalMappingContext(); - context.getRequiredPersistentEntity(SetOfTables.class); + context.getRequiredPersistentEntity(ListOfMapOfNoIdTables.class); LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); - Optional tablesFkOptional = changeSet.getChanges().stream().filter(change -> { + Optional createNoIdTableOptional = changeSet.getChanges().stream().filter(change -> { + return change instanceof CreateTableChange && ((CreateTableChange) change).getTableName().equals("no_id_table"); + }).findFirst(); + assertThat(createNoIdTableOptional.isPresent()).isTrue(); + CreateTableChange createNoIdTable = (CreateTableChange) createNoIdTableOptional.get(); + assertThat(createNoIdTable.getColumns()) + .extracting(ColumnConfig::getName, ColumnConfig::getType, column -> column.getConstraints().isPrimaryKey()) + .containsExactly( + Tuple.tuple("field", "VARCHAR(255 BYTE)", null), + Tuple.tuple("list_id", "INT", true), + Tuple.tuple("list_of_map_of_no_id_tables_key", "INT", true), + Tuple.tuple("map_of_no_id_tables_key", "VARCHAR(255 BYTE)", true)); + + Optional createMapOfNoIdTablesOptional = changeSet.getChanges().stream().filter(change -> { + return change instanceof CreateTableChange && ((CreateTableChange) change).getTableName().equals("map_of_no_id_tables"); + }).findFirst(); + assertThat(createMapOfNoIdTablesOptional.isPresent()).isTrue(); + CreateTableChange createMapOfNoIdTables = (CreateTableChange) createMapOfNoIdTablesOptional.get(); + assertThat(createMapOfNoIdTables.getColumns()) + .extracting(ColumnConfig::getName, ColumnConfig::getType, column -> column.getConstraints().isPrimaryKey()) + .containsExactly( + Tuple.tuple("list_id", "INT", true), + Tuple.tuple("list_of_map_of_no_id_tables_key", "INT", true)); + + Optional createListOfMapOfNoIdTablesOptional = changeSet.getChanges().stream().filter(change -> { + return change instanceof CreateTableChange + && ((CreateTableChange) change).getTableName().equals("list_of_map_of_no_id_tables"); + }).findFirst(); + assertThat(createListOfMapOfNoIdTablesOptional.isPresent()).isTrue(); + CreateTableChange createListOfMapOfNoIdTables = (CreateTableChange) createListOfMapOfNoIdTablesOptional.get(); + assertThat(createListOfMapOfNoIdTables.getColumns().size()).isEqualTo(1); + assertThat(createListOfMapOfNoIdTables.getColumns().get(0)) + .extracting(ColumnConfig::getName, ColumnConfig::getType, column -> column.getConstraints().isPrimaryKey()) + .containsExactly("id", "INT", true); + + Optional noIdTableFkOptional = changeSet.getChanges().stream().filter(change -> { return change instanceof AddForeignKeyConstraintChange - && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("other_table"); + && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("no_id_table"); }).findFirst(); - assertThat(tablesFkOptional.isPresent()).isTrue(); - AddForeignKeyConstraintChange tableFk = (AddForeignKeyConstraintChange) tablesFkOptional.get(); - assertThat(tableFk.getBaseTableName()).isEqualTo("other_table"); - assertThat(tableFk.getBaseColumnNames()).isEqualTo("tables"); - assertThat(tableFk.getReferencedTableName()).isEqualTo("tables"); - assertThat(tableFk.getReferencedColumnNames()).isEqualTo("id"); - - Optional setOfTablesFkOptional = changeSet.getChanges().stream().filter(change -> { + assertThat(noIdTableFkOptional.isPresent()).isTrue(); + AddForeignKeyConstraintChange noIdTableFk = (AddForeignKeyConstraintChange) noIdTableFkOptional.get(); + assertThat(noIdTableFk.getBaseTableName()).isEqualTo("no_id_table"); + assertThat(noIdTableFk.getBaseColumnNames()).isEqualTo("list_id,list_of_map_of_no_id_tables_key"); + assertThat(noIdTableFk.getReferencedTableName()).isEqualTo("map_of_no_id_tables"); + assertThat(noIdTableFk.getReferencedColumnNames()).isEqualTo("list_id,list_of_map_of_no_id_tables_key"); + + Optional mapOfNoIdTablesFkOptional = changeSet.getChanges().stream().filter(change -> { return change instanceof AddForeignKeyConstraintChange - && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("tables"); + && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("map_of_no_id_tables"); }).findFirst(); - assertThat(setOfTablesFkOptional.isPresent()).isTrue(); - AddForeignKeyConstraintChange setOfTablesFk = (AddForeignKeyConstraintChange) setOfTablesFkOptional.get(); - assertThat(setOfTablesFk.getBaseTableName()).isEqualTo("tables"); - assertThat(setOfTablesFk.getBaseColumnNames()).isEqualTo("set_id"); - assertThat(setOfTablesFk.getReferencedTableName()).isEqualTo("set_of_tables"); - assertThat(setOfTablesFk.getReferencedColumnNames()).isEqualTo("id"); + assertThat(mapOfNoIdTablesFkOptional.isPresent()).isTrue(); + AddForeignKeyConstraintChange mapOfNoIdTablesFk = (AddForeignKeyConstraintChange) mapOfNoIdTablesFkOptional.get(); + assertThat(mapOfNoIdTablesFk.getBaseTableName()).isEqualTo("map_of_no_id_tables"); + assertThat(mapOfNoIdTablesFk.getBaseColumnNames()).isEqualTo("list_id"); + assertThat(mapOfNoIdTablesFk.getReferencedTableName()).isEqualTo("list_of_map_of_no_id_tables"); + assertThat(mapOfNoIdTablesFk.getReferencedColumnNames()).isEqualTo("id"); } @org.springframework.data.relational.core.mapping.Table @@ -194,4 +231,22 @@ static class TableWithFkField { int tablesId; } + @org.springframework.data.relational.core.mapping.Table + static class NoIdTable { + String field; + } + + @org.springframework.data.relational.core.mapping.Table + static class MapOfNoIdTables { + @MappedCollection + Map tables; + } + + @org.springframework.data.relational.core.mapping.Table + static class ListOfMapOfNoIdTables { + @Id int id; + @MappedCollection(idColumn = "list_id") + List listOfTables; + } + } From 3489735829c3a230753108fcaedd8bc89c510b6e Mon Sep 17 00:00:00 2001 From: Evgenii Koba Date: Sat, 21 Oct 2023 13:59:55 +0400 Subject: [PATCH 4/4] Add processing one-to-one. Fix bugs. Add tricky test cases. Original pull request #1629 --- .../jdbc/core/mapping/schema/ForeignKey.java | 17 ++ .../data/jdbc/core/mapping/schema/Table.java | 5 + .../data/jdbc/core/mapping/schema/Tables.java | 253 +++++++----------- .../LiquibaseChangeSetWriterUnitTests.java | 212 +++++++++++---- 4 files changed, 276 insertions(+), 211 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java index 16a7764e96..638c78ddac 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java @@ -1,6 +1,7 @@ package org.springframework.data.jdbc.core.mapping.schema; import java.util.List; +import java.util.Objects; /** * Models a Foreign Key for generating SQL for Schema generation. @@ -10,4 +11,20 @@ */ record ForeignKey(String name, String tableName, List columnNames, String referencedTableName, List referencedColumnNames) { + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + ForeignKey that = (ForeignKey) o; + return Objects.equals(tableName, that.tableName) && Objects.equals(columnNames, that.columnNames) && Objects.equals( + referencedTableName, that.referencedTableName) && Objects.equals(referencedColumnNames, + that.referencedColumnNames); + } + + @Override + public int hashCode() { + return Objects.hash(tableName, columnNames, referencedTableName, referencedColumnNames); + } } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java index bd31afebf4..908d87be9a 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Table.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.stream.Collectors; import org.springframework.lang.Nullable; import org.springframework.util.ObjectUtils; @@ -37,6 +38,10 @@ public Table(String name) { this(null, name); } + public List getIdColumns() { + return columns().stream().filter(Column::identity).collect(Collectors.toList()); + } + @Override public boolean equals(Object o) { diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java index e9b4e2a66a..2a9421d49a 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/Tables.java @@ -17,10 +17,11 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.LinkedList; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -52,7 +53,7 @@ public static Tables from(Stream> persis SqlTypeMapping sqlTypeMapping, @Nullable String defaultSchema, MappingContext, ? extends RelationalPersistentProperty> context) { - List nestedEntityMetadataList = new ArrayList<>(); + List foreignKeyMetadataList = new ArrayList<>(); List
tables = persistentEntities .filter(it -> it.isAnnotationPresent(org.springframework.data.relational.core.mapping.Table.class)) // .map(entity -> { @@ -61,11 +62,11 @@ public static Tables from(Stream> persis Set identifierColumns = new LinkedHashSet<>(); entity.getPersistentProperties(Id.class).forEach(identifierColumns::add); - collectNestedEntityMetadata(entity, context, nestedEntityMetadataList, sqlTypeMapping); for (RelationalPersistentProperty property : entity) { if (property.isEntity() && !property.isEmbedded()) { + foreignKeyMetadataList.add(createForeignKeyMetadata(entity, property, context, sqlTypeMapping)); continue; } @@ -76,7 +77,7 @@ public static Tables from(Stream> persis return table; }).collect(Collectors.toList()); - applyNestedEntityMetadata(tables, nestedEntityMetadataList); + applyForeignKeyMetadata(tables, foreignKeyMetadataList); return new Tables(tables); } @@ -86,40 +87,34 @@ public static Tables empty() { } /** - * Apply all information we know about nested entities to correctly create foreign and primary keys + * Apply all information we know about foreign keys to correctly create foreign and primary keys */ - private static void applyNestedEntityMetadata(List
tables, List nestedEntityMetadataList) { - nestedEntityMetadataList.forEach(nestedEntityMetadata -> { - while (nestedEntityMetadata.parentMetadata != null) { - NestedEntityMetadata current = nestedEntityMetadata; - NestedEntityMetadata parentMetadata = current.parentMetadata; - - Table table = tables.stream().filter(t -> t.name().equals(current.tableName)).findAny().get(); - - List parentIdColumns = new ArrayList<>(); - collectIdentityColumns(parentMetadata, parentIdColumns); - List parentIdColumnNames = parentIdColumns.stream().map(Column::name).toList(); - - String foreignKeyName = getForeignKeyName(parentMetadata.tableName, parentIdColumnNames); - if(parentIdColumnNames.size() == 1) { - addIfAbsent(table.columns(), new Column(parentMetadata.referencedIdColumnName, parentMetadata.idColumnType, - false, current.idColumnName == null)); - if(parentMetadata.referencedKeyColumnName != null) { - addIfAbsent(table.columns(), new Column(parentMetadata.referencedKeyColumnName, parentMetadata.referencedKeyColumnType, - false, true)); - } - table.foreignKeys().add(new ForeignKey(foreignKeyName, current.tableName, - List.of(parentMetadata.referencedIdColumnName), parentMetadata.tableName, parentIdColumnNames)); - } else { - addIfAbsent(table.columns(), parentIdColumns.toArray(new Column[0])); - addIfAbsent(table.columns(), new Column(parentMetadata.referencedKeyColumnName, parentMetadata.referencedKeyColumnType, + private static void applyForeignKeyMetadata(List
tables, List foreignKeyMetadataList) { + + foreignKeyMetadataList.forEach(foreignKeyMetadata -> { + Table table = tables.stream().filter(t -> t.name().equals(foreignKeyMetadata.tableName)).findAny().get(); + + List parentIdColumns = collectParentIdentityColumns(foreignKeyMetadata, foreignKeyMetadataList, tables); + List parentIdColumnNames = parentIdColumns.stream().map(Column::name).toList(); + + String foreignKeyName = getForeignKeyName(foreignKeyMetadata.parentTableName, parentIdColumnNames); + if(parentIdColumnNames.size() == 1) { + addIfAbsent(table.columns(), new Column(foreignKeyMetadata.referencingColumnName(), parentIdColumns.get(0).type(), + false, table.getIdColumns().isEmpty())); + if(foreignKeyMetadata.keyColumnName() != null) { + addIfAbsent(table.columns(), new Column(foreignKeyMetadata.keyColumnName(), foreignKeyMetadata.keyColumnType(), false, true)); - table.foreignKeys().add(new ForeignKey(foreignKeyName, current.tableName, parentIdColumnNames, - parentMetadata.tableName, parentIdColumnNames)); } - - nestedEntityMetadata = nestedEntityMetadata.parentMetadata; + addIfAbsent(table.foreignKeys(), new ForeignKey(foreignKeyName, foreignKeyMetadata.tableName(), + List.of(foreignKeyMetadata.referencingColumnName()), foreignKeyMetadata.parentTableName(), parentIdColumnNames)); + } else { + addIfAbsent(table.columns(), parentIdColumns.toArray(new Column[0])); + addIfAbsent(table.columns(), new Column(foreignKeyMetadata.keyColumnName(), foreignKeyMetadata.keyColumnType(), + false, true)); + addIfAbsent(table.foreignKeys(), new ForeignKey(foreignKeyName, foreignKeyMetadata.tableName(), parentIdColumnNames, + foreignKeyMetadata.parentTableName(), parentIdColumnNames)); } + }); } @@ -131,150 +126,86 @@ private static void addIfAbsent(List list, E... elements) { } } - private static void collectIdentityColumns(NestedEntityMetadata nestedEntityMetadata, List identityColumns) { - if(nestedEntityMetadata.idColumnName != null) { - if(identityColumns.isEmpty()) { - identityColumns.add(new Column(nestedEntityMetadata.idColumnName, nestedEntityMetadata.idColumnType, - false, true)); - } else { - identityColumns.add(new Column(nestedEntityMetadata.referencedIdColumnName, nestedEntityMetadata.idColumnType, - false, true)); - } - Collections.reverse(identityColumns); - } else { - NestedEntityMetadata parentMetadata = nestedEntityMetadata.parentMetadata; - identityColumns.add(new Column(parentMetadata.referencedKeyColumnName, parentMetadata.referencedKeyColumnType, - false, true)); - collectIdentityColumns(parentMetadata, identityColumns); - } + private static List collectParentIdentityColumns(ForeignKeyMetadata child, + List foreignKeyMetadataList, List
tables) { + return collectParentIdentityColumns(child, foreignKeyMetadataList, tables, new HashSet<>()); } - private static void collectNestedEntityMetadata(RelationalPersistentEntity entity, - MappingContext, ? extends RelationalPersistentProperty> context, - List nestedEntityMetadataList, SqlTypeMapping sqlTypeMapping) { - - Optional idColumn = Optional.ofNullable(entity.getPersistentProperty(Id.class)); + private static List collectParentIdentityColumns(ForeignKeyMetadata child, + List foreignKeyMetadataList, List
tables, Set excludeTables) { - entity.getPersistentProperties(MappedCollection.class).forEach(property -> { - if (property.isEntity()) { - property.getPersistentEntityTypeInformation().forEach(typeInformation -> { + excludeTables.add(child.tableName()); - String referencedKeyColumnType = null; - if(property.getType() == List.class) { - referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(Integer.class); - } else if (property.getType() == Map.class) { - referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(property.getComponentType()); - } + Table parentTable = findTableByName(tables, child.parentTableName()); + ForeignKeyMetadata parentMetadata = findMetadataByTableName(foreignKeyMetadataList, child.parentTableName(), excludeTables); + List parentIdColumns = parentTable.getIdColumns(); - NestedEntityMetadata parent = new NestedEntityMetadata( - idColumn.map(column -> column.getColumnName().getReference()).orElse(null), - idColumn.map(column -> sqlTypeMapping.getColumnType(column)).orElse(null), - property.getReverseColumnName(entity).getReference(), - Optional.ofNullable(property.getKeyColumn()).map(SqlIdentifier::getReference).orElse(null), - referencedKeyColumnType, - entity.getTableName().getReference(), - null); - - RelationalPersistentEntity childEntity = context.getRequiredPersistentEntity(typeInformation); - Optional childIdColumn = Optional.ofNullable(entity.getPersistentProperty(Id.class)); - NestedEntityMetadata child = new NestedEntityMetadata( - childIdColumn.map(column -> column.getColumnName().getReference()).orElse(null), - childIdColumn.map(column -> sqlTypeMapping.getColumnType(column)).orElse(null), - null, - null, - null, - childEntity.getTableName().getReference(), - parent); - - boolean added = attachNewChild(nestedEntityMetadataList, child); - added = added || attachNewParent(nestedEntityMetadataList, child); - if(!added) { - nestedEntityMetadataList.add(child); - } - - }); + if (!parentIdColumns.isEmpty()) { + return new ArrayList<>(parentIdColumns); + } else if(parentMetadata == null) { + //mustn't happen, probably wrong entity declaration + return new ArrayList<>(); + } else { + List parentParentIdColumns = collectParentIdentityColumns(parentMetadata, foreignKeyMetadataList, tables); + if (parentParentIdColumns.size() == 1) { + Column parentParentIdColumn = parentParentIdColumns.get(0); + Column withChangedName = new Column(parentMetadata.referencingColumnName, parentParentIdColumn.type(), false, true); + parentParentIdColumns = new LinkedList<>(List.of(withChangedName)); } - }); + if (parentMetadata.keyColumnName() != null) { + parentParentIdColumns.add(new Column(parentMetadata.keyColumnName(), parentMetadata.keyColumnType(), false, true)); + } + return parentParentIdColumns; + } } - //TODO should we place it in BasicRelationalPersistentProperty/BasicRelationalPersistentEntity and generate using NamingStrategy? - private static String getForeignKeyName(String referencedTableName, List referencedColumnNames) { - return String.format("%s_%s_fk", referencedTableName, String.join("_", referencedColumnNames)); + @Nullable + private static Table findTableByName(List
tables, String tableName) { + return tables.stream().filter(table -> table.name().equals(tableName)).findAny().orElse(null); } - private static boolean attachNewParent(List nestedEntityMetadataList, NestedEntityMetadata newParent) { - - Optional oldParent - = nestedEntityMetadataList.stream().filter(elem -> elem.getLastParent().equals(newParent)).findAny(); - if(oldParent.isEmpty()) { - return false; - } else { - oldParent.get().parentMetadata.parentMetadata = newParent.parentMetadata; - return true; - } + @Nullable + private static ForeignKeyMetadata findMetadataByTableName(List metadata, String tableName, + Set excludeTables) { + return metadata.stream() + .filter(m -> m.tableName().equals(tableName) && !excludeTables.contains(m.parentTableName())) + .findAny() + .orElse(null); } - private static boolean attachNewChild(List nestedEntityMetadataList, NestedEntityMetadata newChild) { - - int index = nestedEntityMetadataList.indexOf(newChild.parentMetadata); - if(index == -1) { - return false; - } else { - NestedEntityMetadata oldChild = nestedEntityMetadataList.remove(index); - newChild.parentMetadata.parentMetadata = oldChild.parentMetadata; - nestedEntityMetadataList.add(newChild); - return true; - } - } + private static ForeignKeyMetadata createForeignKeyMetadata( + RelationalPersistentEntity entity, + RelationalPersistentProperty property, + MappingContext, ? extends RelationalPersistentProperty> context, + SqlTypeMapping sqlTypeMapping) { - private static class NestedEntityMetadata { - - public NestedEntityMetadata(String idColumnName, String idColumnType, String referencedIdColumnName, - String referencedKeyColumnName, String referencedKeyColumnType, String tableName, - NestedEntityMetadata parentMetadata) { - this.idColumnName = idColumnName; - this.idColumnType = idColumnType; - this.referencedIdColumnName = referencedIdColumnName; - this.referencedKeyColumnName = referencedKeyColumnName; - this.referencedKeyColumnType = referencedKeyColumnType; - this.tableName = tableName; - this.parentMetadata = parentMetadata; - } + RelationalPersistentEntity childEntity = context.getRequiredPersistentEntity(property.getActualType()); - private String idColumnName; - private String idColumnType; - //column name for nested entity set by 'idColumn' of MappedCollection - private String referencedIdColumnName; - //column name for nested entity set by 'keyColumn' of MappedCollection - private String referencedKeyColumnName; - private String referencedKeyColumnType; - private String tableName; - private NestedEntityMetadata parentMetadata; - - public NestedEntityMetadata getLastParent() { - if(parentMetadata == null) { - return null; - } - NestedEntityMetadata lastParent = parentMetadata; - while (lastParent.parentMetadata != null) { - lastParent = lastParent.parentMetadata; + String referencedKeyColumnType = null; + if(property.isAnnotationPresent(MappedCollection.class)) { + if (property.getType() == List.class) { + referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(Integer.class); + } else if (property.getType() == Map.class) { + referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(property.getComponentType()); } - return lastParent; } - @Override - public boolean equals(Object o) { - if (this == o) - return true; - if (o == null || getClass() != o.getClass()) - return false; - NestedEntityMetadata that = (NestedEntityMetadata) o; - return Objects.equals(tableName, that.tableName); - } + return new ForeignKeyMetadata( + childEntity.getTableName().getReference(), + property.getReverseColumnName(entity).getReference(), + Optional.ofNullable(property.getKeyColumn()).map(SqlIdentifier::getReference).orElse(null), + referencedKeyColumnType, + entity.getTableName().getReference() + ); + } + + //TODO should we place it in BasicRelationalPersistentProperty/BasicRelationalPersistentEntity and generate using NamingStrategy? + private static String getForeignKeyName(String referencedTableName, List referencedColumnNames) { + return String.format("%s_%s_fk", referencedTableName, String.join("_", referencedColumnNames)); + } + + private record ForeignKeyMetadata(String tableName, String referencingColumnName, @Nullable String keyColumnName, + @Nullable String keyColumnType, String parentTableName) { - @Override - public int hashCode() { - return Objects.hash(tableName); - } } } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java index b9f773b3ca..3329dd0f2c 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterUnitTests.java @@ -15,6 +15,7 @@ */ package org.springframework.data.jdbc.core.mapping.schema; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Optional; @@ -22,6 +23,7 @@ import static org.assertj.core.api.Assertions.*; +import java.util.stream.Collectors; import liquibase.change.Change; import liquibase.change.ColumnConfig; import liquibase.change.core.AddForeignKeyConstraintChange; @@ -33,8 +35,11 @@ import org.junit.jupiter.api.Test; import org.springframework.data.annotation.Id; import org.springframework.data.jdbc.core.mapping.schema.LiquibaseChangeSetWriter.ChangeSetMetadata; +import org.springframework.data.relational.core.mapping.Column; import org.springframework.data.relational.core.mapping.MappedCollection; import org.springframework.data.relational.core.mapping.RelationalMappingContext; +import org.springframework.data.relational.core.mapping.RelationalPersistentEntity; +import org.springframework.data.util.Optionals; /** * Unit tests for {@link LiquibaseChangeSetWriter}. @@ -131,62 +136,130 @@ void createForeignKeyForNestedEntities() { ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); - Optional createNoIdTableOptional = changeSet.getChanges().stream().filter(change -> { - return change instanceof CreateTableChange && ((CreateTableChange) change).getTableName().equals("no_id_table"); - }).findFirst(); - assertThat(createNoIdTableOptional.isPresent()).isTrue(); - CreateTableChange createNoIdTable = (CreateTableChange) createNoIdTableOptional.get(); - assertThat(createNoIdTable.getColumns()) - .extracting(ColumnConfig::getName, ColumnConfig::getType, column -> column.getConstraints().isPrimaryKey()) - .containsExactly( - Tuple.tuple("field", "VARCHAR(255 BYTE)", null), - Tuple.tuple("list_id", "INT", true), - Tuple.tuple("list_of_map_of_no_id_tables_key", "INT", true), - Tuple.tuple("map_of_no_id_tables_key", "VARCHAR(255 BYTE)", true)); - - Optional createMapOfNoIdTablesOptional = changeSet.getChanges().stream().filter(change -> { - return change instanceof CreateTableChange && ((CreateTableChange) change).getTableName().equals("map_of_no_id_tables"); - }).findFirst(); - assertThat(createMapOfNoIdTablesOptional.isPresent()).isTrue(); - CreateTableChange createMapOfNoIdTables = (CreateTableChange) createMapOfNoIdTablesOptional.get(); - assertThat(createMapOfNoIdTables.getColumns()) - .extracting(ColumnConfig::getName, ColumnConfig::getType, column -> column.getConstraints().isPrimaryKey()) - .containsExactly( - Tuple.tuple("list_id", "INT", true), - Tuple.tuple("list_of_map_of_no_id_tables_key", "INT", true)); + assertCreateTable(changeSet, "no_id_table", + Tuple.tuple("field", "VARCHAR(255 BYTE)", null), + Tuple.tuple("list_id", "INT", true), + Tuple.tuple("list_of_map_of_no_id_tables_key", "INT", true), + Tuple.tuple("map_of_no_id_tables_key", "VARCHAR(255 BYTE)", true)); + + assertCreateTable(changeSet, "map_of_no_id_tables", + Tuple.tuple("list_id", "INT", true), + Tuple.tuple("list_of_map_of_no_id_tables_key", "INT", true)); + + assertCreateTable(changeSet, "list_of_map_of_no_id_tables", Tuple.tuple("id", "INT", true)); + + assertAddForeignKey(changeSet, "no_id_table", "list_id,list_of_map_of_no_id_tables_key", + "map_of_no_id_tables", "list_id,list_of_map_of_no_id_tables_key"); + + assertAddForeignKey(changeSet, "map_of_no_id_tables", "list_id", + "list_of_map_of_no_id_tables", "id"); + } + + @Test // GH-1599 + void createForeignKeyForOneToOneWithMultipleChildren() { + + RelationalMappingContext context = new RelationalMappingContext(); + context.getRequiredPersistentEntity(OneToOneLevel1.class); + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); + + assertCreateTable(changeSet, "other_table", + Tuple.tuple("id", "BIGINT", true), Tuple.tuple("one_to_one_level1", "INT", null)); + + assertCreateTable(changeSet, "one_to_one_level2", Tuple.tuple("one_to_one_level1", "INT", true)); - Optional createListOfMapOfNoIdTablesOptional = changeSet.getChanges().stream().filter(change -> { - return change instanceof CreateTableChange - && ((CreateTableChange) change).getTableName().equals("list_of_map_of_no_id_tables"); + assertCreateTable(changeSet, "no_id_table", Tuple.tuple("field", "VARCHAR(255 BYTE)", null), + Tuple.tuple("one_to_one_level2", "INT", true), Tuple.tuple("additional_one_to_one_level2", "INT", null)); + + assertAddForeignKey(changeSet, "other_table", "one_to_one_level1", + "one_to_one_level1", "id"); + + assertAddForeignKey(changeSet, "one_to_one_level2", "one_to_one_level1", + "one_to_one_level1", "id"); + + assertAddForeignKey(changeSet, "no_id_table", "one_to_one_level2", + "one_to_one_level2", "one_to_one_level1"); + + assertAddForeignKey(changeSet, "no_id_table", "additional_one_to_one_level2", + "one_to_one_level2", "one_to_one_level1"); + + } + + @Test // GH-1599 + void createForeignKeyForCircularWithId() { + RelationalMappingContext context = new RelationalMappingContext() { + @Override + public Collection> getPersistentEntities() { + return List.of(getPersistentEntity(CircularWithId.class), getPersistentEntity(ParentOfCircularWithId.class)); + } + }; + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); + + assertCreateTable(changeSet, "circular_with_id", + Tuple.tuple("id", "INT", true), + Tuple.tuple("circular_with_id", "INT", null), + Tuple.tuple("parent_of_circular_with_id", "INT", null)); + + assertAddForeignKey(changeSet, "circular_with_id", "parent_of_circular_with_id", + "parent_of_circular_with_id", "id"); + + assertAddForeignKey(changeSet, "circular_with_id", "circular_with_id", + "circular_with_id", "id"); + } + + @Test // GH-1599 + void createForeignKeyForCircularNoId() { + RelationalMappingContext context = new RelationalMappingContext() { + @Override + public Collection> getPersistentEntities() { + return List.of(getPersistentEntity(CircularNoId.class), getPersistentEntity(ParentOfCircularNoId.class)); + } + }; + + LiquibaseChangeSetWriter writer = new LiquibaseChangeSetWriter(context); + + ChangeSet changeSet = writer.createChangeSet(ChangeSetMetadata.create(), new DatabaseChangeLog()); + + assertCreateTable(changeSet, "circular_no_id", + Tuple.tuple("circular_no_id", "INT", true), + Tuple.tuple("parent_of_circular_no_id", "INT", null)); + + assertAddForeignKey(changeSet, "circular_no_id", "parent_of_circular_no_id", + "parent_of_circular_no_id", "id"); + + assertAddForeignKey(changeSet, "circular_no_id", "circular_no_id", + "circular_no_id", "parent_of_circular_no_id"); + } + + void assertCreateTable(ChangeSet changeSet, String tableName, Tuple... columnTuples) { + Optional createTableOptional = changeSet.getChanges().stream().filter(change -> { + return change instanceof CreateTableChange && ((CreateTableChange) change).getTableName().equals(tableName); }).findFirst(); - assertThat(createListOfMapOfNoIdTablesOptional.isPresent()).isTrue(); - CreateTableChange createListOfMapOfNoIdTables = (CreateTableChange) createListOfMapOfNoIdTablesOptional.get(); - assertThat(createListOfMapOfNoIdTables.getColumns().size()).isEqualTo(1); - assertThat(createListOfMapOfNoIdTables.getColumns().get(0)) + assertThat(createTableOptional.isPresent()).isTrue(); + CreateTableChange createTable = (CreateTableChange) createTableOptional.get(); + assertThat(createTable.getColumns()) .extracting(ColumnConfig::getName, ColumnConfig::getType, column -> column.getConstraints().isPrimaryKey()) - .containsExactly("id", "INT", true); + .containsExactly(columnTuples); + } - Optional noIdTableFkOptional = changeSet.getChanges().stream().filter(change -> { - return change instanceof AddForeignKeyConstraintChange - && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("no_id_table"); - }).findFirst(); - assertThat(noIdTableFkOptional.isPresent()).isTrue(); - AddForeignKeyConstraintChange noIdTableFk = (AddForeignKeyConstraintChange) noIdTableFkOptional.get(); - assertThat(noIdTableFk.getBaseTableName()).isEqualTo("no_id_table"); - assertThat(noIdTableFk.getBaseColumnNames()).isEqualTo("list_id,list_of_map_of_no_id_tables_key"); - assertThat(noIdTableFk.getReferencedTableName()).isEqualTo("map_of_no_id_tables"); - assertThat(noIdTableFk.getReferencedColumnNames()).isEqualTo("list_id,list_of_map_of_no_id_tables_key"); - - Optional mapOfNoIdTablesFkOptional = changeSet.getChanges().stream().filter(change -> { + void assertAddForeignKey(ChangeSet changeSet, String baseTableName, String baseColumnNames, String referencedTableName, + String referencedColumnNames) { + Optional addFkOptional = changeSet.getChanges().stream().filter(change -> { return change instanceof AddForeignKeyConstraintChange - && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals("map_of_no_id_tables"); + && ((AddForeignKeyConstraintChange) change).getBaseTableName().equals(baseTableName) + && ((AddForeignKeyConstraintChange) change).getBaseColumnNames().equals(baseColumnNames); }).findFirst(); - assertThat(mapOfNoIdTablesFkOptional.isPresent()).isTrue(); - AddForeignKeyConstraintChange mapOfNoIdTablesFk = (AddForeignKeyConstraintChange) mapOfNoIdTablesFkOptional.get(); - assertThat(mapOfNoIdTablesFk.getBaseTableName()).isEqualTo("map_of_no_id_tables"); - assertThat(mapOfNoIdTablesFk.getBaseColumnNames()).isEqualTo("list_id"); - assertThat(mapOfNoIdTablesFk.getReferencedTableName()).isEqualTo("list_of_map_of_no_id_tables"); - assertThat(mapOfNoIdTablesFk.getReferencedColumnNames()).isEqualTo("id"); + assertThat(addFkOptional.isPresent()).isTrue(); + AddForeignKeyConstraintChange addFk = (AddForeignKeyConstraintChange) addFkOptional.get(); + assertThat(addFk.getBaseTableName()).isEqualTo(baseTableName); + assertThat(addFk.getBaseColumnNames()).isEqualTo(baseColumnNames); + assertThat(addFk.getReferencedTableName()).isEqualTo(referencedTableName); + assertThat(addFk.getReferencedColumnNames()).isEqualTo(referencedColumnNames); } @org.springframework.data.relational.core.mapping.Table @@ -249,4 +322,43 @@ static class ListOfMapOfNoIdTables { List listOfTables; } + @org.springframework.data.relational.core.mapping.Table + static class OneToOneLevel1 { + @Id int id; + OneToOneLevel2 oneToOneLevel2; + OtherTable otherTable; + } + + @org.springframework.data.relational.core.mapping.Table + static class OneToOneLevel2 { + NoIdTable table1; + @Column("additional_one_to_one_level2") + NoIdTable table2; + } + + @org.springframework.data.relational.core.mapping.Table + static class ParentOfCircularWithId { + @Id int id; + CircularWithId circularWithId; + + } + + @org.springframework.data.relational.core.mapping.Table + static class CircularWithId { + @Id int id; + CircularWithId circularWithId; + } + + @org.springframework.data.relational.core.mapping.Table + static class ParentOfCircularNoId { + @Id int id; + CircularNoId CircularNoId; + + } + + @org.springframework.data.relational.core.mapping.Table + static class CircularNoId { + CircularNoId CircularNoId; + } + }