From 15a7f0de0d7ec2f6022c89b64cc070fe04dc7be1 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 20 Sep 2019 11:10:47 +0200 Subject: [PATCH] DATACASS-687 - Fix UDT creation order. The ordering of UDT now considers all UDTs before creating the final creation order to consider dependencies between UDTs that refer to nested UDTs. Previously, UDT ordering was already built from iteration over individual UDTs so that UDTs that had dependencies on already seen UDTs wheren't considered properly. --- ...assandraPersistentEntitySchemaCreator.java | 93 ++++++++++++++----- ...ersistentEntitySchemaCreatorUnitTests.java | 85 ++++++++++++++--- 2 files changed, 146 insertions(+), 32 deletions(-) diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreator.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreator.java index daea2d230..a4146e067 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreator.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreator.java @@ -19,12 +19,14 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashSet; -import java.util.LinkedHashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; +import org.jetbrains.annotations.NotNull; + import org.springframework.data.cassandra.core.cql.CqlIdentifier; import org.springframework.data.cassandra.core.cql.generator.CreateIndexCqlGenerator; import org.springframework.data.cassandra.core.cql.generator.CreateTableCqlGenerator; @@ -36,6 +38,7 @@ import org.springframework.data.cassandra.core.mapping.BasicCassandraPersistentE import org.springframework.data.cassandra.core.mapping.CassandraMappingContext; import org.springframework.data.cassandra.core.mapping.CassandraPersistentEntity; import org.springframework.data.cassandra.core.mapping.CassandraPersistentProperty; +import org.springframework.data.util.Streamable; import org.springframework.util.Assert; /** @@ -152,41 +155,89 @@ public class CassandraPersistentEntitySchemaCreator { .collect(Collectors.toMap(CassandraPersistentEntity::getTableName, entity -> entity)); List specifications = new ArrayList<>(); - - Set created = new HashSet<>(); + UserDefinedTypeSet udts = new UserDefinedTypeSet(); entities.forEach(entity -> { - Set seen = new LinkedHashSet<>(); - - seen.add(entity.getTableName()); - visitUserTypes(entity, seen); - - List ordered = new ArrayList<>(seen); - - Collections.reverse(ordered); - - specifications.addAll(ordered - .stream().filter(created::add).map(identifier -> this.mappingContext - .getCreateUserTypeSpecificationFor(byTableName.get(identifier)).ifNotExists(ifNotExists)) - .collect(Collectors.toList())); + udts.add(entity.getTableName()); + visitUserTypes(entity, udts); }); + specifications + .addAll(udts + .stream().map(identifier -> this.mappingContext + .getCreateUserTypeSpecificationFor(byTableName.get(identifier)).ifNotExists(ifNotExists)) + .collect(Collectors.toList())); + return specifications; } - private void visitUserTypes(CassandraPersistentEntity entity, final Set seen) { + private void visitUserTypes(CassandraPersistentEntity entity, UserDefinedTypeSet udts) { for (CassandraPersistentProperty property : entity) { - BasicCassandraPersistentEntity persistentEntity = this.mappingContext.getPersistentEntity(property); + BasicCassandraPersistentEntity propertyType = this.mappingContext.getPersistentEntity(property); - if (persistentEntity == null) { + if (propertyType == null) { continue; } - if (persistentEntity.isUserDefinedType() && seen.add(persistentEntity.getTableName())) { - visitUserTypes(persistentEntity, seen); + if (propertyType.isUserDefinedType()) { + if (udts.add(propertyType.getTableName())) { + visitUserTypes(propertyType, udts); + } else { + udts.updateDependency(entity.getTableName(), propertyType.getTableName()); + } + } + } + } + + /** + * Object to record dependencies and report them in the order of creation. + */ + static class UserDefinedTypeSet implements Streamable { + + private final Set seen = new HashSet<>(); + private final List creationOrder = new ArrayList<>(); + + public boolean add(CqlIdentifier cqlIdentifier) { + + if (seen.add(cqlIdentifier)) { + creationOrder.add(cqlIdentifier); + return true; + } + + return false; + } + + @NotNull + @Override + public Iterator iterator() { + + List reverseCreationOrder = new ArrayList<>(creationOrder); + Collections.reverse(reverseCreationOrder); + + return reverseCreationOrder.iterator(); + } + + /** + * Checks the dependency order. {@code referent} depends on {@code reference} and we need to make sure that + * {@code referent} gets created after {@code reference}. + *

+ * This method therefore updates the dependency order. + * + * @param referent the client of {@code reference}. + * @param reference the dependency required by {@code referent}. + */ + void updateDependency(CqlIdentifier referent, CqlIdentifier reference) { + + int referentIndex = creationOrder.indexOf(referent); + int referenceIndex = creationOrder.indexOf(reference); + + if (referentIndex > referenceIndex) { + + creationOrder.remove(referent); + creationOrder.add(referenceIndex, referent); } } } diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreatorUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreatorUnitTests.java index 074fb9d9b..b07441b83 100644 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreatorUnitTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/CassandraPersistentEntitySchemaCreatorUnitTests.java @@ -15,9 +15,16 @@ */ package org.springframework.data.cassandra.core; -import static org.mockito.ArgumentMatchers.matches; +import static org.assertj.core.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.stream.Collectors; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -25,8 +32,14 @@ import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; + +import org.springframework.data.cassandra.core.cql.CqlIdentifier; import org.springframework.data.cassandra.core.cql.CqlOperations; +import org.springframework.data.cassandra.core.cql.keyspace.CreateUserTypeSpecification; +import org.springframework.data.cassandra.core.cql.keyspace.UserTypeNameSpecification; import org.springframework.data.cassandra.core.mapping.CassandraMappingContext; +import org.springframework.data.cassandra.core.mapping.CassandraPersistentEntity; +import org.springframework.data.cassandra.core.mapping.UserDefinedType; /** * Unit tests for {@link CassandraPersistentEntitySchemaCreator}. @@ -54,14 +67,43 @@ public class CassandraPersistentEntitySchemaCreatorUnitTests extends CassandraPe when(adminOperations.getCqlOperations()).thenReturn(operations); } + @Test // DATACASS-687 + public void shouldConsiderProperUdtOrdering() { + + List> ordered = new ArrayList<>(Arrays.asList(Udt2.class, Udt1.class, RequiredByAll.class)); + + context = new CassandraMappingContext() { + @Override + public Collection> getUserDefinedTypeEntities() { + return ordered.stream().map(this::getRequiredPersistentEntity).collect(Collectors.toList()); + } + }; + + context.setUserTypeResolver(typeName -> { + // make sure that calls to this method pop up. Calling UserTypeResolver while resolving + // to be created user types isn't a good idea because they do not exist at resolution time. + throw new IllegalArgumentException(String.format("Type %s not found", typeName)); + }); + + CassandraPersistentEntitySchemaCreator schemaCreator = new CassandraPersistentEntitySchemaCreator(context, + adminOperations); + + List userTypeSpecifications = schemaCreator.createUserTypeSpecifications(false); + + List collect = userTypeSpecifications.stream().map(UserTypeNameSpecification::getName) + .collect(Collectors.toList()); + + assertThat(collect).hasSize(3).startsWith(CqlIdentifier.of("requiredbyall")); + } + @Test // DATACASS-172, DATACASS-406 public void createsCorrectTypeForSimpleTypes() { context.getPersistentEntity(MoonType.class); context.getPersistentEntity(PlanetType.class); - CassandraPersistentEntitySchemaCreator schemaCreator = - new CassandraPersistentEntitySchemaCreator(context, adminOperations); + CassandraPersistentEntitySchemaCreator schemaCreator = new CassandraPersistentEntitySchemaCreator(context, + adminOperations); schemaCreator.createUserTypes(false); @@ -73,8 +115,8 @@ public class CassandraPersistentEntitySchemaCreatorUnitTests extends CassandraPe context.getPersistentEntity(PlanetType.class); - CassandraPersistentEntitySchemaCreator schemaCreator = - new CassandraPersistentEntitySchemaCreator(context, adminOperations); + CassandraPersistentEntitySchemaCreator schemaCreator = new CassandraPersistentEntitySchemaCreator(context, + adminOperations); schemaCreator.createUserTypes(false); @@ -88,8 +130,8 @@ public class CassandraPersistentEntitySchemaCreatorUnitTests extends CassandraPe context.getPersistentEntity(SpaceAgencyType.class); - CassandraPersistentEntitySchemaCreator schemaCreator = - new CassandraPersistentEntitySchemaCreator(context, adminOperations); + CassandraPersistentEntitySchemaCreator schemaCreator = new CassandraPersistentEntitySchemaCreator(context, + adminOperations); schemaCreator.createUserTypes(false); @@ -103,8 +145,8 @@ public class CassandraPersistentEntitySchemaCreatorUnitTests extends CassandraPe context.getPersistentEntity(PlanetType.class); - CassandraPersistentEntitySchemaCreator schemaCreator = - new CassandraPersistentEntitySchemaCreator(context, adminOperations); + CassandraPersistentEntitySchemaCreator schemaCreator = new CassandraPersistentEntitySchemaCreator(context, + adminOperations); schemaCreator.createUserTypes(false); @@ -116,8 +158,8 @@ public class CassandraPersistentEntitySchemaCreatorUnitTests extends CassandraPe context.getPersistentEntity(IndexedEntity.class); - CassandraPersistentEntitySchemaCreator schemaCreator = - new CassandraPersistentEntitySchemaCreator(context, adminOperations); + CassandraPersistentEntitySchemaCreator schemaCreator = new CassandraPersistentEntitySchemaCreator(context, + adminOperations); schemaCreator.createIndexes(false); @@ -132,4 +174,25 @@ public class CassandraPersistentEntitySchemaCreatorUnitTests extends CassandraPe inOrder.verify(operations).execute(Mockito.contains("CREATE TYPE " + typename)); } } + + abstract static class AbstractModel { + private RequiredByAll attachments; + } + + @UserDefinedType + static class RequiredByAll { + private String name; + } + + @UserDefinedType + static class Udt1 { + + private RequiredByAll attachment; + } + + @UserDefinedType + static class Udt2 extends AbstractModel { + + private Udt1 u1; + } }