Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect sink population #2627

Closed
ReUnioN opened this issue Jul 13, 2023 · 3 comments · Fixed by #2637
Closed

Incorrect sink population #2627

ReUnioN opened this issue Jul 13, 2023 · 3 comments · Fixed by #2637
Labels
status: feedback-provided Feedback has been provided type: bug A general bug

Comments

@ReUnioN
Copy link

ReUnioN commented Jul 13, 2023

Problem

When indexing an entity with a complex field containing multiple nested objects, we experience an exception during the population of the elasticsearch document.

Exception

java.lang.IllegalArgumentException: cannot write xcontent for unknown value of type class com.xxx.ProcessFile
	at org.elasticsearch.xcontent.XContentBuilder.unknownValue(XContentBuilder.java:840)
	at org.elasticsearch.xcontent.XContentBuilder.map(XContentBuilder.java:980)
	at org.elasticsearch.xcontent.XContentBuilder.unknownValue(XContentBuilder.java:829)
	at org.elasticsearch.xcontent.XContentBuilder.value(XContentBuilder.java:1009)
	at org.elasticsearch.xcontent.XContentBuilder.unknownValue(XContentBuilder.java:831)
	at org.elasticsearch.xcontent.XContentBuilder.map(XContentBuilder.java:980)
	at org.elasticsearch.xcontent.XContentBuilder.unknownValue(XContentBuilder.java:829)
	at org.elasticsearch.xcontent.XContentBuilder.map(XContentBuilder.java:980)
	at org.elasticsearch.xcontent.XContentBuilder.map(XContentBuilder.java:929)
	at org.elasticsearch.action.index.IndexRequest.source(IndexRequest.java:452)
	at org.elasticsearch.action.index.IndexRequest.source(IndexRequest.java:441)
	at org.elasticsearch.action.update.UpdateRequest.doc(UpdateRequest.java:701)
	at org.springframework.data.elasticsearch.client.erhlc.RequestFactory.updateRequest(RequestFactory.java:1039)
	at org.springframework.data.elasticsearch.client.erhlc.ElasticsearchRestTemplate.update(ElasticsearchRestTemplate.java:266)

I assume this Exception raises, because there is no custom writer attached to convert an object of type ProcessFile.

Steps to reproduce the issue

We use the MappingElasticsearchConverter from the ElasticsearchRestTemplate instance to populate a Document with our ProcessCommitEntity. That document is passed to an UpdateQuery.Builder used to invoke the elasticsearchOperations.update method.

Class layout

@Document(indexName = PROCESS_INDEX, createIndex = false)
@Setting(shards = 15, refreshInterval = "30s")
public class ProcessCommitEntity extends ESChildEntity {

    @JoinTypeRelations(relations = ...)
    @JsonIgnore
    @Schema(hidden = true)
    private JoinField<String> joinField;

    @Field(type = FieldType.Flattened)
    private PSParameter runtimeParameter;

    ...
}
public class PSParameter extends LinkedHashMap<String, Object> {

    public PSParameter(Map<String, Object> m) {
        if (m != null) {
            putAll(m);
        }
    }

    ...
}
public class ProcessFile {

    private String server;
    private String volume;
    private String path;
    private String protocol;

    ...
}

Integration test

There is an ProcessCommitEntity about to be saved using the corresponding elasticsearch data repository.

@Test
public void testIndexListWithResourceInMap() {

    final ProcessEntity processEntity = new ProcessEntity(1L);
    processRepository.save(processEntity);
    processRepository.refresh();

    // malfunction data structure
    final PSParameter changes = new PSParameter(
        Collections.singletonMap(
            "copies",
            Collections.singletonList(
                Collections.singletonMap(
                    "destination",
                    new ProcessFile("server", "volume", "path", "smb")
                )
            )
        )
    );

    final ProcessCommitEntity processCommitEntity = new ProcessCommitEntity(1L, 1L);
    processCommitEntity.setName("test-commit");
    processCommitEntity.setRuntimeParameter(changes);
    processCommitRepository.save(processCommitEntity); // <- exception
    processCommitRepository.refresh();
}

Population of the update builder

final Document document = Document.create();
elasticsearchOperations.getElasticsearchConverter().write(entity, document); // <- issue

final IndexCoordinates indexCoordinates = elasticsearchOperations.getIndexCoordinatesFor(entity.getClass());

final UpdateQuery.Builder builder = UpdateQuery.builder(entity.getId());
builder.withRouting(((ESChildEntity) entity).getParentId());
builder.withDocument(document).withDocAsUpsert(true);
builder.withRetryOnConflict(5);

elasticsearchOperations.update(builder.build(), indexCoordinates); // <- exception

Issue

Issue is not the update operation itself, but the previously created Document still containing a ProcessFile Objekt instead of a MapDocument representing the ProcessFile. The incorrect conversion is enforced by MappingElasticsearchConverter.writeCollectionInternal:

private List<Object> writeCollectionInternal(Collection<?> source, @Nullable TypeInformation<?> type,
        Collection<?> sink) {

    TypeInformation<?> componentType = null;

    List<Object> collection = sink instanceof List ? (List<Object>) sink : new ArrayList<>(sink);

    if (type != null) {
        componentType = type.getComponentType();
    }

    for (Object element : source) {

        Class<?> elementType = element == null ? null : element.getClass();

        // issue (use implementation like MappingElasticsearchConverter.Writer.isSimpleType instead)
        if (elementType == null || conversions.isSimpleType(elementType)) { 
            collection.add(getPotentiallyConvertedSimpleWrite(element,
                    componentType != null ? componentType.getType() : Object.class));
        } else if (element instanceof Collection || elementType.isArray()) {
            collection.add(writeCollectionInternal(asCollection(element), componentType, new ArrayList<>()));
        } else {
            Map<String, Object> document = Document.create();
            writeInternal(element, document, componentType);
            collection.add(document);
        }
    }

    return collection;
}

The outcoming Document looks like:
without-fix

Expectation

When indexing less nested data the ProcessFile is automatically being converted to a MapDocument. The Map/List/Map/ProcessFile construct breaks the conversion. I expect the ProcessFile to be converted to a MapDocument regardless of the structure containing it.

Quick fix

Registering a custom conversion class adding the missing check for Map just like MappingElasticsearchConverter.Writer.isSimpleType does, fixes the issue.

@Bean
@Override
public ElasticsearchCustomConversions elasticsearchCustomConversions() {
    return new ElasticsearchCustomConversions(Collections.emptyList()) {
        @Override
        public boolean isSimpleType(Class<?> type) {
            return !Map.class.isAssignableFrom(type) && super.isSimpleType(type);
        }
    };
}

In this scenario the Map inside of the List is not threaten as simple type, therefore the usual conversion routine kicks in. The outcoming Document looks like:
with-fix

Possible spring data elasticsearch fix?

MappingElasticsearchConverter.writeCollectionInternal does use conversions.isSimpleType to determine if the contained type is written as simple value, as collection or as map. However, incase of a Map residing in the List it considers the Map to be written as simple value. Therefore no further conversion is taken into account. The original value is written to the sink. If MappingElasticsearchConverter.Writer.isSimpleType would be used instead of conversions.isSimpleType, it would behave correctly.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 13, 2023
@sothawo
Copy link
Collaborator

sothawo commented Jul 17, 2023

Before I commit this change, can you please check if the test I wrote covers your case - I'd say so, but better have a secoond look:

	@Test // #2627
	@DisplayName("should write Map containing collection containing map")
	void shouldWriteMapContainingCollectionContainingMap() throws JSONException {

		class EntityWithMapCollectionMap {
			Map<String, Object> map;
		}
		class InnerEntity {
			String prop1;

			String prop2;

			public InnerEntity() {}

			public InnerEntity(String prop1, String prop2) {
				this.prop1 = prop1;
				this.prop2 = prop2;
			}

		}

		var entity = new EntityWithMapCollectionMap();
		entity.map = Collections.singletonMap("collection",
				Collections.singletonList(Collections.singletonMap("destination", new InnerEntity("prop1", "prop2"))));

		var expected = """
				{
					"_class": "org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverterUnitTests$1EntityWithMapCollectionMap",
					"map": {
						"collection": [
							{
								"destination": {
									"_class": "org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverterUnitTests$1InnerEntity",
									"prop1": "prop1",
									"prop2": "prop2"
								}
							}
						]
					}
				}
				""";

		Document document = Document.create();

		mappingElasticsearchConverter.write(entity, document);

		assertEquals(expected, document.toJson(), false);
	}

If MappingElasticsearchConverter.Writer.isSimpleType would be used instead of conversions.isSimpleType, it would behave correctly.

This change then makes the test pass.

@sothawo sothawo added type: bug A general bug status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 17, 2023
sothawo added a commit to sothawo/spring-data-elasticsearch that referenced this issue Jul 18, 2023
@ReUnioN
Copy link
Author

ReUnioN commented Jul 18, 2023

When applying your test case to my class layout I experience the issue as expected. Adding the !Map check everything resolves correctly. So yes, your test covers my issue. Thanks for your time and effort!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 18, 2023
@sothawo
Copy link
Collaborator

sothawo commented Jul 18, 2023

I will merge and backport this later this evening

@sothawo sothawo added this to the 5.2 M2 (2023.1.0) milestone Jul 18, 2023
sothawo added a commit that referenced this issue Jul 18, 2023
Original Pull Request #2637
Closes #2627
sothawo added a commit that referenced this issue Jul 18, 2023
Original Pull Request #2637
Closes #2627

(cherry picked from commit d9bb991)
(cherry picked from commit c045a8a)
sothawo added a commit that referenced this issue Jul 18, 2023
Original Pull Request #2637
Closes #2627

(cherry picked from commit d9bb991)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants