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.

Sign up for GitHub

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

#437 - AutoIndex label NPE during initialization and index lookup #484

Closed
wants to merge 1 commit into from
Closed

#437 - AutoIndex label NPE during initialization and index lookup #484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 29 additions & 26 deletions core/src/main/java/org/neo4j/ogm/autoindex/AutoIndexManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,6 @@
*/
package org.neo4j.ogm.autoindex;

import static java.util.Collections.*;
import static org.neo4j.ogm.transaction.Transaction.Type.*;

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import org.neo4j.ogm.annotation.CompositeIndex;
import org.neo4j.ogm.config.Configuration;
import org.neo4j.ogm.metadata.ClassInfo;
Expand All @@ -31,11 +21,23 @@
import org.neo4j.ogm.request.Statement;
import org.neo4j.ogm.response.Response;
import org.neo4j.ogm.session.Neo4jSession;
import org.neo4j.ogm.session.SessionFactory;
import org.neo4j.ogm.session.request.DefaultRequest;
import org.neo4j.ogm.session.request.RowDataStatement;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import static java.util.Collections.emptyMap;
import static org.neo4j.ogm.config.AutoIndexMode.NONE;
import static org.neo4j.ogm.transaction.Transaction.Type.READ_WRITE;

/**
* This class controls the deletion and creation of indexes in the OGM.
*
Expand All @@ -46,16 +48,17 @@ public class AutoIndexManager {

private static final Logger LOGGER = LoggerFactory.getLogger(ClassInfo.class);

private final List<AutoIndex> indexes;
private List<AutoIndex> indexes;
private Neo4jSession session;

private final Configuration configuration;

public AutoIndexManager(MetaData metaData, Configuration configuration, Neo4jSession session) {

this.configuration = configuration;
this.indexes = initialiseIndexMetadata(metaData);
this.session = session;
private Configuration configuration;

public AutoIndexManager(MetaData metaData, Configuration configuration, SessionFactory sessionFactory) {
if (configuration.getAutoIndex() != NONE) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be in the controlling structure, that is the one that executes the auto manager (or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please point out the controlling structure location?

this.configuration = configuration;
this.indexes = initialiseIndexMetadata(metaData);
this.session = (Neo4jSession) sessionFactory.openSession();
build();
}
}

private List<AutoIndex> initialiseIndexMetadata(MetaData metaData) {
Expand All @@ -67,7 +70,7 @@ private List<AutoIndex> initialiseIndexMetadata(MetaData metaData) {
for (FieldInfo fieldInfo : classInfo.getIndexFields()) {
IndexType type = fieldInfo.isConstraint() ? IndexType.UNIQUE_CONSTRAINT : IndexType.SINGLE_INDEX;
final AutoIndex autoIndex = new AutoIndex(type, classInfo.neo4jName(),
new String[] { fieldInfo.property() });
new String[]{fieldInfo.property()});
LOGGER.debug("Adding Index [description={}]", autoIndex);
indexMetadata.add(autoIndex);
}
Expand All @@ -87,7 +90,7 @@ private List<AutoIndex> initialiseIndexMetadata(MetaData metaData) {
IndexType.REL_PROP_EXISTENCE_CONSTRAINT : IndexType.NODE_PROP_EXISTENCE_CONSTRAINT;

AutoIndex autoIndex = new AutoIndex(type, classInfo.neo4jName(),
new String[] { requiredField.property() });
new String[]{requiredField.property()});

LOGGER.debug("Adding required constraint [description={}]", autoIndex);
indexMetadata.add(autoIndex);
Expand All @@ -104,7 +107,7 @@ List<AutoIndex> getIndexes() {
/**
* Builds indexes according to the configured mode.
*/
public void build() {
private void build() {
switch (configuration.getAutoIndex()) {
case ASSERT:
assertIndexes();
Expand Down Expand Up @@ -192,7 +195,7 @@ private void assertIndexes() {

// make sure drop and create happen in separate transactions
// neo does not support that
session.doInTransaction( () -> {
session.doInTransaction(() -> {
session.requestHandler().execute(dropIndexesRequest);
}, READ_WRITE);

Expand All @@ -202,7 +205,7 @@ private void assertIndexes() {
private List<AutoIndex> loadIndexesFromDB() {
DefaultRequest indexRequests = buildProcedures();
List<AutoIndex> dbIndexes = new ArrayList<>();
session.doInTransaction( () -> {
session.doInTransaction(() -> {
try (Response<RowModel> response = session.requestHandler().execute(indexRequests)) {
RowModel rowModel;
while ((rowModel = response.next()) != null) {
Expand Down Expand Up @@ -250,7 +253,7 @@ private void executeStatements(List<Statement> statements) {
DefaultRequest request = new DefaultRequest();
request.setStatements(statements);

session.doInTransaction( () -> {
session.doInTransaction(() -> {
try (Response<RowModel> response = session.requestHandler().execute(request)) {
// Success
}
Expand Down Expand Up @@ -280,7 +283,7 @@ private void create() {
request.setStatements(statements);
LOGGER.debug("Creating indexes and constraints.");

session.doInTransaction( () -> {
session.doInTransaction(() -> {
try (Response<RowModel> response = session.requestHandler().execute(request)) {
// Success
}
Expand Down
32 changes: 12 additions & 20 deletions core/src/main/java/org/neo4j/ogm/context/LabelPrimaryId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@

package org.neo4j.ogm.context;

import static java.util.Objects.*;

import org.neo4j.ogm.metadata.ClassInfo;

import java.util.Objects;

import static java.util.Objects.requireNonNull;

/**
* Pair of label and primary id to use for lookups by primary key in MappingContext and CypherContext
* The label is needed because primary id is unique for given label. There might be 1 primary id pointing to
Expand All @@ -33,11 +35,11 @@ class LabelPrimaryId {
/**
* Create LabelPrimaryId
*
* @param classInfo class info containign the primary id
* @param classInfo class info containing the primary id
* @param id the value of the id
*/
public LabelPrimaryId(ClassInfo classInfo, Object id) {
this.label = classInfo.primaryIndexField().containingClassInfo().neo4jName();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change here - as much as I understand it to be in accordance with the docs - does change behavior for everyone that relays on having the 1st label being the parent or containing class. What do u think @meistermeier ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this change is a key aspect of this fix, I'd suggest to communicate it properly during the next release update in a way it gives enough information for those relaying on the buggy behavior.

this.label = classInfo.neo4jName();
this.id = requireNonNull(id);
}

Expand All @@ -49,32 +51,22 @@ public Object getId() {
return id;
}


@Override
public boolean equals(Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;

if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
LabelPrimaryId that = (LabelPrimaryId) o;

if (!label.equals(that.label))
return false;
return id.equals(that.id);
return Objects.equals(label, that.label) && Objects.equals(id, that.id);
}

@Override
public int hashCode() {
int result = label.hashCode();
result = 31 * result + id.hashCode();
return result;
return Objects.hash(label, id);
}

@Override
public String toString() {
return "LabelPrimaryId{" +
"label='" + label + '\'' +
", id=" + id +
'}';
return String.format("LabelPrimaryId{label='%s', id=%s}", label, id);
}
}
14 changes: 6 additions & 8 deletions core/src/main/java/org/neo4j/ogm/session/SessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@

package org.neo4j.ogm.session;

import static java.util.Objects.*;

import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;

import org.neo4j.ogm.autoindex.AutoIndexManager;
import org.neo4j.ogm.config.Configuration;
import org.neo4j.ogm.driver.Driver;
Expand All @@ -28,6 +23,11 @@
import org.neo4j.ogm.metadata.reflect.ReflectionEntityInstantiator;
import org.neo4j.ogm.session.event.EventListener;

import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;

import static java.util.Objects.requireNonNull;

/**
* This is the main initialization point of OGM. Used to create {@link Session} instances for interacting with Neo4j.
* In a typical scenario one instance of SessionFactory is created, shared across whole application.
Expand Down Expand Up @@ -81,9 +81,7 @@ public SessionFactory(Configuration configuration, String... packages) {
this.driver = newDriverInstance(configuration.getDriverClassName());
this.driver.configure(configuration);
this.eventListeners = new CopyOnWriteArrayList<>();
Neo4jSession session = (Neo4jSession) openSession();
AutoIndexManager autoIndexManager = new AutoIndexManager(this.metaData, configuration, session);
autoIndexManager.build();
new AutoIndexManager(this.metaData, configuration, this);
Copy link
Collaborator

@michael-simons michael-simons Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above, should have gone here. That should be an explicit method call making it clear that it is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below.

this.entityInstantiator = new ReflectionEntityInstantiator(metaData);
}

Expand Down
38 changes: 17 additions & 21 deletions test/src/test/java/org/neo4j/ogm/autoindex/BaseAutoIndexManagerTestClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,7 @@

package org.neo4j.ogm.autoindex;

import static com.google.common.collect.Lists.*;
import static java.util.stream.Collectors.*;
import static org.assertj.core.api.Assertions.*;

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Consumer;
import java.util.stream.StreamSupport;

import com.google.common.collect.ObjectArrays;
import org.apache.commons.io.IOUtils;
import org.junit.After;
import org.junit.Before;
Expand All @@ -37,16 +25,28 @@
import org.neo4j.graphdb.schema.IndexDefinition;
import org.neo4j.ogm.config.Configuration;
import org.neo4j.ogm.metadata.MetaData;
import org.neo4j.ogm.session.Neo4jSession;
import org.neo4j.ogm.session.SessionFactory;
import org.neo4j.ogm.testutil.MultiDriverTestClass;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.collect.ObjectArrays;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Consumer;
import java.util.stream.StreamSupport;

import static com.google.common.collect.Lists.newArrayList;
import static java.util.stream.Collectors.toList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

/**
* Must not end with "Test" so it does not run on TC.
*
* @author Frantisek Hartman
*/
public abstract class BaseAutoIndexManagerTestClass extends MultiDriverTestClass {
Expand Down Expand Up @@ -204,9 +204,7 @@ public void testAutoIndexDumpCreatesIndex() throws IOException {
.generatedIndexesOutputFilename(file.getName())
.build();

Neo4jSession session = (Neo4jSession) sessionFactory.openSession();
AutoIndexManager indexManager = new AutoIndexManager(metaData, configuration, session);
indexManager.build();
new AutoIndexManager(metaData, configuration, sessionFactory);

assertThat(file.exists()).isTrue();
try (InputStream is = new FileInputStream(file)) {
Expand All @@ -221,9 +219,7 @@ public void testAutoIndexDumpCreatesIndex() throws IOException {

void runAutoIndex(String mode) {
Configuration configuration = getBaseConfiguration().autoIndex(mode).build();
Neo4jSession session = (Neo4jSession) sessionFactory.openSession();
AutoIndexManager indexManager = new AutoIndexManager(metaData, configuration, session);
indexManager.build();
new AutoIndexManager(metaData, configuration, sessionFactory);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather prefer a solution not having the call to auto index manager here.
Also: If its there, not in this form: Just instantiating a class like this looks like coincidence, something you have to comment with "please don't remove, it has side effects." Those should be made explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concerns and I'd say it's a valid point for a later improvement and should be out-of-scope for this PR, since it will require more work, increase complexity and does not bring value to this particular PR main goal. As I mentioned before, this change was a minor improvement regarding the usage of the sessionFactory.

}

void executeForIndexes(Consumer<List<IndexDefinition>> consumer) {
Expand Down
23 changes: 23 additions & 0 deletions test/src/test/java/org/neo4j/ogm/domain/abstraction/AnotherEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.neo4j.ogm.domain.abstraction;

public abstract class AnotherEntity extends Entity {

protected String anotherValue;

public AnotherEntity() {
super();
}

public AnotherEntity(String uuid) {
super(uuid);
}

public String getAnotherValue() {
return anotherValue;
}

public void setAnotherValue(String anotherValue) {
this.anotherValue = anotherValue;
}

}
40 changes: 40 additions & 0 deletions test/src/test/java/org/neo4j/ogm/domain/abstraction/ChildA.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.neo4j.ogm.domain.abstraction;

import java.util.HashSet;
import java.util.Set;

public class ChildA extends Entity {

private String value;

private Set<AnotherEntity> children = new HashSet<>();

public ChildA() {
super();
}

public ChildA(String uuid) {
super(uuid);
}

public ChildA add(AnotherEntity childB) {
children.add(childB);
return this;
}

public String getValue() {
return value;
}

public void setValue(String value) {
this.value = value;
}

public Set<AnotherEntity> getChildren() {
return children;
}

public void setChildren(Set<AnotherEntity> children) {
this.children = children;
}
}
Loading