clarify usage of @OrderColumn and @MapKeyColumn

[gavinking]

I've realized that there's huge confusion here, because the Javadoc for these annotations fails to mention an important thing.

[gavinking]

Damn, the TCK actually has tests which test the use of these annotations on unowned collections, where the order or key column doesn't exist at all in the other entity, creating a requirement for "partially owned" collections. I really think this is an incredibly bad feature, typically resulting in inefficient SQL, and that we should strongly consider deprecating it.

Thoughts, everyone?

[gavinking]

I mean, the spec also says:

The owning side of a relationship determines the updates to the relationship in the database.

and:

It is the developer's responsibility to keep the in-memory references held on the owning side and those held on the inverse side consistent with each other when they change. ... It is particularly important to ensure that changes to the inverse side of a relationship result in appropriate updates on the owning side, so as to ensure the changes are not lost when they are synchronized to the database.

This notion of a "partially owned" relationship just doesn't exist in the text of the specification, as far as I can tell. It's a pure invention of the TCK.

[beikov]

The @OrderColumn probably doesn't make a lot of sense on the inverse/unowned side, since the order will be determined based on the owning side, so +1 for that clarification.
The @MapKeyColumn might make sense on the inverse/unowned side though, so -1 on changing that text.

[gavinking]

The @MapKeyColumn might make sense on the inverse/unowned side though

It only makes sense if the child entity has a field holding the value of the map key. In which case, you should use @MapKey to do the mapping, not @MapKeyColumn.

[beikov]

It seems to me that with a @ManyToMany where the map key column is placed on the join table, it makes sense to map the inverse side with

@MapKeyColumn(name = "map_key")
@ManyToMany(mappedBy = "theMap")

[gavinking]

It seems to me that with a @ManyToMany where the map key column is placed on the join table

I mean, OK, in principle yes ... but that "map key column" would then wind up being a unique key of the join table. I doubt any JPA implementation handles this mapping elegantly (Hibernate doesn't).

If some JPA implementation wants to support such mapping, that's fine, it can. But I don't think the spec should require support for that.

(I'm assuming you mean that it's a bidirectional many-to-many with a Map on both sides.)

[beikov]

I mean, OK, in principle yes ... but that "map key column" would then wind up being a unique key of the join table. I doubt any JPA implementation handles this mapping elegantly (Hibernate doesn't).

Sure, it would form a unique constraint along with the foreign key columns pointing to the owning side. If Hibernate doesn't behave correctly here, that would be a bug that we should simply fix IMO.

If some JPA implementation wants to support such mapping, that's fine, it can. But I don't think the spec should require support for that.

Right, so maybe change the language to say that support for this kind of mapping is non-portable or something like that.

(I'm assuming you mean that it's a bidirectional many-to-many with a Map on both sides.)

Obviously @MapKeyColumn implies usage of Map, so yeah. I'm not sure if it is possible to map the owning side as Map and the unowned side with @ManyToMany(mappedBy = "...") as Set or Collection/List.

[gavinking]

I'm not sure if it is possible to map the owning side as Map and the unowned side with @ManyToMany(mappedBy = "...") as Set or Collection/List.

That is possible, and completely normal.

What is extremely debatable is whether you should be able to have things like a Set on the owning side, but a MapKeyColumn on the unowned side. I claim that all mapping information should be defined on the side which is pointed to by the mappedBy element. There should be no mixing in of mapping info on a field which says it is mappedBy something else.

[njr-11]

Added a few minor suggestions for grammar corrections and copy/paste mistakes. Otherwise, approving in advance.

[gavinking]

Thanks @njr-11, I applied all your suggestions. Not going to merge this for M1, because I still want to get @sebersole's feedback on it, especially since @beikov was a bit skeptical of this one.

[sebersole]

This notion of a just doesn't exist in the text of the specification, as far as I can tell. It's a pure invention of the TCK.

Shocking

[sebersole]

How about we carve out an exception in the clarification for the many-to-many case?
Yes, that would mean that we'd have to "fix" this in Hibernate, but its more of question of correctness I think.

[gavinking]

that would mean that we'd have to "fix" this in Hibernate

Hibernate actually does allow that IIRC, but it uses two SQL statements to set up the association.

[gavinking]

This is not so much about what is possible, but about what the spec requires.

[sebersole]

that would mean that we'd have to "fix" this in Hibernate

Hibernate actually does allow that IIRC, but it uses two SQL statements to set up the association.

Hence "fix" in quotes. "Improve" if you prefer ;)

[beikov]

Just for your information, there seem to be people out there that really use @OrderColumn on unowned collections: https://hibernate.atlassian.net/browse/HHH-11594

[gavinking]

I have weakened the language slightly, so that it's clear that an implementation might allow these annotations on the unowned side, and what we're saying is just that it's unportable.

[gavinking]

I'm going to pull the trigger on this one, because I believe it correctly states the original intent of these annotations.

[gavinking]

I believe it correctly states the original intent of these annotations

And because we have not defined anywhere in the spec how a "partially owned" collection mapping is supposed to behave. (And such a definition is completely nontrivial.)