From 76ab62e2d348f716e199bea03cac06838c32b6d1 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 12 Apr 2017 13:58:55 +0200 Subject: [PATCH] DATACASS-420 - Do not insert null values. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now no longer insert null values via CassandraOperations.insert(…). Inserting null values creates tombstones in Cassandra which are likely unwanted and impact performance. Omitting null values simply does not create values in Cassandra. Setting properties to null with UPDATE remains unchanged and updating an object containing null values will propagate all null values to Cassandra as part of the UPDATE clause. SimpleCassandraRepository.save(…) needs to consider whether the entity is new or whether it consists only of primary key properties to propagate the optimization. New entities and entities consisting only of primary key columns are saved via INSERT. All other entities are saved via UPDATE. Related ticket: DATACASS-428. --- .../convert/MappingCassandraConverter.java | 4 + .../cassandra/core/CassandraOperations.java | 8 +- .../query/CassandraEntityInformation.java | 24 ++-- .../MappingCassandraEntityInformation.java | 31 ++++- .../support/SimpleCassandraRepository.java | 5 +- ...CassandraConverterUDTIntegrationTests.java | 16 +-- ...ngCassandraEntityInformationUnitTests.java | 106 ++++++++++++++++++ .../CassandraOperationsIntegrationTests.java | 45 ++++---- 8 files changed, 196 insertions(+), 43 deletions(-) create mode 100644 spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformationUnitTests.java diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java index 804fd7623..9c6eb3009 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/convert/MappingCassandraConverter.java @@ -358,6 +358,10 @@ public class MappingCassandraConverter extends AbstractCassandraConverter return; } + if (value == null) { + return; + } + if (log.isDebugEnabled()) { log.debug("Adding insert.value [{}] - [{}]", property.getColumnName().toCql(), value); } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraOperations.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraOperations.java index 9d4b99a80..0112dbc1c 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraOperations.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraOperations.java @@ -42,7 +42,7 @@ import com.datastax.driver.core.querybuilder.Select; *

* {@link CassandraOperations} mixes synchronous and asynchronous methods so asynchronous methods are subject to be * moved into an asynchronous Cassandra template. - * + * * @author Alex Shvid * @author David Webb * @author Matthew Adams @@ -197,7 +197,7 @@ public interface CassandraOperations extends CqlOperations { long count(Class entityClass); /** - * Insert the given entity. + * Insert the given entity without inserting {@literal null} values. * * @param entity The entity to insert * @return The entity given @@ -205,7 +205,7 @@ public interface CassandraOperations extends CqlOperations { T insert(T entity); /** - * Insert the given entity. + * Insert the given entity without inserting {@literal null} values. * * @param entity The entity to insert * @param options The {@link WriteOptions} to use. @@ -525,7 +525,7 @@ public interface CassandraOperations extends CqlOperations { /** * Deletes all entities of a given class. - * + * * @param entityClass The entity type must not be {@literal null}. */ void deleteAll(Class entityClass); diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/query/CassandraEntityInformation.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/query/CassandraEntityInformation.java index 9cc0f3069..0b8e0a66d 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/query/CassandraEntityInformation.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/query/CassandraEntityInformation.java @@ -1,12 +1,12 @@ /* - * Copyright 2013-2014 the original author or authors - * + * Copyright 2013-2017 the original author or authors + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -21,10 +21,20 @@ import org.springframework.data.repository.core.EntityInformation; /** * Cassandra specific {@link EntityInformation}. - * + * * @author Alex Shvid + * @author Mark Paluch */ -public interface CassandraEntityInformation extends EntityInformation, - CassandraEntityMetadata { +public interface CassandraEntityInformation + extends EntityInformation, CassandraEntityMetadata { + /** + * Return {@literal true} if the persistent entity consists entirely of primary key properties (a single Id property, + * composite primary key). + * + * @return {@literal true} if the persistent entity consists entirely of primary key properties (a single Id property, + * composite primary key). + * @since 1.5.2 + */ + boolean isPrimaryKeyEntity(); } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformation.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformation.java index 066903dcb..c78f17deb 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformation.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformation.java @@ -16,6 +16,7 @@ package org.springframework.data.cassandra.repository.support; import java.io.Serializable; +import java.util.concurrent.atomic.AtomicReference; import org.springframework.cassandra.core.cql.CqlIdentifier; import org.springframework.data.cassandra.convert.CassandraConverter; @@ -23,6 +24,7 @@ import org.springframework.data.cassandra.mapping.CassandraPersistentEntity; import org.springframework.data.cassandra.mapping.CassandraPersistentProperty; import org.springframework.data.cassandra.repository.MapId; import org.springframework.data.cassandra.repository.query.CassandraEntityInformation; +import org.springframework.data.mapping.PropertyHandler; import org.springframework.data.repository.core.support.AbstractEntityInformation; import org.springframework.util.Assert; @@ -33,12 +35,14 @@ import org.springframework.util.Assert; * * @author Alex Shvid * @author Matthew T. Adams + * @author Mark Paluch */ public class MappingCassandraEntityInformation extends AbstractEntityInformation implements CassandraEntityInformation { private final CassandraPersistentEntity entityMetadata; - private CassandraConverter converter; + private final CassandraConverter converter; + private final boolean isPrimaryKeyEntity; /** * Creates a new {@link MappingCassandraEntityInformation} for the given {@link CassandraPersistentEntity}. @@ -51,6 +55,7 @@ public class MappingCassandraEntityInformation exten this.entityMetadata = entity; this.converter = converter; + this.isPrimaryKeyEntity = hasNonIdProperties(entity); } @SuppressWarnings("unchecked") @@ -79,4 +84,28 @@ public class MappingCassandraEntityInformation exten public CqlIdentifier getTableName() { return entityMetadata.getTableName(); } + + @Override + public boolean isPrimaryKeyEntity() { + return isPrimaryKeyEntity; + } + + private static boolean hasNonIdProperties(CassandraPersistentEntity entity) { + + final AtomicReference hasPrimaryKeyOnlyProperties = new AtomicReference(true); + + entity.doWithProperties(new PropertyHandler() { + @Override + public void doWithPersistentProperty(CassandraPersistentProperty property) { + + if (property.isCompositePrimaryKey() || property.isPrimaryKeyColumn() || property.isIdProperty()) { + return; + } + + hasPrimaryKeyOnlyProperties.set(false); + } + }); + + return hasPrimaryKeyOnlyProperties.get(); + } } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/SimpleCassandraRepository.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/SimpleCassandraRepository.java index 8030cc9c3..3c2e3b1b5 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/SimpleCassandraRepository.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/repository/support/SimpleCassandraRepository.java @@ -39,6 +39,8 @@ public class SimpleCassandraRepository implements Ty protected CassandraOperations operations; protected CassandraEntityInformation entityInformation; + private final boolean isPrimaryKeyEntity; + /** * Creates a new {@link SimpleCassandraRepository} for the given {@link CassandraEntityInformation} and * {@link CassandraTemplate}. @@ -53,6 +55,7 @@ public class SimpleCassandraRepository implements Ty this.entityInformation = metadata; this.operations = operations; + this.isPrimaryKeyEntity = metadata.isPrimaryKeyEntity(); } /* (non-Javadoc) @@ -63,7 +66,7 @@ public class SimpleCassandraRepository implements Ty Assert.notNull(entity, "Entity must not be null"); - if (entityInformation.isNew(entity)) { + if (entityInformation.isNew(entity) || isPrimaryKeyEntity) { return operations.insert(entity); } diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/convert/MappingCassandraConverterUDTIntegrationTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/convert/MappingCassandraConverterUDTIntegrationTests.java index ca353f554..a66cb04ce 100644 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/convert/MappingCassandraConverterUDTIntegrationTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/convert/MappingCassandraConverterUDTIntegrationTests.java @@ -156,8 +156,8 @@ public class MappingCassandraConverterUDTIntegrationTests extends AbstractSpring Insert insert = QueryBuilder.insertInto("addressbook"); converter.write(addressBook, insert); - assertThat(insert.toString()).isEqualTo("INSERT INTO addressbook (alternate,currentaddress,id,previousaddresses) " - + "VALUES (null,{zip:'69469',city:'Weinheim',streetlines:['Heckenpfad','14']},'1',null);"); + assertThat(insert.toString()).isEqualTo("INSERT INTO addressbook (currentaddress,id) " + + "VALUES ({zip:'69469',city:'Weinheim',streetlines:['Heckenpfad','14']},'1');"); } @Test // DATACASS-172 @@ -193,8 +193,8 @@ public class MappingCassandraConverterUDTIntegrationTests extends AbstractSpring Insert insert = QueryBuilder.insertInto("addressbook"); converter.write(addressBook, insert); - assertThat(insert.toString()).isEqualTo("INSERT INTO addressbook (alternate,currentaddress,id,previousaddresses) " - + "VALUES (null,null,'1',[{zip:'69469',city:'Weinheim',streetlines:['Heckenpfad','14']}]);"); + assertThat(insert.toString()).isEqualTo("INSERT INTO addressbook (id,previousaddresses) " + + "VALUES ('1',[{zip:'69469',city:'Weinheim',streetlines:['Heckenpfad','14']}]);"); } @Test // DATACASS-172 @@ -228,8 +228,8 @@ public class MappingCassandraConverterUDTIntegrationTests extends AbstractSpring Insert insert = QueryBuilder.insertInto("addressbook"); converter.write(addressBook, insert); - assertThat(insert.toString()).isEqualTo("INSERT INTO addressbook (alternate,currentaddress,id,previousaddresses) " - + "VALUES ({zip:'69469',city:'Weinheim',streetlines:['Heckenpfad','14']},null,'1',null);"); + assertThat(insert.toString()).isEqualTo("INSERT INTO addressbook (alternate,id) " + + "VALUES ({zip:'69469',city:'Weinheim',streetlines:['Heckenpfad','14']},'1');"); } @Test // DATACASS-172 @@ -302,7 +302,7 @@ public class MappingCassandraConverterUDTIntegrationTests extends AbstractSpring converter.write(bank, insert); assertThat(insert.toString()) - .isEqualTo("INSERT INTO bank (currency,id,othercurrencies) VALUES ({currency:'EUR'},null,null);"); + .isEqualTo("INSERT INTO bank (currency) VALUES ({currency:'EUR'});"); } @Test // DATACASS-172 @@ -361,7 +361,7 @@ public class MappingCassandraConverterUDTIntegrationTests extends AbstractSpring converter.write(bank, insert); assertThat(insert.toString()) - .isEqualTo("INSERT INTO bank (currency,id,othercurrencies) VALUES (null,null,[{currency:'EUR'}]);"); + .isEqualTo("INSERT INTO bank (othercurrencies) VALUES ([{currency:'EUR'}]);"); } @Test // DATACASS-172 diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformationUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformationUnitTests.java new file mode 100644 index 000000000..1cda1138e --- /dev/null +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/repository/support/MappingCassandraEntityInformationUnitTests.java @@ -0,0 +1,106 @@ +/* + * Copyright 2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.cassandra.repository.support; + +import static org.assertj.core.api.Assertions.*; + +import lombok.Data; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.data.annotation.Id; +import org.springframework.data.cassandra.convert.CassandraConverter; +import org.springframework.data.cassandra.convert.MappingCassandraConverter; +import org.springframework.data.cassandra.domain.AllPossibleTypes; +import org.springframework.data.cassandra.domain.CompositeKey; +import org.springframework.data.cassandra.domain.TypeWithKeyClass; +import org.springframework.data.cassandra.domain.TypeWithMapId; +import org.springframework.data.cassandra.mapping.BasicCassandraMappingContext; +import org.springframework.data.cassandra.mapping.UserTypeResolver; + +/** + * Unit tests for {@link MappingCassandraEntityInformation}. + * + * @author Mark Paluch + */ +@RunWith(MockitoJUnitRunner.class) +@SuppressWarnings("unchecked") +public class MappingCassandraEntityInformationUnitTests { + + BasicCassandraMappingContext context = new BasicCassandraMappingContext(); + + CassandraConverter converter = new MappingCassandraConverter(context); + + @Mock UserTypeResolver userTypeResolver; + + @Before + public void before() { + context.setUserTypeResolver(userTypeResolver); + } + + @Test // DATACASS-420 + public void shouldConsiderSimpleIdEntityAsPrimaryKeyOnly() { + + MappingCassandraEntityInformation information = new MappingCassandraEntityInformation( + context.getPersistentEntity(PrimaryKeyOnly.class), converter); + + assertThat(information.isPrimaryKeyEntity()).isTrue(); + } + + @Test // DATACASS-420 + public void shouldConsiderCompositeIdEntityAsPrimaryKeyOnly() { + + MappingCassandraEntityInformation information = new MappingCassandraEntityInformation( + context.getPersistentEntity(CompositeKey.class), converter); + + assertThat(information.isPrimaryKeyEntity()).isTrue(); + } + + @Test // DATACASS-420 + public void shouldConsiderCompositeKeyClassEntityAsPrimaryKeyOnly() { + + MappingCassandraEntityInformation information = new MappingCassandraEntityInformation( + context.getPersistentEntity(TypeWithKeyClass.class), converter); + + assertThat(information.isPrimaryKeyEntity()).isTrue(); + } + + @Test // DATACASS-420 + public void shouldConsiderMapIdClassEntityAsPrimaryKeyOnly() { + + MappingCassandraEntityInformation information = new MappingCassandraEntityInformation( + context.getPersistentEntity(TypeWithMapId.class), converter); + + assertThat(information.isPrimaryKeyEntity()).isTrue(); + } + + @Test // DATACASS-420 + public void shouldComplexEntityNotAsPrimaryKeyOnly() { + + MappingCassandraEntityInformation information = new MappingCassandraEntityInformation( + context.getPersistentEntity(AllPossibleTypes.class), converter); + + assertThat(information.isPrimaryKeyEntity()).isFalse(); + } + + @Data + static class PrimaryKeyOnly { + @Id String id; + } +} diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/test/integration/core/CassandraOperationsIntegrationTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/test/integration/core/CassandraOperationsIntegrationTests.java index 38e0ae81e..36cdf9c2f 100755 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/test/integration/core/CassandraOperationsIntegrationTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/test/integration/core/CassandraOperationsIntegrationTests.java @@ -627,27 +627,8 @@ public class CassandraOperationsIntegrationTests extends AbstractKeyspaceCreatin assertThat(template.count(Book.class)).isEqualTo(count); } - @Test // DATACASS-182 - public void updateShouldRemoveFields() { - - Book book = new Book(); - book.setIsbn("isbn"); - book.setTitle("title"); - book.setAuthor("author"); - - template.insert(book); - - book.setTitle(null); - template.update(book); - - Book loaded = template.selectOneById(Book.class, book.getIsbn()); - - assertThat(loaded.getTitle()).isNull(); - assertThat(loaded.getAuthor()).isEqualTo("author"); - } - - @Test // DATACASS-182 - public void insertShouldRemoveFields() { + @Test // DATACASS-182, DATACASS-420 + public void insertShouldNotRemoveFields() { Book book = new Book(); book.setIsbn("isbn"); @@ -662,7 +643,7 @@ public class CassandraOperationsIntegrationTests extends AbstractKeyspaceCreatin Book loaded = template.selectOneById(Book.class, book.getIsbn()); - assertThat(loaded.getTitle()).isNull(); + assertThat(loaded.getTitle()).isEqualTo("title"); assertThat(loaded.getAuthor()).isEqualTo("author"); } @@ -683,6 +664,26 @@ public class CassandraOperationsIntegrationTests extends AbstractKeyspaceCreatin assertThat(loaded.getTitle()).isEqualTo("title"); } + @Test // DATACASS-182, DATACASS-420 + public void updateShouldRemoveFields() { + + Book book = new Book(); + book.setIsbn("isbn"); + book.setTitle("title"); + book.setAuthor("author"); + + template.insert(book); + + book.setTitle(null); + + template.update(book); + + Book loaded = template.selectOneById(Book.class, book.getIsbn()); + + assertThat(loaded.getTitle()).isNull(); + assertThat(loaded.getAuthor()).isEqualTo("author"); + } + @Test // DATACASS-182 public void insertAndUpdateToEmptyCollection() {