loadAll() does not deal with multi-type collections well

[tarehart]

I have an inheritance hierarchy on my node entities. When calling loadAll() on a List<ParentClass>, relationships unique to certain subtypes present in the list are not loaded, even when the requested depth is more than adequate.

Currently, the type of the first element is used for fetching metadata that gets applied to the entire collection: https://github.com/neo4j/neo4j-ogm/blob/9c0f192896a534ef6b20ecb0cb57be4d59e0e06b/src/main/java/org/neo4j/ogm/session/delegates/LoadByInstancesDelegate.java#L47

When the metadata clashes with the actual type of a subsequent element, bad things happen. I think I need to work around this by grouping my list by subtype and making a separate call to loadAll() per group.

[luanne]

@tarehart could you share an example of what you mean by "relationships unique to certain subtypes present in the list are not loaded"?

[tarehart]

The field associated with that relationship is unexpectedly null.

Consider these classes:

@NodeEntity
public class ParentClass {
}

public class ChildA extends ParentClass {
    @Relationship(...)
    public ClassX fieldX;
}

public class ChildB extends ParentClass {
    @Relationship(...)
    public ClassY fieldY;
}

If you call loadAll() on a List<ParentClass> that has an instance of ChildA followed by an instance of ChildB, fieldY will always remain null.

I can dig deeper into the mechanics of how things go awry if you're unable to dupe.

[luanne]

ClassX and ClassY not part of the inheritance hierarchy right?

[tarehart]

That's correct. Also, the workaround that I was considering in my original post was successful.

[luanne]

Okay, I'll have a look

[tarehart]

Thanks Luanne!

[luanne]

@tarehart what am I missing? https://github.com/neo4j/neo4j-ogm/commit/544cd7b357a99d3f5de6c3219ff6d83820798772

[tarehart]

I think you should null out fieldX and fieldY before your test calls loadAll(). Correct me if I'm wrong, but I think that's a valid scenario that arises if ChildA and ChildB are at the periphery of some other load.

I'll start trying to get that test running so I can tinker with it in case I'm wrong.

[tarehart]

OK, I think I can clear this up a little. In my application I'm calling loadAll() but not using the list it returns; I just rely on the side effects (it tends to populate the objects within the collection I pass as an argument). That may be a poor practice on my part. The test harness does not seem to have those same side effects, so any such test seems to always fail. However, if I rewrite the test like this:

    // Test unchanged up to this point....

    List<ParentClass> allParentClasses = new ArrayList<>();
    // Don't add parent to list
    allParentClasses.add(childA);
    allParentClasses.add(childB);

    session.clear();

    Collection<ParentClass> all = session.loadAll(allParentClasses, 3);

    boolean foundB = false;

    for (ParentClass parentClass : all) {
        if (parentClass instanceof ChildB) {
            foundB = true;
            break;
        }
    }

    assertTrue(foundB); // Fails

Assertion fails. That's cause for concern, right?

[luanne]

Yes, thanks, will fix.

[luanne]

Can you use <T> Collection<T> loadAll(Class<T> type, Collection<Long> ids);?

  List<Long> allParentClasses = new ArrayList<>();
  allParentClasses.add(childA.getId());
  allParentClasses.add(childB.getId());

  session.clear();
  Collection<ParentClass> all = session.loadAll(ParentClass.class, allParentClasses);

With type erasure in the picture, this might be not that simple to fix.

[tarehart]

Yes, <T> Collection<T> loadAll(Class<T> type, Collection<Long> ids); works as desired, including the side effects. Not too cumbersome either, it's still a one-liner with streams. I'm unblocked, thanks Luanne!

As for the difficulty with type erasure, you could always throw some for loops at the problem. Warning: untested and unpalatable.

private Class getMostSpecificBaseClass(Collection<?> list) {

    if (list.isEmpty()) {
        return null;
    }

    List<ArrayList<Class>> parentListings = new ArrayList<>(list.size());
    Set<Class> handledClasses = new HashSet<>(list.size());

    for (Object o: list) {
        ArrayList<Class> parents = new ArrayList<>();
        Class c = o.getClass();
        if (handledClasses.add(c)) {
            while (c != null) {
                parents.add(c);
                c = c.getSuperclass();
            }
            parentListings.add(parents);
        }
    }

    ArrayList<Class> baseList = parentListings.remove(parentListings.size() - 1);
    int bestDepth = baseList.size() - 1;

    for (ArrayList<Class> g: parentListings) {
        for (int i = 0; i < g.size() && i <= bestDepth; i++) {
            if (g.get(g.size() - i - 1) != baseList.get(baseList.size() - i - 1)) {
                bestDepth = i - 1;
                break;
            }
        }
    }

    int baseIndex = baseList.size() - bestDepth - 1;
    return baseIndex >= 0 ? baseList.get(baseIndex) : null;
}