Skip to content

Commit

Permalink
fix: Correctly filter query results, taking the fact that native ids …
Browse files Browse the repository at this point in the history
…are only sort of unique for the respecting entity type. (#953)

Closes #592.
  • Loading branch information
nioertel committed Jul 26, 2023
1 parent fc90a38 commit bbfbc3f
Show file tree
Hide file tree
Showing 11 changed files with 546 additions and 28 deletions.
69 changes: 69 additions & 0 deletions core/src/main/java/org/neo4j/ogm/context/EntityFilter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright (c) 2002-2023 "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.context;

import java.util.Optional;

import org.neo4j.ogm.model.GraphModel;
import org.neo4j.ogm.model.Node;
import org.neo4j.ogm.response.model.DefaultGraphModel;
import org.neo4j.ogm.response.model.NodeModel;

/**
* Filter for entities to check whether nodes/relationships should be included in the mapping result.
*
* @author Niels Oertel
*/
interface EntityFilter {

/**
* Include any entity.
*/
EntityFilter INCLUDE_ALWAYS = (graphModel, nativeId, isNode) -> true;

/**
* Include all relationships but only nodes which are not generated.
*/
EntityFilter WITHOUT_GENERATED_NODES = (graphModel, nativeId, isNode) -> {
if (!isNode) {
return true;
} else {
Optional<Node> node = ((DefaultGraphModel) graphModel).findNode(nativeId);
if (!node.isPresent()) {
return true; // this should actually never happen but to keep existing behaviour, we are not throwing an exception
}
return node.map(n -> !((NodeModel) n).isGeneratedNode()).get();
}
};

/**
* Check if an object with given native id should be included in the mapping result.
*
* @param graphModel
* The graph model.
* @param nativeObjectId
* The object's native id.
* @param isNode
* True if the object is a node, false if relationship.
*
* @return True if the object should be included.
*/
boolean shouldIncludeModelObject(GraphModel graphModel, long nativeObjectId, boolean isNode);

}
17 changes: 9 additions & 8 deletions core/src/main/java/org/neo4j/ogm/context/GraphEntityMapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@

import java.lang.reflect.InvocationTargetException;
import java.util.*;
import java.util.function.BiFunction;
import java.util.function.Consumer;
import java.util.function.Predicate;

Expand Down Expand Up @@ -54,6 +53,7 @@
* @author Vince Bickers
* @author Luanne Misquitta
* @author Michael J. Simons
* @author Niels Oertel
*/
public class GraphEntityMapper {

Expand Down Expand Up @@ -81,7 +81,7 @@ public GraphEntityMapper(MetaData metaData, MappingContext mappingContext, Entit
}

<T> List<T> map(Class<T> type, Response<GraphModel> listOfGraphModels) {
return map(type, listOfGraphModels, (m, n) -> true);
return map(type, listOfGraphModels, EntityFilter.INCLUDE_ALWAYS);
}

/**
Expand All @@ -92,7 +92,7 @@ <T> List<T> map(Class<T> type, Response<GraphModel> listOfGraphModels) {
* @return The list of entities represented by the list of graph models.
*/
<T> List<T> map(Class<T> type, Response<GraphModel> graphModelResponse,
BiFunction<GraphModel, Long, Boolean> additionalNodeFilter) {
EntityFilter additionalEntityFilter) {

// Those are the ids of all mapped nodes.
Set<Long> mappedNodeIds = new LinkedHashSet<>();
Expand All @@ -103,7 +103,7 @@ <T> List<T> map(Class<T> type, Response<GraphModel> graphModelResponse,

// Execute mapping for each individual model
Consumer<GraphModel> mapContentOfIndividualModel =
graphModel -> mapContentOf(graphModel, additionalNodeFilter, returnedNodeIds, mappedRelationshipIds,
graphModel -> mapContentOf(graphModel, additionalEntityFilter, returnedNodeIds, mappedRelationshipIds,
returnedRelationshipIds, mappedNodeIds);

GraphModel graphModel = null;
Expand Down Expand Up @@ -139,20 +139,21 @@ <T> List<T> map(Class<T> type, Response<GraphModel> graphModelResponse,

private void mapContentOf(
GraphModel graphModel,
BiFunction<GraphModel, Long, Boolean> additionalNodeFilter,
EntityFilter additionalEntityFilter,
Set<Long> returnedNodeIds,
Set<Long> mappedRelationshipIds,
Set<Long> returnedRelationshipIds,
Set<Long> mappedNodeIds
) {
Predicate<Long> includeInResult = id -> additionalNodeFilter.apply(graphModel, id);
Predicate<Long> includeNodeInResult = id -> additionalEntityFilter.shouldIncludeModelObject(graphModel, id, true);
Predicate<Long> includeRelInResult = id -> additionalEntityFilter.shouldIncludeModelObject(graphModel, id, false);
try {
Set<Long> newNodeIds = mapNodes(graphModel);
returnedNodeIds.addAll(newNodeIds.stream().filter(includeInResult).collect(toList()));
returnedNodeIds.addAll(newNodeIds.stream().filter(includeNodeInResult).collect(toList()));
mappedNodeIds.addAll(newNodeIds);

newNodeIds = mapRelationships(graphModel);
returnedRelationshipIds.addAll(newNodeIds.stream().filter(includeInResult).collect(toList()));
returnedRelationshipIds.addAll(newNodeIds.stream().filter(includeRelInResult).collect(toList()));
mappedRelationshipIds.addAll(newNodeIds);
} catch (MappingException e) {
throw e;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2002-2022 "Neo4j,"
* Copyright (c) 2002-2023 "Neo4j,"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
Expand All @@ -22,7 +22,6 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.function.BiFunction;

import org.neo4j.ogm.metadata.MetaData;
import org.neo4j.ogm.model.GraphModel;
Expand All @@ -36,6 +35,7 @@
/**
* @author Vince Bickers
* @author Michael J. Simons
* @author Niels Oertel
*/
public class GraphRowListModelMapper implements ResponseMapper<GraphRowListModel> {

Expand Down Expand Up @@ -96,8 +96,8 @@ public String[] columns() {
};

// although it looks like that the `idsOfResultEntities` will stay empty, they won't, trust us.
BiFunction<GraphModel, Long, Boolean> includeModelObject =
(graphModel, nativeId) -> idsOfResultEntities.contains(nativeId);
EntityFilter includeModelObject =
(graphModel, nativeId, isNode) -> idsOfResultEntities.contains(nativeId);

return delegate.map(type, graphResponse, includeModelObject);
}
Expand Down
19 changes: 3 additions & 16 deletions core/src/main/java/org/neo4j/ogm/context/GraphRowModelMapper.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2002-2022 "Neo4j,"
* Copyright (c) 2002-2023 "Neo4j,"
* Neo4j Sweden AB [http://neo4j.com]
*
* This file is part of Neo4j.
Expand All @@ -18,19 +18,14 @@
*/
package org.neo4j.ogm.context;

import java.util.Optional;
import java.util.function.BiFunction;

import org.neo4j.ogm.metadata.MetaData;
import org.neo4j.ogm.model.GraphModel;
import org.neo4j.ogm.model.Node;
import org.neo4j.ogm.response.Response;
import org.neo4j.ogm.response.model.DefaultGraphModel;
import org.neo4j.ogm.response.model.NodeModel;
import org.neo4j.ogm.session.EntityInstantiator;

/**
* @author Michael J. Simons
* @author Niels Oertel
*/
public class GraphRowModelMapper implements ResponseMapper<GraphModel> {

Expand All @@ -44,14 +39,6 @@ public GraphRowModelMapper(MetaData metaData, MappingContext mappingContext,

@Override
public <T> Iterable<T> map(Class<T> type, Response<GraphModel> response) {

BiFunction<GraphModel, Long, Boolean> isNotGeneratedNode = (graphModel, nativeId) -> {
Optional<Node> node = ((DefaultGraphModel) graphModel).findNode(nativeId);
if (!node.isPresent()) {
return true; // Native id describes a relationship
}
return node.map(n -> !((NodeModel) n).isGeneratedNode()).get();
};
return delegate.map(type, response, isNotGeneratedNode);
return delegate.map(type, response, EntityFilter.WITHOUT_GENERATED_NODES);
}
}
26 changes: 26 additions & 0 deletions neo4j-ogm-tests/neo4j-ogm-integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,17 @@
</configuration>

<executions>
<execution>
<id>default-test</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<excludes>
<exclude>org.neo4j.ogm.persistence.session.capability.QueryCapabilityGH952Test</exclude>
</excludes>
</configuration>
</execution>
<execution>
<id>test-different-encoding</id>
<goals>
Expand All @@ -298,6 +309,21 @@
</includes>
</configuration>
</execution>
<execution>
<id>native-id-tests</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<!--
The tests for correct handling of overlapping native ids for nodes and relationships
(see GH-952) need to be executed on a clean Neo4j instance
-->
<includes>
<include>org.neo4j.ogm.persistence.session.capability.QueryCapabilityGH952Test</include>
</includes>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright (c) 2002-2023 "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.gh952;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import org.neo4j.ogm.annotation.GeneratedValue;
import org.neo4j.ogm.annotation.Id;
import org.neo4j.ogm.annotation.NodeEntity;

@NodeEntity(Book.LABEL)
public class Book {

public static final String LABEL = "Book";

@Id
@GeneratedValue(strategy = UuidGenerationStrategy.class)
private String uuid;

private String title;

private List<Human> readBy = Collections.emptyList();

public String getUuid() {
return uuid;
}

public void setUuid(String uuid) {
this.uuid = uuid;
}

public String getTitle() {
return title;
}

public void setTitle(String title) {
this.title = title;
}

public List<Human> getReadBy() {
return Collections.unmodifiableList(readBy);
}

public void setReadBy(List<Human> readBy) {
this.readBy = new ArrayList<>(readBy);
}

}
Loading

0 comments on commit bbfbc3f

Please sign in to comment.