I have a case of a Many-To-Many relation where the join table has extra attributes. This required me to map the join table as a separate entity with a composite, embeddable id. Basic entity mapping is displayed in the diagram and code snippet below:
In order to make it behave like a classical many-to-many, I activated cascades and orphan delete on "mappings" relation in parent. For a particular application need, I excluded PERSIST and SAVE_UPDATE from cascades. Since parent is the owner of the relation, I expect to be able to persist parent and child first to assign their ids, then persist the mapping without any issues using the following code:
Currently, depending on whether the Parent or the Mapping have a version attribute and whether session.saveOrUpdate() or session.persist() is called, this operation may throw an exception about Mapping instance being transient when parent is saved:
The exception is occurring in delete orphan code of cascade, as stack trace above indicates, even though we are persisting a new entity and delete orphan case should not apply.
Test case is attached showing different scenarios:
Parent has a version attribute: code works
Parent has no version attribute, mapping has no version attribute: code works
Parent has no version attribute, mapping has version attribute: code fails with exception above
Parent has no version attribute, mapping has version attribute, persist is used instead of saveUpdate: code works
In my opinion the structure of the entities should not block this operation from failing in certain circumstances.
I debugged quite extensively the internals of Hibernate and here is what's happening:
Orphan delete code is protected by the following conditions in Cascade.java:545
Moreover, this code is executed only if collection is actually cascaded in Cascade.java:101
Different circumstances in the test cases make it that Hibernate never enters the delete orphan code and persistance works:
When parent has a version attribute, the code in AbstractSaveEventListener.java:265
makes that the method visitCollectionsBeforeSave is never called and the initial HashSet is not wrapped into a PersistentCollection. This in turn makes deleteOrphans check above return false
When parent has no version, the method visitCollectionsBeforeSave will create a PersistentSet on the mappings collection and the cascade will enter the delete orphan verification. However, because mapping has no version attribute, the method ForeignKeys.isTransient returns false and exception is never thrown
When mapping has a version attribute, the method ForeignKeys.isTransient returns true which causes an exception in delete orphan code
Finally, when persist is used instead of saveOrUpdate, the condition style.doCascade( action ) returns false and neither cascade nor delete orphan check is executed. This is because for action saveOrUpdate, the presence of cascade style DELETE_ORPHAN triggers cascade, where as for action persist only the cascade style PERSIST is necessary.
And while the following case is not included in the attached test case, I would like to also mention that if all ids use primitive types int rather than nullable Integer wrappers, the method ForeignKeys.isTransient never returns true and the exception is not thrown.
So my final observation is:
Given the data model above, the behaviour should be consistent whether version attribute is present or not or whether primitive types or wrapper types are used.
Because of these insignificant differences, the code instead branches in different places and one of the branches causes an exception that should not be thrown
I do not have the exact suggestion on how to fix this given the extensive code base that is involved; however, my intuition here is that for all 4 use cases the delete orphan verification should not be executed. There is got to be a better way to validate that we are persisting for the first time than to verify that the mapping collection is an instanceof PersistentCollection
Thanks for taking a look and your comments regarding my report. I do however find myself in disagreement with your assessment and PR.
My principal argument of the issue is that all the provided test cases should pass not fail. Your change instead made the thrown exception the expected outcome. If this change makes it to the next release of Hibernate, I can tell you that the application which originally forced me to investigate will fail miserably. We are currently walking around the exception by making the parent versioned which does not execute delete orphan code and thus everything works. If now, even with versioned parent the exception is thrown, I’m worse off than I was before I created this ticket.
What I discovered is multiple issues not one:
First, there is an issue of versioned vs unversioned parent causing delete orphan code to be executed or bypassed which your PR fixed indeed. And I agree with this change, whether there is version or not, there should not be different branching.
However, your PR does not address the second issue: versioned vs unversioned mapping and in my opinion this difference is important. I can’t see why a presence or absence of version should cause exception to be either thrown or not. It’s the same argument as with versioned vs unversioned parent. You removed the test where the exception is not thrown (which in my opinion is a good behaviour) and kept the test where exception is thrown and made it the expected behaviour (which I believe is not a good behaviour). The only difference between the two is the presence of version attribute on mapping. I think both of these cases should not throw an exception.
And third, the reason I brought forward persist operation is also for consistency. I make a new entity persistent. Without knowing the code of Hibernate, I can reasonably expect that if I use session.persist and session.saveUpdate it will do the same thing. I explicitly do not cascade either operation in my mapping. Yet, because I declared deleteOrphan=true, the cascade code gets executed in saveUpdate anyway causing the delete orphan side effect in the process. To me, whether I use saveUpdate or persist, if I exclude it from cascade it should not execute any of the cascade code on a new entity. I understand why, due to deleteOrphan=true, saveUpdate might need to execute delete orphan code for an update operation, but it does not need to cascade at all for save operation which is what I was doing.
And finally, I want to emphasize to please not take this critique the wrong way. I very much appreciate you taking the time to analyze it and go through a lot of text and code and attempt a fix. I just want to find a solution that is acceptable for this issue. I guess we need to start by debating why you think the exception should be thrown?
Yeah, this ticket might expose multiple issues. I will investigate further to fix other issues (more important).
It turns out the root cause of the unexpected exception is code logic sloppiness while detecting whether new collection is newly instantiated as following code snippet in Cascade#cascadeCollectionElements():
Note that child is always of PersistentCollection type so literally we lack any means to guard against the issue your ticket exposed.
I updated my PR to fix the loophole. Thanks for your bug reporting.
Thank you, I agree with your fix of changing the condition on delete orphan check.
Thanks for your excellent bug reporting