From 092e79b65fd8160fa4e4aa1558a983220dde9735 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sat, 25 Apr 2020 12:12:08 +0400 Subject: [PATCH 01/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Add SeqNoPrimaryTerm support to ElasticsearchPersistentEntity --- .../ElasticsearchPersistentEntity.java | 21 +++++++ .../ElasticsearchPersistentProperty.java | 9 +++ .../core/mapping/SeqNoPrimaryTerm.java | 26 +++++++++ .../SimpleElasticsearchPersistentEntity.java | 27 +++++++++ ...SimpleElasticsearchPersistentProperty.java | 12 ++++ .../core/ElasticsearchTemplateTests.java | 28 ++++++++++ .../ReactiveElasticsearchTemplateTests.java | 30 ++++++++++ ...pleElasticsearchPersistentEntityTests.java | 55 +++++++++++++++++++ ...sticsearchPersistentPropertyUnitTests.java | 24 +++++++- 9 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java index 1111d172c3..6a75ecb486 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java @@ -30,6 +30,7 @@ * @author Oliver Gierke * @author Ivan Greene * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ public interface ElasticsearchPersistentEntity extends PersistentEntity { @@ -96,4 +97,24 @@ public interface ElasticsearchPersistentEntity extends PersistentEntity { @@ -64,6 +65,14 @@ public interface ElasticsearchPersistentProperty extends PersistentProperty extends BasicPersistentEntity implements ElasticsearchPersistentEntity, ApplicationContextAware { @@ -70,6 +71,7 @@ public class SimpleElasticsearchPersistentEntity extends BasicPersistentEntit private @Nullable String parentType; private @Nullable ElasticsearchPersistentProperty parentIdProperty; private @Nullable ElasticsearchPersistentProperty scoreProperty; + private @Nullable ElasticsearchPersistentProperty seqNoPrimaryTermProperty; private @Nullable String settingPath; private @Nullable VersionType versionType; private boolean createIndexAndMapping; @@ -231,6 +233,20 @@ public void addPersistentProperty(ElasticsearchPersistentProperty property) { this.scoreProperty = property; } + + if (property.isSeqNoPrimaryTermProperty()) { + + ElasticsearchPersistentProperty seqNoPrimaryTermProperty = this.seqNoPrimaryTermProperty; + + if (seqNoPrimaryTermProperty != null) { + throw new MappingException(String.format( + "Attempt to add SeqNoPrimaryTerm property %s but already have property %s registered " + + "as SeqNoPrimaryTerm property. Check your mapping configuration!", + property.getField(), seqNoPrimaryTermProperty.getField())); + } + + this.seqNoPrimaryTermProperty = property; + } } /* @@ -262,4 +278,15 @@ public ElasticsearchPersistentProperty getPersistentPropertyWithFieldName(String return propertyRef.get(); }); } + + @Override + public boolean hasSeqNoPrimaryTermProperty() { + return seqNoPrimaryTermProperty != null; + } + + @Override + @Nullable + public ElasticsearchPersistentProperty getSeqNoPrimaryTermProperty() { + return seqNoPrimaryTermProperty; + } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java index a69f6fa20f..fdad3924c3 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java @@ -44,6 +44,7 @@ * @author Sascha Woo * @author Oliver Gierke * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ public class SimpleElasticsearchPersistentProperty extends AnnotationBasedPersistentProperty implements ElasticsearchPersistentProperty { @@ -53,6 +54,7 @@ public class SimpleElasticsearchPersistentProperty extends private final boolean isScore; private final boolean isParent; private final boolean isId; + private final boolean isSeqNoPrimaryTerm; private final @Nullable String annotatedFieldName; @Nullable private ElasticsearchPersistentPropertyConverter propertyConverter; @@ -65,6 +67,7 @@ public SimpleElasticsearchPersistentProperty(Property property, this.isId = super.isIdProperty() || SUPPORTED_ID_PROPERTY_NAMES.contains(getFieldName()); this.isScore = isAnnotationPresent(Score.class); this.isParent = isAnnotationPresent(Parent.class); + this.isSeqNoPrimaryTerm = SeqNoPrimaryTerm.class.isAssignableFrom(getRawType()); if (isVersionProperty() && !getType().equals(Long.class)) { throw new MappingException(String.format("Version property %s must be of type Long!", property.getName())); @@ -209,4 +212,13 @@ public boolean isImmutable() { public boolean isParentProperty() { return isParent; } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty#isSeqNoPrimaryTermProperty() + */ + @Override + public boolean isSeqNoPrimaryTermProperty() { + return isSeqNoPrimaryTerm; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 3e1c0696f7..9187719276 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -58,6 +58,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.annotation.Id; import org.springframework.data.annotation.Version; import org.springframework.data.domain.PageRequest; @@ -74,6 +75,7 @@ import org.springframework.data.elasticsearch.annotations.ScriptedField; import org.springframework.data.elasticsearch.core.geo.GeoPoint; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.*; import org.springframework.data.util.CloseableIterator; import org.springframework.lang.Nullable; @@ -100,6 +102,7 @@ * @author Martin Choraine * @author Farid Azaza * @author Gyula Attila Csorogi + * @author Roman Puchkovskiy */ public abstract class ElasticsearchTemplateTests { @@ -3067,6 +3070,23 @@ void shouldDoExistsWithIndexCoordinates() { assertThat(operations.exists("42", index)).isTrue(); } + @Test // DATAES-799 + void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnEntityWithSeqNoPrimaryTermProperty() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = operations.save(original); + + OptimisticEntity forEdit1 = operations.get(saved.getId(), OptimisticEntity.class); + OptimisticEntity forEdit2 = operations.get(saved.getId(), OptimisticEntity.class); + + forEdit1.setMessage("It'll be ok"); + operations.save(forEdit1); + + forEdit2.setMessage("It'll be great"); + assertThatThrownBy(() -> operations.save(forEdit2)) + .isInstanceOf(OptimisticLockingFailureException.class); + } + protected RequestFactory getRequestFactory() { return ((AbstractElasticsearchTemplate) operations).getRequestFactory(); } @@ -3230,4 +3250,12 @@ static class HighlightEntity { @Id private String id; private String message; } + + @Data + @Document(indexName = "test-index-optimistic-entity-template") + static class OptimisticEntity { + @Id private String id; + private String message; + private SeqNoPrimaryTerm seqNoPrimaryTerm; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java index 90d9833d86..7748089aa7 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java @@ -24,6 +24,8 @@ import lombok.Data; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; +import org.springframework.dao.OptimisticLockingFailureException; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -81,6 +83,7 @@ * @author Martin Choraine * @author Aleksei Arsenev * @author Russell Parry + * @author Roman Puchkovskiy */ @SpringIntegrationTest public class ReactiveElasticsearchTemplateTests { @@ -855,6 +858,25 @@ void shouldReturnEmptyFluxOnSaveAllWithEmptyInput() { .verifyComplete(); } + @Test // DATAES-799 + void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnEntityWithSeqNoPrimaryTermProperty() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = template.save(original).block(); + + OptimisticEntity forEdit1 = template.get(saved.getId(), OptimisticEntity.class).block(); + OptimisticEntity forEdit2 = template.get(saved.getId(), OptimisticEntity.class).block(); + + forEdit1.setMessage("It'll be ok"); + template.save(forEdit1).block(); + + forEdit2.setMessage("It'll be great"); + template.save(forEdit2) + .as(StepVerifier::create) + .expectError(OptimisticLockingFailureException.class) + .verify(); + } + @Data @Document(indexName = "marvel") static class Person { @@ -928,4 +950,12 @@ static class SampleEntity { @Version private Long version; @Score private float score; } + + @Data + @Document(indexName = DEFAULT_INDEX) + static class OptimisticEntity { + @Id private String id; + private String message; + private SeqNoPrimaryTerm seqNoPrimaryTerm; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java index 8ca2276152..1058935bc7 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java @@ -36,6 +36,7 @@ * @author Mark Paluch * @author Oliver Gierke * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ public class SimpleElasticsearchPersistentEntityTests { @@ -95,6 +96,52 @@ void shouldFindPropertiesByMappedName() { assertThat(persistentProperty.getFieldName()).isEqualTo("renamed-field"); } + @Test // DATAES-799 + void shouldReportThatThereIsNoSeqNoPrimaryTermPropertyWhenThereIsNoSuchProperty() { + TypeInformation typeInformation = ClassTypeInformation.from(EntityWithoutSeqNoPrimaryTerm.class); + SimpleElasticsearchPersistentEntity entity = new SimpleElasticsearchPersistentEntity<>( + typeInformation); + + assertThat(entity.hasSeqNoPrimaryTermProperty()).isFalse(); + } + + @Test // DATAES-799 + void shouldReportThatThereIsSeqNoPrimaryTermPropertyWhenThereIsSuchProperty() { + TypeInformation typeInformation = ClassTypeInformation.from(EntityWithSeqNoPrimaryTerm.class); + SimpleElasticsearchPersistentEntity entity = new SimpleElasticsearchPersistentEntity<>( + typeInformation); + + entity.addPersistentProperty(createProperty(entity, "seqNoPrimaryTerm")); + + assertThat(entity.hasSeqNoPrimaryTermProperty()).isTrue(); + } + + @Test // DATAES-799 + void shouldReturnSeqNoPrimaryTermPropertyWhenThereIsSuchProperty() { + TypeInformation typeInformation = ClassTypeInformation.from(EntityWithSeqNoPrimaryTerm.class); + SimpleElasticsearchPersistentEntity entity = new SimpleElasticsearchPersistentEntity<>( + typeInformation); + entity.addPersistentProperty(createProperty(entity, "seqNoPrimaryTerm")); + EntityWithSeqNoPrimaryTerm instance = new EntityWithSeqNoPrimaryTerm(); + SeqNoPrimaryTerm seqNoPrimaryTerm = new SeqNoPrimaryTerm(); + + ElasticsearchPersistentProperty property = entity.getSeqNoPrimaryTermProperty(); + entity.getPropertyAccessor(instance).setProperty(property, seqNoPrimaryTerm); + + assertThat(instance.seqNoPrimaryTerm).isSameAs(seqNoPrimaryTerm); + } + + @Test // DATAES-799 + void shouldNotAllowMoreThanOneSeqNoPrimaryTermProperties() { + TypeInformation typeInformation = ClassTypeInformation.from(EntityWithSeqNoPrimaryTerm.class); + SimpleElasticsearchPersistentEntity entity = new SimpleElasticsearchPersistentEntity<>( + typeInformation); + entity.addPersistentProperty(createProperty(entity, "seqNoPrimaryTerm")); + + assertThatThrownBy(() -> entity.addPersistentProperty(createProperty(entity, "seqNoPrimaryTerm2"))) + .isInstanceOf(MappingException.class); + } + private static SimpleElasticsearchPersistentProperty createProperty(SimpleElasticsearchPersistentEntity entity, String field) { @@ -153,4 +200,12 @@ private static class FieldNameEntity { @Nullable @Id private String id; @Nullable @Field(name = "renamed-field") private String renamedField; } + + private static class EntityWithoutSeqNoPrimaryTerm { + } + + private static class EntityWithSeqNoPrimaryTerm { + private SeqNoPrimaryTerm seqNoPrimaryTerm; + private SeqNoPrimaryTerm seqNoPrimaryTerm2; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java index fa195c87ca..aed1c6bf2f 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java @@ -38,6 +38,7 @@ * * @author Oliver Gierke * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ public class SimpleElasticsearchPersistentPropertyUnitTests { @@ -126,7 +127,7 @@ void shouldConvertFromLegacyDate() { assertThat(converted).isEqualTo("20200419T194400.000Z"); } - @Test // DATES-792 + @Test // DATAES-792 void shouldConvertToLegacyDate() { SimpleElasticsearchPersistentEntity persistentEntity = context.getRequiredPersistentEntity(DatesProperty.class); ElasticsearchPersistentProperty persistentProperty = persistentEntity.getRequiredPersistentProperty("legacyDate"); @@ -140,6 +141,22 @@ void shouldConvertToLegacyDate() { assertThat(converted).isEqualTo(legacyDate); } + @Test //DATAES-799 + void shouldReportSeqNoPrimaryTermPropertyWhenTheTypeIsSeqNoPrimaryTerm() { + SimpleElasticsearchPersistentEntity entity = context.getRequiredPersistentEntity(SeqNoPrimaryTermProperty.class); + ElasticsearchPersistentProperty seqNoProperty = entity.getRequiredPersistentProperty("seqNoPrimaryTerm"); + + assertThat(seqNoProperty.isSeqNoPrimaryTermProperty()).isTrue(); + } + + @Test //DATAES-799 + void shouldNotReportSeqNoPrimaryTermPropertyWhenTheTypeIsNotSeqNoPrimaryTerm() { + SimpleElasticsearchPersistentEntity entity = context.getRequiredPersistentEntity(SeqNoPrimaryTermProperty.class); + ElasticsearchPersistentProperty stringProperty = entity.getRequiredPersistentProperty("string"); + + assertThat(stringProperty.isSeqNoPrimaryTermProperty()).isFalse(); + } + static class InvalidScoreProperty { @Nullable @Score String scoreProperty; } @@ -157,4 +174,9 @@ static class DatesProperty { @Nullable @Field(type = FieldType.Date, format = DateFormat.basic_date_time) LocalDateTime localDateTime; @Nullable @Field(type = FieldType.Date, format = DateFormat.basic_date_time) Date legacyDate; } + + static class SeqNoPrimaryTermProperty { + SeqNoPrimaryTerm seqNoPrimaryTerm; + String string; + } } From 72d6177f28a856e7a616a8d82322a13ef8a89873 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sat, 25 Apr 2020 14:48:15 +0400 Subject: [PATCH 02/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Add seqNo- and primaryTerm-related methods to Document --- .../elasticsearch/core/document/Document.java | 65 +++++++++ .../core/document/DocumentAdapters.java | 130 +++++++++++++++++- .../core/document/MapDocument.java | 65 +++++++++ .../core/DocumentAdaptersUnitTests.java | 66 ++++++++- 4 files changed, 317 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/Document.java b/src/main/java/org/springframework/data/elasticsearch/core/document/Document.java index 82c91ccfc7..c6311ed75a 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/Document.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/Document.java @@ -39,6 +39,7 @@ * * @author Mark Paluch * @author Peter-Josef Meisch + * @author Roman Puchkovskiy * @since 4.0 */ public interface Document extends Map { @@ -165,6 +166,70 @@ default void setVersion(long version) { throw new UnsupportedOperationException(); } + /** + * Return {@literal true} if this {@link Document} is associated with a seq_no. + * + * @return {@literal true} if this {@link Document} is associated with a seq_no, {@literal false} otherwise. + */ + default boolean hasSeqNo() { + return false; + } + + /** + * Retrieve the seq_no associated with this {@link Document}. + *

+ * The default implementation throws {@link UnsupportedOperationException}. It's recommended to check + * {@link #hasSeqNo()} prior to calling this method. + * + * @return the seq_no associated with this {@link Document}. + * @throws IllegalStateException if the underlying implementation supports seq_no's but no seq_no was yet + * associated with the document. + */ + default long getSeqNo() { + throw new UnsupportedOperationException(); + } + + /** + * Set the seq_no for this {@link Document}. + *

+ * The default implementation throws {@link UnsupportedOperationException}. + */ + default void setSeqNo(long seqNo) { + throw new UnsupportedOperationException(); + } + + /** + * Return {@literal true} if this {@link Document} is associated with a primary_term. + * + * @return {@literal true} if this {@link Document} is associated with a primary_term, {@literal false} otherwise. + */ + default boolean hasPrimaryTerm() { + return false; + } + + /** + * Retrieve the primary_term associated with this {@link Document}. + *

+ * The default implementation throws {@link UnsupportedOperationException}. It's recommended to check + * {@link #hasPrimaryTerm()} prior to calling this method. + * + * @return the primary_term associated with this {@link Document}. + * @throws IllegalStateException if the underlying implementation supports primary_term's but no primary_term was + * yet associated with the document. + */ + default long getPrimaryTerm() { + throw new UnsupportedOperationException(); + } + + /** + * Set the primary_term for this {@link Document}. + *

+ * The default implementation throws {@link UnsupportedOperationException}. + */ + default void setPrimaryTerm(long primaryTerm) { + throw new UnsupportedOperationException(); + } + /** * Returns the value to which the specified {@code key} is mapped, or {@literal null} if this document contains no * mapping for the key. The value is casted within the method which makes it useful for calling code as it does not diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java b/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java index fb1a491a81..c804e61115 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java @@ -54,6 +54,7 @@ * * @author Mark Paluch * @author Peter-Josef Meisch + * @author Roman Puchkovskiy * @since 4.0 */ public class DocumentAdapters { @@ -76,12 +77,15 @@ public static Document from(GetResponse source) { } if (source.isSourceEmpty()) { - return fromDocumentFields(source, source.getId(), source.getVersion()); + return fromDocumentFields(source, source.getId(), source.getVersion(), + source.getSeqNo(), source.getPrimaryTerm()); } Document document = Document.from(source.getSourceAsMap()); document.setId(source.getId()); document.setVersion(source.getVersion()); + document.setSeqNo(source.getSeqNo()); + document.setPrimaryTerm(source.getPrimaryTerm()); return document; } @@ -104,12 +108,15 @@ public static Document from(GetResult source) { } if (source.isSourceEmpty()) { - return fromDocumentFields(source, source.getId(), source.getVersion()); + return fromDocumentFields(source, source.getId(), source.getVersion(), + source.getSeqNo(), source.getPrimaryTerm()); } Document document = Document.from(source.getSource()); document.setId(source.getId()); document.setVersion(source.getVersion()); + document.setSeqNo(source.getSeqNo()); + document.setPrimaryTerm(source.getPrimaryTerm()); return document; } @@ -150,7 +157,8 @@ public static SearchDocument from(SearchHit source) { if (sourceRef == null || sourceRef.length() == 0) { return new SearchDocumentAdapter(source.getScore(), source.getSortValues(), source.getFields(), highlightFields, - fromDocumentFields(source, source.getId(), source.getVersion())); + fromDocumentFields(source, source.getId(), source.getVersion(), + source.getSeqNo(), source.getPrimaryTerm())); } Document document = Document.from(source.getSourceAsMap()); @@ -159,6 +167,8 @@ public static SearchDocument from(SearchHit source) { if (source.getVersion() >= 0) { document.setVersion(source.getVersion()); } + document.setSeqNo(source.getSeqNo()); + document.setPrimaryTerm(source.getPrimaryTerm()); return new SearchDocumentAdapter(source.getScore(), source.getSortValues(), source.getFields(), highlightFields, document); @@ -170,10 +180,11 @@ public static SearchDocument from(SearchHit source) { * @param documentFields the {@link DocumentField}s backing the {@link Document}. * @return the adapted {@link Document}. */ - public static Document fromDocumentFields(Iterable documentFields, String id, long version) { + public static Document fromDocumentFields(Iterable documentFields, String id, long version, + long seqNo, long primaryTerm) { if (documentFields instanceof Collection) { - return new DocumentFieldAdapter((Collection) documentFields, id, version); + return new DocumentFieldAdapter((Collection) documentFields, id, version, seqNo, primaryTerm); } List fields = new ArrayList<>(); @@ -181,7 +192,7 @@ public static Document fromDocumentFields(Iterable documentFields fields.add(documentField); } - return new DocumentFieldAdapter(fields, id, version); + return new DocumentFieldAdapter(fields, id, version, seqNo, primaryTerm); } // TODO: Performance regarding keys/values/entry-set @@ -190,11 +201,16 @@ static class DocumentFieldAdapter implements Document { private final Collection documentFields; private final String id; private final long version; + private final long seqNo; + private final long primaryTerm; - DocumentFieldAdapter(Collection documentFields, String id, long version) { + DocumentFieldAdapter(Collection documentFields, String id, long version, + long seqNo, long primaryTerm) { this.documentFields = documentFields; this.id = id; this.version = version; + this.seqNo = seqNo; + this.primaryTerm = primaryTerm; } /* @@ -238,6 +254,52 @@ public long getVersion() { return this.version; } + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#hasSeqNo() + */ + @Override + public boolean hasSeqNo() { + return this.seqNo >= 0; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#getSeqNo() + */ + @Override + public long getSeqNo() { + + if (!hasSeqNo()) { + throw new IllegalStateException("No seq_no associated with this Document"); + } + + return this.seqNo; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#hasPrimaryTerm() + */ + @Override + public boolean hasPrimaryTerm() { + return this.primaryTerm > 0; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#getPrimaryTerm() + */ + @Override + public long getPrimaryTerm() { + + if (!hasPrimaryTerm()) { + throw new IllegalStateException("No primary_term associated with this Document"); + } + + return this.primaryTerm; + } + /* * (non-Javadoc) * @see java.util.Map#size() @@ -556,6 +618,60 @@ public long getVersion() { public void setVersion(long version) { delegate.setVersion(version); } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#hasSeqNo() + */ + @Override + public boolean hasSeqNo() { + return delegate.hasSeqNo(); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#getSeqNo() + */ + @Override + public long getSeqNo() { + return delegate.getSeqNo(); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#setSeqNo(long) + */ + @Override + public void setSeqNo(long seqNo) { + delegate.setSeqNo(seqNo); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#hasPrimaryTerm() + */ + @Override + public boolean hasPrimaryTerm() { + return delegate.hasPrimaryTerm(); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#getPrimaryTerm() + */ + @Override + public long getPrimaryTerm() { + return delegate.getPrimaryTerm(); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#setPrimaryTerm(long) + */ + @Override + public void setPrimaryTerm(long primaryTerm) { + delegate.setPrimaryTerm(primaryTerm); + } /* * (non-Javadoc) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java b/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java index a9f114969b..89345fd66b 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java @@ -31,6 +31,7 @@ * {@link Document} implementation backed by a {@link LinkedHashMap}. * * @author Mark Paluch + * @author Roman Puchkovskiy * @since 4.0 */ class MapDocument implements Document { @@ -41,6 +42,8 @@ class MapDocument implements Document { private @Nullable String id; private @Nullable Long version; + private @Nullable Long seqNo; + private @Nullable Long primaryTerm; MapDocument() { this(new LinkedHashMap<>()); @@ -114,6 +117,68 @@ public void setVersion(long version) { this.version = version; } + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#hasSeqNo() + */ + @Override + public boolean hasSeqNo() { + return this.seqNo != null && this.seqNo >= 0; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#getSeqNo() + */ + @Override + public long getSeqNo() { + + if (!hasSeqNo()) { + throw new IllegalStateException("No seq_no associated with this Document"); + } + + return this.seqNo; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#setSeqNo() + */ + public void setSeqNo(long seqNo) { + this.seqNo = seqNo; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#hasPrimaryTerm() + */ + @Override + public boolean hasPrimaryTerm() { + return this.primaryTerm != null && this.primaryTerm > 0; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#getPrimaryTerm() + */ + @Override + public long getPrimaryTerm() { + + if (!hasPrimaryTerm()) { + throw new IllegalStateException("No primary_term associated with this Document"); + } + + return this.primaryTerm; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.elasticsearch.core.document.Document#setPrimaryTerm() + */ + public void setPrimaryTerm(long primaryTerm) { + this.primaryTerm = primaryTerm; + } + /* * (non-Javadoc) * @see java.util.Map#size() diff --git a/src/test/java/org/springframework/data/elasticsearch/core/DocumentAdaptersUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/DocumentAdaptersUnitTests.java index 852e8f0dae..8568e6d463 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/DocumentAdaptersUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/DocumentAdaptersUnitTests.java @@ -38,6 +38,7 @@ * * @author Mark Paluch * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ public class DocumentAdaptersUnitTests { @@ -47,7 +48,7 @@ public void shouldAdaptGetResponse() { Map fields = Collections.singletonMap("field", new DocumentField("field", Collections.singletonList("value"))); - GetResult getResult = new GetResult("index", "type", "my-id", 1, 1, 42, true, null, fields, null); + GetResult getResult = new GetResult("index", "type", "my-id", 1, 2, 42, true, null, fields, null); GetResponse response = new GetResponse(getResult); Document document = DocumentAdapters.from(response); @@ -57,6 +58,10 @@ public void shouldAdaptGetResponse() { assertThat(document.hasVersion()).isTrue(); assertThat(document.getVersion()).isEqualTo(42); assertThat(document.get("field")).isEqualTo("value"); + assertThat(document.hasSeqNo()).isTrue(); + assertThat(document.getSeqNo()).isEqualTo(1); + assertThat(document.hasPrimaryTerm()).isTrue(); + assertThat(document.getPrimaryTerm()).isEqualTo(2); } @Test // DATAES-628 @@ -64,7 +69,7 @@ public void shouldAdaptGetResponseSource() { BytesArray source = new BytesArray("{\"field\":\"value\"}"); - GetResult getResult = new GetResult("index", "type", "my-id", 1, 1, 42, true, source, Collections.emptyMap(), null); + GetResult getResult = new GetResult("index", "type", "my-id", 1, 2, 42, true, source, Collections.emptyMap(), null); GetResponse response = new GetResponse(getResult); Document document = DocumentAdapters.from(response); @@ -74,6 +79,51 @@ public void shouldAdaptGetResponseSource() { assertThat(document.hasVersion()).isTrue(); assertThat(document.getVersion()).isEqualTo(42); assertThat(document.get("field")).isEqualTo("value"); + assertThat(document.hasSeqNo()).isTrue(); + assertThat(document.getSeqNo()).isEqualTo(1); + assertThat(document.hasPrimaryTerm()).isTrue(); + assertThat(document.getPrimaryTerm()).isEqualTo(2); + } + + @Test // DATAES-799 + public void shouldAdaptGetResult() { + + Map fields = Collections.singletonMap("field", + new DocumentField("field", Collections.singletonList("value"))); + + GetResult getResult = new GetResult("index", "type", "my-id", 1, 2, 42, true, null, fields, null); + + Document document = DocumentAdapters.from(getResult); + + assertThat(document.hasId()).isTrue(); + assertThat(document.getId()).isEqualTo("my-id"); + assertThat(document.hasVersion()).isTrue(); + assertThat(document.getVersion()).isEqualTo(42); + assertThat(document.get("field")).isEqualTo("value"); + assertThat(document.hasSeqNo()).isTrue(); + assertThat(document.getSeqNo()).isEqualTo(1); + assertThat(document.hasPrimaryTerm()).isTrue(); + assertThat(document.getPrimaryTerm()).isEqualTo(2); + } + + @Test // DATAES-799 + public void shouldAdaptGetResultSource() { + + BytesArray source = new BytesArray("{\"field\":\"value\"}"); + + GetResult getResult = new GetResult("index", "type", "my-id", 1, 2, 42, true, source, Collections.emptyMap(), null); + + Document document = DocumentAdapters.from(getResult); + + assertThat(document.hasId()).isTrue(); + assertThat(document.getId()).isEqualTo("my-id"); + assertThat(document.hasVersion()).isTrue(); + assertThat(document.getVersion()).isEqualTo(42); + assertThat(document.get("field")).isEqualTo("value"); + assertThat(document.hasSeqNo()).isTrue(); + assertThat(document.getSeqNo()).isEqualTo(1); + assertThat(document.hasPrimaryTerm()).isTrue(); + assertThat(document.getPrimaryTerm()).isEqualTo(2); } @Test // DATAES-628 @@ -83,6 +133,8 @@ public void shouldAdaptSearchResponse() { new DocumentField("field", Collections.singletonList("value"))); SearchHit searchHit = new SearchHit(123, "my-id", new Text("type"), fields); + searchHit.setSeqNo(1); + searchHit.setPrimaryTerm(2); searchHit.score(42); SearchDocument document = DocumentAdapters.from(searchHit); @@ -92,6 +144,10 @@ public void shouldAdaptSearchResponse() { assertThat(document.hasVersion()).isFalse(); assertThat(document.getScore()).isBetween(42f, 42f); assertThat(document.get("field")).isEqualTo("value"); + assertThat(document.hasSeqNo()).isTrue(); + assertThat(document.getSeqNo()).isEqualTo(1); + assertThat(document.hasPrimaryTerm()).isTrue(); + assertThat(document.getPrimaryTerm()).isEqualTo(2); } @Test // DATAES-628 @@ -151,6 +207,8 @@ public void shouldAdaptSearchResponseSource() { SearchHit searchHit = new SearchHit(123, "my-id", new Text("type"), Collections.emptyMap()); searchHit.sourceRef(source).score(42); searchHit.version(22); + searchHit.setSeqNo(1); + searchHit.setPrimaryTerm(2); SearchDocument document = DocumentAdapters.from(searchHit); @@ -160,5 +218,9 @@ public void shouldAdaptSearchResponseSource() { assertThat(document.getVersion()).isEqualTo(22); assertThat(document.getScore()).isBetween(42f, 42f); assertThat(document.get("field")).isEqualTo("value"); + assertThat(document.hasSeqNo()).isTrue(); + assertThat(document.getSeqNo()).isEqualTo(1); + assertThat(document.hasPrimaryTerm()).isTrue(); + assertThat(document.getPrimaryTerm()).isEqualTo(2); } } From d45997e927ccc72790e766011d07a16960cb179b Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sat, 25 Apr 2020 15:03:53 +0400 Subject: [PATCH 03/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Fill SeqNoPrimaryTerm during mapping when reading responses --- .../MappingElasticsearchConverter.java | 29 ++++++++++++------- .../core/ElasticsearchTemplateTests.java | 15 ++++++++++ 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index 7865660938..dd7bfb488a 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java @@ -15,17 +15,8 @@ */ package org.springframework.data.elasticsearch.core.convert; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.Map.Entry; -import java.util.Optional; -import java.util.Set; import org.springframework.beans.BeansException; import org.springframework.beans.factory.InitializingBean; @@ -43,6 +34,7 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentPropertyConverter; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.context.MappingContext; @@ -205,6 +197,12 @@ protected R readEntity(ElasticsearchPersistentEntity entity, Map R readEntity(ElasticsearchPersistentEntity entity, Map R readProperties(ElasticsearchPersistentEntity entity, R instance, ElasticsearchPropertyValueProvider valueProvider) { diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 9187719276..1e8f936f31 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -3070,6 +3070,21 @@ void shouldDoExistsWithIndexCoordinates() { assertThat(operations.exists("42", index)).isTrue(); } + @Test // DATAES-799 + void getShouldReturnSeqNoPrimaryTerm() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = operations.save(original); + + OptimisticEntity retrieved = operations.get(saved.getId(), OptimisticEntity.class); + + assertThat(retrieved.seqNoPrimaryTerm).isNotNull(); + assertThat(retrieved.seqNoPrimaryTerm.getSequenceNumber()).isNotNull(); + assertThat(retrieved.seqNoPrimaryTerm.getSequenceNumber()).isNotNegative(); + assertThat(retrieved.seqNoPrimaryTerm.getPrimaryTerm()).isNotNull(); + assertThat(retrieved.seqNoPrimaryTerm.getPrimaryTerm()).isPositive(); + } + @Test // DATAES-799 void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnEntityWithSeqNoPrimaryTermProperty() { OptimisticEntity original = new OptimisticEntity(); From c209fb4a67d0963f55b029f5d130baa2a3766172 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sat, 25 Apr 2020 15:35:04 +0400 Subject: [PATCH 04/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Pass seq_no and primary_term to IndexRequest --- .../core/AbstractElasticsearchTemplate.java | 27 +++++++++++++++-- .../ElasticsearchExceptionTranslator.java | 21 ++++++++++++++ .../elasticsearch/core/RequestFactory.java | 8 +++++ .../core/mapping/SeqNoPrimaryTerm.java | 12 +++++--- .../elasticsearch/core/query/IndexQuery.java | 22 +++++++++++++- .../core/query/IndexQueryBuilder.java | 12 ++++++++ ...ElasticsearchExceptionTranslatorTests.java | 29 +++++++++++++++++++ .../core/RequestFactoryTests.java | 16 ++++++++++ 8 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java diff --git a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java index 5aa64604ae..b45c7b9c74 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java @@ -46,6 +46,7 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; import org.springframework.data.elasticsearch.core.query.GetQuery; import org.springframework.data.elasticsearch.core.query.IndexQuery; @@ -438,6 +439,22 @@ private Long getEntityVersion(Object entity) { return null; } + @Nullable + private SeqNoPrimaryTerm getEntitySeqNoPrimaryTerm(Object entity) { + ElasticsearchPersistentEntity persistentEntity = getRequiredPersistentEntity(entity.getClass()); + ElasticsearchPersistentProperty property = persistentEntity.getSeqNoPrimaryTermProperty(); + + if (property != null) { + Object seqNoPrimaryTerm = persistentEntity.getPropertyAccessor(entity).getProperty(property); + + if (seqNoPrimaryTerm != null && SeqNoPrimaryTerm.class.isAssignableFrom(seqNoPrimaryTerm.getClass())) { + return (SeqNoPrimaryTerm) seqNoPrimaryTerm; + } + } + + return null; + } + private IndexQuery getIndexQuery(T entity) { String id = getEntityId(entity); @@ -445,11 +462,15 @@ private IndexQuery getIndexQuery(T entity) { id = elasticsearchConverter.convertId(id); } - return new IndexQueryBuilder() // + IndexQueryBuilder builder = new IndexQueryBuilder() // .withId(id) // .withVersion(getEntityVersion(entity)) // - .withObject(entity) // - .build(); + .withObject(entity); + SeqNoPrimaryTerm seqNoPrimaryTerm = getEntitySeqNoPrimaryTerm(entity); + if (seqNoPrimaryTerm != null) { + builder.withSeqNoPrimaryTerm(seqNoPrimaryTerm); + } + return builder.build(); } /** diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java index f4aeb1c00e..5f8ae5b970 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java @@ -22,9 +22,11 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.common.ValidationException; +import org.elasticsearch.rest.RestStatus; import org.springframework.dao.DataAccessException; import org.springframework.dao.DataAccessResourceFailureException; import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.dao.support.PersistenceExceptionTranslator; import org.springframework.data.elasticsearch.NoSuchIndexException; import org.springframework.data.elasticsearch.UncategorizedElasticsearchException; @@ -35,6 +37,7 @@ /** * @author Christoph Strobl * @author Peter-Josef Meisch + * @author Roman Puchkovskiy * @since 3.2 */ public class ElasticsearchExceptionTranslator implements PersistenceExceptionTranslator { @@ -50,6 +53,12 @@ public DataAccessException translateExceptionIfPossible(RuntimeException ex) { return new NoSuchIndexException(ObjectUtils.nullSafeToString(elasticsearchException.getMetadata("es.index")), ex); } + + if (isSeqNoConflict(elasticsearchException)) { + return new OptimisticLockingFailureException("Cannot index a document due to seq_no+primary_term conflict", + elasticsearchException); + } + return new UncategorizedElasticsearchException(ex.getMessage(), ex); } @@ -65,6 +74,18 @@ public DataAccessException translateExceptionIfPossible(RuntimeException ex) { return null; } + private boolean isSeqNoConflict(ElasticsearchException exception) { + if (!(exception instanceof ElasticsearchStatusException)) { + return false; + } + + ElasticsearchStatusException statusException = (ElasticsearchStatusException) exception; + return statusException.status() == RestStatus.CONFLICT + && statusException.getMessage() != null + && statusException.getMessage().contains("type=version_conflict_engine_exception") + && statusException.getMessage().contains("version conflict, required seqNo"); + } + private boolean indexAvailable(ElasticsearchException ex) { List metadata = ex.getMetadata("es.index_uuid"); diff --git a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java index 73220fce2a..0308944106 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java @@ -83,6 +83,7 @@ * * @author Peter-Josef Meisch * @author Sascha Woo + * @author Roman Puchkovskiy * @since 4.0 */ class RequestFactory { @@ -342,6 +343,13 @@ public IndexRequest indexRequest(IndexQuery query, IndexCoordinates index) { indexRequest.versionType(versionType); } + if (query.getSeqNo() != null) { + indexRequest.setIfSeqNo(query.getSeqNo()); + } + if (query.getPrimaryTerm() != null) { + indexRequest.setIfPrimaryTerm(query.getPrimaryTerm()); + } + return indexRequest; } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java index 8adf33290a..0e78761eee 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java @@ -1,26 +1,30 @@ package org.springframework.data.elasticsearch.core.mapping; +import org.springframework.lang.Nullable; + /** * @author Roman Puchkovskiy * @since 4.0 */ public class SeqNoPrimaryTerm { - private Long sequenceNumber; - private Long primaryTerm; + @Nullable private Long sequenceNumber; + @Nullable private Long primaryTerm; + @Nullable public Long getSequenceNumber() { return sequenceNumber; } - public void setSequenceNumber(Long sequenceNumber) { + public void setSequenceNumber(@Nullable Long sequenceNumber) { this.sequenceNumber = sequenceNumber; } + @Nullable public Long getPrimaryTerm() { return primaryTerm; } - public void setPrimaryTerm(Long primaryTerm) { + public void setPrimaryTerm(@Nullable Long primaryTerm) { this.primaryTerm = primaryTerm; } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQuery.java b/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQuery.java index 5865571e41..fa0ba5748f 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQuery.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQuery.java @@ -23,8 +23,8 @@ * @author Rizwan Idrees * @author Mohsin Husen * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ - public class IndexQuery { @Nullable private String id; @@ -32,6 +32,8 @@ public class IndexQuery { @Nullable private Long version; @Nullable private String source; @Nullable private String parentId; + @Nullable private Long seqNo; + @Nullable private Long primaryTerm; @Nullable public String getId() { @@ -87,4 +89,22 @@ public String getParentId() { public void setParentId(String parentId) { this.parentId = parentId; } + + @Nullable + public Long getSeqNo() { + return seqNo; + } + + public void setSeqNo(Long seqNo) { + this.seqNo = seqNo; + } + + @Nullable + public Long getPrimaryTerm() { + return primaryTerm; + } + + public void setPrimaryTerm(Long primaryTerm) { + this.primaryTerm = primaryTerm; + } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java b/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java index 33652ba60f..e8cf5cc8ae 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java @@ -15,6 +15,7 @@ */ package org.springframework.data.elasticsearch.core.query; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.lang.Nullable; /** @@ -23,6 +24,7 @@ * @author Rizwan Idrees * @author Mohsin Husen * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ public class IndexQueryBuilder { @@ -31,6 +33,8 @@ public class IndexQueryBuilder { @Nullable private Long version; @Nullable private String source; @Nullable private String parentId; + @Nullable private Long seqNo; + @Nullable private Long primaryTerm; public IndexQueryBuilder withId(String id) { this.id = id; @@ -57,6 +61,12 @@ public IndexQueryBuilder withParentId(String parentId) { return this; } + public IndexQueryBuilder withSeqNoPrimaryTerm(SeqNoPrimaryTerm seqNoPrimaryTerm) { + this.seqNo = seqNoPrimaryTerm.getSequenceNumber(); + this.primaryTerm = seqNoPrimaryTerm.getPrimaryTerm(); + return this; + } + public IndexQuery build() { IndexQuery indexQuery = new IndexQuery(); indexQuery.setId(id); @@ -64,6 +74,8 @@ public IndexQuery build() { indexQuery.setParentId(parentId); indexQuery.setSource(source); indexQuery.setVersion(version); + indexQuery.setSeqNo(seqNo); + indexQuery.setPrimaryTerm(primaryTerm); return indexQuery; } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java new file mode 100644 index 0000000000..c5e45c7616 --- /dev/null +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java @@ -0,0 +1,29 @@ +package org.springframework.data.elasticsearch.core; + +import static org.assertj.core.api.Assertions.*; + +import org.elasticsearch.ElasticsearchStatusException; +import org.elasticsearch.rest.RestStatus; +import org.junit.jupiter.api.Test; +import org.springframework.dao.DataAccessException; +import org.springframework.dao.OptimisticLockingFailureException; + +/** + * @author Roman Puchkovskiy + */ +class ElasticsearchExceptionTranslatorTests { + private final ElasticsearchExceptionTranslator translator = new ElasticsearchExceptionTranslator(); + + @Test // DATAES-799 + void shouldConvertElasticsearchStatusExceptionWithSeqNoConflictToOptimisticLockingFailureException() { + ElasticsearchStatusException ex = new ElasticsearchStatusException( + "Elasticsearch exception [type=version_conflict_engine_exception, reason=[WPUUsXEB6uuA6j8_A7AB]: version conflict, required seqNo [34], primary term [16]. current document has seqNo [35] and primary term [16]]", + RestStatus.CONFLICT); + + DataAccessException translated = translator.translateExceptionIfPossible(ex); + + assertThat(translated).isInstanceOf(OptimisticLockingFailureException.class); + assertThat(translated.getMessage()).startsWith("Cannot index a document due to seq_no+primary_term conflict"); + assertThat(translated.getCause()).isSameAs(ex); + } +} \ No newline at end of file diff --git a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java index f7d0bb6daa..6866e26d3c 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java @@ -22,6 +22,7 @@ import java.util.Collections; +import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchRequestBuilder; @@ -44,12 +45,14 @@ import org.springframework.data.elasticsearch.core.query.Criteria; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; import org.springframework.data.elasticsearch.core.query.GeoDistanceOrder; +import org.springframework.data.elasticsearch.core.query.IndexQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; import org.springframework.data.elasticsearch.core.query.Query; import org.springframework.lang.Nullable; /** * @author Peter-Josef Meisch + * @author Roman Puchkovskiy */ @ExtendWith(MockitoExtension.class) class RequestFactoryTests { @@ -153,6 +156,19 @@ void shouldAddMaxQueryWindowForUnpagedToRequestBuilder() { assertThat(searchRequestBuilder.request().source().size()).isEqualTo(RequestFactory.INDEX_MAX_RESULT_WINDOW); } + @Test // DATAES-799 + void shouldIncludeSeqNoAndPrimaryTermFromIndexQueryToIndexRequest() { + IndexQuery query = new IndexQuery(); + query.setObject(new Person()); + query.setSeqNo(1L); + query.setPrimaryTerm(2L); + + IndexRequest request = requestFactory.indexRequest(query, IndexCoordinates.of("persons")); + + assertThat(request.ifSeqNo()).isEqualTo(1L); + assertThat(request.ifPrimaryTerm()).isEqualTo(2L); + } + static class Person { @Nullable @Id String id; @Nullable @Field(name = "last-name") String lastName; From 7ab32c4911ba12c821bb09a966e3b7bde273cbf3 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sun, 26 Apr 2020 14:32:57 +0400 Subject: [PATCH 05/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Ignore SeqNoPrimaryTerm property during mapping --- .../MappingElasticsearchConverter.java | 2 +- .../ElasticsearchPersistentProperty.java | 8 +++++ .../core/mapping/SeqNoPrimaryTerm.java | 16 ++++++++++ ...SimpleElasticsearchPersistentProperty.java | 10 +++++++ ...appingElasticsearchConverterUnitTests.java | 30 +++++++++++++++++++ .../core/index/MappingBuilderTests.java | 17 +++++++++++ ...sticsearchPersistentPropertyUnitTests.java | 20 +++++++++++-- 7 files changed, 100 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index dd7bfb488a..89995000c6 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java @@ -237,7 +237,7 @@ protected R readProperties(ElasticsearchPersistentEntity entity, R instan for (ElasticsearchPersistentProperty prop : entity) { - if (entity.isConstructorArgument(prop) || prop.isScoreProperty()) { + if (entity.isConstructorArgument(prop) || prop.isScoreProperty() || !prop.isReadable()) { continue; } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentProperty.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentProperty.java index f49f71b1df..569fbb7fe0 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentProperty.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentProperty.java @@ -86,6 +86,14 @@ public interface ElasticsearchPersistentProperty extends PersistentProperty { INSTANCE; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java index 0e78761eee..1d1cb845e4 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java @@ -10,6 +10,14 @@ public class SeqNoPrimaryTerm { @Nullable private Long sequenceNumber; @Nullable private Long primaryTerm; + public SeqNoPrimaryTerm() { + } + + public SeqNoPrimaryTerm(@Nullable Long sequenceNumber, @Nullable Long primaryTerm) { + this.sequenceNumber = sequenceNumber; + this.primaryTerm = primaryTerm; + } + @Nullable public Long getSequenceNumber() { return sequenceNumber; @@ -27,4 +35,12 @@ public Long getPrimaryTerm() { public void setPrimaryTerm(@Nullable Long primaryTerm) { this.primaryTerm = primaryTerm; } + + @Override + public String toString() { + return "SeqNoPrimaryTerm{" + + "sequenceNumber=" + sequenceNumber + + ", primaryTerm=" + primaryTerm + + '}'; + } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java index fdad3924c3..9bbb7d038b 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java @@ -96,6 +96,16 @@ public ElasticsearchPersistentPropertyConverter getPropertyConverter() { return propertyConverter; } + @Override + public boolean isWritable() { + return super.isWritable() && !isSeqNoPrimaryTermProperty(); + } + + @Override + public boolean isReadable() { + return !isTransient() && !isSeqNoPrimaryTermProperty(); + } + /** * Initializes an {@link ElasticsearchPersistentPropertyConverter} if this property is annotated as a Field with type * {@link FieldType#Date}, has a {@link DateFormat} set and if the type of the property is one of the Java8 temporal diff --git a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java index 1347f98800..87bbbd0685 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java @@ -15,6 +15,7 @@ */ package org.springframework.data.elasticsearch.core.convert; +import static java.util.Collections.*; import static org.assertj.core.api.Assertions.*; import static org.skyscreamer.jsonassert.JSONAssert.*; @@ -54,6 +55,7 @@ import org.springframework.data.elasticsearch.annotations.GeoPointField; import org.springframework.data.elasticsearch.core.document.Document; import org.springframework.data.elasticsearch.core.geo.GeoPoint; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; import org.springframework.data.geo.Box; import org.springframework.data.geo.Circle; @@ -69,6 +71,7 @@ * @author Mark Paluch * @author Peter-Josef Meisch * @author Konrad Kurdej + * @author Roman Puchkovskiy */ public class MappingElasticsearchConverterUnitTests { @@ -695,6 +698,26 @@ void readGenericListWithMaps() { assertThat(wrapper.getSchemaLessObject()).isEqualTo(mapWithSimpleList); } + @Test // DATAES-799 + void shouldNotWriteSeqNoPrimaryTermProperty() { + EntityWithSeqNoPrimaryTerm entity = new EntityWithSeqNoPrimaryTerm(); + entity.seqNoPrimaryTerm = new SeqNoPrimaryTerm(1L, 2L); + Document document = Document.create(); + + mappingElasticsearchConverter.write(entity, document); + + assertThat(document).doesNotContainKey("seqNoPrimaryTerm"); + } + + @Test // DATAES-799 + void shouldNotReadSeqNoPrimaryTermProperty() { + Document document = Document.create().append("seqNoPrimaryTerm", emptyMap()); + + EntityWithSeqNoPrimaryTerm entity = mappingElasticsearchConverter.read(EntityWithSeqNoPrimaryTerm.class, document); + + assertThat(entity.seqNoPrimaryTerm).isNull(); + } + private String pointTemplate(String name, Point point) { return String.format(Locale.ENGLISH, "\"%s\":{\"lat\":%.1f,\"lon\":%.1f}", name, point.getX(), point.getY()); } @@ -901,4 +924,11 @@ static class SchemaLessObjectWrapper { private Map schemaLessObject; } + + @Data + @org.springframework.data.elasticsearch.annotations.Document(indexName = "test-index-entity-with-seq-no-primary-term-mapper") + static class EntityWithSeqNoPrimaryTerm { + + @Nullable private SeqNoPrimaryTerm seqNoPrimaryTerm; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java b/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java index f652044def..d5d5951776 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java @@ -24,6 +24,7 @@ import lombok.AllArgsConstructor; import lombok.Builder; +import lombok.Data; import lombok.Getter; import lombok.NoArgsConstructor; import lombok.Setter; @@ -57,6 +58,7 @@ import org.springframework.data.elasticsearch.core.completion.Completion; import org.springframework.data.elasticsearch.core.geo.GeoPoint; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.IndexQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; @@ -79,6 +81,7 @@ * @author Sascha Woo * @author Peter-Josef Meisch * @author Xiao Yu + * @author Roman Puchkovskiy */ @SpringIntegrationTest @ContextConfiguration(classes = { ElasticsearchTemplateConfiguration.class }) @@ -572,6 +575,13 @@ void shouldWriteCompletionContextInfo() throws JSONException { assertEquals(expected, mapping, false); } + @Test // DATAES-799 + void shouldNotIncludeSeqNoPrimaryTermPropertyFromMappingWhenNotAnnotatedWithField() { + String propertyMapping = getMappingBuilder().buildPropertyMapping(EntityWithSeqNoPrimaryTerm.class); + + assertThat(propertyMapping).doesNotContain("seqNoPrimaryTerm"); + } + /** * @author Xiao Yu */ @@ -1052,4 +1062,11 @@ static class CompletionDocument { @CompletionField(contexts = { @CompletionContext(name = "location", type = ContextMapping.Type.GEO, path = "proppath") }) private Completion suggest; } + + @Data + @Document(indexName = "test-index-entity-with-seq-no-primary-term-mapping-builder") + static class EntityWithSeqNoPrimaryTerm { + + @Nullable private SeqNoPrimaryTerm seqNoPrimaryTerm; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java index aed1c6bf2f..9665aab155 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java @@ -141,7 +141,7 @@ void shouldConvertToLegacyDate() { assertThat(converted).isEqualTo(legacyDate); } - @Test //DATAES-799 + @Test // DATAES-799 void shouldReportSeqNoPrimaryTermPropertyWhenTheTypeIsSeqNoPrimaryTerm() { SimpleElasticsearchPersistentEntity entity = context.getRequiredPersistentEntity(SeqNoPrimaryTermProperty.class); ElasticsearchPersistentProperty seqNoProperty = entity.getRequiredPersistentProperty("seqNoPrimaryTerm"); @@ -149,7 +149,7 @@ void shouldReportSeqNoPrimaryTermPropertyWhenTheTypeIsSeqNoPrimaryTerm() { assertThat(seqNoProperty.isSeqNoPrimaryTermProperty()).isTrue(); } - @Test //DATAES-799 + @Test // DATAES-799 void shouldNotReportSeqNoPrimaryTermPropertyWhenTheTypeIsNotSeqNoPrimaryTerm() { SimpleElasticsearchPersistentEntity entity = context.getRequiredPersistentEntity(SeqNoPrimaryTermProperty.class); ElasticsearchPersistentProperty stringProperty = entity.getRequiredPersistentProperty("string"); @@ -157,6 +157,22 @@ void shouldNotReportSeqNoPrimaryTermPropertyWhenTheTypeIsNotSeqNoPrimaryTerm() { assertThat(stringProperty.isSeqNoPrimaryTermProperty()).isFalse(); } + @Test // DATAES-799 + void seqNoPrimaryTermPropertyShouldNotBeWritable() { + SimpleElasticsearchPersistentEntity entity = context.getRequiredPersistentEntity(SeqNoPrimaryTermProperty.class); + ElasticsearchPersistentProperty seqNoProperty = entity.getRequiredPersistentProperty("seqNoPrimaryTerm"); + + assertThat(seqNoProperty.isWritable()).isFalse(); + } + + @Test // DATAES-799 + void seqNoPrimaryTermPropertyShouldNotBeReadable() { + SimpleElasticsearchPersistentEntity entity = context.getRequiredPersistentEntity(SeqNoPrimaryTermProperty.class); + ElasticsearchPersistentProperty seqNoProperty = entity.getRequiredPersistentProperty("seqNoPrimaryTerm"); + + assertThat(seqNoProperty.isReadable()).isFalse(); + } + static class InvalidScoreProperty { @Nullable @Score String scoreProperty; } From 7742d8c3e8cfeb7b93809b242a3e9a47a5d311f4 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sun, 26 Apr 2020 14:39:21 +0400 Subject: [PATCH 06/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Introduce SequenceNumbers --- .../elasticsearch/core/SequenceNumbers.java | 17 +++++++++++++ .../core/document/DocumentAdapters.java | 5 ++-- .../core/document/MapDocument.java | 5 ++-- .../core/SequenceNumbersTests.java | 24 +++++++++++++++++++ 4 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java create mode 100644 src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java diff --git a/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java b/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java new file mode 100644 index 0000000000..ea79624bd8 --- /dev/null +++ b/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java @@ -0,0 +1,17 @@ +package org.springframework.data.elasticsearch.core; + +/** + * @author Roman Puchkovskiy + * @since 4.0 + */ +public class SequenceNumbers { + public static boolean isAssignedSeqNo(long seqNo) { + return seqNo >= 0; + } + + public static boolean isAssignedPrimaryTerm(long primaryTerm) { + return primaryTerm > 0; + } + + private SequenceNumbers() {} +} diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java b/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java index c804e61115..86d74bee2d 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java @@ -38,6 +38,7 @@ import org.elasticsearch.index.get.GetResult; import org.elasticsearch.search.SearchHit; import org.springframework.data.elasticsearch.ElasticsearchException; +import org.springframework.data.elasticsearch.core.SequenceNumbers; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -260,7 +261,7 @@ public long getVersion() { */ @Override public boolean hasSeqNo() { - return this.seqNo >= 0; + return SequenceNumbers.isAssignedSeqNo(this.seqNo); } /* @@ -283,7 +284,7 @@ public long getSeqNo() { */ @Override public boolean hasPrimaryTerm() { - return this.primaryTerm > 0; + return SequenceNumbers.isAssignedPrimaryTerm(this.primaryTerm); } /* diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java b/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java index 89345fd66b..62818656d8 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java @@ -22,6 +22,7 @@ import java.util.function.BiConsumer; import org.springframework.data.elasticsearch.ElasticsearchException; +import org.springframework.data.elasticsearch.core.SequenceNumbers; import org.springframework.lang.Nullable; import com.fasterxml.jackson.core.JsonProcessingException; @@ -123,7 +124,7 @@ public void setVersion(long version) { */ @Override public boolean hasSeqNo() { - return this.seqNo != null && this.seqNo >= 0; + return this.seqNo != null && SequenceNumbers.isAssignedSeqNo(this.seqNo); } /* @@ -154,7 +155,7 @@ public void setSeqNo(long seqNo) { */ @Override public boolean hasPrimaryTerm() { - return this.primaryTerm != null && this.primaryTerm > 0; + return this.primaryTerm != null && SequenceNumbers.isAssignedPrimaryTerm(this.primaryTerm); } /* diff --git a/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java b/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java new file mode 100644 index 0000000000..2da43c5c3d --- /dev/null +++ b/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java @@ -0,0 +1,24 @@ +package org.springframework.data.elasticsearch.core; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * @author Roman Puchkovskiy + */ +class SequenceNumbersTests { + @Test // DATAES-799 + void isAssignedSeqNoShouldReturnTrueIffSeqNoIsNonNegative() { + assertFalse(SequenceNumbers.isAssignedSeqNo(org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_SEQ_NO)); + assertFalse(SequenceNumbers.isAssignedSeqNo(org.elasticsearch.index.seqno.SequenceNumbers.NO_OPS_PERFORMED)); + assertTrue(SequenceNumbers.isAssignedSeqNo(0)); + assertTrue(SequenceNumbers.isAssignedSeqNo(1)); + } + + @Test // DATAES-799 + void isAssignedPrimaryTermShouldReturnTrueIffPrimaryTermIsPositive() { + assertFalse(SequenceNumbers.isAssignedPrimaryTerm(org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_PRIMARY_TERM)); + assertTrue(SequenceNumbers.isAssignedPrimaryTerm(1)); + } +} \ No newline at end of file From a4be9dfe3c5db47584927874c30dd4cb7accabe4 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sun, 26 Apr 2020 15:40:57 +0400 Subject: [PATCH 07/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Make sure that search responses contain seq_no and primary_term when the entity needs SeqNoPrimaryTerm to be filled --- .../core/DocumentOperations.java | 2 +- .../elasticsearch/core/RequestFactory.java | 18 +++++- .../core/ElasticsearchTemplateTests.java | 64 ++++++++++++++++++- .../core/RequestFactoryTests.java | 38 ++++++++++- 4 files changed, 116 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/DocumentOperations.java b/src/main/java/org/springframework/data/elasticsearch/core/DocumentOperations.java index 1c05cd801b..279f2156e4 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/DocumentOperations.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/DocumentOperations.java @@ -250,7 +250,7 @@ default void bulkUpdate(List queries, IndexCoordinates index) { * @param clazz the type of the object to be returned * @param index the index from which the object is read. * @return the found object - * @deprecated since 4.0, use {@link #getById(String, Class, IndexCoordinates)} + * @deprecated since 4.0, use {@link #get(String, Class, IndexCoordinates)} */ @Deprecated @Nullable diff --git a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java index 0308944106..5c0b012e65 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java @@ -626,6 +626,9 @@ private SearchRequest prepareSearchRequest(Query query, @Nullable Class clazz SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); sourceBuilder.version(true); sourceBuilder.trackScores(query.getTrackScores()); + if (hasSeqNoPrimaryTermProperty(clazz)) { + sourceBuilder.seqNoAndPrimaryTerm(true); + } if (query.getSourceFilter() != null) { SourceFilter sourceFilter = query.getSourceFilter(); @@ -689,7 +692,20 @@ private SearchRequest prepareSearchRequest(Query query, @Nullable Class clazz return request; } - @SuppressWarnings("unchecked") + private boolean hasSeqNoPrimaryTermProperty(@Nullable Class entityClass) { + + if (entityClass == null) { + return false; + } + + if (!elasticsearchConverter.getMappingContext().hasPersistentEntityFor(entityClass)) { + return false; + } + + ElasticsearchPersistentEntity entity = elasticsearchConverter.getMappingContext().getRequiredPersistentEntity(entityClass); + return entity.hasSeqNoPrimaryTermProperty(); + } + public PutMappingRequest putMappingRequest(IndexCoordinates index, Document mapping) { PutMappingRequest request = new PutMappingRequest(index.getIndexName()); request.source(mapping); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 1e8f936f31..31d3b8a170 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -15,6 +15,7 @@ */ package org.springframework.data.elasticsearch.core; +import static java.util.Collections.*; import static org.apache.commons.lang.RandomStringUtils.*; import static org.assertj.core.api.Assertions.*; import static org.elasticsearch.index.query.QueryBuilders.*; @@ -3077,7 +3078,11 @@ void getShouldReturnSeqNoPrimaryTerm() { OptimisticEntity saved = operations.save(original); OptimisticEntity retrieved = operations.get(saved.getId(), OptimisticEntity.class); - + + assertThatSeqNoPrimaryTermIsFilled(retrieved); + } + + private void assertThatSeqNoPrimaryTermIsFilled(OptimisticEntity retrieved) { assertThat(retrieved.seqNoPrimaryTerm).isNotNull(); assertThat(retrieved.seqNoPrimaryTerm.getSequenceNumber()).isNotNull(); assertThat(retrieved.seqNoPrimaryTerm.getSequenceNumber()).isNotNegative(); @@ -3085,6 +3090,61 @@ void getShouldReturnSeqNoPrimaryTerm() { assertThat(retrieved.seqNoPrimaryTerm.getPrimaryTerm()).isPositive(); } + @Test // DATAES-799 + void multigetShouldReturnSeqNoPrimaryTerm() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = operations.save(original); + + List retrievedList = operations.multiGet(queryForOne(saved.getId()), OptimisticEntity.class, + operations.getIndexCoordinatesFor(OptimisticEntity.class)); + OptimisticEntity retrieved = retrievedList.get(0); + + assertThatSeqNoPrimaryTermIsFilled(retrieved); + } + + private Query queryForOne(String id) { + return new NativeSearchQueryBuilder().withIds(singletonList(id)).build(); + } + + @Test // DATAES-799 + void searchShouldReturnSeqNoPrimaryTerm() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = operations.save(original); + + SearchHits retrievedHits = operations.search(queryForOne(saved.getId()), OptimisticEntity.class); + OptimisticEntity retrieved = retrievedHits.getSearchHit(0).getContent(); + + assertThatSeqNoPrimaryTermIsFilled(retrieved); + } + + @Test // DATAES-799 + void multiSearchShouldReturnSeqNoPrimaryTerm() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = operations.save(original); + + List> retrievedHits = operations.multiSearch(singletonList(queryForOne(saved.getId())), + OptimisticEntity.class, operations.getIndexCoordinatesFor(OptimisticEntity.class)); + OptimisticEntity retrieved = retrievedHits.get(0).getSearchHit(0).getContent(); + + assertThatSeqNoPrimaryTermIsFilled(retrieved); + } + + @Test // DATAES-799 + void searchForStreamShouldReturnSeqNoPrimaryTerm() { + OptimisticEntity original = new OptimisticEntity(); + original.setMessage("It's fine"); + OptimisticEntity saved = operations.save(original); + + SearchHitsIterator retrievedHits = operations.searchForStream(queryForOne(saved.getId()), + OptimisticEntity.class); + OptimisticEntity retrieved = retrievedHits.next().getContent(); + + assertThatSeqNoPrimaryTermIsFilled(retrieved); + } + @Test // DATAES-799 void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnEntityWithSeqNoPrimaryTermProperty() { OptimisticEntity original = new OptimisticEntity(); @@ -3093,7 +3153,7 @@ void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnEnt OptimisticEntity forEdit1 = operations.get(saved.getId(), OptimisticEntity.class); OptimisticEntity forEdit2 = operations.get(saved.getId(), OptimisticEntity.class); - + forEdit1.setMessage("It'll be ok"); operations.save(forEdit1); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java index 6866e26d3c..93b456acef 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java @@ -20,7 +20,8 @@ import static org.mockito.Mockito.*; import static org.skyscreamer.jsonassert.JSONAssert.*; -import java.util.Collections; +import java.util.Arrays; +import java.util.HashSet; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchAction; @@ -41,6 +42,7 @@ import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter; import org.springframework.data.elasticsearch.core.geo.GeoPoint; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; import org.springframework.data.elasticsearch.core.query.Criteria; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; @@ -65,7 +67,7 @@ class RequestFactoryTests { @BeforeAll static void setUpAll() { SimpleElasticsearchMappingContext mappingContext = new SimpleElasticsearchMappingContext(); - mappingContext.setInitialEntitySet(Collections.singleton(Person.class)); + mappingContext.setInitialEntitySet(new HashSet<>(Arrays.asList(Person.class, EntityWithSeqNoAndPrimaryTerm.class))); mappingContext.afterPropertiesSet(); converter = new MappingElasticsearchConverter(mappingContext, new GenericConversionService()); @@ -169,9 +171,41 @@ void shouldIncludeSeqNoAndPrimaryTermFromIndexQueryToIndexRequest() { assertThat(request.ifPrimaryTerm()).isEqualTo(2L); } + @Test // DATAES-799 + void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassDoesNotContainSeqNoPrimaryTermProperty() { + Query query = new NativeSearchQueryBuilder().build(); + + SearchRequest request = requestFactory.searchRequest(query, Person.class, IndexCoordinates.of("persons")); + + assertThat(request.source().seqNoAndPrimaryTerm()).isNull(); + } + + @Test // DATAES-799 + void shouldRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassContainsSeqNoPrimaryTermProperty() { + Query query = new NativeSearchQueryBuilder().build(); + + SearchRequest request = requestFactory.searchRequest(query, EntityWithSeqNoAndPrimaryTerm.class, + IndexCoordinates.of("seqNoPrimaryTerm")); + + assertThat(request.source().seqNoAndPrimaryTerm()).isTrue(); + } + + @Test // DATAES-799 + void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassIsNull() { + Query query = new NativeSearchQueryBuilder().build(); + + SearchRequest request = requestFactory.searchRequest(query, null, IndexCoordinates.of("persons")); + + assertThat(request.source().seqNoAndPrimaryTerm()).isNull(); + } + static class Person { @Nullable @Id String id; @Nullable @Field(name = "last-name") String lastName; @Nullable @Field(name = "current-location") GeoPoint location; } + + static class EntityWithSeqNoAndPrimaryTerm { + @Nullable private SeqNoPrimaryTerm seqNoPrimaryTerm; + } } From a8643c4b95e78a151ab0246194f7648610063288 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sun, 26 Apr 2020 16:03:43 +0400 Subject: [PATCH 08/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Make sure that usage of SeqNoPrimaryTerm together with @Version does not cause index rejections --- .../core/AbstractElasticsearchTemplate.java | 4 +- .../core/ElasticsearchTemplateTests.java | 38 +++++++++++++++++++ .../core/RequestFactoryTests.java | 6 +-- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java index b45c7b9c74..cebe310b84 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java @@ -464,11 +464,13 @@ private IndexQuery getIndexQuery(T entity) { IndexQueryBuilder builder = new IndexQueryBuilder() // .withId(id) // - .withVersion(getEntityVersion(entity)) // .withObject(entity); SeqNoPrimaryTerm seqNoPrimaryTerm = getEntitySeqNoPrimaryTerm(entity); if (seqNoPrimaryTerm != null) { builder.withSeqNoPrimaryTerm(seqNoPrimaryTerm); + } else { + // version cannot be used together with seq_no and primary_term + builder.withVersion(getEntityVersion(entity)); } return builder.build(); } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 31d3b8a170..1c9c552ade 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -3162,6 +3162,35 @@ void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnEnt .isInstanceOf(OptimisticLockingFailureException.class); } + @Test // DATAES-799 + void shouldThrowOptimisticLockingFailureExceptionWhenConcurrentUpdateOccursOnVersionedEntityWithSeqNoPrimaryTermProperty() { + OptimisticAndVersionedEntity original = new OptimisticAndVersionedEntity(); + original.setMessage("It's fine"); + OptimisticAndVersionedEntity saved = operations.save(original); + + OptimisticAndVersionedEntity forEdit1 = operations.get(saved.getId(), OptimisticAndVersionedEntity.class); + OptimisticAndVersionedEntity forEdit2 = operations.get(saved.getId(), OptimisticAndVersionedEntity.class); + + forEdit1.setMessage("It'll be ok"); + operations.save(forEdit1); + + forEdit2.setMessage("It'll be great"); + assertThatThrownBy(() -> operations.save(forEdit2)) + .isInstanceOf(OptimisticLockingFailureException.class); + } + + @Test // DATAES-799 + void shouldAllowFullReplaceOfEntityWithBothSeqNoPrimaryTermAndVersion() { + OptimisticAndVersionedEntity original = new OptimisticAndVersionedEntity(); + original.setMessage("It's fine"); + OptimisticAndVersionedEntity saved = operations.save(original); + + OptimisticAndVersionedEntity forEdit = operations.get(saved.getId(), OptimisticAndVersionedEntity.class); + + forEdit.setMessage("It'll be ok"); + operations.save(forEdit); + } + protected RequestFactory getRequestFactory() { return ((AbstractElasticsearchTemplate) operations).getRequestFactory(); } @@ -3333,4 +3362,13 @@ static class OptimisticEntity { private String message; private SeqNoPrimaryTerm seqNoPrimaryTerm; } + + @Data + @Document(indexName = "test-index-optimistic-and-versioned-entity-template") + static class OptimisticAndVersionedEntity { + @Id private String id; + private String message; + private SeqNoPrimaryTerm seqNoPrimaryTerm; + @Version private Long version; + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java index 93b456acef..4de35a9df8 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java @@ -67,7 +67,7 @@ class RequestFactoryTests { @BeforeAll static void setUpAll() { SimpleElasticsearchMappingContext mappingContext = new SimpleElasticsearchMappingContext(); - mappingContext.setInitialEntitySet(new HashSet<>(Arrays.asList(Person.class, EntityWithSeqNoAndPrimaryTerm.class))); + mappingContext.setInitialEntitySet(new HashSet<>(Arrays.asList(Person.class, EntityWithSeqNoPrimaryTerm.class))); mappingContext.afterPropertiesSet(); converter = new MappingElasticsearchConverter(mappingContext, new GenericConversionService()); @@ -184,7 +184,7 @@ void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassDoesNotC void shouldRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassContainsSeqNoPrimaryTermProperty() { Query query = new NativeSearchQueryBuilder().build(); - SearchRequest request = requestFactory.searchRequest(query, EntityWithSeqNoAndPrimaryTerm.class, + SearchRequest request = requestFactory.searchRequest(query, EntityWithSeqNoPrimaryTerm.class, IndexCoordinates.of("seqNoPrimaryTerm")); assertThat(request.source().seqNoAndPrimaryTerm()).isTrue(); @@ -205,7 +205,7 @@ static class Person { @Nullable @Field(name = "current-location") GeoPoint location; } - static class EntityWithSeqNoAndPrimaryTerm { + static class EntityWithSeqNoPrimaryTerm { @Nullable private SeqNoPrimaryTerm seqNoPrimaryTerm; } } From e38f94199def4667ae63f4f188b71c68ebd0964b Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sun, 26 Apr 2020 17:17:19 +0400 Subject: [PATCH 09/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Make tests for ElasticsearchTemplate pass --- .../ElasticsearchExceptionTranslator.java | 22 ++++--- .../core/ElasticsearchTemplate.java | 31 +++++++++- .../elasticsearch/core/RequestFactory.java | 10 ++++ ...ElasticsearchExceptionTranslatorTests.java | 17 ++++++ .../core/RequestFactoryTests.java | 57 ++++++++++++++++++- 5 files changed, 126 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java index 5f8ae5b970..77a8de10dc 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslator.java @@ -22,6 +22,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.common.ValidationException; +import org.elasticsearch.index.engine.VersionConflictEngineException; import org.elasticsearch.rest.RestStatus; import org.springframework.dao.DataAccessException; import org.springframework.dao.DataAccessResourceFailureException; @@ -75,15 +76,22 @@ public DataAccessException translateExceptionIfPossible(RuntimeException ex) { } private boolean isSeqNoConflict(ElasticsearchException exception) { - if (!(exception instanceof ElasticsearchStatusException)) { - return false; + + if (exception instanceof ElasticsearchStatusException) { + ElasticsearchStatusException statusException = (ElasticsearchStatusException) exception; + return statusException.status() == RestStatus.CONFLICT + && statusException.getMessage() != null + && statusException.getMessage().contains("type=version_conflict_engine_exception") + && statusException.getMessage().contains("version conflict, required seqNo"); + } + + if (exception instanceof VersionConflictEngineException) { + VersionConflictEngineException versionConflictEngineException = (VersionConflictEngineException) exception; + return versionConflictEngineException.getMessage() != null + && versionConflictEngineException.getMessage().contains("version conflict, required seqNo"); } - ElasticsearchStatusException statusException = (ElasticsearchStatusException) exception; - return statusException.status() == RestStatus.CONFLICT - && statusException.getMessage() != null - && statusException.getMessage().contains("type=version_conflict_engine_exception") - && statusException.getMessage().contains("version conflict, required seqNo"); + return false; } private boolean indexAvailable(ElasticsearchException ex) { diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java index 43ca3b1c5f..bf884de60d 100755 --- a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java @@ -27,6 +27,7 @@ import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.action.get.MultiGetRequestBuilder; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.action.index.IndexResponse; import org.elasticsearch.action.search.MultiSearchRequest; import org.elasticsearch.action.search.MultiSearchResponse; import org.elasticsearch.action.search.SearchRequestBuilder; @@ -89,6 +90,9 @@ public class ElasticsearchTemplate extends AbstractElasticsearchTemplate { private Client client; @Nullable private String searchTimeout; + // TODO: is it correct to use it here? + private final ElasticsearchExceptionTranslator exceptionTranslator = new ElasticsearchExceptionTranslator(); + // region Initialization public ElasticsearchTemplate(Client client) { this.client = client; @@ -145,7 +149,14 @@ public String index(IndexQuery query, IndexCoordinates index) { maybeCallbackBeforeConvertWithQuery(query, index); IndexRequestBuilder indexRequestBuilder = requestFactory.indexRequestBuilder(client, query, index); - String documentId = indexRequestBuilder.execute().actionGet().getId(); + ActionFuture future = indexRequestBuilder.execute(); + IndexResponse response; + try { + response = future.actionGet(); + } catch (RuntimeException e) { + throw translateException(e); + } + String documentId = response.getId(); // We should call this because we are not going through a mapper. Object queryObject = query.getObject(); @@ -360,4 +371,22 @@ public Client getClient() { return client; } // endregion + + /** + * translates an Exception if possible. Exceptions that are no {@link RuntimeException}s are wrapped in a + * RuntimeException + * + * @param exception the Exception to map + * @return the potentially translated RuntimeException. + * @since 4.0 + */ + private RuntimeException translateException(Exception exception) { + + RuntimeException runtimeException = exception instanceof RuntimeException ? (RuntimeException) exception + : new RuntimeException(exception.getMessage(), exception); + RuntimeException potentiallyTranslatedException = exceptionTranslator + .translateExceptionIfPossible(runtimeException); + + return potentiallyTranslatedException != null ? potentiallyTranslatedException : runtimeException; + } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java index 5c0b012e65..c4361cf325 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java @@ -382,6 +382,13 @@ public IndexRequestBuilder indexRequestBuilder(Client client, IndexQuery query, indexRequestBuilder.setVersionType(versionType); } + if (query.getSeqNo() != null) { + indexRequestBuilder.setIfSeqNo(query.getSeqNo()); + } + if (query.getPrimaryTerm() != null) { + indexRequestBuilder.setIfPrimaryTerm(query.getPrimaryTerm()); + } + return indexRequestBuilder; } @@ -808,6 +815,9 @@ private SearchRequestBuilder prepareSearchRequestBuilder(Query query, Client cli .setSearchType(query.getSearchType()) // .setVersion(true) // .setTrackScores(query.getTrackScores()); + if (hasSeqNoPrimaryTermProperty(clazz)) { + searchRequestBuilder.seqNoAndPrimaryTerm(true); + } if (query.getSourceFilter() != null) { SourceFilter sourceFilter = query.getSourceFilter(); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java index c5e45c7616..273cfbe266 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java @@ -3,11 +3,15 @@ import static org.assertj.core.api.Assertions.*; import org.elasticsearch.ElasticsearchStatusException; +import org.elasticsearch.index.engine.VersionConflictEngineException; +import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.rest.RestStatus; import org.junit.jupiter.api.Test; import org.springframework.dao.DataAccessException; import org.springframework.dao.OptimisticLockingFailureException; +import java.util.UUID; + /** * @author Roman Puchkovskiy */ @@ -26,4 +30,17 @@ void shouldConvertElasticsearchStatusExceptionWithSeqNoConflictToOptimisticLocki assertThat(translated.getMessage()).startsWith("Cannot index a document due to seq_no+primary_term conflict"); assertThat(translated.getCause()).isSameAs(ex); } + + @Test // DATAES-799 + void shouldConvertVersionConflictEngineExceptionWithSeqNoConflictToOptimisticLockingFailureException() { + VersionConflictEngineException ex = new VersionConflictEngineException( + new ShardId("index", "uuid", 1), "exception-id", + "Elasticsearch exception [type=version_conflict_engine_exception, reason=[WPUUsXEB6uuA6j8_A7AB]: version conflict, required seqNo [34], primary term [16]. current document has seqNo [35] and primary term [16]]"); + + DataAccessException translated = translator.translateExceptionIfPossible(ex); + + assertThat(translated).isInstanceOf(OptimisticLockingFailureException.class); + assertThat(translated.getMessage()).startsWith("Cannot index a document due to seq_no+primary_term conflict"); + assertThat(translated.getCause()).isSameAs(ex); + } } \ No newline at end of file diff --git a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java index 4de35a9df8..019f15e29d 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java @@ -23,7 +23,9 @@ import java.util.Arrays; import java.util.HashSet; +import org.elasticsearch.action.index.IndexAction; import org.elasticsearch.action.index.IndexRequest; +import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchRequestBuilder; @@ -172,7 +174,23 @@ void shouldIncludeSeqNoAndPrimaryTermFromIndexQueryToIndexRequest() { } @Test // DATAES-799 - void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassDoesNotContainSeqNoPrimaryTermProperty() { + void shouldIncludeSeqNoAndPrimaryTermFromIndexQueryToIndexRequestBuilder() { + when(client.prepareIndex(anyString(), anyString())) + .thenReturn(new IndexRequestBuilder(client, IndexAction.INSTANCE)); + + IndexQuery query = new IndexQuery(); + query.setObject(new Person()); + query.setSeqNo(1L); + query.setPrimaryTerm(2L); + + IndexRequestBuilder builder = requestFactory.indexRequestBuilder(client, query, IndexCoordinates.of("persons")); + + assertThat(builder.request().ifSeqNo()).isEqualTo(1L); + assertThat(builder.request().ifPrimaryTerm()).isEqualTo(2L); + } + + @Test // DATAES-799 + void shouldNotRequestSeqNoAndPrimaryTermViaSearchRequestWhenEntityClassDoesNotContainSeqNoPrimaryTermProperty() { Query query = new NativeSearchQueryBuilder().build(); SearchRequest request = requestFactory.searchRequest(query, Person.class, IndexCoordinates.of("persons")); @@ -181,7 +199,7 @@ void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassDoesNotC } @Test // DATAES-799 - void shouldRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassContainsSeqNoPrimaryTermProperty() { + void shouldRequestSeqNoAndPrimaryTermViaSearchRequestWhenEntityClassContainsSeqNoPrimaryTermProperty() { Query query = new NativeSearchQueryBuilder().build(); SearchRequest request = requestFactory.searchRequest(query, EntityWithSeqNoPrimaryTerm.class, @@ -191,7 +209,7 @@ void shouldRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassContainsSeq } @Test // DATAES-799 - void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassIsNull() { + void shouldNotRequestSeqNoAndPrimaryTermViaSearchRequestWhenEntityClassIsNull() { Query query = new NativeSearchQueryBuilder().build(); SearchRequest request = requestFactory.searchRequest(query, null, IndexCoordinates.of("persons")); @@ -199,6 +217,39 @@ void shouldNotRequestSeqNoAndPrimartyTermViaSearchRequestWhenEntityClassIsNull() assertThat(request.source().seqNoAndPrimaryTerm()).isNull(); } + @Test // DATAES-799 + void shouldNotRequestSeqNoAndPrimaryTermViaSearchRequestBuilderWhenEntityClassDoesNotContainSeqNoPrimaryTermProperty() { + when(client.prepareSearch(any())).thenReturn(new SearchRequestBuilder(client, SearchAction.INSTANCE)); + Query query = new NativeSearchQueryBuilder().build(); + + SearchRequestBuilder builder = requestFactory.searchRequestBuilder(client, query, Person.class, + IndexCoordinates.of("persons")); + + assertThat(builder.request().source().seqNoAndPrimaryTerm()).isNull(); + } + + @Test // DATAES-799 + void shouldRequestSeqNoAndPrimaryTermViaSearchRequestBuilderWhenEntityClassContainsSeqNoPrimaryTermProperty() { + when(client.prepareSearch(any())).thenReturn(new SearchRequestBuilder(client, SearchAction.INSTANCE)); + Query query = new NativeSearchQueryBuilder().build(); + + SearchRequestBuilder builder = requestFactory.searchRequestBuilder(client, query, + EntityWithSeqNoPrimaryTerm.class, IndexCoordinates.of("seqNoPrimaryTerm")); + + assertThat(builder.request().source().seqNoAndPrimaryTerm()).isTrue(); + } + + @Test // DATAES-799 + void shouldNotRequestSeqNoAndPrimaryTermViaSearchRequestBuilderWhenEntityClassIsNull() { + when(client.prepareSearch(any())).thenReturn(new SearchRequestBuilder(client, SearchAction.INSTANCE)); + Query query = new NativeSearchQueryBuilder().build(); + + SearchRequestBuilder builder = requestFactory.searchRequestBuilder(client, query, null, + IndexCoordinates.of("persons")); + + assertThat(builder.request().source().seqNoAndPrimaryTerm()).isNull(); + } + static class Person { @Nullable @Id String id; @Nullable @Field(name = "last-name") String lastName; From 32c1af13e531b5829bc677a4dc8d087d65233464 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Sun, 26 Apr 2020 18:06:09 +0400 Subject: [PATCH 10/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Implement seq_no + primary_term support for ReactiveElasticsearchTemplate --- .../elasticsearch/core/EntityOperations.java | 40 +++++++ .../core/ReactiveElasticsearchTemplate.java | 20 +++- .../ElasticsearchPersistentEntity.java | 10 ++ .../ReactiveElasticsearchTemplateTests.java | 103 +++++++++++++++++- 4 files changed, 171 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java b/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java index 1a8567d9a0..ceb1b1da4a 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java @@ -21,6 +21,7 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.mapping.IdentifierAccessor; import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.context.MappingContext; @@ -35,6 +36,7 @@ * @author Mark Paluch * @author Christoph Strobl * @author Peter-Josef Meisch + * @author Roman Puchkovskiy * @since 3.2 */ class EntityOperations { @@ -256,6 +258,21 @@ interface AdaptibleEntity extends Entity { @Override @Nullable Number getVersion(); + + /** + * Returns whether there is a property with type SeqNoPrimaryTerm in this entity. + * + * @return true if there is SeqNoPrimaryTerm property + * @since 4.0 + */ + boolean hasSeqNoPrimaryTerm(); + + /** + * Returns SeqNoPropertyTerm for this entity. + * + * @return SeqNoPrimaryTerm, may be {@literal null} + */ + @Nullable SeqNoPrimaryTerm getSeqNoPrimaryTerm(); } /** @@ -333,6 +350,16 @@ public Number getVersion() { return null; } + @Override + public boolean hasSeqNoPrimaryTerm() { + return false; + } + + @Override + public SeqNoPrimaryTerm getSeqNoPrimaryTerm() { + return null; + } + /* * (non-Javadoc) * @see org.springframework.data.elasticsearch.core.EntityOperations.AdaptibleEntity#incrementVersion() @@ -588,6 +615,19 @@ public Number getVersion() { return propertyAccessor.getProperty(versionProperty, Number.class); } + @Override + public boolean hasSeqNoPrimaryTerm() { + return entity.hasSeqNoPrimaryTermProperty(); + } + + @Override + public SeqNoPrimaryTerm getSeqNoPrimaryTerm() { + + ElasticsearchPersistentProperty seqNoPrimaryTermProperty = entity.getRequiredSeqNoPrimaryTermProperty(); + + return propertyAccessor.getProperty(seqNoPrimaryTermProperty, SeqNoPrimaryTerm.class); + } + /* * (non-Javadoc) * @see org.springframework.data.elasticsearch.core.EntityOperations.AdaptibleEntity#initializeVersionProperty() diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java index 8d5e859f7a..c22ac6fd44 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java @@ -17,6 +17,7 @@ import static org.elasticsearch.index.VersionType.*; +import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.function.Tuple2; @@ -347,7 +348,23 @@ private IndexRequest getIndexRequest(Object value, AdaptibleEntity entity, In request.source(converter.mapObject(value).toJson(), Requests.INDEX_CONTENT_TYPE); - if (entity.isVersionedEntity()) { + boolean usingSeqNo = false; + if (entity.hasSeqNoPrimaryTerm()) { + SeqNoPrimaryTerm seqNoPrimaryTerm = entity.getSeqNoPrimaryTerm(); + + if (seqNoPrimaryTerm != null) { + if (seqNoPrimaryTerm.getSequenceNumber() != null) { + request.setIfSeqNo(seqNoPrimaryTerm.getSequenceNumber()); + usingSeqNo = true; + } + if (seqNoPrimaryTerm.getPrimaryTerm() != null) { + request.setIfPrimaryTerm(seqNoPrimaryTerm.getPrimaryTerm()); + } + } + } + + // seq_no and version are incompatible in the same request + if (!usingSeqNo && entity.isVersionedEntity()) { Number version = entity.getVersion(); @@ -356,6 +373,7 @@ private IndexRequest getIndexRequest(Object value, AdaptibleEntity entity, In request.versionType(EXTERNAL); } } + return request; } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java index 6a75ecb486..1fcb8cd8be 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java @@ -117,4 +117,14 @@ public interface ElasticsearchPersistentEntity extends PersistentEntity Date: Sun, 26 Apr 2020 18:20:32 +0400 Subject: [PATCH 11/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Add refresh() calls --- .../elasticsearch/core/ElasticsearchTemplateTests.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 1c9c552ade..2c62547753 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -3095,6 +3095,7 @@ void multigetShouldReturnSeqNoPrimaryTerm() { OptimisticEntity original = new OptimisticEntity(); original.setMessage("It's fine"); OptimisticEntity saved = operations.save(original); + operations.refresh(OptimisticEntity.class); List retrievedList = operations.multiGet(queryForOne(saved.getId()), OptimisticEntity.class, operations.getIndexCoordinatesFor(OptimisticEntity.class)); @@ -3112,6 +3113,7 @@ void searchShouldReturnSeqNoPrimaryTerm() { OptimisticEntity original = new OptimisticEntity(); original.setMessage("It's fine"); OptimisticEntity saved = operations.save(original); + operations.refresh(OptimisticEntity.class); SearchHits retrievedHits = operations.search(queryForOne(saved.getId()), OptimisticEntity.class); OptimisticEntity retrieved = retrievedHits.getSearchHit(0).getContent(); @@ -3124,8 +3126,10 @@ void multiSearchShouldReturnSeqNoPrimaryTerm() { OptimisticEntity original = new OptimisticEntity(); original.setMessage("It's fine"); OptimisticEntity saved = operations.save(original); + operations.refresh(OptimisticEntity.class); - List> retrievedHits = operations.multiSearch(singletonList(queryForOne(saved.getId())), + List queries = singletonList(queryForOne(saved.getId())); + List> retrievedHits = operations.multiSearch(queries, OptimisticEntity.class, operations.getIndexCoordinatesFor(OptimisticEntity.class)); OptimisticEntity retrieved = retrievedHits.get(0).getSearchHit(0).getContent(); @@ -3137,6 +3141,7 @@ void searchForStreamShouldReturnSeqNoPrimaryTerm() { OptimisticEntity original = new OptimisticEntity(); original.setMessage("It's fine"); OptimisticEntity saved = operations.save(original); + operations.refresh(OptimisticEntity.class); SearchHitsIterator retrievedHits = operations.searchForStream(queryForOne(saved.getId()), OptimisticEntity.class); From 73b2132e404c2832deaae494052e4d246545e08a Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Mon, 27 Apr 2020 16:17:12 +0400 Subject: [PATCH 12/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Never add SeqNoPrimaryTerm property to mapping. Warn if it is marked for inclusion with @Field or a similar annotation. --- .../data/elasticsearch/core/index/MappingBuilder.java | 7 +++++++ .../data/elasticsearch/core/index/MappingBuilderTests.java | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java b/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java index c75e1d3328..9bc50a7c51 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java @@ -167,6 +167,13 @@ private void mapEntity(XContentBuilder builder, @Nullable ElasticsearchPersisten return; } + if (property.isSeqNoPrimaryTermProperty()) { + logger.warn("Property {} of {} is annotated for inclusion in mapping, but its type is " + // + "SeqNoPrimaryTerm that is never mapped, so it is skipped", // + property.getFieldName(), entity.getType()); + return; + } + buildPropertyMapping(builder, isRootObject, property); } catch (IOException e) { logger.warn("error mapping property with name {}", property.getName(), e); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java b/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java index d5d5951776..02f1924f7d 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java @@ -576,7 +576,7 @@ void shouldWriteCompletionContextInfo() throws JSONException { } @Test // DATAES-799 - void shouldNotIncludeSeqNoPrimaryTermPropertyFromMappingWhenNotAnnotatedWithField() { + void shouldNotIncludeSeqNoPrimaryTermPropertyInMappingEvenWhenAnnotatedWithField() { String propertyMapping = getMappingBuilder().buildPropertyMapping(EntityWithSeqNoPrimaryTerm.class); assertThat(propertyMapping).doesNotContain("seqNoPrimaryTerm"); @@ -1067,6 +1067,6 @@ static class CompletionDocument { @Document(indexName = "test-index-entity-with-seq-no-primary-term-mapping-builder") static class EntityWithSeqNoPrimaryTerm { - @Nullable private SeqNoPrimaryTerm seqNoPrimaryTerm; + @Field(type = Object) private SeqNoPrimaryTerm seqNoPrimaryTerm; } } From dbd5e96f044e24833021ae0bb4d4646b552433dd Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Mon, 27 Apr 2020 16:21:16 +0400 Subject: [PATCH 13/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Remove a TODO and leave ExceptionTranslator hard-wired for now --- .../data/elasticsearch/core/ElasticsearchTemplate.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java index bf884de60d..066fc8bcfa 100755 --- a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplate.java @@ -90,7 +90,6 @@ public class ElasticsearchTemplate extends AbstractElasticsearchTemplate { private Client client; @Nullable private String searchTimeout; - // TODO: is it correct to use it here? private final ElasticsearchExceptionTranslator exceptionTranslator = new ElasticsearchExceptionTranslator(); // region Initialization From 58fae7e3f956dc472365b1a11a90dd92f3c2e13e Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Mon, 27 Apr 2020 16:27:18 +0400 Subject: [PATCH 14/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Warn if SeqNoPrimaryTerm property is defined for a versioned entity. --- .../SimpleElasticsearchPersistentEntity.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java index dd7b01a38b..c22a7dfe31 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java @@ -23,6 +23,8 @@ import java.util.concurrent.atomic.AtomicReference; import org.elasticsearch.index.VersionType; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.BeansException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; @@ -58,6 +60,8 @@ public class SimpleElasticsearchPersistentEntity extends BasicPersistentEntity implements ElasticsearchPersistentEntity, ApplicationContextAware { + private static final Logger logger = LoggerFactory.getLogger(SimpleElasticsearchPersistentEntity.class); + private final StandardEvaluationContext context; private final SpelExpressionParser parser; @@ -246,6 +250,16 @@ public void addPersistentProperty(ElasticsearchPersistentProperty property) { } this.seqNoPrimaryTermProperty = property; + + if (hasVersionProperty()) { + logger.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); + } + } + + if (property.isVersionProperty()) { + if (hasSeqNoPrimaryTermProperty()) { + logger.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); + } } } From 672ba9c3abec02e3ad1560351fe644b0de24b6c7 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Mon, 27 Apr 2020 16:30:17 +0400 Subject: [PATCH 15/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Refresh before searching --- .../elasticsearch/core/ReactiveElasticsearchTemplateTests.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java index e55b2ef018..52cfec7453 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java @@ -902,6 +902,7 @@ void searchShouldReturnSeqNoPrimaryTerm() { OptimisticEntity original = new OptimisticEntity(); original.setMessage("It's fine"); OptimisticEntity saved = template.save(original).block(); + restTemplate.refresh(OptimisticEntity.class); template.search(searchQueryForOne(saved.getId()), OptimisticEntity.class, template.getIndexCoordinatesFor(OptimisticEntity.class)) .map(SearchHit::getContent) From bcf64b0c74899695d5ebcd0557a3214370255f4b Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Mon, 27 Apr 2020 16:38:52 +0400 Subject: [PATCH 16/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Add documentation --- .../elasticsearch/core/SequenceNumbers.java | 15 +++++++++ .../core/mapping/SeqNoPrimaryTerm.java | 31 +++++++++++++++++++ ...ElasticsearchExceptionTranslatorTests.java | 15 +++++++++ .../core/SequenceNumbersTests.java | 15 +++++++++ 4 files changed, 76 insertions(+) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java b/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java index ea79624bd8..99f106600a 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java @@ -1,3 +1,18 @@ +/* + * Copyright 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 org.springframework.data.elasticsearch.core; /** diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java index 1d1cb845e4..b544288477 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java @@ -1,8 +1,39 @@ +/* + * Copyright 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 org.springframework.data.elasticsearch.core.mapping; import org.springframework.lang.Nullable; /** + *

A container for seq_no and primary_term values. When an entity class contains a field of this type, + * it will be automatically filled with SeqNoPrimaryTerm instance on read operations (like get or search), + * and also, when the SeqNoPrimaryTerm is not {@literal null} and filled with seq_no and primary_term, + * they will be sent to Elasticsearch when indexing such an entity. + *

+ *

This allows to implement optimistic locking pattern for full-update scenario, when an entity is first + * read from Elasticsearch and then gets reindexed with new _content. + * Index operations will throw an {@link org.springframework.dao.OptimisticLockingFailureException} if the + * seq_no + primary_term pair already has different values for the given document. See Elasticsearch documentation + * for more information: https://www.elastic.co/guide/en/elasticsearch/reference/current/optimistic-concurrency-control.html + *

+ *

+ * A property of this type is implicitly @{@link org.springframework.data.annotation.Transient} and never gets included + * into a mapping at Elasticsearch side. + *

+ * * @author Roman Puchkovskiy * @since 4.0 */ diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java index 273cfbe266..4f2ec13395 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchExceptionTranslatorTests.java @@ -1,3 +1,18 @@ +/* + * Copyright 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 org.springframework.data.elasticsearch.core; import static org.assertj.core.api.Assertions.*; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java b/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java index 2da43c5c3d..17fe36a913 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java @@ -1,3 +1,18 @@ +/* + * Copyright 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 org.springframework.data.elasticsearch.core; import org.junit.jupiter.api.Test; From 5cb14d5e8a64ec04d55a64c69de6a898ca1b7841 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 12:42:49 +0400 Subject: [PATCH 17/25] Make logger field uppercase Co-Authored-By: Peter-Josef Meisch --- .../core/mapping/SimpleElasticsearchPersistentEntity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java index c22a7dfe31..99e5a0e27d 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java @@ -60,7 +60,7 @@ public class SimpleElasticsearchPersistentEntity extends BasicPersistentEntity implements ElasticsearchPersistentEntity, ApplicationContextAware { - private static final Logger logger = LoggerFactory.getLogger(SimpleElasticsearchPersistentEntity.class); + private static final Logger LOGGER = LoggerFactory.getLogger(SimpleElasticsearchPersistentEntity.class); private final StandardEvaluationContext context; private final SpelExpressionParser parser; From 99a68176584ad82f078d456c97508c984259031a Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 12:44:06 +0400 Subject: [PATCH 18/25] Clarify the warning message Co-Authored-By: Peter-Josef Meisch --- .../core/mapping/SimpleElasticsearchPersistentEntity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java index 99e5a0e27d..7a0c10ec45 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java @@ -245,7 +245,7 @@ public void addPersistentProperty(ElasticsearchPersistentProperty property) { if (seqNoPrimaryTermProperty != null) { throw new MappingException(String.format( "Attempt to add SeqNoPrimaryTerm property %s but already have property %s registered " - + "as SeqNoPrimaryTerm property. Check your mapping configuration!", + + "as SeqNoPrimaryTerm property. Check your entity configuration!", property.getField(), seqNoPrimaryTermProperty.getField())); } From 3df15290dfd218b8c0e05134ad15eaa3508fa4f4 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 12:49:51 +0400 Subject: [PATCH 19/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Move SeqNoPrimaryTerm to core.query package --- .../core/AbstractElasticsearchTemplate.java | 2 +- .../data/elasticsearch/core/EntityOperations.java | 2 +- .../core/ReactiveElasticsearchTemplate.java | 2 +- .../convert/MappingElasticsearchConverter.java | 2 +- .../SimpleElasticsearchPersistentEntity.java | 4 ++-- .../SimpleElasticsearchPersistentProperty.java | 1 + .../core/query/IndexQueryBuilder.java | 1 - .../core/{mapping => query}/SeqNoPrimaryTerm.java | 2 +- .../core/ElasticsearchTemplateTests.java | 1 - .../core/ReactiveElasticsearchTemplateTests.java | 15 +++------------ .../elasticsearch/core/RequestFactoryTests.java | 2 +- .../MappingElasticsearchConverterUnitTests.java | 2 +- .../core/index/MappingBuilderTests.java | 3 ++- .../SimpleElasticsearchPersistentEntityTests.java | 1 + ...eElasticsearchPersistentPropertyUnitTests.java | 2 +- 15 files changed, 17 insertions(+), 25 deletions(-) rename src/main/java/org/springframework/data/elasticsearch/core/{mapping => query}/SeqNoPrimaryTerm.java (97%) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java index cebe310b84..16d85ad749 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java @@ -46,7 +46,6 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; import org.springframework.data.elasticsearch.core.query.GetQuery; import org.springframework.data.elasticsearch.core.query.IndexQuery; @@ -54,6 +53,7 @@ import org.springframework.data.elasticsearch.core.query.MoreLikeThisQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; import org.springframework.data.elasticsearch.core.query.Query; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.support.VersionInfo; import org.springframework.data.mapping.callback.EntityCallbacks; import org.springframework.data.util.CloseableIterator; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java b/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java index ceb1b1da4a..65467ec0d1 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java @@ -21,7 +21,7 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.IdentifierAccessor; import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.context.MappingContext; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java index c22ac6fd44..28915b8ac9 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java @@ -17,7 +17,6 @@ import static org.elasticsearch.index.VersionType.*; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.function.Tuple2; @@ -85,6 +84,7 @@ import org.springframework.data.elasticsearch.core.query.IndexQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQuery; import org.springframework.data.elasticsearch.core.query.Query; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.StringQuery; import org.springframework.data.elasticsearch.core.query.UpdateQuery; import org.springframework.data.elasticsearch.support.VersionInfo; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index 89995000c6..dd382a9ac4 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java @@ -34,8 +34,8 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentPropertyConverter; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.context.MappingContext; import org.springframework.data.mapping.model.ConvertingPropertyAccessor; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java index 7a0c10ec45..c403c34926 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java @@ -252,13 +252,13 @@ public void addPersistentProperty(ElasticsearchPersistentProperty property) { this.seqNoPrimaryTermProperty = property; if (hasVersionProperty()) { - logger.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); + LOGGER.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); } } if (property.isVersionProperty()) { if (hasSeqNoPrimaryTermProperty()) { - logger.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); + LOGGER.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); } } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java index 9bbb7d038b..beab6b433e 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentProperty.java @@ -26,6 +26,7 @@ import org.springframework.data.elasticsearch.annotations.Parent; import org.springframework.data.elasticsearch.annotations.Score; import org.springframework.data.elasticsearch.core.convert.ElasticsearchDateConverter; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.Association; import org.springframework.data.mapping.MappingException; import org.springframework.data.mapping.PersistentEntity; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java b/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java index e8cf5cc8ae..588f557d50 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/IndexQueryBuilder.java @@ -15,7 +15,6 @@ */ package org.springframework.data.elasticsearch.core.query; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.lang.Nullable; /** diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java b/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java similarity index 97% rename from src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java rename to src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java index b544288477..51cc9af784 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SeqNoPrimaryTerm.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.springframework.data.elasticsearch.core.mapping; +package org.springframework.data.elasticsearch.core.query; import org.springframework.lang.Nullable; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 2c62547753..e5eed540e2 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -76,7 +76,6 @@ import org.springframework.data.elasticsearch.annotations.ScriptedField; import org.springframework.data.elasticsearch.core.geo.GeoPoint; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.*; import org.springframework.data.util.CloseableIterator; import org.springframework.lang.Nullable; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java index 52cfec7453..e78e93853d 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplateTests.java @@ -25,10 +25,6 @@ import lombok.Data; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; -import org.elasticsearch.index.query.IdsQueryBuilder; -import org.springframework.dao.OptimisticLockingFailureException; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; -import org.springframework.data.elasticsearch.core.query.Query; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -44,6 +40,7 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; +import org.elasticsearch.index.query.IdsQueryBuilder; import org.elasticsearch.search.aggregations.AggregationBuilders; import org.elasticsearch.search.aggregations.bucket.terms.ParsedStringTerms; import org.elasticsearch.search.sort.FieldSortBuilder; @@ -52,6 +49,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.dao.DataAccessResourceFailureException; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.annotation.Id; import org.springframework.data.annotation.Version; import org.springframework.data.domain.PageRequest; @@ -64,14 +62,7 @@ import org.springframework.data.elasticsearch.annotations.Score; import org.springframework.data.elasticsearch.client.reactive.ReactiveElasticsearchClient; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; -import org.springframework.data.elasticsearch.core.query.Criteria; -import org.springframework.data.elasticsearch.core.query.CriteriaQuery; -import org.springframework.data.elasticsearch.core.query.IndexQuery; -import org.springframework.data.elasticsearch.core.query.IndexQueryBuilder; -import org.springframework.data.elasticsearch.core.query.NativeSearchQuery; -import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; -import org.springframework.data.elasticsearch.core.query.StringQuery; -import org.springframework.data.elasticsearch.core.query.UpdateQuery; +import org.springframework.data.elasticsearch.core.query.*; import org.springframework.data.elasticsearch.junit.junit4.ElasticsearchVersion; import org.springframework.data.elasticsearch.junit.jupiter.SpringIntegrationTest; import org.springframework.util.StringUtils; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java index 019f15e29d..ec4f319e69 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/RequestFactoryTests.java @@ -44,7 +44,6 @@ import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter; import org.springframework.data.elasticsearch.core.geo.GeoPoint; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; import org.springframework.data.elasticsearch.core.query.Criteria; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; @@ -52,6 +51,7 @@ import org.springframework.data.elasticsearch.core.query.IndexQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; import org.springframework.data.elasticsearch.core.query.Query; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.lang.Nullable; /** diff --git a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java index 87bbbd0685..45aac4f9eb 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java @@ -55,8 +55,8 @@ import org.springframework.data.elasticsearch.annotations.GeoPointField; import org.springframework.data.elasticsearch.core.document.Document; import org.springframework.data.elasticsearch.core.geo.GeoPoint; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.geo.Box; import org.springframework.data.geo.Circle; import org.springframework.data.geo.Point; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java b/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java index 02f1924f7d..5045aa726b 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/index/MappingBuilderTests.java @@ -20,6 +20,7 @@ import static org.elasticsearch.index.query.QueryBuilders.*; import static org.skyscreamer.jsonassert.JSONAssert.*; import static org.springframework.data.elasticsearch.annotations.FieldType.*; +import static org.springframework.data.elasticsearch.annotations.FieldType.Object; import static org.springframework.data.elasticsearch.utils.IndexBuilder.*; import lombok.AllArgsConstructor; @@ -58,10 +59,10 @@ import org.springframework.data.elasticsearch.core.completion.Completion; import org.springframework.data.elasticsearch.core.geo.GeoPoint; import org.springframework.data.elasticsearch.core.mapping.IndexCoordinates; -import org.springframework.data.elasticsearch.core.mapping.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.query.IndexQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQuery; import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.junit.jupiter.ElasticsearchTemplateConfiguration; import org.springframework.data.elasticsearch.junit.jupiter.SpringIntegrationTest; import org.springframework.data.geo.Box; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java index 1058935bc7..53212862af 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java @@ -22,6 +22,7 @@ import org.springframework.data.annotation.Version; import org.springframework.data.elasticsearch.annotations.Field; import org.springframework.data.elasticsearch.annotations.Score; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.MappingException; import org.springframework.data.mapping.model.Property; import org.springframework.data.mapping.model.SimpleTypeHolder; diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java index 9665aab155..182fb75cd7 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentPropertyUnitTests.java @@ -23,13 +23,13 @@ import java.time.ZonedDateTime; import java.util.Date; import java.util.GregorianCalendar; -import java.util.TimeZone; import org.junit.jupiter.api.Test; import org.springframework.data.elasticsearch.annotations.DateFormat; import org.springframework.data.elasticsearch.annotations.Field; import org.springframework.data.elasticsearch.annotations.FieldType; import org.springframework.data.elasticsearch.annotations.Score; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.MappingException; import org.springframework.lang.Nullable; From 5e030ea94309d2bbc71f1ed459555c3e4a330c82 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 15:04:50 +0400 Subject: [PATCH 20/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Make SeqNoPrimaryTerm an immutable value object. Only interpret seq_no and primary_term validity at the moment when SeqNoPrimaryTerm instance is constructed. Remove SequenceNumbers. --- .../core/ReactiveElasticsearchTemplate.java | 10 +-- .../elasticsearch/core/SequenceNumbers.java | 32 ------- .../MappingElasticsearchConverter.java | 20 ++--- .../core/document/DocumentAdapters.java | 5 +- .../core/document/MapDocument.java | 5 +- .../ElasticsearchPersistentEntity.java | 1 + .../core/query/SeqNoPrimaryTerm.java | 87 +++++++++++++++---- .../core/SequenceNumbersTests.java | 39 --------- ...appingElasticsearchConverterUnitTests.java | 2 +- ...pleElasticsearchPersistentEntityTests.java | 2 +- .../core/query/SeqNoPrimaryTermTests.java | 58 +++++++++++++ 11 files changed, 141 insertions(+), 120 deletions(-) delete mode 100644 src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java delete mode 100644 src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java create mode 100644 src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java index 28915b8ac9..cdbfb1af35 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ReactiveElasticsearchTemplate.java @@ -353,13 +353,9 @@ private IndexRequest getIndexRequest(Object value, AdaptibleEntity entity, In SeqNoPrimaryTerm seqNoPrimaryTerm = entity.getSeqNoPrimaryTerm(); if (seqNoPrimaryTerm != null) { - if (seqNoPrimaryTerm.getSequenceNumber() != null) { - request.setIfSeqNo(seqNoPrimaryTerm.getSequenceNumber()); - usingSeqNo = true; - } - if (seqNoPrimaryTerm.getPrimaryTerm() != null) { - request.setIfPrimaryTerm(seqNoPrimaryTerm.getPrimaryTerm()); - } + request.setIfSeqNo(seqNoPrimaryTerm.getSequenceNumber()); + request.setIfPrimaryTerm(seqNoPrimaryTerm.getPrimaryTerm()); + usingSeqNo = true; } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java b/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java deleted file mode 100644 index 99f106600a..0000000000 --- a/src/main/java/org/springframework/data/elasticsearch/core/SequenceNumbers.java +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright 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 org.springframework.data.elasticsearch.core; - -/** - * @author Roman Puchkovskiy - * @since 4.0 - */ -public class SequenceNumbers { - public static boolean isAssignedSeqNo(long seqNo) { - return seqNo >= 0; - } - - public static boolean isAssignedPrimaryTerm(long primaryTerm) { - return primaryTerm > 0; - } - - private SequenceNumbers() {} -} diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index dd382a9ac4..dced14a456 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java @@ -198,10 +198,11 @@ protected R readEntity(ElasticsearchPersistentEntity entity, Map { + ElasticsearchPersistentProperty property = targetEntity.getRequiredSeqNoPrimaryTermProperty(); + targetEntity.getPropertyAccessor(result).setProperty(property, seqNoPrimaryTerm); + }); } } @@ -218,17 +219,6 @@ protected R readEntity(ElasticsearchPersistentEntity entity, Map R readProperties(ElasticsearchPersistentEntity entity, R instance, ElasticsearchPropertyValueProvider valueProvider) { diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java b/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java index 86d74bee2d..0dbc7aed98 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/DocumentAdapters.java @@ -38,7 +38,6 @@ import org.elasticsearch.index.get.GetResult; import org.elasticsearch.search.SearchHit; import org.springframework.data.elasticsearch.ElasticsearchException; -import org.springframework.data.elasticsearch.core.SequenceNumbers; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -261,7 +260,7 @@ public long getVersion() { */ @Override public boolean hasSeqNo() { - return SequenceNumbers.isAssignedSeqNo(this.seqNo); + return true; } /* @@ -284,7 +283,7 @@ public long getSeqNo() { */ @Override public boolean hasPrimaryTerm() { - return SequenceNumbers.isAssignedPrimaryTerm(this.primaryTerm); + return true; } /* diff --git a/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java b/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java index 62818656d8..fb80c35e12 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/document/MapDocument.java @@ -22,7 +22,6 @@ import java.util.function.BiConsumer; import org.springframework.data.elasticsearch.ElasticsearchException; -import org.springframework.data.elasticsearch.core.SequenceNumbers; import org.springframework.lang.Nullable; import com.fasterxml.jackson.core.JsonProcessingException; @@ -124,7 +123,7 @@ public void setVersion(long version) { */ @Override public boolean hasSeqNo() { - return this.seqNo != null && SequenceNumbers.isAssignedSeqNo(this.seqNo); + return this.seqNo != null; } /* @@ -155,7 +154,7 @@ public void setSeqNo(long seqNo) { */ @Override public boolean hasPrimaryTerm() { - return this.primaryTerm != null && SequenceNumbers.isAssignedPrimaryTerm(this.primaryTerm); + return this.primaryTerm != null; } /* diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java index 1fcb8cd8be..3cdeacec60 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java @@ -17,6 +17,7 @@ import org.elasticsearch.index.VersionType; import org.springframework.data.elasticsearch.annotations.Field; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.PersistentEntity; import org.springframework.lang.Nullable; diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java b/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java index 51cc9af784..d0f78d84d6 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java @@ -15,7 +15,8 @@ */ package org.springframework.data.elasticsearch.core.query; -import org.springframework.lang.Nullable; +import java.util.Objects; +import java.util.Optional; /** *

A container for seq_no and primary_term values. When an entity class contains a field of this type, @@ -29,42 +30,72 @@ * seq_no + primary_term pair already has different values for the given document. See Elasticsearch documentation * for more information: https://www.elastic.co/guide/en/elasticsearch/reference/current/optimistic-concurrency-control.html *

- *

- * A property of this type is implicitly @{@link org.springframework.data.annotation.Transient} and never gets included + *

A property of this type is implicitly @{@link org.springframework.data.annotation.Transient} and never gets included * into a mapping at Elasticsearch side. *

+ *

Please note that an instance constructed via {@link SeqNoPrimaryTerm#of(long, long)} may contain unassigned values + * for seq_no and primary_term. Such values will be accepted by Elasticsearch as if they were not provided at all. + * {@link SeqNoPrimaryTerm#ofAssigned(long, long)} may be used to get either an instance with assigned seq_no and + * primary_term values, or no instance at all. + *

* * @author Roman Puchkovskiy * @since 4.0 */ -public class SeqNoPrimaryTerm { - @Nullable private Long sequenceNumber; - @Nullable private Long primaryTerm; +public final class SeqNoPrimaryTerm { + private final long sequenceNumber; + private final long primaryTerm; - public SeqNoPrimaryTerm() { + /** + * Returns an instance of SeqNoPrimaryTerm with the given seq_no and primary_term. No validation is made of whether + * the passed values are valid (and assigned) seq_no and primary_term. If you need such a validation to be + * performed, please use {@link #ofAssigned(long, long)}, + * + * @param sequenceNumber seq_no + * @param primaryTerm primary_term + * @return SeqNoPrimaryTerm instance with the given seq_no and primary_term + * @see #ofAssigned(long, long) + */ + public static SeqNoPrimaryTerm of(long sequenceNumber, long primaryTerm) { + return new SeqNoPrimaryTerm(sequenceNumber, primaryTerm); } - public SeqNoPrimaryTerm(@Nullable Long sequenceNumber, @Nullable Long primaryTerm) { - this.sequenceNumber = sequenceNumber; - this.primaryTerm = primaryTerm; + /** + * Returns either an instance with valid (and assigned) values of seq_no and primary_term, or nothing. + * seq_no is valid and assigned when it is non-negative. primary_term is valid and assigned when it is positive. + * + * @param sequenceNumber seq_no + * @param primaryTerm primary_term + * @return either an instance with valid (and assigned) values of seq_no and primary_term, or nothing + */ + public static Optional ofAssigned(long sequenceNumber, long primaryTerm) { + + if (isAssignedSeqNo(sequenceNumber) && isAssignedPrimaryTerm(primaryTerm)) { + return Optional.of(SeqNoPrimaryTerm.of(sequenceNumber, primaryTerm)); + } + + return Optional.empty(); } - @Nullable - public Long getSequenceNumber() { - return sequenceNumber; + private static boolean isAssignedSeqNo(long seqNo) { + return seqNo >= 0; + } + + private static boolean isAssignedPrimaryTerm(long primaryTerm) { + return primaryTerm > 0; } - public void setSequenceNumber(@Nullable Long sequenceNumber) { + private SeqNoPrimaryTerm(long sequenceNumber, long primaryTerm) { this.sequenceNumber = sequenceNumber; + this.primaryTerm = primaryTerm; } - @Nullable - public Long getPrimaryTerm() { - return primaryTerm; + public long getSequenceNumber() { + return sequenceNumber; } - public void setPrimaryTerm(@Nullable Long primaryTerm) { - this.primaryTerm = primaryTerm; + public long getPrimaryTerm() { + return primaryTerm; } @Override @@ -74,4 +105,22 @@ public String toString() { ", primaryTerm=" + primaryTerm + '}'; } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SeqNoPrimaryTerm that = (SeqNoPrimaryTerm) o; + return sequenceNumber == that.sequenceNumber && + primaryTerm == that.primaryTerm; + } + + @Override + public int hashCode() { + return Objects.hash(sequenceNumber, primaryTerm); + } } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java b/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java deleted file mode 100644 index 17fe36a913..0000000000 --- a/src/test/java/org/springframework/data/elasticsearch/core/SequenceNumbersTests.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Copyright 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 org.springframework.data.elasticsearch.core; - -import org.junit.jupiter.api.Test; - -import static org.junit.jupiter.api.Assertions.*; - -/** - * @author Roman Puchkovskiy - */ -class SequenceNumbersTests { - @Test // DATAES-799 - void isAssignedSeqNoShouldReturnTrueIffSeqNoIsNonNegative() { - assertFalse(SequenceNumbers.isAssignedSeqNo(org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_SEQ_NO)); - assertFalse(SequenceNumbers.isAssignedSeqNo(org.elasticsearch.index.seqno.SequenceNumbers.NO_OPS_PERFORMED)); - assertTrue(SequenceNumbers.isAssignedSeqNo(0)); - assertTrue(SequenceNumbers.isAssignedSeqNo(1)); - } - - @Test // DATAES-799 - void isAssignedPrimaryTermShouldReturnTrueIffPrimaryTermIsPositive() { - assertFalse(SequenceNumbers.isAssignedPrimaryTerm(org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_PRIMARY_TERM)); - assertTrue(SequenceNumbers.isAssignedPrimaryTerm(1)); - } -} \ No newline at end of file diff --git a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java index 45aac4f9eb..ab3bcf5c5d 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java @@ -701,7 +701,7 @@ void readGenericListWithMaps() { @Test // DATAES-799 void shouldNotWriteSeqNoPrimaryTermProperty() { EntityWithSeqNoPrimaryTerm entity = new EntityWithSeqNoPrimaryTerm(); - entity.seqNoPrimaryTerm = new SeqNoPrimaryTerm(1L, 2L); + entity.seqNoPrimaryTerm = SeqNoPrimaryTerm.of(1L, 2L); Document document = Document.create(); mappingElasticsearchConverter.write(entity, document); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java index 53212862af..a4bc77d179 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java @@ -124,7 +124,7 @@ void shouldReturnSeqNoPrimaryTermPropertyWhenThereIsSuchProperty() { typeInformation); entity.addPersistentProperty(createProperty(entity, "seqNoPrimaryTerm")); EntityWithSeqNoPrimaryTerm instance = new EntityWithSeqNoPrimaryTerm(); - SeqNoPrimaryTerm seqNoPrimaryTerm = new SeqNoPrimaryTerm(); + SeqNoPrimaryTerm seqNoPrimaryTerm = SeqNoPrimaryTerm.of(1, 2); ElasticsearchPersistentProperty property = entity.getSeqNoPrimaryTermProperty(); entity.getPropertyAccessor(instance).setProperty(property, seqNoPrimaryTerm); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java b/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java new file mode 100644 index 0000000000..7607042634 --- /dev/null +++ b/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java @@ -0,0 +1,58 @@ +/* + * Copyright 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 org.springframework.data.elasticsearch.core.query; + +import static org.assertj.core.api.Assertions.*; + +import java.util.Optional; + +import org.elasticsearch.index.seqno.SequenceNumbers; +import org.junit.jupiter.api.Test; + +/** + * @author Roman Puchkovskiy + */ +class SeqNoPrimaryTermTests { + @Test + void ofAssignedShouldReturnSomeSeqNoPrimaryTermWhenBothSeqNoAndPrimaryTermAreAssigned() { + Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned(1, 2); + + assertThat(maybeSeqNoPrimaryTerm).contains(SeqNoPrimaryTerm.of(1, 2)); + } + + @Test + void ofAssignedShouldReturnEmptyWhenSeqNoIsUnassigned() { + Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned(SequenceNumbers.UNASSIGNED_SEQ_NO, 2); + + assertThat(maybeSeqNoPrimaryTerm).isEmpty(); + } + + @Test + void ofAssignedShouldReturnEmptyWhenSeqNoIsNoOpsPerformed() { + Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned( + SequenceNumbers.NO_OPS_PERFORMED, 2); + + assertThat(maybeSeqNoPrimaryTerm).isEmpty(); + } + + @Test + void ofAssignedShouldReturnEmptyWhenPrimaryTermIsUnassigned() { + Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned(1, + SequenceNumbers.UNASSIGNED_PRIMARY_TERM); + + assertThat(maybeSeqNoPrimaryTerm).isEmpty(); + } +} \ No newline at end of file From bddf1d376879505fcd72164ea6d8055ccf06a934 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 15:07:23 +0400 Subject: [PATCH 21/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Add javadoc. --- .../core/mapping/ElasticsearchPersistentEntity.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java index 3cdeacec60..9e2c0d9597 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java @@ -118,7 +118,14 @@ public interface ElasticsearchPersistentEntity extends PersistentEntity Date: Tue, 28 Apr 2020 15:10:28 +0400 Subject: [PATCH 22/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Reduce duplication. --- .../mapping/SimpleElasticsearchPersistentEntity.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java index c403c34926..348ac676db 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java @@ -252,17 +252,23 @@ public void addPersistentProperty(ElasticsearchPersistentProperty property) { this.seqNoPrimaryTermProperty = property; if (hasVersionProperty()) { - LOGGER.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); + warnAboutBothSeqNoPrimaryTermAndVersionProperties(); } } if (property.isVersionProperty()) { if (hasSeqNoPrimaryTermProperty()) { - LOGGER.warn("Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", getType()); + warnAboutBothSeqNoPrimaryTermAndVersionProperties(); } } } + private void warnAboutBothSeqNoPrimaryTermAndVersionProperties() { + LOGGER.warn( + "Both SeqNoPrimaryTerm and @Version properties are defined on {}. Version will not be sent in index requests when seq_no is sent!", + getType()); + } + /* * (non-Javadoc) * @see org.springframework.data.mapping.model.BasicPersistentEntity#setPersistentPropertyAccessorFactory(org.springframework.data.mapping.model.PersistentPropertyAccessorFactory) From 4b1d77939212a18fbd02d37230d85e32dc492338 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 15:15:49 +0400 Subject: [PATCH 23/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Only log a warning if a SeqNoPrimaryTerm property is annotated with @Field. --- .../data/elasticsearch/core/index/MappingBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java b/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java index 9bc50a7c51..0dc89765f3 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java @@ -167,7 +167,7 @@ private void mapEntity(XContentBuilder builder, @Nullable ElasticsearchPersisten return; } - if (property.isSeqNoPrimaryTermProperty()) { + if (property.isSeqNoPrimaryTermProperty() && property.isAnnotationPresent(Field.class)) { logger.warn("Property {} of {} is annotated for inclusion in mapping, but its type is " + // "SeqNoPrimaryTerm that is never mapped, so it is skipped", // property.getFieldName(), entity.getType()); From d49e307da72d19b798ef97f71ce00b53665d2e7e Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Tue, 28 Apr 2020 23:56:09 +0400 Subject: [PATCH 24/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Still skip a property if it is not annotated with @Field --- .../data/elasticsearch/core/index/MappingBuilder.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java b/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java index 0dc89765f3..2eea93ca91 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/index/MappingBuilder.java @@ -167,10 +167,12 @@ private void mapEntity(XContentBuilder builder, @Nullable ElasticsearchPersisten return; } - if (property.isSeqNoPrimaryTermProperty() && property.isAnnotationPresent(Field.class)) { - logger.warn("Property {} of {} is annotated for inclusion in mapping, but its type is " + // - "SeqNoPrimaryTerm that is never mapped, so it is skipped", // - property.getFieldName(), entity.getType()); + if (property.isSeqNoPrimaryTermProperty()) { + if (property.isAnnotationPresent(Field.class)) { + logger.warn("Property {} of {} is annotated for inclusion in mapping, but its type is " + // + "SeqNoPrimaryTerm that is never mapped, so it is skipped", // + property.getFieldName(), entity.getType()); + } return; } From 2fbca4f95ed7d224ea2e3f6e94f69e2aafa5cb58 Mon Sep 17 00:00:00 2001 From: Roman Puchkovskiy Date: Wed, 29 Apr 2020 13:34:00 +0400 Subject: [PATCH 25/25] DATAES-799 - Support optimistic locking for full update scenario using seq_no + primary_term. Remove static factories from SeqNoPrimaryTerm, make its constructor public, add assertions to the constructor --- .../MappingElasticsearchConverter.java | 13 ++++- .../core/query/SeqNoPrimaryTerm.java | 53 +++++-------------- ...appingElasticsearchConverterUnitTests.java | 2 +- ...pleElasticsearchPersistentEntityTests.java | 2 +- .../core/query/SeqNoPrimaryTermTests.java | 32 +++++------ 5 files changed, 39 insertions(+), 63 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index dced14a456..9e196a2734 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java @@ -199,10 +199,11 @@ protected R readEntity(ElasticsearchPersistentEntity entity, Map { + if (isAssignedSeqNo(document.getSeqNo()) && isAssignedPrimaryTerm(document.getPrimaryTerm())) { + SeqNoPrimaryTerm seqNoPrimaryTerm = new SeqNoPrimaryTerm(document.getSeqNo(), document.getPrimaryTerm()); ElasticsearchPersistentProperty property = targetEntity.getRequiredSeqNoPrimaryTermProperty(); targetEntity.getPropertyAccessor(result).setProperty(property, seqNoPrimaryTerm); - }); + } } } @@ -219,6 +220,14 @@ protected R readEntity(ElasticsearchPersistentEntity entity, Map= 0; + } + + private boolean isAssignedPrimaryTerm(long primaryTerm) { + return primaryTerm > 0; + } + protected R readProperties(ElasticsearchPersistentEntity entity, R instance, ElasticsearchPropertyValueProvider valueProvider) { diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java b/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java index d0f78d84d6..c7bf434d03 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTerm.java @@ -16,7 +16,6 @@ package org.springframework.data.elasticsearch.core.query; import java.util.Objects; -import java.util.Optional; /** *

A container for seq_no and primary_term values. When an entity class contains a field of this type, @@ -33,10 +32,7 @@ *

A property of this type is implicitly @{@link org.springframework.data.annotation.Transient} and never gets included * into a mapping at Elasticsearch side. *

- *

Please note that an instance constructed via {@link SeqNoPrimaryTerm#of(long, long)} may contain unassigned values - * for seq_no and primary_term. Such values will be accepted by Elasticsearch as if they were not provided at all. - * {@link SeqNoPrimaryTerm#ofAssigned(long, long)} may be used to get either an instance with assigned seq_no and - * primary_term values, or no instance at all. + *

A SeqNoPrimaryTerm instance cannot contain an invalid or unassigned seq_no or primary_term. *

* * @author Roman Puchkovskiy @@ -47,45 +43,22 @@ public final class SeqNoPrimaryTerm { private final long primaryTerm; /** - * Returns an instance of SeqNoPrimaryTerm with the given seq_no and primary_term. No validation is made of whether - * the passed values are valid (and assigned) seq_no and primary_term. If you need such a validation to be - * performed, please use {@link #ofAssigned(long, long)}, + * Creates an instance of SeqNoPrimaryTerm with the given seq_no and primary_term. The passed values are validated: + * sequenceNumber must be non-negative, primaryTerm must be positive. If validation fails, + * an IllegalArgumentException is thrown. * - * @param sequenceNumber seq_no - * @param primaryTerm primary_term - * @return SeqNoPrimaryTerm instance with the given seq_no and primary_term - * @see #ofAssigned(long, long) + * @param sequenceNumber seq_no, must not be negative + * @param primaryTerm primary_term, must be positive + * @throws IllegalArgumentException if seq_no or primary_term is not valid */ - public static SeqNoPrimaryTerm of(long sequenceNumber, long primaryTerm) { - return new SeqNoPrimaryTerm(sequenceNumber, primaryTerm); - } - - /** - * Returns either an instance with valid (and assigned) values of seq_no and primary_term, or nothing. - * seq_no is valid and assigned when it is non-negative. primary_term is valid and assigned when it is positive. - * - * @param sequenceNumber seq_no - * @param primaryTerm primary_term - * @return either an instance with valid (and assigned) values of seq_no and primary_term, or nothing - */ - public static Optional ofAssigned(long sequenceNumber, long primaryTerm) { - - if (isAssignedSeqNo(sequenceNumber) && isAssignedPrimaryTerm(primaryTerm)) { - return Optional.of(SeqNoPrimaryTerm.of(sequenceNumber, primaryTerm)); + public SeqNoPrimaryTerm(long sequenceNumber, long primaryTerm) { + if (sequenceNumber < 0) { + throw new IllegalArgumentException("seq_no should not be negative, but it's " + sequenceNumber); + } + if (primaryTerm <= 0) { + throw new IllegalArgumentException("primary_term should be positive, but it's " + primaryTerm); } - return Optional.empty(); - } - - private static boolean isAssignedSeqNo(long seqNo) { - return seqNo >= 0; - } - - private static boolean isAssignedPrimaryTerm(long primaryTerm) { - return primaryTerm > 0; - } - - private SeqNoPrimaryTerm(long sequenceNumber, long primaryTerm) { this.sequenceNumber = sequenceNumber; this.primaryTerm = primaryTerm; } diff --git a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java index ab3bcf5c5d..45aac4f9eb 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java @@ -701,7 +701,7 @@ void readGenericListWithMaps() { @Test // DATAES-799 void shouldNotWriteSeqNoPrimaryTermProperty() { EntityWithSeqNoPrimaryTerm entity = new EntityWithSeqNoPrimaryTerm(); - entity.seqNoPrimaryTerm = SeqNoPrimaryTerm.of(1L, 2L); + entity.seqNoPrimaryTerm = new SeqNoPrimaryTerm(1L, 2L); Document document = Document.create(); mappingElasticsearchConverter.write(entity, document); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java index a4bc77d179..ddb5ea7c81 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntityTests.java @@ -124,7 +124,7 @@ void shouldReturnSeqNoPrimaryTermPropertyWhenThereIsSuchProperty() { typeInformation); entity.addPersistentProperty(createProperty(entity, "seqNoPrimaryTerm")); EntityWithSeqNoPrimaryTerm instance = new EntityWithSeqNoPrimaryTerm(); - SeqNoPrimaryTerm seqNoPrimaryTerm = SeqNoPrimaryTerm.of(1, 2); + SeqNoPrimaryTerm seqNoPrimaryTerm = new SeqNoPrimaryTerm(1, 2); ElasticsearchPersistentProperty property = entity.getSeqNoPrimaryTermProperty(); entity.getPropertyAccessor(instance).setProperty(property, seqNoPrimaryTerm); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java b/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java index 7607042634..6f31ba6a4f 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/query/SeqNoPrimaryTermTests.java @@ -17,8 +17,6 @@ import static org.assertj.core.api.Assertions.*; -import java.util.Optional; - import org.elasticsearch.index.seqno.SequenceNumbers; import org.junit.jupiter.api.Test; @@ -27,32 +25,28 @@ */ class SeqNoPrimaryTermTests { @Test - void ofAssignedShouldReturnSomeSeqNoPrimaryTermWhenBothSeqNoAndPrimaryTermAreAssigned() { - Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned(1, 2); + void shouldConstructInstanceWithAssignedSeqNoAndPrimaryTerm() { + SeqNoPrimaryTerm instance = new SeqNoPrimaryTerm(1, 2); - assertThat(maybeSeqNoPrimaryTerm).contains(SeqNoPrimaryTerm.of(1, 2)); + assertThat(instance.getSequenceNumber()).isEqualTo(1); + assertThat(instance.getPrimaryTerm()).isEqualTo(2); } @Test - void ofAssignedShouldReturnEmptyWhenSeqNoIsUnassigned() { - Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned(SequenceNumbers.UNASSIGNED_SEQ_NO, 2); - - assertThat(maybeSeqNoPrimaryTerm).isEmpty(); + void shouldThrowAnExceptionWhenTryingToConstructWithUnassignedSeqNo() { + assertThatThrownBy(() -> new SeqNoPrimaryTerm(SequenceNumbers.UNASSIGNED_SEQ_NO, 2)) + .isInstanceOf(IllegalArgumentException.class); } @Test - void ofAssignedShouldReturnEmptyWhenSeqNoIsNoOpsPerformed() { - Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned( - SequenceNumbers.NO_OPS_PERFORMED, 2); - - assertThat(maybeSeqNoPrimaryTerm).isEmpty(); + void shouldThrowAnExceptionWhenTryingToConstructWithSeqNoForNoOpsPerformed() { + assertThatThrownBy(() -> new SeqNoPrimaryTerm(SequenceNumbers.NO_OPS_PERFORMED, 2)) + .isInstanceOf(IllegalArgumentException.class); } @Test - void ofAssignedShouldReturnEmptyWhenPrimaryTermIsUnassigned() { - Optional maybeSeqNoPrimaryTerm = SeqNoPrimaryTerm.ofAssigned(1, - SequenceNumbers.UNASSIGNED_PRIMARY_TERM); - - assertThat(maybeSeqNoPrimaryTerm).isEmpty(); + void shouldThrowAnExceptionWhenTryingToConstructWithUnassignedPrimaryTerm() { + assertThatThrownBy(() -> new SeqNoPrimaryTerm(1, SequenceNumbers.UNASSIGNED_PRIMARY_TERM)) + .isInstanceOf(IllegalArgumentException.class); } } \ No newline at end of file