From de13e06c490c9951aab312df1c6576baccb9eed6 Mon Sep 17 00:00:00 2001 From: Peter-Josef Meisch Date: Wed, 8 Nov 2023 07:38:53 +0100 Subject: [PATCH] Fix handling of @ID property in Java records. Closes #2756 --- .../client/elc/ElasticsearchTemplate.java | 12 +- .../elc/ReactiveElasticsearchTemplate.java | 17 ++- .../core/AbstractElasticsearchTemplate.java | 60 ++------ ...AbstractReactiveElasticsearchTemplate.java | 69 ++------- .../elasticsearch/core/EntityOperations.java | 64 ++++++++ .../core/EntityOperationsUnitTests.java | 143 +++++++++++++++++- 6 files changed, 252 insertions(+), 113 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchTemplate.java index 5f20e37f0b..89f346e7c5 100644 --- a/src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchTemplate.java @@ -224,8 +224,16 @@ public String doIndex(IndexQuery query, IndexCoordinates indexCoordinates) { Object queryObject = query.getObject(); if (queryObject != null) { - query.setObject(updateIndexedObject(queryObject, new IndexedObjectInformation(indexResponse.id(), - indexResponse.index(), indexResponse.seqNo(), indexResponse.primaryTerm(), indexResponse.version()))); + query.setObject(entityOperations.updateIndexedObject( + queryObject, + new IndexedObjectInformation( + indexResponse.id(), + indexResponse.index(), + indexResponse.seqNo(), + indexResponse.primaryTerm(), + indexResponse.version()), + elasticsearchConverter, + routingResolver)); } return indexResponse.id(); diff --git a/src/main/java/org/springframework/data/elasticsearch/client/elc/ReactiveElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/client/elc/ReactiveElasticsearchTemplate.java index d45163a697..06b177b51a 100644 --- a/src/main/java/org/springframework/data/elasticsearch/client/elc/ReactiveElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/client/elc/ReactiveElasticsearchTemplate.java @@ -141,13 +141,16 @@ public Flux saveAll(Mono> entitiesPubli .flatMap(indexAndResponse -> { T savedEntity = entities.entityAt(indexAndResponse.getT1()); BulkResponseItem response = indexAndResponse.getT2(); - updateIndexedObject(savedEntity, new IndexedObjectInformation( // - response.id(), // - response.index(), // - response.seqNo(), // - response.primaryTerm(), // - response.version())); - return maybeCallbackAfterSave(savedEntity, index); + var updatedEntity = entityOperations.updateIndexedObject( + savedEntity, new IndexedObjectInformation( // + response.id(), // + response.index(), // + response.seqNo(), // + response.primaryTerm(), // + response.version()), + converter, + routingResolver); + return maybeCallbackAfterSave(updatedEntity, index); }); }); } 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 496ce3ee61..1eab3bdd4e 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java @@ -54,7 +54,6 @@ import org.springframework.data.elasticsearch.core.routing.RoutingResolver; import org.springframework.data.elasticsearch.core.script.Script; import org.springframework.data.elasticsearch.support.VersionInfo; -import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.callback.EntityCallbacks; import org.springframework.data.mapping.context.MappingContext; import org.springframework.data.util.Streamable; @@ -240,7 +239,11 @@ public Iterable save(Iterable entities, IndexCoordinates index) { // noinspection unchecked return indexQueries.stream() // .map(IndexQuery::getObject) // - .map(entity -> (T) updateIndexedObject(entity, iterator.next())) // + .map(entity -> (T) entityOperations.updateIndexedObject( + entity, + iterator.next(), + elasticsearchConverter, + routingResolver)) // .collect(Collectors.toList()); // } @@ -397,47 +400,6 @@ protected UpdateQuery buildUpdateQueryByEntity(T entity) { return updateQueryBuilder.build(); } - - protected T updateIndexedObject(T entity, IndexedObjectInformation indexedObjectInformation) { - - ElasticsearchPersistentEntity persistentEntity = elasticsearchConverter.getMappingContext() - .getPersistentEntity(entity.getClass()); - - if (persistentEntity != null) { - PersistentPropertyAccessor propertyAccessor = persistentEntity.getPropertyAccessor(entity); - ElasticsearchPersistentProperty idProperty = persistentEntity.getIdProperty(); - - // Only deal with text because ES generated Ids are strings! - if (indexedObjectInformation.id() != null && idProperty != null && idProperty.isReadable() - && idProperty.getType().isAssignableFrom(String.class)) { - propertyAccessor.setProperty(idProperty, indexedObjectInformation.id()); - } - - if (indexedObjectInformation.seqNo() != null && indexedObjectInformation.primaryTerm() != null - && persistentEntity.hasSeqNoPrimaryTermProperty()) { - ElasticsearchPersistentProperty seqNoPrimaryTermProperty = persistentEntity.getSeqNoPrimaryTermProperty(); - // noinspection ConstantConditions - propertyAccessor.setProperty(seqNoPrimaryTermProperty, - new SeqNoPrimaryTerm(indexedObjectInformation.seqNo(), indexedObjectInformation.primaryTerm())); - } - - if (indexedObjectInformation.version() != null && persistentEntity.hasVersionProperty()) { - ElasticsearchPersistentProperty versionProperty = persistentEntity.getVersionProperty(); - // noinspection ConstantConditions - propertyAccessor.setProperty(versionProperty, indexedObjectInformation.version()); - } - - var indexedIndexNameProperty = persistentEntity.getIndexedIndexNameProperty(); - if (indexedIndexNameProperty != null) { - propertyAccessor.setProperty(indexedIndexNameProperty, indexedObjectInformation.index()); - } - - // noinspection unchecked - return (T) propertyAccessor.getBean(); - } - return entity; - } - // endregion // region SearchOperations @@ -736,7 +698,11 @@ protected void updateIndexedObjectsWithQueries(List queries, Object queryObject = indexQuery.getObject(); if (queryObject != null) { - indexQuery.setObject(updateIndexedObject(queryObject, indexedObjectInformationList.get(i))); + indexQuery.setObject(entityOperations.updateIndexedObject( + queryObject, + indexedObjectInformationList.get(i), + elasticsearchConverter, + routingResolver)); } } } @@ -802,7 +768,11 @@ public T doWith(@Nullable Document document) { documentAfterLoad.hasSeqNo() ? documentAfterLoad.getSeqNo() : null, // documentAfterLoad.hasPrimaryTerm() ? documentAfterLoad.getPrimaryTerm() : null, // documentAfterLoad.hasVersion() ? documentAfterLoad.getVersion() : null); // - entity = updateIndexedObject(entity, indexedObjectInformation); + entity = entityOperations.updateIndexedObject( + entity, + indexedObjectInformation, + elasticsearchConverter, + routingResolver); return maybeCallbackAfterConvert(entity, documentAfterLoad, index); } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/AbstractReactiveElasticsearchTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/AbstractReactiveElasticsearchTemplate.java index cc6400f0a3..476148f929 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/AbstractReactiveElasticsearchTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/AbstractReactiveElasticsearchTemplate.java @@ -43,7 +43,6 @@ import org.springframework.data.elasticsearch.core.event.ReactiveAfterSaveCallback; import org.springframework.data.elasticsearch.core.event.ReactiveBeforeConvertCallback; 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.SimpleElasticsearchMappingContext; import org.springframework.data.elasticsearch.core.query.ByQueryResponse; @@ -55,7 +54,6 @@ import org.springframework.data.elasticsearch.core.script.Script; import org.springframework.data.elasticsearch.core.suggest.response.Suggest; import org.springframework.data.elasticsearch.support.VersionInfo; -import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.callback.ReactiveEntityCallbacks; import org.springframework.lang.NonNull; import org.springframework.lang.Nullable; @@ -320,51 +318,6 @@ protected IndexQuery getIndexQuery(Object value) { return query; } - protected T updateIndexedObject(T entity, IndexedObjectInformation indexedObjectInformation) { - - ElasticsearchPersistentEntity persistentEntity = converter.getMappingContext() - .getPersistentEntity(entity.getClass()); - - if (persistentEntity != null) { - // noinspection DuplicatedCode - PersistentPropertyAccessor propertyAccessor = persistentEntity.getPropertyAccessor(entity); - ElasticsearchPersistentProperty idProperty = persistentEntity.getIdProperty(); - - // Only deal with text because ES generated Ids are strings! - if (indexedObjectInformation.id() != null && idProperty != null && idProperty.isReadable() - && idProperty.getType().isAssignableFrom(String.class)) { - propertyAccessor.setProperty(idProperty, indexedObjectInformation.id()); - } - - if (indexedObjectInformation.seqNo() != null && indexedObjectInformation.primaryTerm() != null - && persistentEntity.hasSeqNoPrimaryTermProperty()) { - ElasticsearchPersistentProperty seqNoPrimaryTermProperty = persistentEntity.getSeqNoPrimaryTermProperty(); - // noinspection ConstantConditions - propertyAccessor.setProperty(seqNoPrimaryTermProperty, - new SeqNoPrimaryTerm(indexedObjectInformation.seqNo(), indexedObjectInformation.primaryTerm())); - } - - if (indexedObjectInformation.version() != null && persistentEntity.hasVersionProperty()) { - ElasticsearchPersistentProperty versionProperty = persistentEntity.getVersionProperty(); - // noinspection ConstantConditions - propertyAccessor.setProperty(versionProperty, indexedObjectInformation.version()); - } - - var indexedIndexNameProperty = persistentEntity.getIndexedIndexNameProperty(); - if (indexedIndexNameProperty != null) { - propertyAccessor.setProperty(indexedIndexNameProperty, indexedObjectInformation.index()); - } - - // noinspection unchecked - return (T) propertyAccessor.getBean(); - } else { - EntityOperations.AdaptableEntity adaptableEntity = entityOperations.forEntity(entity, - converter.getConversionService(), routingResolver); - adaptableEntity.populateIdIfNecessary(indexedObjectInformation.id()); - } - return entity; - } - @Override public Flux> multiGet(Query query, Class clazz) { return multiGet(query, clazz, getIndexCoordinatesFor(clazz)); @@ -391,12 +344,16 @@ public Mono save(T entity, IndexCoordinates index) { .map(it -> { T savedEntity = it.getT1(); IndexResponseMetaData indexResponseMetaData = it.getT2(); - return updateIndexedObject(savedEntity, new IndexedObjectInformation( - indexResponseMetaData.id(), - indexResponseMetaData.index(), - indexResponseMetaData.seqNo(), - indexResponseMetaData.primaryTerm(), - indexResponseMetaData.version())); + return entityOperations.updateIndexedObject( + savedEntity, + new IndexedObjectInformation( + indexResponseMetaData.id(), + indexResponseMetaData.index(), + indexResponseMetaData.seqNo(), + indexResponseMetaData.primaryTerm(), + indexResponseMetaData.version()), + converter, + routingResolver); }).flatMap(saved -> maybeCallbackAfterSave(saved, index)); } @@ -652,7 +609,11 @@ public Mono toEntity(@Nullable Document document) { documentAfterLoad.hasSeqNo() ? documentAfterLoad.getSeqNo() : null, documentAfterLoad.hasPrimaryTerm() ? documentAfterLoad.getPrimaryTerm() : null, documentAfterLoad.hasVersion() ? documentAfterLoad.getVersion() : null); - entity = updateIndexedObject(entity, indexedObjectInformation); + entity = entityOperations.updateIndexedObject( + entity, + indexedObjectInformation, + converter, + routingResolver); return maybeCallbackAfterConvert(entity, documentAfterLoad, index); }); 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 fdf701c7c0..68c14b08d7 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/EntityOperations.java @@ -18,6 +18,7 @@ import java.util.Map; import org.springframework.core.convert.ConversionService; +import org.springframework.data.elasticsearch.core.convert.ElasticsearchConverter; import org.springframework.data.elasticsearch.core.join.JoinField; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentEntity; import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentProperty; @@ -94,6 +95,69 @@ public AdaptableEntity forEntity(T entity, ConversionService conversionSe return AdaptableMappedEntity.of(entity, context, conversionService, routingResolver); } + /** + * Updates an entity after it is stored in Elasticsearch with additional data like id, version, seqno... + * + * @param the entity class + * @param entity the entity to update + * @param indexedObjectInformation the update information + * @param elasticsearchConverter the converter providing necessary mapping information + * @param routingResolver routing resolver to use + * @return + */ + public T updateIndexedObject(T entity, + IndexedObjectInformation indexedObjectInformation, + ElasticsearchConverter elasticsearchConverter, + RoutingResolver routingResolver) { + + Assert.notNull(entity, "entity must not be null"); + Assert.notNull(indexedObjectInformation, "indexedObjectInformation must not be null"); + Assert.notNull(elasticsearchConverter, "elasticsearchConverter must not be null"); + + ElasticsearchPersistentEntity persistentEntity = elasticsearchConverter.getMappingContext() + .getPersistentEntity(entity.getClass()); + + if (persistentEntity != null) { + PersistentPropertyAccessor propertyAccessor = persistentEntity.getPropertyAccessor(entity); + ElasticsearchPersistentProperty idProperty = persistentEntity.getIdProperty(); + + // Only deal with text because ES generated Ids are strings! + if (indexedObjectInformation.id() != null && idProperty != null + // isReadable from the base class is false in case of records + && (idProperty.isReadable() || idProperty.getOwner().getType().isRecord()) + && idProperty.getType().isAssignableFrom(String.class)) { + propertyAccessor.setProperty(idProperty, indexedObjectInformation.id()); + } + + if (indexedObjectInformation.seqNo() != null && indexedObjectInformation.primaryTerm() != null + && persistentEntity.hasSeqNoPrimaryTermProperty()) { + ElasticsearchPersistentProperty seqNoPrimaryTermProperty = persistentEntity.getSeqNoPrimaryTermProperty(); + // noinspection ConstantConditions + propertyAccessor.setProperty(seqNoPrimaryTermProperty, + new SeqNoPrimaryTerm(indexedObjectInformation.seqNo(), indexedObjectInformation.primaryTerm())); + } + + if (indexedObjectInformation.version() != null && persistentEntity.hasVersionProperty()) { + ElasticsearchPersistentProperty versionProperty = persistentEntity.getVersionProperty(); + // noinspection ConstantConditions + propertyAccessor.setProperty(versionProperty, indexedObjectInformation.version()); + } + + var indexedIndexNameProperty = persistentEntity.getIndexedIndexNameProperty(); + if (indexedIndexNameProperty != null) { + propertyAccessor.setProperty(indexedIndexNameProperty, indexedObjectInformation.index()); + } + + // noinspection unchecked + return (T) propertyAccessor.getBean(); + } else { + EntityOperations.AdaptableEntity adaptableEntity = forEntity(entity, + elasticsearchConverter.getConversionService(), routingResolver); + adaptableEntity.populateIdIfNecessary(indexedObjectInformation.id()); + } + return entity; + } + /** * Determine index name and type name from {@link Entity} with {@code index} and {@code type} overrides. Allows using * preferred values for index and type if provided, otherwise fall back to index and type defined on entity level. diff --git a/src/test/java/org/springframework/data/elasticsearch/core/EntityOperationsUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/EntityOperationsUnitTests.java index 1374ee9894..78fcebe2c5 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/EntityOperationsUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/EntityOperationsUnitTests.java @@ -20,17 +20,24 @@ import java.util.Arrays; import java.util.HashSet; +import org.assertj.core.api.SoftAssertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.GenericConversionService; import org.springframework.data.annotation.Id; +import org.springframework.data.annotation.Version; import org.springframework.data.elasticsearch.annotations.Document; +import org.springframework.data.elasticsearch.annotations.Field; +import org.springframework.data.elasticsearch.annotations.FieldType; +import org.springframework.data.elasticsearch.annotations.IndexedIndexName; import org.springframework.data.elasticsearch.annotations.Routing; +import org.springframework.data.elasticsearch.core.convert.ElasticsearchConverter; import org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter; import org.springframework.data.elasticsearch.core.join.JoinField; import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; +import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.elasticsearch.core.routing.DefaultRoutingResolver; import org.springframework.lang.Nullable; @@ -39,22 +46,24 @@ */ class EntityOperationsUnitTests { - @Nullable private static ConversionService conversionService; - @Nullable private static EntityOperations entityOperations; @Nullable private static SimpleElasticsearchMappingContext mappingContext; + @Nullable private static EntityOperations entityOperations; + @Nullable private static ElasticsearchConverter elasticsearchConverter; + @Nullable private static ConversionService conversionService; @BeforeAll static void setUpAll() { mappingContext = new SimpleElasticsearchMappingContext(); mappingContext.setInitialEntitySet(new HashSet<>(Arrays.asList(EntityWithRouting.class))); mappingContext.afterPropertiesSet(); + entityOperations = new EntityOperations(mappingContext); - MappingElasticsearchConverter converter = new MappingElasticsearchConverter(mappingContext, + elasticsearchConverter = new MappingElasticsearchConverter(mappingContext, new GenericConversionService()); - converter.afterPropertiesSet(); + ((MappingElasticsearchConverter) elasticsearchConverter).afterPropertiesSet(); - conversionService = converter.getConversionService(); + conversionService = elasticsearchConverter.getConversionService(); } @Test // #1218 @@ -104,6 +113,64 @@ void shouldReturnRoutingFromRoutingWhenJoinFieldIsSet() { assertThat(routing).isEqualTo("theRoute"); } + @Test // #2756 + @DisplayName("should update indexed information of class entity") + void shouldUpdateIndexedInformationOfClassEntity() { + + var entity = new EntityFromClass(); + entity.setId(null); + entity.setText("some text"); + var indexedObjectInformation = new IndexedObjectInformation( + "id-42", + "index-42", + 1L, + 2L, + 3L); + entity = entityOperations.updateIndexedObject(entity, + indexedObjectInformation, + elasticsearchConverter, + new DefaultRoutingResolver(mappingContext)); + + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(entity.getId()).isEqualTo(indexedObjectInformation.id()); + softly.assertThat(entity.getSeqNoPrimaryTerm().sequenceNumber()).isEqualTo(indexedObjectInformation.seqNo()); + softly.assertThat(entity.getSeqNoPrimaryTerm().primaryTerm()).isEqualTo(indexedObjectInformation.primaryTerm()); + softly.assertThat(entity.getVersion()).isEqualTo(indexedObjectInformation.version()); + softly.assertThat(entity.getIndexName()).isEqualTo(indexedObjectInformation.index()); + softly.assertAll(); + } + + @Test // #2756 + @DisplayName("should update indexed information of record entity") + void shouldUpdateIndexedInformationOfRecordEntity() { + + var entity = new EntityFromRecord( + null, + "someText", + null, + null, + null); + + var indexedObjectInformation = new IndexedObjectInformation( + "id-42", + "index-42", + 1L, + 2L, + 3L); + entity = entityOperations.updateIndexedObject(entity, + indexedObjectInformation, + elasticsearchConverter, + new DefaultRoutingResolver(mappingContext)); + + SoftAssertions softly = new SoftAssertions(); + softly.assertThat(entity.id()).isEqualTo(indexedObjectInformation.id()); + softly.assertThat(entity.seqNoPrimaryTerm().sequenceNumber()).isEqualTo(indexedObjectInformation.seqNo()); + softly.assertThat(entity.seqNoPrimaryTerm().primaryTerm()).isEqualTo(indexedObjectInformation.primaryTerm()); + softly.assertThat(entity.version()).isEqualTo(indexedObjectInformation.version()); + softly.assertThat(entity.indexName()).isEqualTo(indexedObjectInformation.index()); + softly.assertAll(); + } + @Document(indexName = "entity-operations-test") @Routing("routing") static class EntityWithRouting { @@ -165,4 +232,70 @@ public void setJoinField(@Nullable JoinField joinField) { this.joinField = joinField; } } + + @Document(indexName = "entity-operations-test") + static class EntityFromClass { + @Id + @Nullable private String id; + @Field(type = FieldType.Text) + @Nullable private String text; + @Version + @Nullable private Long version; + @Nullable private SeqNoPrimaryTerm seqNoPrimaryTerm; + @IndexedIndexName + @Nullable private String indexName; + + public String getId() { + return id; + } + + public void setId(String id) { + this.id = id; + } + + @Nullable + public String getText() { + return text; + } + + public void setText(@Nullable String text) { + this.text = text; + } + + @Nullable + public Long getVersion() { + return version; + } + + public void setVersion(@Nullable Long version) { + this.version = version; + } + + @Nullable + public SeqNoPrimaryTerm getSeqNoPrimaryTerm() { + return seqNoPrimaryTerm; + } + + public void setSeqNoPrimaryTerm(@Nullable SeqNoPrimaryTerm seqNoPrimaryTerm) { + this.seqNoPrimaryTerm = seqNoPrimaryTerm; + } + + @Nullable + public String getIndexName() { + return indexName; + } + + public void setIndexName(@Nullable String indexName) { + this.indexName = indexName; + } + } + + @Document(indexName = "entity-operations-test") + static record EntityFromRecord( + @Id @Nullable String id, + @Field(type = FieldType.Text) @Nullable String text, + @Version @Nullable Long version, + @Nullable SeqNoPrimaryTerm seqNoPrimaryTerm, + @IndexedIndexName @Nullable String indexName) { + } }