Skip to content

Commit

Permalink
GH-748 - Fix return type for known entity classes.
Browse files Browse the repository at this point in the history
Use null instead of an empty list when the target isn’t a collection as well. This verifies the indirect illegal argument exception in case target property and query won’t fit but keeps the lenient behaviour in place when a query returning collections actually returns single valued collections.

The scopes of the logback dependencies have been clarified.

Thanks to @nioertel for reporting the issue and helping to solve it.

This fixes #748.
  • Loading branch information
michael-simons committed Jan 23, 2020
1 parent 95f5d18 commit 0c25e46
Show file tree
Hide file tree
Showing 7 changed files with 251 additions and 82 deletions.
1 change: 1 addition & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<scope>test</scope>
</dependency>

<dependency>
Expand Down
71 changes: 38 additions & 33 deletions core/src/main/java/org/neo4j/ogm/context/SingleUseEntityMapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -120,23 +120,26 @@ private ClassInfo resolveClassInfoFor(Class<?> type) {

private void writeProperty(ClassInfo classInfo, Object instance, Map.Entry<String, Object> property) {

FieldInfo writer = classInfo.getFieldInfo(property.getKey());
String key = property.getKey();
FieldInfo writer = classInfo.getFieldInfo(key);

if (writer == null) {
FieldInfo fieldInfo = classInfo.relationshipFieldByName(property.getKey());
FieldInfo fieldInfo = classInfo.relationshipFieldByName(key);
if (fieldInfo != null) {
writer = fieldInfo;
}
}

if (writer == null) {
logger.warn("Unable to find property: {} on class: {} for writing", property.getKey(), classInfo.name());
logger.warn("Unable to find property: {} on class: {} for writing", key, classInfo.name());
} else {
Class elementType = DescriptorMappings.getType(writer.getTypeDescriptor());
boolean targetIsCollection = writer.type().isArray() || Iterable.class.isAssignableFrom(writer.type());

Object value = mapKnownNestedClasses(elementType, property.getKey(), property.getValue(),
targetIsCollection);
Object value = property.getValue();
if (metadata.classInfo(elementType) != null) {
value = mapKnownEntityType(elementType, key, value, targetIsCollection);
}

// merge iterable / arrays and co-erce to the correct attribute type
if (targetIsCollection) {
Expand All @@ -158,46 +161,48 @@ private void writeProperty(ClassInfo classInfo, Object instance, Map.Entry<Strin
}

/**
* If the element type is a known class, it will be mapped, either into a single value or into a collection of things.
* If the element type is unknown or cannot be mapped to a single property, the original value is returned.
*
* @param elementType The target type, must not be null
* @param property The name of the property
* @param value The value (can be null)
* @param asCollection whether to create a collection or not
* @return The mapped value
*/
Object mapKnownNestedClasses(Class elementType, String property, Object value, boolean asCollection) {

Object mappedValue = value;
if (metadata.classInfo(elementType) != null) {
List<Object> nestedObjects = new ArrayList<>();

for (Object nestedPropertyMap : iterableOf(value)) {
if (nestedPropertyMap instanceof Map) {
// Recursively map maps
nestedObjects.add(map(elementType, (Map<String, Object>) nestedPropertyMap));
} else if (elementType.isInstance(nestedPropertyMap) || ClassUtils.isEnum(elementType)) {
// Add fitting types and enums directly
nestedObjects.add(nestedPropertyMap);
} else {
logger.warn("Cannot map {} to a nested result object for property {}", nestedPropertyMap,
property);
}
}
if (asCollection) {
mappedValue = nestedObjects;
} else if (nestedObjects.isEmpty()) {
mappedValue = Collections.emptyList();
} else if (nestedObjects.size() == 1) {
mappedValue = nestedObjects.get(0);
Object mapKnownEntityType(Class elementType, String property, Object value, boolean asCollection) {

List<Object> nestedObjects = new ArrayList<>();

for (Object nestedPropertyMap : iterableOf(value)) {
if (nestedPropertyMap instanceof Map) {
// Recursively map maps
nestedObjects.add(map(elementType, (Map<String, Object>) nestedPropertyMap));
} else if (elementType.isInstance(nestedPropertyMap) || ClassUtils.isEnum(elementType)) {
// Add fitting types and enums directly
nestedObjects.add(nestedPropertyMap);
} else {
logger.warn(
"Cannot map property {} from result set: The result contains more than one entry for the property.",
property);
logger.warn("Cannot map {} to a nested result object for property {}", nestedPropertyMap, property);
}
}
return mappedValue;

if (asCollection) {
return nestedObjects;
} else if (nestedObjects.size() > 1) {
logger.warn(
"Cannot map property {} from result set: The result contains more than one entry for the property.",
property);
// Returning the original value here is done on purpose to not change in edge cases in SDN
// for which we don't have tests yet. Edge cases can be weird queries with weird query result classes
// that fit together non the less.
return value;
} else {
return nestedObjects.isEmpty() ? null : nestedObjects.get(0);
}
}

Iterable iterableOf(Object thingToIterator) {

if (thingToIterator == null) {
return Collections.emptyList();
} else if (thingToIterator instanceof Iterable) {
Expand Down
3 changes: 1 addition & 2 deletions neo4j-ogm-tests/neo4j-ogm-integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,7 @@
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<scope>test</scope>
<optional>true</optional>
<scope>compile</scope>
</dependency>

<dependency>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* Copyright (c) 2002-2020 "Neo4j,"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* 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
*
* http://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.neo4j.ogm.testutil;

import ch.qos.logback.classic.Logger;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.read.ListAppender;

import java.util.List;
import java.util.stream.Collectors;

import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
import org.slf4j.LoggerFactory;

/**
* Naive rule to capture logging output. It assumes that logback is used during tests as SLF4J binding.
*
* @author Michael J. Simons
*/
public class LoggerRule implements TestRule {

private final ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
private final Logger logger = (Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);

@Override
public Statement apply(Statement base, Description description) {
return new Statement() {
@Override
public void evaluate() throws Throwable {
setup();
base.evaluate();
teardown();
}
};
}

private void setup() {
logger.addAppender(listAppender);
listAppender.start();
}

private void teardown() {
listAppender.stop();
listAppender.list.clear();
logger.detachAppender(listAppender);
}

public List<String> getMessages() {
return listAppender.list.stream().map(e -> e.getMessage()).collect(Collectors.toList());
}

public List<String> getFormattedMessages() {
return listAppender.list.stream().map(e -> e.getFormattedMessage()).collect(Collectors.toList());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,26 @@
import java.util.HashMap;
import java.util.Map;

import org.assertj.core.api.Condition;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.neo4j.ogm.domain.cineasts.minimum.Actor;
import org.neo4j.ogm.domain.cineasts.minimum.Movie;
import org.neo4j.ogm.domain.cineasts.minimum.Role;
import org.neo4j.ogm.domain.cineasts.minimum.SomeQueryResult;
import org.neo4j.ogm.domain.gh391.SomeContainer;
import org.neo4j.ogm.domain.gh551.AnotherThing;
import org.neo4j.ogm.domain.gh551.ThingEntity;
import org.neo4j.ogm.domain.gh551.ThingResult;
import org.neo4j.ogm.domain.gh551.ThingResult2;
import org.neo4j.ogm.domain.gh551.ThingWIthId;
import org.neo4j.ogm.domain.gh552.Thing;
import org.neo4j.ogm.metadata.MetaData;
import org.neo4j.ogm.metadata.reflect.ReflectionEntityInstantiator;
import org.neo4j.ogm.session.Session;
import org.neo4j.ogm.session.SessionFactory;
import org.neo4j.ogm.testutil.LoggerRule;
import org.neo4j.ogm.testutil.TestContainersTestBase;

/**
Expand All @@ -52,6 +57,9 @@ public class SingleUseEntityMapperTest extends TestContainersTestBase {

private static SessionFactory sessionFactory;

@Rule
public final LoggerRule loggerRule = new LoggerRule();

@BeforeClass
public static void oneTimeSetUp() {
sessionFactory = new SessionFactory(getDriver(), "org.neo4j.ogm.domain.gh551", "org.neo4j.ogm.domain.gh391",
Expand Down Expand Up @@ -93,6 +101,89 @@ public void singleUseEntityMapperShouldWorkWithNestedObjects() {
.allSatisfy(s -> s.startsWith("Thing"));
}

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

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();
}

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

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 {name: 'Thing 7'}) 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()).isNotNull().extracting(ThingEntity::getName).isEqualTo("Thing 7");
}

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

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) RETURN something, COLLECT(t) as entity",
EMPTY_MAP)
.queryResults();

assertThat(results).hasSize(1);

assertThatIllegalArgumentException()
.isThrownBy(() -> entityMapper.map(ThingResult2.class, results.iterator().next()))
.withMessageContaining(
"Can not set org.neo4j.ogm.domain.gh551.ThingEntity field org.neo4j.ogm.domain.gh551.ThingResult2.entity to java.util.ArrayList");
Condition<String> stringMatches = new Condition<>(s -> s.contains(
"Cannot map property entity from result set: The result contains more than one entry for the property."),
"String matches");
assertThat(loggerRule.getFormattedMessages()).areAtLeastOne(stringMatches);
}

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

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 {name: 'Thing 7'}) RETURN something, COLLECT(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()).isNotNull().extracting(ThingEntity::getName).isEqualTo("Thing 7");
}

@Test // GH-718
public void queryResultShouldHandleNodeAndRelationshipEntities() {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright (c) 2002-2020 "Neo4j,"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
*
* 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
*
* http://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.neo4j.ogm.domain.gh551;

/**
* @author Michael J. Simons
*/
public class ThingResult2 {

private String something;

private ThingEntity entity;

public String getSomething() {
return something;
}

public void setSomething(String something) {
this.something = something;
}

public ThingEntity getEntity() {
return entity;
}

public void setEntity(ThingEntity entity) {
this.entity = entity;
}
}
Loading

0 comments on commit 0c25e46

Please sign in to comment.