Subclass Repository returns also Superclass objects [DATAMONGO-1142]

[spring-projects-issues]

Mateusz Rasiński opened DATAMONGO-1142 and commented

Having two types of entities, that are mapped to two Java classes in the single MongoDB collection:

@Document
public class Superclass { ... }

@Document(collection = "superclass")
public class Subclass extends Superclass { ... }

and two repositories for those entities:

public interface SuperclassRepository extends MongoRepository<Superclass, String> {}
public interface SubclassRepository extends MongoRepository<Subclass, String> {}

MongoRepositories don't handle the inheritance of the entities correctly. While querying for all Subclass objects (e.g. SubclassRepository.findAll()) the result set contains Superclass objects, that are instantiated (or at least have been tried to be instantiated) with null values for fields that are part of the Subclass, but are not part of the Superclass.

The expected result would be that SubclassRepository should return only Subclass objects, while SuperclassRepository should return Superclass and Subclass objects. It works this way in Spring Data JPA

---

Affects: 1.6.1 (Evans SR1)

Reference URL: http://stackoverflow.com/questions/28025710/spring-data-mongodb-repositories-dont-implement-inheritance-correctly

Referenced from: pull request #266

4 votes, 7 watchers

[spring-projects-issues]

Oliver Drotbohm commented

The tricky bit here is to find out when to apply type constraints and when not. Unfortunately we cannot generally (even outside an inheritance scenario) enable applying type constraints as people might want to query an existing database that doesn't have any type information stored. In your concrete scenario how would we differentiate between Superclass just existing for responsibility reasons or actually being the root of a polymorphic relationship. SubclassRepository doesn't know anything about SuperclassRepository. I mean, what you're basically asking for is: turn on type restricted queries because the super-type of the type to return is basically a standalone entity and can be queried on its own.

I am not saying we can't improve on that at all. My point is we can't simply change the out-of-the-box behavior as this might break clients querying untyped data. So we need to come up with advanced mapping metadata to clearly express that special usecase. Open for suggestions

[spring-projects-issues]

Mateusz Rasiński commented

My suggestion is stated clearly in my pull request.

No other tests hurt from that change, so I believe that it would not break things. Take a look at it and see if that would be acceptable

[spring-projects-issues]

Oliver Drotbohm commented

I've seen the PR and thanks for that but removing query results is probably not the way to go. Mostly for performance reasons as you might end up reading a ton of documents but end up not converting a single one of them. Also it, almost by definition, would break pagination as you might end up with less items on a page than you requested.

The only way of properly implementing is to find a dedicated clause to apply the type constraint onto the query being executed. This not only makes sure we don't have to sprinkle all these addition bits of code everywhere (the template, the converter etc.) but to a canonical place, which is the query execution

[spring-projects-issues]

Mateusz Rasiński commented

It removes the result only if they are null (you're talking about findMultiple...(), right?). That shouldn't be that bad, should it?

EDIT: It removes the result (returns null) ONLY when the target type is a subclass and the result is a superclass of that subclass. Projections, for instance, still work well for that scenario. Also it does nothing for the results that doesn't contain _class (or custom type specifier)

[spring-projects-issues]

Oliver Drotbohm commented

See the edits on my comment. Generally speaking: if a fix touches a lot of unrelated places of the codebase, it's very unlikely to be a proper solution :). Let's give the team a chance to think through this and all its consequences and we'll find a fix for that

[spring-projects-issues]

Mateusz Rasiński commented

OK, I get it. I was thinking of creating a specific query, but have thought that for performance reasons ( ;) ) finding all subclasses of the class we want to find would be inefficient. But, as you said, it may be the way to go.

Anyway, thanks for such a quick response!

[spring-projects-issues]

Antoine Vandecreme commented

So, wouldn't adding the following criteria to the query in SimpleMongoRepository.findAll(Query query) solve the problem:

query.addCriteria(
        where("_class").is(entityInformation.getJavaType().getTypeName()));

The exact type name is too restrictive, but that gives the idea.

EDIT: to make it clearer, here is the workaround repository I use for my concrete classes: https://gist.github.com/avandecreme/6ee4cc44c54fd8f2c1ae

[spring-projects-issues]

Mateusz Rasiński commented

"The exact type name is too restrictive, but that gives the idea." - that's the whole point.

Imagine class hierarchy: A (superclass) <- B <- C. If you put in the _class cause the name of B, than the query won't find the superclass A (OK! we want this!), but it also won't find the subclass of B - C (no, we don't want this).

The possible solution is the one I've come up with few comments above, but it can, as Oliver said, break something somewhere else.

The other solution I can think of is to "get to know" all of the subclasses of the queried class (B in the example) and add them to the _class clause. Unfortunately, it's not an easy task in Java. It would probably require a full classpath scanning or at least a scanning of the classes in the specific package (as Spring Framework Core does). Actually, now that I think about it, it should be OK to implement it this way ;)

[spring-projects-issues]

Param commented

I am not sure if this thread is still active, but found activity on this ticket. So would like to share this with you:

Take a look at this: Spring Data MongoDB — My take on inheritance support

I think that is a better approach to use the TypeAlias annotated value. If a class is inherited and a TypeAlias annotation is present, then only add the criteria - I think with this it will hurt less other users and will less likely to break something that is already there. Also if the TypeAlias annotation can have a new attribute (inheritanceStrategy or something) that can accept a list of types to be included in the type checking condition to handle cases like A <- B <- C .

I think it is also fine to have a InheritanceAwareSimpleMongoRepository in the API separate for the cases like this if its too much risky that a change like this will break something

[opensourcegeek]

Just wondering if there's been any progress on this ticket, I had a look at the demo repo that uses TypeAlias, which makes sense. I'm thinking of adding a separate field in the base class that I could use as the discriminator, following the approach in that demo app. Is there any class diagram or some documentation on how the whole class structure for spring-data-mongo-db works, to be able to come up with custom impls or extensions of spring provided classes. I was struggling to get a handle on what things to extend to add another criteria to the derived query methods implicitly. I stumbled upon that article which is useful but it'd be much better to know how these classes interact.