Polishing.

Removed tests for circular references since they are not supported by Spring Data Relational. Closes #1599 See #756, #1600 Original pull request #1629
spring-projects · Nov 24, 2023 · 5342e02 · 5342e02
1 parent 9136d86
commit 5342e02
Show file tree

Hide file tree

Showing 10 changed files with 115 additions and 176 deletions.
diff --git a/...rc/main/java/org/springframework/data/jdbc/core/mapping/schema/DefaultSqlTypeMapping.java b/...rc/main/java/org/springframework/data/jdbc/core/mapping/schema/DefaultSqlTypeMapping.java
@@ -32,6 +32,8 @@
  * instance of a class implementing {@link SqlTypeMapping} interface can be set on the {@link Tables} class
  *
  * @author Kurt Niemi
+ * @author Evgenii Koba
+ * @author Jens Schauder
  * @since 3.2
  */
 public class DefaultSqlTypeMapping implements SqlTypeMapping {
@@ -61,11 +63,11 @@ public DefaultSqlTypeMapping() {
 
 	@Override
 	public String getColumnType(RelationalPersistentProperty property) {
-		return typeMap.get(ClassUtils.resolvePrimitiveIfNecessary(property.getActualType()));
+		return getColumnType(property.getActualType());
 	}
 
 	@Override
-	public String getColumnTypeByClass(Class clazz) {
-		return typeMap.get(clazz);
+	public String getColumnType(Class<?> type) {
+		return typeMap.get(ClassUtils.resolvePrimitiveIfNecessary(type));
 	}
 }
diff --git a/...data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java b/...data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/ForeignKey.java
@@ -7,20 +7,20 @@
  * Models a Foreign Key for generating SQL for Schema generation.
  *
  * @author Evgenii Koba
- * @since 3.2
+ * @since 3.3
  */
 record ForeignKey(String name, String tableName, List<String> columnNames, String referencedTableName,
-									List<String> referencedColumnNames) {
+		List<String> 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);
+		return Objects.equals(tableName, that.tableName) && Objects.equals(columnNames, that.columnNames)
+				&& Objects.equals(referencedTableName, that.referencedTableName)
+				&& Objects.equals(referencedColumnNames, that.referencedColumnNames);
 	}
 
 	@Override

diff --git a/...main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java b/...main/java/org/springframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriter.java
@@ -55,6 +55,7 @@
 import java.util.function.BiPredicate;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.springframework.core.io.Resource;
 import org.springframework.data.mapping.context.MappingContext;
@@ -90,6 +91,7 @@
  *
  * @author Kurt Niemi
  * @author Mark Paluch
+ * @author Evgenii Koba
  * @since 3.2
  */
 public class LiquibaseChangeSetWriter {
@@ -323,16 +325,18 @@ private ChangeSet createChangeSet(ChangeSetMetadata metadata, SchemaDiff differe
 
 	private SchemaDiff initial() {
 
-		Tables mappedEntities = Tables.from(mappingContext.getPersistentEntities().stream().filter(schemaFilter),
-				sqlTypeMapping, null, mappingContext);
+		Stream<? extends RelationalPersistentEntity<?>> entities = mappingContext.getPersistentEntities().stream()
+				.filter(schemaFilter);
+		Tables mappedEntities = Tables.from(entities, sqlTypeMapping, null, mappingContext);
 		return SchemaDiff.diff(mappedEntities, Tables.empty(), nameComparator);
 	}
 
 	private SchemaDiff differenceOf(Database database) throws LiquibaseException {
 
 		Tables existingTables = getLiquibaseModel(database);
-		Tables mappedEntities = Tables.from(mappingContext.getPersistentEntities().stream().filter(schemaFilter),
-				sqlTypeMapping, database.getDefaultCatalogName(), mappingContext);
+		Stream<? extends RelationalPersistentEntity<?>> entities = mappingContext.getPersistentEntities().stream()
+				.filter(schemaFilter);
+		Tables mappedEntities = Tables.from(entities, sqlTypeMapping, database.getDefaultCatalogName(), mappingContext);
 
 		return SchemaDiff.diff(mappedEntities, existingTables, nameComparator);
 	}
@@ -482,12 +486,15 @@ private Tables getLiquibaseModel(Database targetDatabase) throws LiquibaseExcept
 	private static List<ForeignKey> extractForeignKeys(liquibase.structure.core.Table table) {
 
 		return table.getOutgoingForeignKeys().stream().map(foreignKey -> {
+
 			String tableName = foreignKey.getForeignKeyTable().getName();
 			List<String> columnNames = foreignKey.getForeignKeyColumns().stream()
 					.map(liquibase.structure.core.Column::getName).toList();
+
 			String referencedTableName = foreignKey.getPrimaryKeyTable().getName();
 			List<String> referencedColumnNames = foreignKey.getPrimaryKeyColumns().stream()
 					.map(liquibase.structure.core.Column::getName).toList();
+
 			return new ForeignKey(foreignKey.getName(), tableName, columnNames, referencedTableName, referencedColumnNames);
 		}).collect(Collectors.toList());
 	}
@@ -582,6 +589,7 @@ private static AddForeignKeyConstraintChange addForeignKey(ForeignKey foreignKey
 		change.setBaseColumnNames(String.join(",", foreignKey.columnNames()));
 		change.setReferencedTableName(foreignKey.referencedTableName());
 		change.setReferencedColumnNames(String.join(",", foreignKey.referencedColumnNames()));
+
 		return change;
 	}
 
@@ -590,6 +598,7 @@ private static DropForeignKeyConstraintChange dropForeignKey(ForeignKey foreignK
 		DropForeignKeyConstraintChange change = new DropForeignKeyConstraintChange();
 		change.setConstraintName(foreignKey.name());
 		change.setBaseTableName(foreignKey.tableName());
+
 		return change;
 	}
 

diff --git a/...data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java b/...data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SchemaDiff.java
@@ -31,6 +31,7 @@
  * or delete)
  *
  * @author Kurt Niemi
+ * @author Evgenii Koba
  * @since 3.2
  */
 record SchemaDiff(List<Table> tableAdditions, List<Table> tableDeletions, List<TableDiff> tableDiffs) {
@@ -120,6 +121,7 @@ private static List<TableDiff> diffTable(Tables mappedEntities, Map<String, Tabl
 
 	private static <T> Collection<T> findDiffs(Map<String, T> baseMapping, Map<String, T> toCompareMapping,
 			Comparator<String> nameComparator) {
+
 		Map<String, T> diff = new TreeMap<>(nameComparator);
 		diff.putAll(toCompareMapping);
 		baseMapping.keySet().forEach(diff::remove);

diff --git a/...-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SqlTypeMapping.java b/...-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/SqlTypeMapping.java
@@ -25,6 +25,8 @@
  *
  * @author Kurt Niemi
  * @author Mark Paluch
+ * @author Evgenii Koba
+ * @author Jens Schauder
  * @since 3.2
  */
 @FunctionalInterface
@@ -43,12 +45,14 @@ public interface SqlTypeMapping {
 	/**
 	 * Determines a column type for Class.
 	 *
-	 * @param clazz class for which the type should be determined.
+	 * @param type 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.
+	 *
+	 * @since 3.3
 	 */
 	@Nullable
-	default String getColumnTypeByClass(Class clazz) {
+	default String getColumnType(Class<?> type) {
 		return null;
 	}
 

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
@@ -26,6 +26,7 @@
  * Models a Table for generating SQL for Schema generation.
  *
  * @author Kurt Niemi
+ * @author Evgenii Koba
  * @since 3.2
  */
 record Table(@Nullable String schema, String name, List<Column> columns, List<ForeignKey> foreignKeys) {

diff --git a/...-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/TableDiff.java b/...-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/schema/TableDiff.java
@@ -23,6 +23,7 @@
  * target {@link Tables}.
  *
  * @author Kurt Niemi
+ * @author Evgenii Koba
  * @since 3.2
  */
 record TableDiff(Table table, List<Column> columnsToAdd, List<Column> columnsToDrop, List<ForeignKey> fkToAdd,

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
@@ -26,6 +26,7 @@
 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;
@@ -34,11 +35,13 @@
 import org.springframework.data.relational.core.mapping.RelationalPersistentProperty;
 import org.springframework.data.relational.core.sql.SqlIdentifier;
 import org.springframework.lang.Nullable;
+import org.springframework.util.Assert;
 
 /**
  * Model class that contains Table/Column information that can be used to generate SQL for Schema generation.
  *
  * @author Kurt Niemi
+ * @author Evgenii Koba
  * @since 3.2
  */
 record Tables(List<Table> tables) {
@@ -71,7 +74,7 @@ public static Tables from(Stream<? extends RelationalPersistentEntity<?>> persis
 						}
 
 						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;
@@ -92,34 +95,40 @@ public static Tables empty() {
 	private static void applyForeignKeyMetadata(List<Table> tables, List<ForeignKeyMetadata> foreignKeyMetadataList) {
 
 		foreignKeyMetadataList.forEach(foreignKeyMetadata -> {
-			Table table = tables.stream().filter(t -> t.name().equals(foreignKeyMetadata.tableName)).findAny().get();
+
+			Table table = tables.stream().filter(t -> t.name().equals(foreignKeyMetadata.tableName)).findAny().orElseThrow();
 
 			List<Column> parentIdColumns = collectParentIdentityColumns(foreignKeyMetadata, foreignKeyMetadataList, tables);
 			List<String> 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));
+			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));
 				}
-				addIfAbsent(table.foreignKeys(), new ForeignKey(foreignKeyName, foreignKeyMetadata.tableName(),
-						List.of(foreignKeyMetadata.referencingColumnName()), foreignKeyMetadata.parentTableName(), parentIdColumnNames));
+				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));
+				addIfAbsent(table.columns(),
+						new Column(foreignKeyMetadata.keyColumnName(), foreignKeyMetadata.keyColumnType(), false, true));
+				addIfAbsent(table.foreignKeys(), new ForeignKey(foreignKeyName, foreignKeyMetadata.tableName(),
+						parentIdColumnNames, foreignKeyMetadata.parentTableName(), parentIdColumnNames));
 			}
 
 		});
 	}
 
-	private static  <E> void addIfAbsent(List<E> list, E... elements) {
-		for(E element : elements) {
+	private static <E> void addIfAbsent(List<E> list, E... elements) {
+
+		for (E element : elements) {
 			if (!list.contains(element)) {
 				list.add(element);
 			}
@@ -137,26 +146,28 @@ private static List<Column> collectParentIdentityColumns(ForeignKeyMetadata chil
 		excludeTables.add(child.tableName());
 
 		Table parentTable = findTableByName(tables, child.parentTableName());
-		ForeignKeyMetadata parentMetadata = findMetadataByTableName(foreignKeyMetadataList, child.parentTableName(), excludeTables);
+		ForeignKeyMetadata parentMetadata = findMetadataByTableName(foreignKeyMetadataList, child.parentTableName(),
+				excludeTables);
 		List<Column> parentIdColumns = parentTable.getIdColumns();
 
 		if (!parentIdColumns.isEmpty()) {
 			return new ArrayList<>(parentIdColumns);
-		} else if(parentMetadata == null) {
-				//mustn't happen, probably wrong entity declaration
-				return new ArrayList<>();
-		} else {
-			List<Column> 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;
 		}
+
+		Assert.state(parentMetadata != null, "parentMetadata must not be null at this stage");
+
+		List<Column> 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;
 	}
 
 	@Nullable
@@ -167,45 +178,40 @@ private static Table findTableByName(List<Table> tables, String tableName) {
 	@Nullable
 	private static ForeignKeyMetadata findMetadataByTableName(List<ForeignKeyMetadata> metadata, String tableName,
 			Set<String> excludeTables) {
+
 		return metadata.stream()
-				.filter(m -> m.tableName().equals(tableName) && !excludeTables.contains(m.parentTableName()))
-				.findAny()
+				.filter(m -> m.tableName().equals(tableName) && !excludeTables.contains(m.parentTableName())).findAny()
 				.orElse(null);
 	}
 
-	private static ForeignKeyMetadata createForeignKeyMetadata(
-			RelationalPersistentEntity<?> entity,
+	private static ForeignKeyMetadata createForeignKeyMetadata(RelationalPersistentEntity<?> entity,
 			RelationalPersistentProperty property,
 			MappingContext<? extends RelationalPersistentEntity<?>, ? extends RelationalPersistentProperty> context,
 			SqlTypeMapping sqlTypeMapping) {
 
 		RelationalPersistentEntity childEntity = context.getRequiredPersistentEntity(property.getActualType());
 
 		String referencedKeyColumnType = null;
-		if(property.isAnnotationPresent(MappedCollection.class)) {
+		if (property.isAnnotationPresent(MappedCollection.class)) {
 			if (property.getType() == List.class) {
-				referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(Integer.class);
+				referencedKeyColumnType = sqlTypeMapping.getColumnType(Integer.class);
 			} else if (property.getType() == Map.class) {
-				referencedKeyColumnType = sqlTypeMapping.getColumnTypeByClass(property.getComponentType());
+				referencedKeyColumnType = sqlTypeMapping.getColumnType(property.getComponentType());
 			}
 		}
 
-		return new ForeignKeyMetadata(
-				childEntity.getTableName().getReference(),
+		return new ForeignKeyMetadata(childEntity.getTableName().getReference(),
 				property.getReverseColumnName(entity).getReference(),
 				Optional.ofNullable(property.getKeyColumn()).map(SqlIdentifier::getReference).orElse(null),
-				referencedKeyColumnType,
-				entity.getTableName().getReference()
-		);
+				referencedKeyColumnType, entity.getTableName().getReference());
 	}
 
-	//TODO should we place it in BasicRelationalPersistentProperty/BasicRelationalPersistentEntity and generate using NamingStrategy?
 	private static String getForeignKeyName(String referencedTableName, List<String> 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) {
+			@Nullable String keyColumnType, String parentTableName) {
 
 	}
 }
diff --git a/...ringframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterIntegrationTests.java b/...ringframework/data/jdbc/core/mapping/schema/LiquibaseChangeSetWriterIntegrationTests.java
@@ -55,6 +55,7 @@
  * Integration tests for {@link LiquibaseChangeSetWriter}.
  *
  * @author Mark Paluch
+ * @author Evgenii Koba
  */
 class LiquibaseChangeSetWriterIntegrationTests {