jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
187 stars 55 forks source link

add mappedBy to @ManyToOne and @OneToOne #527

Open gavinking opened 9 months ago

gavinking commented 9 months ago

The couple of weeks I've been thinking about the following sort of code, which is already possible in JPA:

@Entity
class Child {
     @Id 
     Long id;

     Long parentId;

     @ManyToOne 
     @Column(name="parentId", 
             insertable=false, updatable=false)
     Parent parent;

     ...
}

Here the parentId field and the parent association are mapped to the same column.

It seems to me that this sort of thing is not uncommon, but unfortunately that @Column annotation is quite clumsy.

We could make it quite a lot more elegant by allowing the following:

@Entity
class Child {
     @Id 
     Long id;

     Long parentId;

     @ManyToOne(mappedBy=Child.PARENT_ID)
     Parent parent;

     ...
}

This is quite analogous to the use of mappedBy in @OneToMany and @ManyToMany, with the difference that here mappedBy points to a different field of the current entity.

An argument against this features is that it overlaps with @MapsId, and that's true, I can use it this way:

@Entity
@IdClass(ChildId.class)
class Child {
     @Id 
     Long id;

     @Id
     Long parentId;

     @ManyToOne(mappedBy=Child.PARENT_ID)
     Parent parent;

     ...
}

or this way:

@Entity
class Child {
     @Id 
     Long id;

     @OneToOne(mappedBy=Child.ID)
     Parent parent;

     ...
}

to do things that are already possible with @MapsId.

On the other hand, I find @MapsId to be quite confusing in general, and sort-of backwards compared to my intuition. So I actually prefer the above code to the equivalent thing with @MapsId.

Reactions?

t-beckmann commented 9 months ago

Current mappedBy refers to the owning side of a bidirectional relationship. At the database level this is where the key lives.

I would not mix your convenience usecase with it.

gavinking commented 4 months ago

Current mappedBy refers to the owning side of a bidirectional relationship.

It's true that it is currently limited to that specific case.

At the database level this is where the key lives.

And this is also true in my examples above. In those examples, mappedBy points to the attribute which provides the mapping for the FK.

I would not mix your convenience usecase with it.

OK, that's interesting, but do you have any specific reason for why you would not do this or is it just vibes?

t-beckmann commented 2 weeks ago

The way I understand your point is, all these mappedBys define a redundant, secondary attribute.

However in case of a bidirectional relationship the corresponding primary attribute is a member of the other entity. Whereas in your suggested use it is of the entity containing the mappedBy. In a OneToOne what entity the mappedBy refers to?

As a concept, to me the insertable/updatable=false means to suppress the column value in insert and update statements because this is a computed value taken care of by the database. My impression is that using it for a redundant secondary attribute is a bit of a hack. I fully support we should have a better way of expressing this.

Loosely speaking, MapsId means persist the other entity firstly, then copy its id to this entity's id before persisting that. Please excuse my lean interpretation, here the roles of which of the attributes is leading are reversed. The MapsId one is primary and the id becomes the redundant secondary attribute. So it's not a match but like you say there is some overlap.

I like the feature suggested much and see a need for it, but would not reuse the mappedBy annotation attribute for it.

gavinking commented 2 weeks ago

The way I understand your point is, all these mappedBys define a redundant, secondary attribute.

Yes.

However in case of a bidirectional relationship the corresponding primary attribute is a member of the other entity. Whereas in your suggested use it is of the entity containing the mappedBy.

That is correct.

As a concept, to me the insertable/updatable=false means to suppress the column value in insert and update statements ...

That's all it means.

because this is a computed value taken care of by the database.

I mean, not necessarily. And it doesn't even really work terribly well for that.

My impression is that using it for a redundant secondary attribute is a bit of a hack.

That's not a hack, it was always intended to be used in that way. (On the other hand, it's not terribly ergonomic.)

MapsId means persist the other entity firstly,

Actually it doesn't imply that. You are supposed to persist the other entity for yourself.

then copy its id to this entity's id before persisting that.

It does mean this.

But more importantly it means that the mapping for the id field is determined by the mapping of the association annotated @MapsId.

Which, as I said, always seemed pretty backwards to me. I would prefer if the association was derived from the id, not the other way around as with @MapsId.

but would not reuse the mappedBy annotation attribute for it.

So I think I understand that the resistance to this is that people understand mappedBy to always point to a field of a target entity, rather than to a field of the current entity.

But in fact it's perfectly normal in JPA for a mapping to have the "opposite" interpretation when it occurs on a @OneToMany. The same thing happens with @JoinColumn. @OneToMany @JoinColumn(name="fk") names a column belonging to the target entity, not to the annotated entity.