Skip to content

Commit

Permalink
GH-684 - Fix possible NPE in IdentityMap.
Browse files Browse the repository at this point in the history
The identity map used the wrong map to look up the expected hash value when both a node and relationship with the same id have been loaded and a NPE happened when casting literal Null to long. This has been fixed by first selecting the correct hash.

The error first appeared in the performance optimizations in #576, #577 and #579.
  • Loading branch information
michael-simons committed Nov 15, 2019
1 parent 815a639 commit 75d6cbb
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 4 deletions.
13 changes: 9 additions & 4 deletions core/src/main/java/org/neo4j/ogm/context/IdentityMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,22 @@ void remember(Object object, Long entityId) {
*/
boolean remembered(Object object, Long entityId) {

// Bail out early if the native id is null or neither the hashes of nodes nor relationships contain the entity id.
if (entityId == null || !(nodeHashes.containsKey(entityId) || relEntityHashes.containsKey(entityId))) {
// Bail out early if the native id is null...
if (entityId == null) {
return false;
}

ClassInfo classInfo = metaData.classInfo(object);
boolean isRelEntity = metaData.isRelationshipEntity(classInfo.name());
Map<Long, Long> hashes = isRelEntity ? relEntityHashes : nodeHashes;

long actual = hash(object, classInfo);
long expected = isRelEntity ? relEntityHashes.get(entityId) : nodeHashes.get(entityId);
// ... or a little later when the hashes in question doesnt contain the entities id
if (!hashes.containsKey(entityId)) {
return false;
}

long actual = hash(object, classInfo);
long expected = hashes.get(entityId);
return actual == expected;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@
import org.junit.Test;
import org.neo4j.ogm.domain.education.School;
import org.neo4j.ogm.domain.education.Teacher;
import org.neo4j.ogm.domain.education.TeachesAt;
import org.neo4j.ogm.metadata.MetaData;

/**
* @author Vince Bickers
* @author Michael J. Simons
*/
public class IdentityMapTest {

Expand Down Expand Up @@ -70,4 +72,29 @@ public void testRelatedObjectChangeDoesNotAffectNodeMemoisation() {

assertThat(mappingContext.isDirty(teacher)).isFalse();
}

@Test // GH-684
public void testNodeAndRelationshipWithSameId() {

// Create a Node and set the Id.
Teacher mrsJones = new Teacher();
mrsJones.setId(1L);

// Remember the entity.
mappingContext.addNodeEntity(mrsJones);

// Create a Relationship with the same Id.
TeachesAt teachesAtRelationship = new TeachesAt();
teachesAtRelationship.setId(1L);

IdentityMap identityMap = new IdentityMap(metaData);
identityMap.remember(mrsJones, mrsJones.getId());

assertThat(identityMap.remembered(mrsJones, mrsJones.getId())).isTrue();
assertThat(identityMap.remembered(teachesAtRelationship, teachesAtRelationship.getId())).isFalse();

identityMap.remember(teachesAtRelationship, teachesAtRelationship.getId());

assertThat(identityMap.remembered(teachesAtRelationship, teachesAtRelationship.getId())).isTrue();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright (c) 2002-2019 "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.education;

import org.neo4j.ogm.annotation.Id;
import org.neo4j.ogm.annotation.RelationshipEntity;

/**
* @author Michael J. Simons
*/
@RelationshipEntity
public class TeachesAt {

@Id
private Long id;

public long getId() { return id; }

public void setId(long id) { this.id = id; }
}

0 comments on commit 75d6cbb

Please sign in to comment.