AutoIndex label NPE during initialization and index lookup

jdorleans commented 7 years ago

Expected Behavior

All my entity nodes extend an abstract base class called Entity which contains @Id and common methods for all entities. Entity is not annotated with @NodeEntity since I do not want to save its label.

Due to @Id and the new AutoIndex functionality in OGM v3.x, the SessionFactory initialization fails with a NPE during AutoIndex creation. This is caused because @Id is considered a potential index by AutoIndexManager at initialiseIndexMetadata() and since Entity has no label AutoIndex creation throws a NPE.

To solve that I'm now forced to use @NodeEntity in my abstract Entity class, so it contains a valid label to be used during AutoIndex creation. However, I'd except AutoIndex to pick the simple class name as label for the leaf children in the hierarchy. For instance:

Entity --> OtherEntity --> FinalEntity label = FinalEntity where: Entity and OtherEntity are abstractions

NOTE: This behaviour is also suggested on the documentation: https://neo4j.com/docs/ogm-manual/current/tutorial/#tutorial:annotations:graphid

Current Behavior

NPE during AutoIndex creation for label param at: https://github.com/neo4j/neo4j-ogm/blob/master/core/src/main/java/org/neo4j/ogm/autoindex/AutoIndex.java#L66

java.lang.NullPointerException
    at org.neo4j.ogm.autoindex.AutoIndex.createDescription(AutoIndex.java:66)
    at org.neo4j.ogm.autoindex.AutoIndex.<init>(AutoIndex.java:61)
    at org.neo4j.ogm.autoindex.AutoIndexManager.initialiseIndexMetadata(AutoIndexManager.java:72)
    at org.neo4j.ogm.autoindex.AutoIndexManager.<init>(AutoIndexManager.java:57)
    at org.neo4j.ogm.session.SessionFactory.<init>(SessionFactory.java:84)

Possible Solution

It seems the change would be somewhere inside ClassInfo or MetaData. The framework should be capable of checking the hierarchy in between the classes and set the potential indexes to the end classes and all annotated abstract ones.

Steps to Reproduce (for bugs)

Create an abstract Entity with @Id and do not use @NodeEntity
Extend Entity with a second child AnotherEntity class and do not use @NodeEntity
That should be enough to fail during SessionFactory initialization of AutoIndex

Your Environment

OGM Version used: 3

frant-hartm commented 7 years ago

Thank you for reporting this.

Unless you also have @GeneratedValue with internal strategy the @Idannotation is supposed to be used within an annotated entity class, because:

id is used to merge the nodes when saving, doing so without index/constraint would be slow on larger database
the constraint is needed to ensure id's uniqueness across all subclasses, e.g. For Employee abstract class and Manager and Worker subclasses there must not be Worker and Manager with same id. If there is more. Also querying Employee by e.g. set of ids should return both Worker and Manager instances and use index on Employee.

We will certainly provide a better error message and make this clearer in the documentation.

Also see #367.

jdorleans commented 7 years ago

I'm not sure if I've understood you well. So far, I've been using @Id without annotating any of my classes just fine until the new v3 release. From my understanding, abstract classes should not generate any label, unless it is annotated with @NodeEntity. Instead, the label should be grabbed from its lower level non-abstract children and that's what I'm trying to point out here. This behavior is already in place, except for the new AutoIndex functionality that cannot identify @Id as been part of the children of Entity.

In your example, if Employee is an abstraction and it is not annotated with @NodeEntity it should not contain any label into the database and thus no index should be created for it. Therefore, Worker and Manager would get an individual index each. That's the behavior I'm looking for to keep.

Notice there is no meaning on having a label for my highest abstraction Entity.

Please, let me know if this makes sense to you.

dangercris commented 6 years ago

A posible solution is not define the property Id in the abstract Class and define in all the child classes

jdorleans commented 6 years ago

That could be possible if my apps weren't in production, but unfortunately it isn't a trivial solution to my cases. I'm using @Id for an UUID property existing in the Entity base class which also contains logics around equals, hashcode and other methods that depends on ID. To change this behaviour, it'd require my applications to move all this logic together with @Id annotation down to all Entity children.

Instead, the AutoIndex should not consider classes that do not contain @NodeEntity such as my Entity abstraction.

mmbradle commented 6 years ago

I have this same bug/issue. I think the bug report is very well written and concise so I have nothing much to add to the report.

To add my +1 to wanting this fixed, I do think it is valid in java to have an AbstactBaseClass for something like Entity. I do think putting fields on such a class is valid and having an id at the level is a common practice. Not wanting every node in the system to have a label like "Entity" also seems like a valid desire. All of this was achievable via prior versions of OGM, but not in this version, which makes the problem seem like a bug.

The issue as Frant points out seems to be that then there cannot be a constraint that works across all sub-types in this case. Perhaps this wasn't working in old versions and the new behavior is to fail loading in the condition instead of just loading without the constraint. Is that what used to happen? If so, I want that old behavior as an option. Personally, I'd rather live without the constraint (if that was the old behavior) than being forced by the system to not use base-classes, or be forced by the system to add a label for those base classes. Perhaps there is a different set of annotations we should be using to achieve these same ends?

ctytgat commented 6 years ago

I just upgraded to ogm 3.x and lost several hours trying to figure out how this @Id stuff is supposed to work. The constraint that @Id annotations must be used in conjunction with @NodeEntity doesn't make sense from a user perspective. We also have abstract classes that don't have @NodeEntity annotations but have common id fields and related behaviour. To workaround this, I had to introduce abstract getId() methods, and put @Id fields in all leaf classes, which is ugly as hell.

OlivierCroisier commented 6 years ago

Basically, I think most of us have a class hierarchy with an abstract base class like this :

public abstract class GraphEntity {

    @Id @GeneratedValue
    protected Long graphId;

    // + getters/setters, etc.
}

And regular business classes inheriting that base graph-related class, like so :

@NodeEntity
public class SomeModelClass extends GraphEntity {
    // ...
}

Also, considering Neo4J technical IDs (annotated with @Id) can be reused by Neo4j's and should not be considered stable over time, we often need to define class-specific, business-oriented IDs. Those IDs will be used a lot in queries, so it makes sense to configure them with @Index(unique=true) :

@NodeEntity
public class Person extends GraphEntity {

    @Index(unique = true)
    protected String ssn; 

    // + other fields, getter/setters, etc.
}

This setup used to work perfectly with previous versions of Neo4j-ogm, but it doesn't anymore.

Now, trying to put any @Index on business entities throws a NullPointerException during startup. Also, as described by OP, putting the @NodeEntity selectively on the business classes (to avoid ending up with the :GraphEntity label on every single node) does not work anymore.

I'd really appreciate if this bug could be fixed soon, if possible.

jdorleans commented 6 years ago

Whoever is tracking this, please, check on my suggested PR #484. This issue is driving my development team crazy!

grimace commented 6 years ago

This is definitely a bug. The docs say use an abstract class as base entity. Has there been any movement on this?

TWiStErRob commented 6 years ago

Bump, there is a PR, please make https://github.com/neo4j/neo4j-ogm/issues/437#issuecomment-380878496 code work. Or even if not accepting please at least update the code to throw a proper error and explain how to work around it.

yacosta738 commented 6 years ago

Can someone tell me if there is already a solution for this problem? I have spent hours searching the internet to see a solution for this problem but I have not found anything if someone could tell me if it is already solved or is working on that error. I am currently using Spring Data Neo4j Version 5.0.9 and Server Neo4j version 3.0.10

michael-simons commented 6 years ago

@yuniel-acosta What's "Server Neo4j version 3.0.10". Do you mean Neo4j OGM 3.1.0?

Apart from that, see my comment on @jdorleans PR.

TWiStErRob commented 6 years ago

@yuniel-acosta there are multiple workarounds having different drawbacks, read all the comments here.

yacosta738 commented 6 years ago

I refer to the Neo4j OGM version 3.1.0, but I use it as a separate server only to manage the database, I have read all the comments but I do not see any solution for example, as OlivierCroisier proposes I have an Abstract class for the general entity with @Id and several common methods for all my entities and I do not have it marked as @NodeEntity because I do not want it to be stored in the database and then I have all my classes that inherit from the base classes, but when executing the project I have the errors that are presented in the post. I have searched the internet for solutions to this but the ones I find are just like a patch to the problem such as creating an abstract method getId () in the super class and placing the corresponding @Id in each subclass, so I read this post but I do not see that there is a solution to the problem, if it exists, I would be very grateful if you could tell me how to solve it. Thank you for your attention

michael-simons commented 6 years ago

Fixed with #514. Will be in OGM 3.1.3.

neo4j / neo4j-ogm