From 7415e294ee290691037418363ea0e4a5b73451f4 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 5 Jun 2019 16:10:04 +0200 Subject: [PATCH] DATACASS-651 - Fix DataType resolution for TupleValue in Maps. We now correctly resolve Map key and value types if they are raw/mapped tuple values. Previously, we inspected only the first type without considering whether the property is a map/collection. Resolution of types is now recursive and considers whether the property is a map or a collection. We also consistently use MappingException to indicate type resolution failures. --- .../convert/MappingCassandraConverter.java | 6 +- .../core/mapping/CassandraMappingContext.java | 152 ++++++++++++------ ...ssandraConverterTupleIntegrationTests.java | 92 +++++++++-- .../CassandraMappingContextUnitTests.java | 71 +++++--- 4 files changed, 241 insertions(+), 80 deletions(-) diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverter.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverter.java index e9342e65a..8bf95f144 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverter.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverter.java @@ -730,13 +730,15 @@ public class MappingCassandraConverter extends AbstractCassandraConverter return getPropertyTargetType(property); }); - } private Class getPropertyTargetType(CassandraPersistentProperty property) { - DataType dataType = getMappingContext().getDataType(property); + if (property.isCollectionLike() || property.isMapLike()) { + return property.getType(); + } + DataType dataType = getMappingContext().getDataType(property); if (dataType instanceof UserType || dataType instanceof TupleType) { return property.getType(); } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContext.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContext.java index 7381278ab..a18ab047d 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContext.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContext.java @@ -15,19 +15,9 @@ */ package org.springframework.data.cassandra.core.mapping; -import static org.springframework.data.cassandra.core.cql.keyspace.CreateTableSpecification.createTable; +import static org.springframework.data.cassandra.core.cql.keyspace.CreateTableSpecification.*; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.Set; +import java.util.*; import java.util.function.Supplier; import java.util.stream.StreamSupport; @@ -36,6 +26,7 @@ import org.springframework.beans.factory.BeanClassLoaderAware; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; import org.springframework.core.annotation.AnnotatedElementUtils; +import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.cassandra.core.convert.CassandraCustomConversions; import org.springframework.data.cassandra.core.cql.CqlIdentifier; import org.springframework.data.cassandra.core.cql.keyspace.CreateIndexSpecification; @@ -59,6 +50,8 @@ import org.springframework.util.StringUtils; import com.datastax.driver.core.DataType; import com.datastax.driver.core.DataType.Name; import com.datastax.driver.core.TupleType; +import com.datastax.driver.core.TupleValue; +import com.datastax.driver.core.UDTValue; /** * Default implementation of a {@link MappingContext} for Cassandra using {@link CassandraPersistentEntity} and @@ -76,8 +69,7 @@ public class CassandraMappingContext private @Nullable ApplicationContext applicationContext; - private CassandraPersistentEntityMetadataVerifier verifier = - new CompositeCassandraPersistentEntityMetadataVerifier(); + private CassandraPersistentEntityMetadataVerifier verifier = new CompositeCassandraPersistentEntityMetadataVerifier(); private @Nullable ClassLoader beanClassLoader; @@ -547,7 +539,6 @@ public class CassandraMappingContext return getDataTypeWithUserTypeFactory(property, DataTypeProvider.EntityUserType); } - @Nullable private DataType getDataTypeWithUserTypeFactory(CassandraPersistentProperty property, DataTypeProvider dataTypeProvider) { @@ -584,59 +575,124 @@ public class CassandraMappingContext return property.getDataType(); } - return getDataTypeWithUserTypeFactory(property.getTypeInformation(), dataTypeProvider, property::getDataType); + if (TupleValue.class.isAssignableFrom(property.getType())) { + throw new MappingException(String.format( + "Unsupported raw TupleType to DataType for property [%s] in entity [%s]; Consider adding @CassandraType.", + property.getName(), property.getOwner().getName())); + } + + if (UDTValue.class.isAssignableFrom(property.getType())) { + throw new MappingException(String.format( + "Unsupported raw UDTValue to DataType for property [%s] in entity [%s]; Consider adding @CassandraType.", + property.getName(), property.getOwner().getName())); + } + + try { + DataType dataType = getDataTypeWithUserTypeFactory(property.getTypeInformation(), dataTypeProvider, + property::getDataType); + + if (dataType == null) { + throw new MappingException( + String.format("Cannot resolve DataType for property [%s] in entity [%s]; Consider adding @CassandraType.", + property.getName(), property.getOwner().getName())); + } + + return dataType; + } catch (InvalidDataAccessApiUsageException e) { + throw new MappingException(String.format("%s. Consider adding @CassandraType.", e.getMessage()), e); + } } @Nullable private DataType getDataTypeWithUserTypeFactory(TypeInformation typeInformation, DataTypeProvider dataTypeProvider, Supplier fallback) { - BasicCassandraPersistentEntity persistentEntity = getPersistentEntity(typeInformation.getRequiredActualType()); + Optional customWriteTarget = this.customConversions.getCustomWriteTarget(typeInformation.getType()) + .map(it -> doGetDataType(typeInformation.getType(), it)); + + DataType dataType = customWriteTarget.orElseGet(() -> { + + Class propertyType = typeInformation.getRequiredActualType().getType(); + + return this.customConversions.getCustomWriteTarget(propertyType).filter(it -> !typeInformation.isMap()) + .map(it -> { + + if (typeInformation.isCollectionLike()) { + if (List.class.isAssignableFrom(typeInformation.getType())) { + return DataType.list(doGetDataType(propertyType, it)); + } + + if (Set.class.isAssignableFrom(typeInformation.getType())) { + return DataType.set(doGetDataType(propertyType, it)); + } + } + + return doGetDataType(propertyType, it); + + }).orElse(null); + }); + + if (dataType != null) { + return dataType; + } + + if (typeInformation.isCollectionLike()) { + + TypeInformation componentType = typeInformation.getRequiredActualType(); + BasicCassandraPersistentEntity persistentEntity = getPersistentEntity(componentType); + TypeInformation typeToUse = persistentEntity != null ? persistentEntity.getTypeInformation() : componentType; + + if (List.class.isAssignableFrom(typeInformation.getType())) { + return DataType.list(getDataTypeWithUserTypeFactory(typeToUse, dataTypeProvider, fallback)); + } + + if (Set.class.isAssignableFrom(typeInformation.getType())) { + return DataType.set(getDataTypeWithUserTypeFactory(typeToUse, dataTypeProvider, fallback)); + } + + throw new IllegalArgumentException("Unsupported collection type: " + typeInformation); + } + + if (typeInformation.isMap()) { + return getMapDataType(typeInformation, dataTypeProvider); + } + + BasicCassandraPersistentEntity persistentEntity = getPersistentEntity(typeInformation); if (persistentEntity != null) { if (persistentEntity.isUserDefinedType()) { - DataType dataType = getUserDataType(typeInformation, dataTypeProvider.getDataType(persistentEntity)); + DataType udtType = dataTypeProvider.getDataType(persistentEntity); - if (dataType != null) { - return dataType; + if (udtType != null) { + return udtType; } + } else if (persistentEntity.isTupleType()) { + return getTupleType(dataTypeProvider, persistentEntity); } - if (persistentEntity.isTupleType()) { - return getUserDataType(typeInformation, getTupleType(dataTypeProvider, persistentEntity)); - } + return dataTypeProvider.getDataType(persistentEntity); } - Optional customWriteTarget = this.customConversions.getCustomWriteTarget(typeInformation.getType()) - .map(it -> doGetDataType(typeInformation.getType(), it)); + DataType determinedType = doGetDataType(typeInformation); - DataType dataType = customWriteTarget - .orElseGet(() -> { + if (determinedType != null) { + return determinedType; + } - Class propertyType = typeInformation.getRequiredActualType().getType(); + return fallback.get(); + } - return this.customConversions.getCustomWriteTarget(propertyType).filter(it -> !typeInformation.isMap()) - .map(it -> { - - if (typeInformation.isCollectionLike()) { - if (List.class.isAssignableFrom(typeInformation.getType())) { - return DataType.list(doGetDataType(propertyType, it)); - } - - if (Set.class.isAssignableFrom(typeInformation.getType())) { - return DataType.set(doGetDataType(propertyType, it)); - } - } - - return doGetDataType(propertyType, it); - - }).orElse(null); - }); - - return dataType != null ? dataType - : typeInformation.isMap() ? getMapDataType(typeInformation, dataTypeProvider) : fallback.get(); + /** + * Resolve Cassandra {@link DataType}. + * + * @param typeInformation + * @return + */ + @Nullable + private DataType doGetDataType(TypeInformation typeInformation) { + return doGetDataType(typeInformation.getType(), typeInformation.getType()); } /** diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverterTupleIntegrationTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverterTupleIntegrationTests.java index 7b66b8237..6ebf9e8ee 100644 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverterTupleIntegrationTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/MappingCassandraConverterTupleIntegrationTests.java @@ -23,19 +23,19 @@ import java.util.Arrays; import java.util.Collections; import java.util.Currency; import java.util.List; +import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Configuration; import org.springframework.core.convert.converter.Converter; import org.springframework.data.annotation.Id; import org.springframework.data.cassandra.config.SchemaAction; -import org.springframework.data.cassandra.core.cql.generator.CreateTableCqlGenerator; import org.springframework.data.cassandra.core.cql.generator.CreateUserTypeCqlGenerator; -import org.springframework.data.cassandra.core.cql.keyspace.CreateTableSpecification; import org.springframework.data.cassandra.core.cql.keyspace.CreateUserTypeSpecification; import org.springframework.data.cassandra.core.mapping.CassandraMappingContext; import org.springframework.data.cassandra.core.mapping.Element; @@ -49,8 +49,12 @@ import org.springframework.data.convert.CustomConversions; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import com.datastax.driver.core.DataType; import com.datastax.driver.core.ResultSet; +import com.datastax.driver.core.Row; import com.datastax.driver.core.Session; +import com.datastax.driver.core.TupleType; +import com.datastax.driver.core.TupleValue; import com.datastax.driver.core.querybuilder.Insert; import com.datastax.driver.core.querybuilder.QueryBuilder; @@ -103,15 +107,39 @@ public class MappingCassandraConverterTupleIntegrationTests extends AbstractSpri this.session.execute(CreateUserTypeCqlGenerator.toCql(createAddress)); - CreateTableSpecification createPerson = mappingContext - .getCreateTableSpecificationFor(mappingContext.getRequiredPersistentEntity(Person.class)); - - this.session.execute(CreateTableCqlGenerator.toCql(createPerson)); + String ddl = "CREATE TABLE person (id text, " + "tuplevalue tuple," // + + "mapoftuples map, text>>>, " // + + "mapoftuplevalues map>>, " // + + "mappedtuple frozen, text>>, " // + + "mappedtuples list, text>>>, " // + + "PRIMARY KEY (id));"; + this.session.execute(ddl); } else { this.session.execute("TRUNCATE person;"); } } + @Test // DATACASS-651 + public void shouldInsertRowWithTuple() { + + TupleType tupleType = this.session.getCluster().getMetadata().newTupleType(DataType.varchar(), DataType.cint()); + + Person person = new Person(); + + person.setId("foo"); + person.setTupleValue(tupleType.newValue("hello", 42)); + + Insert insert = QueryBuilder.insertInto("person"); + + this.converter.write(person, insert); + this.session.execute(insert); + + ResultSet rows = this.session.execute("SELECT * FROM person"); + Row row = rows.one(); + + assertThat(row.getObject("tuplevalue")).isEqualTo(person.getTupleValue()); + } + @Test // DATACASS-523 public void shouldInsertRowWithComplexTuple() { @@ -141,8 +169,7 @@ public class MappingCassandraConverterTupleIntegrationTests extends AbstractSpri @Test // DATACASS-523 public void shouldReadRowWithComplexTuple() { - this.session.execute("INSERT INTO person (id,mappedtuple,mappedtuples) VALUES (" - + "'foo'," // + this.session.execute("INSERT INTO person (id,mappedtuple,mappedtuples) VALUES (" + "'foo'," // + "({zip:'myzip'},['EUR','USD'],'bar')," // + "[({zip:'myzip'},['EUR','USD'],'bar')]);\n"); @@ -161,14 +188,62 @@ public class MappingCassandraConverterTupleIntegrationTests extends AbstractSpri assertThat(mappedTuple.getCurrency()).containsSequence(Currency.getInstance("EUR"), Currency.getInstance("USD")); } + @Test // DATACASS-651 + public void shouldInsertRowWithTupleMap() { + + Person person = new Person(); + person.setId("foo"); + + MappedTuple tuple = new MappedTuple(); + tuple.setCurrency(Arrays.asList(Currency.getInstance("EUR"), Currency.getInstance("USD"))); + tuple.setName("bar"); + + person.setMapOfTuples(Collections.singletonMap("foo", tuple)); + + TupleType tupleType = this.session.getCluster().getMetadata().newTupleType(DataType.varchar(), DataType.cint()); + person.setMapOfTupleValues(Collections.singletonMap("mykey", tupleType.newValue("hello", 42))); + + Insert insert = QueryBuilder.insertInto("person"); + + this.converter.write(person, insert); + this.session.execute(insert); + + ResultSet rows = this.session.execute("SELECT * FROM person"); + Row row = rows.one(); + + assertThat(row.getObject("mapoftuples")).isInstanceOf(Map.class); + assertThat(row.getObject("mapoftuplevalues")).isInstanceOf(Map.class); + } + + @Test // DATACASS-651 + public void shouldReadRowWithMapOfTuples() { + + this.session.execute("INSERT INTO person (id,mapoftuples,mapoftuplevalues) VALUES " + + "('foo',{'foo':(NULL,['EUR','USD'],'bar')},{'mykey':('hello',42)});\n"); + + ResultSet resultSet = this.session.execute("SELECT * FROM person;"); + + Person person = this.converter.read(Person.class, resultSet.one()); + + assertThat(person.getMapOfTupleValues()).hasSize(1); + assertThat(person.getMapOfTupleValues().get("mykey").getString(0)).isEqualTo("hello"); + + MappedTuple mappedTuple = person.getMapOfTuples().get("foo"); + assertThat(mappedTuple.getName()).isEqualTo("bar"); + assertThat(mappedTuple.getCurrency()).containsSequence(Currency.getInstance("EUR"), Currency.getInstance("USD")); + } + @Data @Table static class Person { @Id private String id; + TupleValue tupleValue; MappedTuple mappedTuple; List mappedTuples; + Map mapOfTuples; + Map mapOfTupleValues; } @Data @@ -178,7 +253,6 @@ public class MappingCassandraConverterTupleIntegrationTests extends AbstractSpri @Element(0) AddressUserType addressUserType; @Element(1) List currency; @Element(2) String name; - } @UserDefinedType("address") diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContextUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContextUnitTests.java index 81c9ffe2a..bb9fc517a 100755 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContextUnitTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/mapping/CassandraMappingContextUnitTests.java @@ -15,10 +15,8 @@ */ package org.springframework.data.cassandra.core.mapping; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.fail; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; import java.io.Serializable; import java.util.Collection; @@ -29,8 +27,8 @@ import java.util.NoSuchElementException; import org.junit.Before; import org.junit.Test; + import org.springframework.core.convert.converter.Converter; -import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.annotation.Id; import org.springframework.data.cassandra.core.convert.CassandraCustomConversions; import org.springframework.data.cassandra.core.cql.CqlIdentifier; @@ -366,10 +364,16 @@ public class CassandraMappingContextUnitTests { assertThat(entries.getColumnFunction()).isEqualTo(ColumnFunction.NONE); } - @Test(expected = InvalidDataAccessApiUsageException.class) // DATACASS-284 + @Test // DATACASS-284, DATACASS-651 public void shouldRejectUntypedTuples() { - this.mappingContext - .getCreateTableSpecificationFor(this.mappingContext.getRequiredPersistentEntity(UntypedTupleEntity.class)); + + assertThatThrownBy(() -> this.mappingContext + .getCreateTableSpecificationFor(this.mappingContext.getRequiredPersistentEntity(UntypedTupleEntity.class))) + .isInstanceOf(MappingException.class); + + assertThatThrownBy(() -> this.mappingContext + .getCreateTableSpecificationFor(this.mappingContext.getRequiredPersistentEntity(UntypedTupleMapEntity.class))) + .isInstanceOf(MappingException.class); } @Test // DATACASS-284 @@ -387,6 +391,21 @@ public class CassandraMappingContextUnitTests { CodecRegistry.DEFAULT_INSTANCE, DataType.varchar(), DataType.bigint())); } + @Test // DATACASS-651 + public void shouldCreateTableForEntityWithMapOfTuples() { + + CreateTableSpecification tableSpecification = this.mappingContext + .getCreateTableSpecificationFor(this.mappingContext.getRequiredPersistentEntity(EntityWithMapOfTuples.class)); + + assertThat(tableSpecification.getColumns()).hasSize(2); + + ColumnSpecification column = tableSpecification.getColumns().get(1); + + assertThat(column.getType()).isInstanceOf(DataType.CollectionType.class); + assertThat(column.getType()).isEqualTo(DataType.map(DataType.text(), + TupleType.of(ProtocolVersion.NEWEST_SUPPORTED, CodecRegistry.DEFAULT_INSTANCE, DataType.text()))); + } + private static CreateIndexSpecification getSpecificationFor(String column, List specifications) { @@ -456,8 +475,8 @@ public class CassandraMappingContextUnitTests { mappingContext.setCustomConversions( new CassandraCustomConversions(Collections.singletonList(HumanToStringConverter.INSTANCE))); - CassandraPersistentEntity persistentEntity = - mappingContext.getRequiredPersistentEntity(TypeWithListOfHumans.class); + CassandraPersistentEntity persistentEntity = mappingContext + .getRequiredPersistentEntity(TypeWithListOfHumans.class); assertThat(mappingContext.getDataType(persistentEntity.getRequiredPersistentProperty("humans"))) .isEqualTo(DataType.list(DataType.varchar())); @@ -466,8 +485,7 @@ public class CassandraMappingContextUnitTests { @Test // DATACASS-302 public void propertyTypeShouldMapToTime() { - CassandraPersistentEntity persistentEntity = - mappingContext.getRequiredPersistentEntity(AllPossibleTypes.class); + CassandraPersistentEntity persistentEntity = mappingContext.getRequiredPersistentEntity(AllPossibleTypes.class); assertThat(mappingContext.getDataType(persistentEntity.getRequiredPersistentProperty("localTime"))) .isEqualTo(DataType.time()); @@ -490,8 +508,7 @@ public class CassandraMappingContextUnitTests { @Test // DATACASS-523 public void shouldCreateMappedTupleType() { - CassandraPersistentEntity persistentEntity = - this.mappingContext.getRequiredPersistentEntity(MappedTuple.class); + CassandraPersistentEntity persistentEntity = this.mappingContext.getRequiredPersistentEntity(MappedTuple.class); assertThat(persistentEntity).isInstanceOf(BasicCassandraPersistentTupleEntity.class); @@ -566,24 +583,24 @@ public class CassandraMappingContextUnitTests { try { mappingContext.getCreateTableSpecificationFor( mappingContext.getRequiredPersistentEntity(EntityWithComplexPrimaryKeyColumn.class)); - fail("Missing InvalidDataAccessApiUsageException"); - } catch (InvalidDataAccessApiUsageException e) { + fail("Missing MappingException"); + } catch (MappingException e) { assertThat(e).hasMessageContaining("Unknown type [class java.lang.Object] for property [complexObject]"); } try { mappingContext .getCreateTableSpecificationFor(mappingContext.getRequiredPersistentEntity(EntityWithComplexId.class)); - fail("Missing InvalidDataAccessApiUsageException"); - } catch (InvalidDataAccessApiUsageException e) { + fail("Missing MappingException"); + } catch (MappingException e) { assertThat(e).hasMessageContaining("Unknown type [class java.lang.Object] for property [complexObject]"); } try { mappingContext.getCreateTableSpecificationFor( mappingContext.getRequiredPersistentEntity(EntityWithPrimaryKeyClassWithComplexId.class)); - fail("Missing InvalidDataAccessApiUsageException"); - } catch (InvalidDataAccessApiUsageException e) { + fail("Missing MappingException"); + } catch (MappingException e) { assertThat(e).hasMessageContaining("Unknown type [class java.lang.Object] for property [complexObject]"); } } @@ -719,9 +736,21 @@ public class CassandraMappingContextUnitTests { @CassandraType(type = Name.TUPLE, typeArguments = { Name.VARCHAR, Name.BIGINT }) TupleValue typed; } + @Table + static class EntityWithMapOfTuples { + @Id String id; + Map map; + } + @Table static class UntypedTupleEntity { @Id String id; - TupleType untyped; + TupleValue untyped; + } + + @Table + static class UntypedTupleMapEntity { + @Id String id; + Map untyped; } }