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.

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

Entity view inheritance with polymorphic depth 3 has too restrictive type constraints #475

Closed
beikov opened this issue Nov 13, 2017 · 13 comments

Comments

@beikov
Copy link
Member

beikov commented Nov 13, 2017

Currently the entity view inheritance for polymorphic models with depth 3 generates type constraints for the type discriminator that are contradictory like TYPE(this) = B AND TYPE(this) = C when C > B > A.
The test is provided in #465 which is easily fixed by simply not adding type constraints for proper entity super types.
However there is a testcase that assumes BView which doesn't have @EntityViewInheritance annotated will also use entity view inheritance. I'm not sure what the default should be, maybe @jwgmeligmeyling could argue for one or the other? If we enable inheritance by default just because a super type added the @EntityViewInheritance annotation, every entity view subtype that shouldn't use inheritance by default would have to annotate @EntityViewInheritance({THIS_TYPE.class}) to disable inheritance.
If we don't enable this by default, people have to annotate @EntityViewInheritance explicitly in this case on BView. What do you say?

@beikov beikov added this to the 1.2.0 milestone Nov 13, 2017
@beikov beikov self-assigned this Nov 13, 2017
@jwgmeligmeyling
Copy link
Collaborator

I merely removed the second @EntityViewInheritance annotation there to test whether the issue would still appear. As @EntityViewInheritance is not @Inherited, I actually assumed it would have to be declared again.

Nevertheless, I think it would in as sense be most logical to stay as close as possible to the behaviour of JPAs @Inheritance.

  • In Hibernate, a concrete type C with super types B and A will be initialised, no matter whether you query for C, B or A.
  • Even though @Inheritance is not @Inherited, it has characteristics as if it would be. So if you query for B instead of A, an C will still be returned as C (obviously the expression gets slightly easier because the fields for types that extend A but not B are not necessary any more).

Thus I would find it logical that:

  • given concrete type C with super types B and A, an EntityViewInheritance enabled AView or BView should return a CView for C.
  • If BView extends AView, it inherits its EntityViewInheritance capabilities (if it has subtypes).

Given that BView is already in an entity view inheritance hierarchy, I find any other expected behaviour very unlikely.

@beikov
Copy link
Member Author

beikov commented Nov 14, 2017

The thing is, if at some point AView gets annotated with a specific subtype set @EntityViewInheritance({AView.class, CView.class}) then querying for BView will suddenly not produce CView anymore. Would you say that if the super type specifies the classes explicitly, that this should not use inheritance any more or should it "ignore" the definitions and simply consider all subtypes?
I'd like to cover the common case best, as working around the default is always possible but just a bit ugly.

@jwgmeligmeyling
Copy link
Collaborator

jwgmeligmeyling commented Nov 14, 2017

Ah I see the problem. I forgot about that case as Hibernate does not allow subtypes to be filtered in such a manner.

  • Obviously we cannot inherit all the considered subtypes from the super interfaces EntityViewInheritance annotation, because they might not all satisfy the EntityView's mapped class (eg. its inherited considered subtypes should be the subtypes which are assignable to the subviews type). So an exception has to be made in this case.
  • Naturally if only AViews and CViews are taken into account for AView, then querying for BViews will not list BViews that are not CViews, I think that this makes sense this way.

I hope class hierarchies don't get any more complex than everything we can imagine to support now. Being able to override the considered subtypes for a subview type might be something that could be implement, but I find it very vague and ambiguous how it should work and why this would not have side effects for its super entity view .

Agreed?

@beikov
Copy link
Member Author

beikov commented Nov 14, 2017

So to make this clear:

@EntityView(A.class)
@EntityViewInheritance
public inteface AView {...}

@EntityView(B.class)
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

When querying for AView, you can get instances of types: AView, BView and CView
When querying for BView, you can get instances of types: BView and CView

and the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: CView

finally in the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
@EntityViewInheritance({ })
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: BView and CView

Is that what you would expect?

@jwgmeligmeyling
Copy link
Collaborator

I think so yes. Its still a little confusing, but I think this makes the most sense!

@beikov
Copy link
Member Author

beikov commented Nov 14, 2017

The alternative could be:

@EntityView(A.class)
@EntityViewInheritance
public inteface AView {...}

@EntityView(B.class)
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView, BView, CView and DView
When querying for BView, you can get instances of types: BView, CView and DView

and the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: BView, CView and DView

finally in the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
@EntityViewInheritance({ BView.class, CView.class })
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: BView and CView

The difference is, that regardless of the content of the @EntityViewInheritance value of a super type, entity view subtypes always consider all it's subtypes if at least one of the parents has @EntityViewInheritance annotated.

@jwgmeligmeyling
Copy link
Collaborator

The alternative inherits the EntityViewInheritance from the super interface, but ignores the classes to consider specified in the annotations value. I think that could be confusing for users that do make use of this functionality.

I think the alternative makes sense only if EntityViewInheritance is not inherited and we enforce the EntityViewInheritance to be on all interfaces used as inheritance roots, so that the EntityViewInheritance annotation is on BView as well, so that the value of the annotation is naturally different as well.

@beikov
Copy link
Member Author

beikov commented Nov 14, 2017

Currently the behavior(after the bug fix) is like this:

@EntityView(A.class)
@EntityViewInheritance
public inteface AView {...}

@EntityView(B.class)
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView, BView, CView and DView
When querying for BView, you can get instances of types: BView

and the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: BView

finally in the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
@EntityViewInheritance({ BView.class, CView.class })
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: BView and CView

So currently, for every entity view type that you query, it is required that an @EntityViewInheritance annotation is present in order to also get subtypes.

@jwgmeligmeyling
Copy link
Collaborator

And the case:

@EntityView(A.class)
@EntityViewInheritance({ AView.class, CView.class })
public inteface AView {...}

@EntityView(B.class)
@EntityViewInheritance()
public inteface BView extends AView {...}

@EntityView(C.class)
public inteface CView extends BView {...}

@EntityView(D.class)
public inteface DView extends BView {...}

When querying for AView, you can get instances of types: AView and CView
When querying for BView, you can get instances of types: BView and CView and DView?

That works for me!

@beikov
Copy link
Member Author

beikov commented Nov 14, 2017

Correct, that's the current behavior.
Good that it already fits your expectations 😄

@jwgmeligmeyling
Copy link
Collaborator

Is that fix already on the remote? 😄

beikov added a commit to beikov/blaze-persistence that referenced this issue Nov 14, 2017
@beikov
Copy link
Member Author

beikov commented Nov 14, 2017

Here is the fix: #478
I'm merging after the Travis CI build passes

@jwgmeligmeyling
Copy link
Collaborator

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants