Open quaff opened 6 months ago
Well, that would be a breaking change, so we can't do that. Also, to handle generics correctly, the method would need to return a Type
, not Class
.
So it would have to be a new method.
But honestly I just don't see this as a very high priority at all. Entities don't usually benefit from parametric polymorphism, since they must be mapped to a database table which is not generic. In the particular example shown, it's quite easy to move the mapping for owner
down to the concrete entities which implement the mapped superclass. You're not really gaining much here.
that would be a breaking change
Let's discuss the real impact of this change, I think it's a enhancement instead of breaking change.
the method would need to return a Type, not Class. So it would have to be a new method.
I agree that, but it should be discussed in another feature request issue.
it's quite easy to move the mapping for owner down to the concrete entities which implement the mapped superclass.
Moving the field down add lots of boilerplate code is not elegant, and lots of existing code need changes to adapt the breaking change which introduced by Hibernate 6.
I think you're underestimating this. If it's worth doing, it's worth doing properly.
First of all, if we're going to have metamodel objects representing actualized versions of inherited members, the first thing I want to know is: what is the way to get hold of these things via the static metamodel. Now, that's a solvable problem, but we would have to solve it.
Second, consider the signature of this method:
Attribute<? super X, ?> getAttribute(String name);
That's actually wrong for the operation you're proposing. The correct signature would be:
Attribute<X, ?> getActualAttribute(String name);
without the wildcard.
Now, perhaps it's not that big of a deal, but it's still a whole new operation, along with the addition of an operation on Attribute
returning Type
.
So in the end that's—just right off the top of my head—much more than a trivial change for something that's of pretty marginal value.
"as concrete as possible" as title said, If it's not possible then fall back to current behavior.
So you would rather have
entityManager.getMetamodel().entity(Book.class).getAttribute("owner").getJavaType()
return the concrete type i.e. Owner.class
if OwnerContainer
is only ever extended with specifying Owner
as type variable, but start returning Object.class
as soon as there is another subclass passing a different type as type variable?
Sorry, but I think this is horrible. Hibernate ORM 5 actually suffers a bug where it randomly uses one of the possible type variable assignments. I guess you might have relied on this broken behavior, but as soon as you have more type variable assignments you will see the problem.
If the JPA spec ought to handle generics, it will need a new API for that.
but start returning Object.class as soon as there is another subclass passing a different type as type variable?
I'm not sure I understand what you means, for other subclass such as House
:
@Entity
public class House extends OwnerContainer<HouseOwner> {
@Id
private Long id;
}
@Entity
public class HouseOwner {
@Id
private Long id;
}
then
entityManager.getMetamodel().entity(House.class).getAttribute("owner").getJavaType()
should return HouseOwner
I guess you mean
entityManager.getMetamodel().entity(House.class).getAttribute("owner").getJavaType()
should return HouseOwner
?
This is where you seem to lack an understanding of how the metamodel is structured. An attribute is declared by a managed type and clearly OwnerContainer
declares the owner
attribute. This means that every call to getAttribute("owner")
, regardless on which subtype you call it, will have to return that one attribute object. Clearly you understand that an object can only return one result, so if you want to know the attribute type based on a container subtype, there is a need for a new method.
I guess you mean should return
HouseOwner
?
Yes, It's a typo.
An attribute is declared by a managed type and clearly
OwnerContainer
declares theowner
attribute. This means that every call togetAttribute("owner")
, regardless on which subtype you call it, will have to return that one attribute object.
that's why I'm opening this issue, there is something need to improve at specification level, and It's a small fix for hibernate.
For an given entity, the owner
type is concrete, and I'm querying attribute of that given entity, why not return the concrete type? If I'm querying owner
attribute on OwnerContainer
, the type is not concrete, but OwnerContainer
is not an entity, so that will not going happen.
For static metamodel, It should generated like this:
@StaticMetamodel(OwnerContainer.class)
public abstract class OwnerContainer_ {
public static volatile SingularAttribute<OwnerContainer, Object> owner;
}
@StaticMetamodel(Book.class)
public abstract class Book_ {
public static volatile SingularAttribute<Book, Long> id;
public static volatile SingularAttribute<Book, Owner> owner; // The missing part
}
I've tested with EclipseLink 3.0.4, It does return the expected Owner.class
instead of erased type Object.class
, so I guess there is no technical limitation you worried.
I think the spec should be revised to clarify the expected behavior, WDYT @gavinking
Not a single person here claimed that it cannot be done.
The claim is that it would be wrong and inelegant to do so.
As I've already said above, it's pretty clear from the signature of the method (i.e. from its return type) that that operation is supposed to return the member as declared by the supertype. See that wildcard there? It's there for a reason, not just for decoration!
If the operation were supposed to behave the way you claim it should behave, that is, if it were supposed to return the member as actually inherited by the subtype then its signature would be Attribute<X, ?> getAttribute(String name)
and 6.2.1.1 of the spec would read:
For every persistent non-collection-valued attribute y declared [or inherited] by class X, where the type of y is Y, the metamodel class must contain a declaration as follows:
public static volatile SingularAttribute<X, Y> y;
But the words "or inherited" just aren't there. I added them in right now.
Now, do the spec and Javadoc leave some ambiguity here, well, yeah, sure they do, and that's a bit unfortunate. But I insist that the most reasonable interpretation is that the JPA metamodel works just like the Java reflection API in this aspect.
So sure, this issue can be solved: properly, by introducing a new operation with the correct signature, in a future version of JPA (i.e. Attribute<X, ?> getActualAttribute(String name);
as above).
IMO, there is no need to introduce a new method getActualAttribute()
, It means I want the actual attribute if I called getAttribute()
, I cannot imagine which use case would want the non-actual attribute.
I repeat my request in a nutshell:
Wish other developers state their opinions here too.
Given
Then
Should return
Owner.class
instead of erased typeObject.class
see https://github.com/hibernate/hibernate-orm/pull/7630#discussion_r1427790370