From 510a2417ffc8de39cc70d40f983e8d25a137bb1c Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Wed, 9 Dec 2020 19:16:34 -0500 Subject: [PATCH 1/8] optimistic locking --- .../GcpFirestoreAutoConfiguration.java | 4 +- .../data/firestore/FirestoreTemplate.java | 33 ++- .../mapping/FirestoreClassMapper.java | 4 + .../mapping/FirestoreDefaultClassMapper.java | 34 ++- .../mapping/FirestorePersistentEntity.java | 2 + .../FirestorePersistentEntityImpl.java | 19 ++ .../data/firestore/mapping/UpdateTime.java | 29 +++ .../firestore/FirestoreTemplateTests.java | 203 ++++++++++++++++-- .../spring/data/firestore/entities/User.java | 19 +- .../it/FirestoreIntegrationTests.java | 45 ++++ ...irestoreIntegrationTestsConfiguration.java | 4 +- .../query/FirestoreRepositoryTests.java | 4 +- .../query/PartTreeFirestoreQueryTests.java | 2 +- ...activeFirestoreTransactionManagerTest.java | 4 +- 14 files changed, 373 insertions(+), 33 deletions(-) create mode 100644 spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java diff --git a/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java b/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java index c2200cd95d..30d62429ad 100644 --- a/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java +++ b/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java @@ -131,8 +131,8 @@ public FirestoreMappingContext firestoreMappingContext() { @Bean @ConditionalOnMissingBean - public FirestoreClassMapper getClassMapper() { - return new FirestoreDefaultClassMapper(); + public FirestoreClassMapper getClassMapper(FirestoreMappingContext mappingContext) { + return new FirestoreDefaultClassMapper(mappingContext); } @Bean diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java index c17f8bbbe4..8f05a7bafe 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java @@ -22,10 +22,12 @@ import java.util.function.Consumer; import java.util.function.Function; +import com.google.cloud.Timestamp; import com.google.cloud.spring.data.firestore.mapping.FirestoreClassMapper; import com.google.cloud.spring.data.firestore.mapping.FirestoreMappingContext; import com.google.cloud.spring.data.firestore.mapping.FirestorePersistentEntity; import com.google.cloud.spring.data.firestore.mapping.FirestorePersistentProperty; +import com.google.cloud.spring.data.firestore.mapping.UpdateTime; import com.google.cloud.spring.data.firestore.transaction.ReactiveFirestoreResourceHolder; import com.google.cloud.spring.data.firestore.util.ObservableReactiveUtil; import com.google.cloud.spring.data.firestore.util.Util; @@ -208,7 +210,7 @@ public Flux saveAll(Publisher instances) { //In a transaction, all write operations should be sent in the commit request, so we just collect them return Flux.from(instances).doOnNext(t -> writes.add(createUpdateWrite(t))); } - return commitWrites(instances, this::createUpdateWrite); + return commitWrites(instances, this::createUpdateWrite, true); }); } @@ -288,11 +290,12 @@ private Flux deleteDocumentsByName(Flux documentNames) { //In a transaction, all write operations should be sent in the commit request, so we just collect them return Flux.from(documentNames).doOnNext(t -> writes.add(createDeleteWrite(t))); } - return commitWrites(documentNames, this::createDeleteWrite); + return commitWrites(documentNames, this::createDeleteWrite, false); }); } - private Flux commitWrites(Publisher instances, Function converterToWrite) { + private Flux commitWrites(Publisher instances, Function converterToWrite, + boolean setUpdateTime) { return Flux.from(instances).bufferTimeout(this.writeBufferSize, this.writeBufferTimeout) .flatMap(batch -> { CommitRequest.Builder builder = CommitRequest.newBuilder() @@ -302,7 +305,16 @@ private Flux commitWrites(Publisher instances, Function conv return ObservableReactiveUtil .unaryCall(obs -> this.firestore.commit(builder.build(), obs)) - .thenMany(Flux.fromIterable(batch)); + .flatMapMany( + response -> { + if (setUpdateTime) { + for (T entity : batch) { + getClassMapper() + .setUpdateTime(entity, Timestamp.fromProto(response.getCommitTime())); + } + } + return Flux.fromIterable(batch); + }); }); } @@ -376,6 +388,19 @@ private Write createUpdateWrite(T entity) { } String resourceName = buildResourceName(entity); Document document = getClassMapper().entityToDocument(entity, resourceName); + FirestorePersistentEntity persistentEntity = + this.mappingContext.getPersistentEntity(entity.getClass()); + FirestorePersistentProperty updateTimeProperty = persistentEntity.getUpdateTimeProperty(); + if (updateTimeProperty != null && updateTimeProperty.findAnnotation(UpdateTime.class).version()) { + Object version = persistentEntity.getPropertyAccessor(entity).getProperty(updateTimeProperty); + if (version != null) { + builder.setCurrentDocument( + Precondition.newBuilder().setUpdateTime(((Timestamp) version).toProto()).build()); + } + else { + builder.setCurrentDocument(Precondition.newBuilder().setExists(false).build()); + } + } return builder.setUpdate(document).build(); } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreClassMapper.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreClassMapper.java index d786b9c543..b740a14994 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreClassMapper.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreClassMapper.java @@ -16,6 +16,7 @@ package com.google.cloud.spring.data.firestore.mapping; +import com.google.cloud.Timestamp; import com.google.firestore.v1.Document; import com.google.firestore.v1.Value; @@ -55,4 +56,7 @@ public interface FirestoreClassMapper { * @return the entity that the Firestore document was converted to */ T documentToEntity(Document document, Class clazz); + + T setUpdateTime(T entity, Timestamp updateTime); + } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java index e8718f4d16..26bb49eb36 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java @@ -43,7 +43,10 @@ public final class FirestoreDefaultClassMapper implements FirestoreClassMapper { private static final String NOT_USED_PATH = "/not/used/path"; - public FirestoreDefaultClassMapper() { + private FirestoreMappingContext mappingContext; + + public FirestoreDefaultClassMapper(FirestoreMappingContext mappingContext) { + this.mappingContext = mappingContext; } public Value toFirestoreValue(T sourceValue) { @@ -54,14 +57,37 @@ public Value toFirestoreValue(T sourceValue) { public Document entityToDocument(T entity, String documentResourceName) { DocumentSnapshot documentSnapshot = INTERNAL.snapshotFromObject(NOT_USED_PATH, entity); - Map valuesMap = INTERNAL.protoFromSnapshot(documentSnapshot); return Document.newBuilder() - .putAllFields(valuesMap) + .putAllFields(removeUpdateTimestamp(INTERNAL.protoFromSnapshot(documentSnapshot), entity)) .setName(documentResourceName).build(); } public T documentToEntity(Document document, Class clazz) { DocumentSnapshot documentSnapshot = INTERNAL.snapshotFromProto(Timestamp.now(), document); - return documentSnapshot.toObject(clazz); + T entity = documentSnapshot.toObject(clazz); + return setUpdateTime(entity, documentSnapshot.getUpdateTime()); + } + + public T setUpdateTime(T entity, Timestamp updateTime) { + FirestorePersistentEntity persistentEntity = + this.mappingContext.getPersistentEntity(entity.getClass()); + FirestorePersistentProperty versionProperty = persistentEntity.getUpdateTimeProperty(); + + if (versionProperty != null) { + persistentEntity.getPropertyAccessor(entity).setProperty(versionProperty, updateTime); + } + + return entity; + } + + private Map removeUpdateTimestamp(Map valuesMap, Object entity) { + FirestorePersistentEntity persistentEntity = + this.mappingContext.getPersistentEntity(entity.getClass()); + FirestorePersistentProperty versionProperty = persistentEntity.getUpdateTimeProperty(); + if (versionProperty != null) { + valuesMap.remove(versionProperty.getFieldName()); + } + return valuesMap; } + } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntity.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntity.java index af169d9008..ad3b4ccdfe 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntity.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntity.java @@ -44,4 +44,6 @@ public interface FirestorePersistentEntity extends * @return the ID property. */ FirestorePersistentProperty getIdPropertyOrFail(); + + FirestorePersistentProperty getUpdateTimeProperty(); } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java index 87dba9f89e..323f8edca6 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java @@ -16,6 +16,7 @@ package com.google.cloud.spring.data.firestore.mapping; +import com.google.cloud.Timestamp; import com.google.cloud.spring.data.firestore.Document; import com.google.cloud.spring.data.firestore.FirestoreDataException; @@ -37,6 +38,8 @@ public class FirestorePersistentEntityImpl private final String collectionName; + private FirestorePersistentProperty updateTimeProperty; + public FirestorePersistentEntityImpl(TypeInformation information) { super(information); this.collectionName = getEntityCollectionName(information); @@ -62,6 +65,11 @@ public FirestorePersistentProperty getIdPropertyOrFail() { return idProperty; } + @Override + public FirestorePersistentProperty getUpdateTimeProperty() { + return updateTimeProperty; + } + private static String getEntityCollectionName(TypeInformation typeInformation) { Document document = AnnotationUtils.findAnnotation(typeInformation.getType(), Document.class); String collectionName = (String) AnnotationUtils.getValue(document, "collectionName"); @@ -74,4 +82,15 @@ private static String getEntityCollectionName(TypeInformation typeInforma return collectionName; } } + + @Override + public void addPersistentProperty(FirestorePersistentProperty property) { + super.addPersistentProperty(property); + if (property.findAnnotation(UpdateTime.class) != null) { + if (property.getActualType() != Timestamp.class) { + throw new FirestoreDataException("@UpdateTime annotated field should be of com.google.protobuf.Timestamp type"); + } + updateTimeProperty = property; + } + } } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java new file mode 100644 index 0000000000..5518d9810f --- /dev/null +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java @@ -0,0 +1,29 @@ +/* + * Copyright 2020-2020 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 + * + * https://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 com.google.cloud.spring.data.firestore.mapping; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** Marks a field as excluded from the Database. */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ ElementType.METHOD, ElementType.FIELD }) +public @interface UpdateTime { + boolean version() default false; +} diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java index fa421b9ea7..7cbddeabcc 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java @@ -20,15 +20,18 @@ import java.util.Map; import java.util.Objects; +import com.google.cloud.Timestamp; import com.google.cloud.firestore.annotation.DocumentId; import com.google.cloud.spring.data.firestore.mapping.FirestoreDefaultClassMapper; import com.google.cloud.spring.data.firestore.mapping.FirestoreMappingContext; +import com.google.cloud.spring.data.firestore.mapping.UpdateTime; import com.google.firestore.v1.CommitRequest; import com.google.firestore.v1.CommitResponse; import com.google.firestore.v1.Document.Builder; import com.google.firestore.v1.DocumentMask; import com.google.firestore.v1.FirestoreGrpc.FirestoreStub; import com.google.firestore.v1.GetDocumentRequest; +import com.google.firestore.v1.Precondition; import com.google.firestore.v1.RunQueryRequest; import com.google.firestore.v1.RunQueryResponse; import com.google.firestore.v1.StructuredQuery; @@ -62,8 +65,9 @@ public class FirestoreTemplateTests { @Before public void setup() { + FirestoreMappingContext mappingContext = new FirestoreMappingContext(); this.firestoreTemplate = new FirestoreTemplate(this.firestoreStub, this.parent, - new FirestoreDefaultClassMapper(), new FirestoreMappingContext()); + new FirestoreDefaultClassMapper(mappingContext), mappingContext); } @Test @@ -99,13 +103,100 @@ public void saveAllTest() { CommitRequest.Builder builder = CommitRequest.newBuilder() .setDatabase("projects/my-project/databases/(default)"); - builder.addWrites(Write.newBuilder().setUpdate(buildDocument("e1", 100)).build()); - builder.addWrites(Write.newBuilder().setUpdate(buildDocument("e2", 200)).build()); + builder.addWrites(Write.newBuilder().setUpdate(buildDocument("e1", 100L)).build()); + builder.addWrites(Write.newBuilder().setUpdate(buildDocument("e2", 200L)).build()); verify(this.firestoreStub, times(1)).commit(eq(builder.build()), any()); } + + @Test + public void updateTimeVersionSaveTest() { + mockCommitMethod(); + + Timestamp expectedUpdateTime = Timestamp.ofTimeMicroseconds(123456789); + StepVerifier.create( + this.firestoreTemplate + .saveAll(Flux.just(new TestEntityUpdateTimeVersion("e1"), new TestEntityUpdateTimeVersion("e2")))) + .expectNext( + new TestEntityUpdateTimeVersion("e1", expectedUpdateTime), + new TestEntityUpdateTimeVersion("e2", expectedUpdateTime)) + .verifyComplete(); + + CommitRequest.Builder builder = CommitRequest.newBuilder() + .setDatabase("projects/my-project/databases/(default)"); + + Precondition doesNotExistPrecondition = Precondition.newBuilder().setExists(false).build(); + builder.addWrites( + Write.newBuilder().setUpdate(buildDocument("e1", null)) + .setCurrentDocument(doesNotExistPrecondition).build()); + builder.addWrites( + Write.newBuilder().setUpdate(buildDocument("e2", null)) + .setCurrentDocument(doesNotExistPrecondition).build()); + + verify(this.firestoreStub, times(1)).commit(eq(builder.build()), any()); + } + + @Test + public void updateTimeVersionUpdateTest() { + mockCommitMethod(); + + Timestamp expectedUpdateTime = Timestamp.ofTimeMicroseconds(123456789); + Timestamp previousUpdateTimeE1 = Timestamp.ofTimeMicroseconds(987654321); + Timestamp previousUpdateTimeE2 = Timestamp.ofTimeMicroseconds(918273645); + StepVerifier.create( + this.firestoreTemplate + .saveAll(Flux.just( + new TestEntityUpdateTimeVersion("e1", previousUpdateTimeE1), + new TestEntityUpdateTimeVersion("e2", previousUpdateTimeE2)))) + .expectNext( + new TestEntityUpdateTimeVersion("e1", expectedUpdateTime), + new TestEntityUpdateTimeVersion("e2", expectedUpdateTime)) + .verifyComplete(); + + CommitRequest.Builder builder = CommitRequest.newBuilder() + .setDatabase("projects/my-project/databases/(default)"); + + Precondition preconditionE1 = + Precondition.newBuilder().setUpdateTime(previousUpdateTimeE1.toProto()).build(); + Precondition preconditionE2 = + Precondition.newBuilder().setUpdateTime(previousUpdateTimeE2.toProto()).build(); + + builder.addWrites( + Write.newBuilder().setUpdate(buildDocument("e1", null)) + .setCurrentDocument(preconditionE1).build()); + builder.addWrites( + Write.newBuilder().setUpdate(buildDocument("e2", null)) + .setCurrentDocument(preconditionE2).build()); + + verify(this.firestoreStub, times(1)).commit(eq(builder.build()), any()); + } + + @Test + public void updateTimeSaveTest() { + mockCommitMethod(); + + Timestamp expectedUpdateTime = Timestamp.ofTimeMicroseconds(123456789); + StepVerifier.create( + this.firestoreTemplate + .saveAll(Flux.just(new TestEntityUpdateTime("e1"), new TestEntityUpdateTime("e2")))) + .expectNext( + new TestEntityUpdateTime("e1", expectedUpdateTime), + new TestEntityUpdateTime("e2", expectedUpdateTime)) + .verifyComplete(); + + CommitRequest.Builder builder = CommitRequest.newBuilder() + .setDatabase("projects/my-project/databases/(default)"); + + builder.addWrites( + Write.newBuilder().setUpdate(buildDocument("e1", null)).build()); + builder.addWrites( + Write.newBuilder().setUpdate(buildDocument("e2", null)).build()); + + verify(this.firestoreStub, times(1)).commit(eq(builder.build()), any()); + } + @Test public void deleteTest() { mockCommitMethod(); @@ -128,7 +219,10 @@ public void deleteTest() { private void mockCommitMethod() { doAnswer(invocation -> { StreamObserver streamObserver = invocation.getArgument(1); - streamObserver.onNext(CommitResponse.newBuilder().build()); + CommitResponse response = CommitResponse.newBuilder() + .setCommitTime(Timestamp.ofTimeMicroseconds(123456789).toProto()) + .build(); + streamObserver.onNext(response); streamObserver.onCompleted(); return null; }).when(this.firestoreStub).commit(any(), any()); @@ -350,19 +444,21 @@ public void existsByIdNotFoundTest() { } - private static Map createValuesMap(String test_entity_1, long value) { + private static Map createValuesMap(long value) { Map valuesMap = new HashMap<>(); valuesMap.put("longField", Value.newBuilder().setIntegerValue(value).build()); return valuesMap; } - public static com.google.firestore.v1.Document buildDocument(String name, long l) { + public static com.google.firestore.v1.Document buildDocument(String name, Long l) { Builder documentBuilder = com.google.firestore.v1.Document.newBuilder(); if (name != null) { documentBuilder.setName(parent + "/testEntities/" + name); } - return documentBuilder - .putAllFields(createValuesMap(name, l)).build(); + if (l != null) { + documentBuilder.putAllFields(createValuesMap(l)); + } + return documentBuilder.build(); } private void mockRunQueryMethod() { @@ -429,16 +525,95 @@ public int hashCode() { } } - @Document - class TestEntityIntegerId { + @Document(collectionName = "testEntities") + class TestEntityUpdateTimeVersion { @DocumentId - public Integer id; + public String id; - public String value; + @UpdateTime(version = true) + public Timestamp updateTime; + + TestEntityUpdateTimeVersion(String id) { + this.id = id; + } - TestEntityIntegerId(Integer id, String value) { + TestEntityUpdateTimeVersion(String id, Timestamp updateTime) { this.id = id; - this.value = value; + this.updateTime = updateTime; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof TestEntityUpdateTimeVersion)) { + return false; + } + + TestEntityUpdateTimeVersion that = (TestEntityUpdateTimeVersion) o; + + if (!Objects.equals(id, that.id)) { + return false; + } + return Objects.equals(updateTime, that.updateTime); + } + + @Override + public int hashCode() { + int result = id != null ? id.hashCode() : 0; + result = 31 * result + (updateTime != null ? updateTime.hashCode() : 0); + return result; + } + } + + @Document(collectionName = "testEntities") + class TestEntityUpdateTime { + @DocumentId + public String id; + + @UpdateTime + public Timestamp updateTime; + + TestEntityUpdateTime(String id) { + this.id = id; + } + + TestEntityUpdateTime(String id, Timestamp updateTime) { + this.id = id; + this.updateTime = updateTime; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof TestEntityUpdateTime)) { + return false; + } + + TestEntityUpdateTime that = (TestEntityUpdateTime) o; + + if (!Objects.equals(id, that.id)) { + return false; + } + return Objects.equals(updateTime, that.updateTime); + } + + @Override + public int hashCode() { + int result = id != null ? id.hashCode() : 0; + result = 31 * result + (updateTime != null ? updateTime.hashCode() : 0); + return result; + } + + @Override + public String toString() { + return "TestEntityUpdateTime{" + + "id='" + id + '\'' + + ", updateTime=" + updateTime + + '}'; } } } diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java index fa8b7460a5..2532e8b5f8 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java @@ -19,9 +19,11 @@ import java.util.List; import java.util.Objects; +import com.google.cloud.Timestamp; import com.google.cloud.firestore.annotation.DocumentId; import com.google.cloud.firestore.annotation.PropertyName; import com.google.cloud.spring.data.firestore.Document; +import com.google.cloud.spring.data.firestore.mapping.UpdateTime; /** * Sample entity for integration tests. @@ -53,6 +55,9 @@ public class User { //end::embedded_class_collections[] + @UpdateTime(version = true) + private Timestamp updateTime; + public User(String name, Integer age) { this.name = name; this.age = age; @@ -110,6 +115,14 @@ public void setAddresses(List
addresses) { this.addresses = addresses; } + public Timestamp getUpdateTime() { + return updateTime; + } + + public void setUpdateTime(Timestamp updateTime) { + this.updateTime = updateTime; + } + @PropertyName("address") public Address getHomeAddress() { return this.homeAddress; @@ -144,7 +157,8 @@ public boolean equals(Object o) { Objects.equals(getAge(), user.getAge()) && Objects.equals(getPets(), user.getPets()) && Objects.equals(getAddresses(), user.getAddresses()) && - Objects.equals(getHomeAddress(), user.getHomeAddress()); + Objects.equals(getHomeAddress(), user.getHomeAddress()) && + Objects.equals(getUpdateTime(), user.getUpdateTime()); } @Override @@ -184,7 +198,8 @@ public boolean equals(Object o) { } Address address = (Address) o; return Objects.equals(getStreetAddress(), address.getStreetAddress()) && - Objects.equals(getCountry(), address.getCountry()); + Objects.equals(getCountry(), address.getCountry() + ); } @Override diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java index 313928bbee..60e392588a 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java @@ -20,12 +20,14 @@ import java.util.UUID; import ch.qos.logback.classic.Level; +import com.google.cloud.Timestamp; import com.google.cloud.spring.data.firestore.FirestoreDataException; import com.google.cloud.spring.data.firestore.FirestoreReactiveOperations; import com.google.cloud.spring.data.firestore.FirestoreTemplate; import com.google.cloud.spring.data.firestore.entities.PhoneNumber; import com.google.cloud.spring.data.firestore.entities.User; import com.google.cloud.spring.data.firestore.transaction.ReactiveFirestoreTransactionManager; +import io.grpc.StatusRuntimeException; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; @@ -41,6 +43,7 @@ import org.springframework.transaction.support.DefaultTransactionDefinition; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.hamcrest.Matchers.is; import static org.junit.Assume.assumeThat; import static org.mockito.ArgumentMatchers.any; @@ -92,6 +95,48 @@ public void generateIdTest() { assertThat(users).containsExactlyInAnyOrder(savedUsers.toArray(new User[0])); } + @Test + public void updateTimeTest() { + User bob = new User("Bob", 60, null); + this.firestoreTemplate.saveAll(Flux.just(bob)).collectList().block(); + + List users = this.firestoreTemplate.findAll(User.class).collectList().block(); + assertThat(users).containsExactly(bob); + assertThat(bob.getUpdateTime()).isNotNull(); + } + + @Test + public void optimisticLockingTest() { + firestoreTemplate.deleteAll(User.class).block(); + User bob = new User("Bob", 60, null); + + this.firestoreTemplate.saveAll(Flux.just(bob)).collectList().block(); + Timestamp bobUpdateTime = bob.getUpdateTime(); + assertThat(bobUpdateTime).isNotNull(); + + User bob2 = new User("Bob", 60, null); + + assertThatThrownBy(() -> this.firestoreTemplate.saveAll(Flux.just(bob2)).collectList().block()) + .isInstanceOf(StatusRuntimeException.class) + .hasMessageContaining("ALREADY_EXISTS"); + + bob.setAge(15); + this.firestoreTemplate.saveAll(Flux.just(bob)).collectList().block(); + assertThat(bob.getUpdateTime()).isGreaterThan(bobUpdateTime); + + + List users = this.firestoreTemplate.findAll(User.class).collectList().block(); + assertThat(users).containsExactly(bob); + + User bob3 = users.get(0); + bob3.setAge(20); + this.firestoreTemplate.saveAll(Flux.just(bob3)).collectList().block(); + + assertThatThrownBy(() -> this.firestoreTemplate.saveAll(Flux.just(bob)).collectList().block()) + .isInstanceOf(StatusRuntimeException.class) + .hasMessageContaining("does not match the required base version"); + } + @Test public void transactionTest() { User alice = new User("Alice", 29); diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java index 104dba1e32..0f61715877 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java @@ -91,7 +91,7 @@ public UserService userService() { @Bean @ConditionalOnMissingBean - public FirestoreClassMapper getClassMapper() { - return new FirestoreDefaultClassMapper(); + public FirestoreClassMapper getClassMapper(FirestoreMappingContext mappingContext) { + return new FirestoreDefaultClassMapper(mappingContext); } } diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/FirestoreRepositoryTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/FirestoreRepositoryTests.java index 59223dfd6c..dc2757222b 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/FirestoreRepositoryTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/FirestoreRepositoryTests.java @@ -96,8 +96,8 @@ public FirestoreTemplate firestoreTemplate( @Bean @ConditionalOnMissingBean - public FirestoreClassMapper getClassMapper() { - return new FirestoreDefaultClassMapper(); + public FirestoreClassMapper getClassMapper(FirestoreMappingContext mappingContext) { + return new FirestoreDefaultClassMapper(mappingContext); } } } diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/PartTreeFirestoreQueryTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/PartTreeFirestoreQueryTests.java index 4fe94e77e5..ecc2bd91bd 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/PartTreeFirestoreQueryTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/repository/query/PartTreeFirestoreQueryTests.java @@ -41,7 +41,7 @@ import static org.mockito.Mockito.when; public class PartTreeFirestoreQueryTests { - private FirestoreClassMapper classMapper = new FirestoreDefaultClassMapper(); + private FirestoreClassMapper classMapper = new FirestoreDefaultClassMapper(new FirestoreMappingContext()); private static final User DUMMY_USER = new User("Hello", 23); private static final Consumer NOOP = invocation -> { }; diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java index fd82813802..5972859d53 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java @@ -197,9 +197,9 @@ private FirestoreTemplate getFirestoreTemplate() { return null; }).when(this.firestoreStub).getDocument(any(), any()); - + FirestoreMappingContext mappingContext = new FirestoreMappingContext(); FirestoreTemplate template = new FirestoreTemplate(this.firestoreStub, this.parent, - new FirestoreDefaultClassMapper(), new FirestoreMappingContext()); + new FirestoreDefaultClassMapper(mappingContext), mappingContext); StepVerifier.setDefaultTimeout(Duration.ofSeconds(5)); return template; From 8fddbe5285d71dd24c2981a08278e939c4683184 Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Thu, 10 Dec 2020 15:29:00 -0500 Subject: [PATCH 2/8] PR comments --- .../spring/data/firestore/FirestoreTemplate.java | 2 ++ .../mapping/FirestoreDefaultClassMapper.java | 12 ++++++------ .../spring/data/firestore/mapping/UpdateTime.java | 7 ++++++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java index 8f05a7bafe..39ce0f41a0 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java @@ -398,6 +398,8 @@ private Write createUpdateWrite(T entity) { Precondition.newBuilder().setUpdateTime(((Timestamp) version).toProto()).build()); } else { + //If an entity with an empty update time field is being saved, it must be new. + //Otherwise it will overwrite an existing document. builder.setCurrentDocument(Precondition.newBuilder().setExists(false).build()); } } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java index 26bb49eb36..5be3247760 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java @@ -71,10 +71,10 @@ public T documentToEntity(Document document, Class clazz) { public T setUpdateTime(T entity, Timestamp updateTime) { FirestorePersistentEntity persistentEntity = this.mappingContext.getPersistentEntity(entity.getClass()); - FirestorePersistentProperty versionProperty = persistentEntity.getUpdateTimeProperty(); + FirestorePersistentProperty updateTimeProperty = persistentEntity.getUpdateTimeProperty(); - if (versionProperty != null) { - persistentEntity.getPropertyAccessor(entity).setProperty(versionProperty, updateTime); + if (updateTimeProperty != null) { + persistentEntity.getPropertyAccessor(entity).setProperty(updateTimeProperty, updateTime); } return entity; @@ -83,9 +83,9 @@ public T setUpdateTime(T entity, Timestamp updateTime) { private Map removeUpdateTimestamp(Map valuesMap, Object entity) { FirestorePersistentEntity persistentEntity = this.mappingContext.getPersistentEntity(entity.getClass()); - FirestorePersistentProperty versionProperty = persistentEntity.getUpdateTimeProperty(); - if (versionProperty != null) { - valuesMap.remove(versionProperty.getFieldName()); + FirestorePersistentProperty updateTimeProperty = persistentEntity.getUpdateTimeProperty(); + if (updateTimeProperty != null) { + valuesMap.remove(updateTimeProperty.getFieldName()); } return valuesMap; } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java index 5518d9810f..9a997f144a 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java @@ -21,7 +21,12 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -/** Marks a field as excluded from the Database. */ +/** Marks a field to be used for update time. + * + * @author Dmitry Solomakha + * + * @since 2.0.0 + */ @Retention(RetentionPolicy.RUNTIME) @Target({ ElementType.METHOD, ElementType.FIELD }) public @interface UpdateTime { From 3911b6b41d7522a712bcd33a92b588567087b33c Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Thu, 10 Dec 2020 16:09:39 -0500 Subject: [PATCH 3/8] PR comments --- .../data/firestore/mapping/FirestorePersistentEntityImpl.java | 2 +- .../google/cloud/spring/data/firestore/mapping/UpdateTime.java | 3 ++- .../spring/data/firestore/it/FirestoreIntegrationTests.java | 1 - 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java index 323f8edca6..e2783c6063 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestorePersistentEntityImpl.java @@ -88,7 +88,7 @@ public void addPersistentProperty(FirestorePersistentProperty property) { super.addPersistentProperty(property); if (property.findAnnotation(UpdateTime.class) != null) { if (property.getActualType() != Timestamp.class) { - throw new FirestoreDataException("@UpdateTime annotated field should be of com.google.protobuf.Timestamp type"); + throw new FirestoreDataException("@UpdateTime annotated field should be of com.google.cloud.Timestamp type"); } updateTimeProperty = property; } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java index 9a997f144a..0289bd0dda 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/UpdateTime.java @@ -21,7 +21,8 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -/** Marks a field to be used for update time. +/** + * Marks a field to be used for update time. * * @author Dmitry Solomakha * diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java index 60e392588a..952c0bbd6f 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java @@ -107,7 +107,6 @@ public void updateTimeTest() { @Test public void optimisticLockingTest() { - firestoreTemplate.deleteAll(User.class).block(); User bob = new User("Bob", 60, null); this.firestoreTemplate.saveAll(Flux.just(bob)).collectList().block(); From 378764ce995dcb879bf361c4e4da338c385505b0 Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Fri, 11 Dec 2020 18:22:19 -0500 Subject: [PATCH 4/8] fix tests --- .../GcpFirestoreAutoConfiguration.java | 4 ++-- .../data/firestore/FirestoreTemplate.java | 6 ++++-- .../ReactiveFirestoreResourceHolder.java | 6 ++++++ .../ReactiveFirestoreTransactionManager.java | 19 ++++++++++++++++--- .../spring/data/firestore/entities/User.java | 4 ++-- .../it/FirestoreIntegrationTests.java | 5 ++++- ...irestoreIntegrationTestsConfiguration.java | 4 ++-- ...activeFirestoreTransactionManagerTest.java | 12 +++++++++--- 8 files changed, 45 insertions(+), 15 deletions(-) diff --git a/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java b/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java index 30d62429ad..ab8650767a 100644 --- a/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java +++ b/spring-cloud-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/firestore/GcpFirestoreAutoConfiguration.java @@ -146,9 +146,9 @@ public FirestoreTemplate firestoreTemplate(FirestoreGrpc.FirestoreStub firestore @Bean @ConditionalOnMissingBean public ReactiveFirestoreTransactionManager firestoreTransactionManager( - FirestoreGrpc.FirestoreStub firestoreStub) { + FirestoreGrpc.FirestoreStub firestoreStub, FirestoreClassMapper classMapper) { return new ReactiveFirestoreTransactionManager(firestoreStub, - GcpFirestoreAutoConfiguration.this.firestoreRootPath); + GcpFirestoreAutoConfiguration.this.firestoreRootPath, classMapper); } @Bean diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java index 39ce0f41a0..96ffdceec5 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java @@ -206,9 +206,11 @@ public Flux saveAll(Publisher instances) { if (transactionContext.isPresent()) { ReactiveFirestoreResourceHolder holder = (ReactiveFirestoreResourceHolder) transactionContext.get() .getResources().get(this.firestore); - List writes = holder.getWrites(); //In a transaction, all write operations should be sent in the commit request, so we just collect them - return Flux.from(instances).doOnNext(t -> writes.add(createUpdateWrite(t))); + return Flux.from(instances).doOnNext(t -> { + holder.getWrites().add(createUpdateWrite(t)); + holder.getEntities().add(t); + }); } return commitWrites(instances, this::createUpdateWrite, true); }); diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreResourceHolder.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreResourceHolder.java index 2bd7bc4cd1..78198d47ed 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreResourceHolder.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreResourceHolder.java @@ -34,6 +34,8 @@ public class ReactiveFirestoreResourceHolder extends ResourceHolderSupport { private List writes = new ArrayList<>(); + private List entities = new ArrayList<>(); + public ReactiveFirestoreResourceHolder(ByteString transactionId) { this.transactionId = transactionId; } @@ -45,4 +47,8 @@ public ByteString getTransactionId() { public List getWrites() { return this.writes; } + + public List getEntities() { + return entities; + } } diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java index 1bc5406936..7c4d0949df 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java @@ -16,6 +16,8 @@ package com.google.cloud.spring.data.firestore.transaction; +import com.google.cloud.Timestamp; +import com.google.cloud.spring.data.firestore.mapping.FirestoreClassMapper; import com.google.cloud.spring.data.firestore.util.ObservableReactiveUtil; import com.google.cloud.spring.data.firestore.util.Util; import com.google.firestore.v1.BeginTransactionRequest; @@ -23,6 +25,7 @@ import com.google.firestore.v1.CommitRequest; import com.google.firestore.v1.CommitResponse; import com.google.firestore.v1.FirestoreGrpc; +import com.google.firestore.v1.FirestoreGrpc.FirestoreStub; import com.google.firestore.v1.RollbackRequest; import com.google.firestore.v1.TransactionOptions; import com.google.protobuf.ByteString; @@ -50,16 +53,19 @@ public class ReactiveFirestoreTransactionManager extends AbstractReactiveTransac private final String databasePath; + private FirestoreClassMapper classMapper; + /** * Constructor for ReactiveFirestoreTransactionManager. * @param firestore Firestore gRPC stub * @param parent the parent resource. For example: * projects/{project_id}/databases/{database_id}/documents or - * projects/{project_id}/databases/{database_id}/documents/chatrooms/{chatroom_id} + * @param classMapper Firestore class mapper */ - public ReactiveFirestoreTransactionManager(FirestoreGrpc.FirestoreStub firestore, String parent) { + public ReactiveFirestoreTransactionManager(FirestoreStub firestore, String parent, FirestoreClassMapper classMapper) { this.firestore = firestore; this.databasePath = Util.extractDatabasePath(parent); + this.classMapper = classMapper; } @Override @@ -101,10 +107,17 @@ protected Mono doCommit(TransactionSynchronizationManager transactionSynch return ObservableReactiveUtil .unaryCall(obs -> this.firestore.commit(builder.build(), obs)) - .then(); + .flatMap((response) -> { + for (Object entity : resourceHolder.getEntities()) { + this.classMapper.setUpdateTime(entity, Timestamp.fromProto(response.getCommitTime())); + } + return Mono.empty(); + } + ).then(); }); } + @Override protected Mono doRollback(TransactionSynchronizationManager transactionSynchronizationManager, GenericReactiveTransaction genericReactiveTransaction) throws TransactionException { diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java index 2532e8b5f8..88574582d6 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/entities/User.java @@ -141,6 +141,7 @@ public String toString() { ", pets=" + pets + ", addresses=" + addresses + ", homeAddress=" + homeAddress + + ", updateTime=" + updateTime + '}'; } @@ -198,8 +199,7 @@ public boolean equals(Object o) { } Address address = (Address) o; return Objects.equals(getStreetAddress(), address.getStreetAddress()) && - Objects.equals(getCountry(), address.getCountry() - ); + Objects.equals(getCountry(), address.getCountry()); } @Override diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java index 952c0bbd6f..66a5d64835 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java @@ -210,6 +210,8 @@ public void writeReadDeleteTest() { .containsExactlyInAnyOrder(bob, alice); List usersBeforeDelete = this.firestoreTemplate.findAll(User.class).collectList().block(); + assertThat(usersBeforeDelete).containsExactlyInAnyOrder(alice, bob); + assertThat(this.firestoreTemplate.count(User.class).block()).isEqualTo(2); this.firestoreTemplate.delete(Mono.just(bob)).block(); @@ -219,11 +221,12 @@ public void writeReadDeleteTest() { this.firestoreTemplate.deleteById(Mono.just("Alice"), User.class).block(); assertThat(this.firestoreTemplate.count(User.class).block()).isEqualTo(0); + alice.setUpdateTime(null); this.firestoreTemplate.save(alice).block(); + bob.setUpdateTime(null); this.firestoreTemplate.save(bob).block(); assertThat(this.firestoreTemplate.deleteAll(User.class).block()).isEqualTo(2); - assertThat(usersBeforeDelete).containsExactlyInAnyOrder(alice, bob); assertThat(this.firestoreTemplate.findAll(User.class).collectList().block()).isEmpty(); //tag::subcollection[] diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java index 0f61715877..c53d73e1d5 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTestsConfiguration.java @@ -77,8 +77,8 @@ public FirestoreTemplate firestoreTemplate(FirestoreGrpc.FirestoreStub firestore @Bean @ConditionalOnMissingBean public ReactiveFirestoreTransactionManager firestoreTransactionManager( - FirestoreGrpc.FirestoreStub firestoreStub) { - return Mockito.spy(new ReactiveFirestoreTransactionManager(firestoreStub, this.defaultParent)); + FirestoreGrpc.FirestoreStub firestoreStub, FirestoreClassMapper classMapper) { + return Mockito.spy(new ReactiveFirestoreTransactionManager(firestoreStub, this.defaultParent, classMapper)); } //tag::user_service_bean[] diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java index 5972859d53..e2cd3113c2 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java @@ -53,12 +53,16 @@ public class ReactiveFirestoreTransactionManagerTest { private final String parent = "projects/my-project/databases/(default)/documents"; + private FirestoreDefaultClassMapper classMapper; + @Test public void triggerCommitCorrectly() { FirestoreTemplate template = getFirestoreTemplate(); - ReactiveFirestoreTransactionManager txManager = new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent); + classMapper = new FirestoreDefaultClassMapper(new FirestoreMappingContext()); + ReactiveFirestoreTransactionManager txManager = + new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent, this.classMapper); TransactionalOperator operator = TransactionalOperator.create(txManager); template.findById(Mono.just("e1"), FirestoreTemplateTests.TestEntity.class) @@ -92,7 +96,8 @@ public void triggerRollbackCorrectly() { FirestoreTemplate template = getFirestoreTemplate(); - ReactiveFirestoreTransactionManager txManager = new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent); + ReactiveFirestoreTransactionManager txManager = + new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent, this.classMapper); TransactionalOperator operator = TransactionalOperator.create(txManager); template.findById(Mono.defer(() -> { @@ -115,7 +120,8 @@ public void writeTransaction() { FirestoreTemplate template = getFirestoreTemplate(); - ReactiveFirestoreTransactionManager txManager = new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent); + ReactiveFirestoreTransactionManager txManager = + new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent, this.classMapper); TransactionalOperator operator = TransactionalOperator.create(txManager); doAnswer(invocation -> { From 11b703976bf55d402cd0e431020b9688f23440f6 Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Wed, 23 Dec 2020 17:56:54 -0500 Subject: [PATCH 5/8] optimistic locking in transactions --- .../ReactiveFirestoreTransactionManager.java | 2 +- .../firestore/FirestoreTemplateTests.java | 11 ++++++ .../it/FirestoreIntegrationTests.java | 38 +++++++++++++++++++ ...activeFirestoreTransactionManagerTest.java | 18 ++++++--- 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java index 7c4d0949df..984c728b45 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManager.java @@ -113,7 +113,7 @@ protected Mono doCommit(TransactionSynchronizationManager transactionSynch } return Mono.empty(); } - ).then(); + ); }); } diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java index 7cbddeabcc..2a58e6bd20 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/FirestoreTemplateTests.java @@ -482,6 +482,9 @@ public static class TestEntity { Long longField; + @UpdateTime + Timestamp updateTimestamp; + TestEntity() { } @@ -506,6 +509,14 @@ public void setLongField(Long longField) { this.longField = longField; } + public Timestamp getUpdateTimestamp() { + return updateTimestamp; + } + + public void setUpdateTimestamp(Timestamp updateTimestamp) { + this.updateTimestamp = updateTimestamp; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java index 66a5d64835..d85d506836 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/it/FirestoreIntegrationTests.java @@ -35,6 +35,7 @@ import org.slf4j.LoggerFactory; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.test.context.ContextConfiguration; @@ -136,6 +137,43 @@ public void optimisticLockingTest() { .hasMessageContaining("does not match the required base version"); } + @Test + public void optimisticLockingTransactionTest() { + User bob = new User("Bob", 60, null); + + TransactionalOperator operator = TransactionalOperator.create(txManager); + + this.firestoreTemplate.saveAll(Flux.just(bob)).collectList().block(); + Timestamp bobUpdateTime = bob.getUpdateTime(); + assertThat(bobUpdateTime).isNotNull(); + + User bob2 = new User("Bob", 60, null); + + this.firestoreTemplate.saveAll(Flux.just(bob2)).collectList() + .as(operator::transactional) + .as(StepVerifier::create) + .expectError() + .verify(); + + bob.setAge(15); + this.firestoreTemplate.saveAll(Flux.just(bob)).as(operator::transactional).collectList().block(); + assertThat(bob.getUpdateTime()).isGreaterThan(bobUpdateTime); + + + List users = this.firestoreTemplate.findAll(User.class).collectList().block(); + assertThat(users).containsExactly(bob); + + User bob3 = users.get(0); + bob3.setAge(20); + this.firestoreTemplate.saveAll(Flux.just(bob3)).as(operator::transactional).collectList().block(); + + this.firestoreTemplate.saveAll(Flux.just(bob)).as(operator::transactional).collectList() + .as(operator::transactional) + .as(StepVerifier::create) + .expectError() + .verify(); + } + @Test public void transactionTest() { User alice = new User("Alice", 29); diff --git a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java index e2cd3113c2..43173ad177 100644 --- a/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java +++ b/spring-cloud-gcp-data-firestore/src/test/java/com/google/cloud/spring/data/firestore/transaction/ReactiveFirestoreTransactionManagerTest.java @@ -21,6 +21,7 @@ import com.google.cloud.spring.data.firestore.FirestoreDataException; import com.google.cloud.spring.data.firestore.FirestoreTemplate; import com.google.cloud.spring.data.firestore.FirestoreTemplateTests; +import com.google.cloud.spring.data.firestore.FirestoreTemplateTests.TestEntity; import com.google.cloud.spring.data.firestore.mapping.FirestoreDefaultClassMapper; import com.google.cloud.spring.data.firestore.mapping.FirestoreMappingContext; import com.google.firestore.v1.BeginTransactionResponse; @@ -32,6 +33,7 @@ import com.google.firestore.v1.RollbackRequest; import com.google.protobuf.ByteString; import com.google.protobuf.Empty; +import com.google.protobuf.Timestamp; import io.grpc.stub.StreamObserver; import org.junit.Test; import reactor.core.publisher.Mono; @@ -53,14 +55,13 @@ public class ReactiveFirestoreTransactionManagerTest { private final String parent = "projects/my-project/databases/(default)/documents"; - private FirestoreDefaultClassMapper classMapper; + private FirestoreDefaultClassMapper classMapper = new FirestoreDefaultClassMapper(new FirestoreMappingContext()); @Test public void triggerCommitCorrectly() { FirestoreTemplate template = getFirestoreTemplate(); - classMapper = new FirestoreDefaultClassMapper(new FirestoreMappingContext()); ReactiveFirestoreTransactionManager txManager = new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent, this.classMapper); TransactionalOperator operator = TransactionalOperator.create(txManager); @@ -124,6 +125,7 @@ public void writeTransaction() { new ReactiveFirestoreTransactionManager(this.firestoreStub, this.parent, this.classMapper); TransactionalOperator operator = TransactionalOperator.create(txManager); + Timestamp commitTime = Timestamp.newBuilder().setSeconds(3456).build(); doAnswer(invocation -> { CommitRequest commitRequest = invocation.getArgument(0); StreamObserver streamObserver = invocation.getArgument(1); @@ -133,21 +135,27 @@ public void writeTransaction() { assertThat(commitRequest.getWritesList().get(1).getUpdate().getName()).isEqualTo(this.parent + "/testEntities/" + "e3"); assertThat(commitRequest.getWritesList().get(2).getDelete()).isEqualTo(this.parent + "/testEntities/" + "e3"); - streamObserver.onNext(CommitResponse.newBuilder().build()); + streamObserver.onNext(CommitResponse.newBuilder() + .setCommitTime(commitTime) + .build()); streamObserver.onCompleted(); return null; }).when(this.firestoreStub).commit(any(), any()); + TestEntity e2 = new TestEntity("e2", 100L); + TestEntity e3 = new TestEntity("e3", 100L); template.findById(Mono.just("e1"), FirestoreTemplateTests.TestEntity.class) - .flatMap(testEntity -> template.save(new FirestoreTemplateTests.TestEntity("e2", 100L))) - .flatMap(testEntity -> template.save(new FirestoreTemplateTests.TestEntity("e3", 100L))) + .flatMap(testEntity -> template.save(e2)) + .flatMap(testEntity -> template.save(e3)) .flatMap(testEntity -> template.delete(Mono.just(testEntity))) .then() .as(operator::transactional) .as(StepVerifier::create) .verifyComplete(); + assertThat(e2.getUpdateTimestamp().toProto()).isEqualTo(commitTime); + assertThat(e3.getUpdateTimestamp().toProto()).isEqualTo(commitTime); verify(this.firestoreStub).beginTransaction(any(), any()); verify(this.firestoreStub).commit(any(), any()); From 5bdc505d4ee7579a32e7ca0efee036f7f22e6082 Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Mon, 28 Dec 2020 12:02:51 -0500 Subject: [PATCH 6/8] check for nullity --- .../cloud/spring/data/firestore/FirestoreTemplate.java | 7 +++++-- .../firestore/mapping/FirestoreDefaultClassMapper.java | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java index 96ffdceec5..e18b57426f 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/FirestoreTemplate.java @@ -18,6 +18,7 @@ import java.time.Duration; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.function.Consumer; import java.util.function.Function; @@ -392,8 +393,10 @@ private Write createUpdateWrite(T entity) { Document document = getClassMapper().entityToDocument(entity, resourceName); FirestorePersistentEntity persistentEntity = this.mappingContext.getPersistentEntity(entity.getClass()); - FirestorePersistentProperty updateTimeProperty = persistentEntity.getUpdateTimeProperty(); - if (updateTimeProperty != null && updateTimeProperty.findAnnotation(UpdateTime.class).version()) { + FirestorePersistentProperty updateTimeProperty = + Objects.requireNonNull(persistentEntity).getUpdateTimeProperty(); + if (updateTimeProperty != null + && Objects.requireNonNull(updateTimeProperty.findAnnotation(UpdateTime.class)).version()) { Object version = persistentEntity.getPropertyAccessor(entity).getProperty(updateTimeProperty); if (version != null) { builder.setCurrentDocument( diff --git a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java index 5be3247760..2887dfdb48 100644 --- a/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java +++ b/spring-cloud-gcp-data-firestore/src/main/java/com/google/cloud/spring/data/firestore/mapping/FirestoreDefaultClassMapper.java @@ -17,6 +17,7 @@ package com.google.cloud.spring.data.firestore.mapping; import java.util.Map; +import java.util.Objects; import com.google.cloud.Timestamp; import com.google.cloud.firestore.DocumentSnapshot; @@ -71,7 +72,8 @@ public T documentToEntity(Document document, Class clazz) { public T setUpdateTime(T entity, Timestamp updateTime) { FirestorePersistentEntity persistentEntity = this.mappingContext.getPersistentEntity(entity.getClass()); - FirestorePersistentProperty updateTimeProperty = persistentEntity.getUpdateTimeProperty(); + FirestorePersistentProperty updateTimeProperty = + Objects.requireNonNull(persistentEntity).getUpdateTimeProperty(); if (updateTimeProperty != null) { persistentEntity.getPropertyAccessor(entity).setProperty(updateTimeProperty, updateTime); @@ -83,7 +85,8 @@ public T setUpdateTime(T entity, Timestamp updateTime) { private Map removeUpdateTimestamp(Map valuesMap, Object entity) { FirestorePersistentEntity persistentEntity = this.mappingContext.getPersistentEntity(entity.getClass()); - FirestorePersistentProperty updateTimeProperty = persistentEntity.getUpdateTimeProperty(); + FirestorePersistentProperty updateTimeProperty = + Objects.requireNonNull(persistentEntity).getUpdateTimeProperty(); if (updateTimeProperty != null) { valuesMap.remove(updateTimeProperty.getFieldName()); } From ebe473a547b485b359d97a82b6d34e8e46fc9978 Mon Sep 17 00:00:00 2001 From: dsolomakha Date: Wed, 30 Dec 2020 15:06:07 -0500 Subject: [PATCH 7/8] add documentation --- docs/src/main/asciidoc/firestore.adoc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/src/main/asciidoc/firestore.adoc b/docs/src/main/asciidoc/firestore.adoc index a07a59ed11..d887836f04 100644 --- a/docs/src/main/asciidoc/firestore.adoc +++ b/docs/src/main/asciidoc/firestore.adoc @@ -321,6 +321,31 @@ include::{project-root}/spring-cloud-gcp-data-firestore/src/test/java/com/google ---- +==== Update time +Firestore stores update time for every document. +If you would like to retrieve it you can add a field of `com.google.cloud.Timestamp` type to your entity and annotate it with `@UpdateTime` annotation. + +[source,java,indent=0] +---- +@UpdateTime +Timestamp updateTime; +---- + +===== Using update time for optimistic locking +A field annotated with `@UpdateTime` can be used for optimistic locking. +To enable that, you need to set `version` parameter to true: + +[source,java,indent=0] +---- +@UpdateTime(version = true) +Timestamp updateTime; +---- + +When you enable optimistic locking, a precondition will be automatically added to the write request to ensure that the document you are updating was not changed since your last read. +It uses this field's value as a document version and checks that the version of the document you write is the same as the one you've read. + +If the field is empty, a precondition would check that the document with the same id does not exist to ensure you don't overwrite existing documents unintentionally. + === Cloud Firestore Spring Boot Starter If you prefer using Firestore client only, Spring Cloud GCP provides a convenience starter which automatically configures authentication settings and client objects needed to begin using https://cloud.google.com/firestore/[Google Cloud Firestore] in native mode. From 9f3fdb82e2a4d1777db1d94cd6e0156263e95d43 Mon Sep 17 00:00:00 2001 From: Mike Eltsufin Date: Wed, 30 Dec 2020 17:13:32 -0500 Subject: [PATCH 8/8] documentation polish --- docs/src/main/asciidoc/firestore.adoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/src/main/asciidoc/firestore.adoc b/docs/src/main/asciidoc/firestore.adoc index d887836f04..7ff789b2ca 100644 --- a/docs/src/main/asciidoc/firestore.adoc +++ b/docs/src/main/asciidoc/firestore.adoc @@ -297,7 +297,7 @@ Now when you call the methods annotated with `@Transactional` on your service ob If an error occurs during the execution of a method annotated with `@Transactional`, the transaction will be rolled back. If no error occurs, the transaction will be committed. -==== Subcollections +=== Subcollections A subcollection is a collection associated with a specific entity. Documents in subcollections can contain subcollections as well, allowing you to further nest data. You can nest data up to 100 levels deep. @@ -321,9 +321,9 @@ include::{project-root}/spring-cloud-gcp-data-firestore/src/test/java/com/google ---- -==== Update time +=== Update Time and Optimistic Locking Firestore stores update time for every document. -If you would like to retrieve it you can add a field of `com.google.cloud.Timestamp` type to your entity and annotate it with `@UpdateTime` annotation. +If you would like to retrieve it, you can add a field of `com.google.cloud.Timestamp` type to your entity and annotate it with `@UpdateTime` annotation. [source,java,indent=0] ---- @@ -333,7 +333,7 @@ Timestamp updateTime; ===== Using update time for optimistic locking A field annotated with `@UpdateTime` can be used for optimistic locking. -To enable that, you need to set `version` parameter to true: +To enable that, you need to set `version` parameter to `true`: [source,java,indent=0] ----