Mapping of null values broken since 3.2.4

[nioertel]

I'm executing a query that returns combinations of String properties and a nodes.
E.g. MATCH (m:Movie) OPTIONAL MATCH (m)<-[:ACTS_IN]-(a:Actor) RETURN m.name AS name, a AS actor
The result of the query is mapped to a List of @QueryResult containing String name and Actor actor. Actor is defined as @NodeEntity.
For the movies which have no actors assigned the query returns combination "some movie", null.

Since OGM 3.2.4 an IllegalArgumentExeption is thrown by OGM as SingleUseEntityMapper#mapKnownNestedClasses returns Collections.emptyList() instead of null.
The issue was likely introduced with this commit c4d3046

Expected Behavior

The mapping should work in any case no matter if there are actors in the movie or not.

Current Behavior

The mapping fails with an IllegalArgumentException.

Possible Solution

The easy fix would be to replace mappedValue = Collections.emptyList(); by mappedValue = null; in line 188.
However I think the split of logic between writeProperty and mapKnownNestedClasses is not clear & clean. There is already some special logic for mapping collections in mapKnownNestedClasses and then again in writeProperty. This should be cleaned up as part of the fix.

I have the fix ready locally. If you confirm this is the right approach, I can prepare a pull request.

Your Environment

• OGM Version used: 3.2.4, 3.2.5, 3.2.6, 3.2.7-SNAPSHOT-2020-01-11T11:00:00
• Java Version used: Oracle JDK 1.8.0_202
• Operating System and Version: macOS 10.14.6

PS: It would be great to get a 3.2.7 release containing the fix before the end of this week if possible.

[michael-simons]

Hi @nioertel I'd love to see your fix.

Thansk for being so understandable… We have polished the SingleUseEntityMapper quite a bit, but obviously missed some rough edges.

[michael-simons]

Does OGM thrown an IllegalArgumentExeption or SDN?

Never mind, I have a reproducer…

@Test // GH-748
    public void ff() {

        SingleUseEntityMapper entityMapper =
            new SingleUseEntityMapper(sessionFactory.metaData(),
                new ReflectionEntityInstantiator(sessionFactory.metaData()));

        Iterable<Map<String, Object>> results = sessionFactory.openSession()
            .query("WITH 'a name' AS something OPTIONAL MATCH (t:ThingEntity {na:false}) RETURN something, t as entity", EMPTY_MAP)
            .queryResults();

        assertThat(results).hasSize(1);

        ThingResult2 thingResult = entityMapper.map(ThingResult2.class, results.iterator().next());
        assertThat(thingResult.getSomething()).isEqualTo("a name");
        assertThat(thingResult.getEntity()).isNull();
    }

Please share your approach… I think it should go into the right direction judging from your comment. We have some other fixes piling up, so we can wrap this up here in a 3.2.7 release this week.

[michael-simons]

WDYT about that approach:

 if (asCollection) {
                mappedValue = nestedObjects;
            } else if (nestedObjects.size() == 1) {
                mappedValue = nestedObjects.isEmpty() ? null : nestedObjects.get(0);
            } else {
                logger.warn(
                    "Cannot map property {} from result set: The result contains more than one entry for the property.",
                    property);
            }

We have to keep the logic separate, but we can be more clear about it:

writeProperty stays as is and we rename mapKnownNestedClasses to valueOrMappedKnownClass. That makes the necessity clearer that the method may returns the value untouch and so the coercion into the right collection type must be done in writeProperty in all cases.

[nioertel]

Thanks. I think the fix broke a functionality that made the mapping of invalid query results fail before. I added a review comment to the commit. Please check.

[michael-simons]

Could you add a test for it? That's green before and red after my fix? Btw are you on the community slack? https://neo4j-users-slack-invite.herokuapp.com/ Maybe we can have a quick chat about it…

The SingleEntityMapper is kinda hard to test. It has been developed for use with Spring Data Neo4j only and there had been not a single good test before I started working on that class in pure OGM.

[nioertel]

This test should do it:

	@Test // GH-748
	@Ignore("Broken since ac116c2")
	public void nullValuesShouldBeMappedForNestedObjects() {
		SingleUseEntityMapper entityMapper =
				new SingleUseEntityMapper(sessionFactory.metaData(), new ReflectionEntityInstantiator(sessionFactory.metaData()));

		Iterable<Map<String, Object>> results = sessionFactory.openSession()
				.query("MATCH (t:ThingEntity) WHERE t.name IN ['Thing 1', 'Thing 2'] RETURN 'FooBar' AS something, COLLECT(t) as entity",
						EMPTY_MAP)
				.queryResults();

		assertThat(results).hasSize(1);
		assertThatThrownBy(() -> entityMapper.map(ThingResult2.class, results.iterator().next())).isInstanceOf(Exception.class);
	}

[michael-simons]

Added and pushed an additional commit. Let's see what CI says.