borinquenkid commented on PR #16532:
URL: https://github.com/apache/grails-core/pull/16532#issuecomment-6018705848
> The fix is right, but it changes runtime behavior for existing mappings,
so I don't think it can go into `8.0.x` (`8.0.0` is staged, so this would ship
in `8.0.1`) or into `8.1.x` either: our versioning policy only allows breaking
changes in a major.
>
> I ran the same domain (`Left`/`Right`, `hasMany` on both sides, no
`belongsTo`) against `8.0.x` and this branch:
>
> `8.0.x` this PR
> Join tables `left_rights` + `right_lefts` `left_rights`
> Join table keys 2 FKs, one column nullable, no PK 2 FKs, both
columns not null, composite PK
> Deleting a linked `Right` (the non-owning side) OK
`DataIntegrityViolationException`
> Once rows are actually written, deleting an entity on the non-owning side
fails unless the application first removes it from the owner's collection. That
is correct many-to-many behavior, but code that works today will start throwing
after the upgrade. `dbCreate: update` also won't add the PK or change
nullability on existing tables, so upgraded and fresh schemas differ.
>
> A few more points on the approach:
>
> * Overriding `isOwningSide()` on `HibernateManyToManyProperty` reaches
beyond the binder. `Association` derives its default cascade operations from it
(`ALL` instead of `PERSIST` for the chosen side), and it also changes
`cascadeValidate: 'owned'` and `DirtyCheckingSupport`. The description says
cascade defaults are unchanged, which holds for the Hibernate cascade from
`CascadeBehaviorFetcher` but not for these.
> * The owner is chosen by `getOwner().getName()`, the fully qualified class
name, so moving a class to another package can flip the owner and with it the
join table name.
>
> My suggestion:
>
> 1. For `8.0.x`: only log a warning at startup when neither side declares
`belongsTo`, telling the user to add it. The mapping stays as it is. I've
opened the same warning for Hibernate 5 against `7.0.x` in #16542; it reaches
the H5 module in 8.0 through the merge forward, so H7 needs the same warning
here.
> 2. For `9.0.x`: fail at startup for this mapping instead of choosing an
owner. That is explicit, matches the docs ("having a `belongsTo` on the owned
side") and Grails 2 behavior, and avoids the package-rename problem. The
fixtures that use this shape would need `belongsTo`, which is fine in a major.
If we prefer choosing an owner instead, that should also go to `9.0.x`, with an
upgrade note covering the delete behavior and the schema difference.
Done as suggested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
