[hibernate-issues] [Hibernate-JIRA] Closed: (HHH-1021) OneToManyPersister issues redundant update in "insertRows" when using non-nullable JoinColumn

OneToManyPersister issues redundant update in "insertRows" when using non-nullable JoinColumn --------------------------------------------------------------------------------------------- Key: HHH-1021 URL: http://opensource.atlassian.com/projects/hibernate/browse/HHH-1021 Project: Hibernate Core Issue Type: Improvement Components: core Environment: Hibernate 3.1beta3, annotations 3.1beta5, MSSQL Reporter: John Priority: Minor I have a parent/child relationship that is unidirectional, mapped by the parent side, with the foreign key field in the child table. (The docs say this isn't recommended, but no reason is given, so I'm doing this because it fits what I need.) This relationship works, but the way the sql is generated varies depending on two factors: the nullability of the join column (@JoinColumn), and the cascade settings. Consider code like this (each step a new session): 1. save new parent and child 2. remove old child (Set.clear) and add new one 3. remove parent If the join column is nullable, the somewhat ungainly set of sql is executed: 1. insert into parent (id, prop) insert into child (id, prop) update child set parent_id = ? where id = ? 2. insert into child (id, prop) (add new child) update child set parent = null where id = ? (remove old child) update child set parent = ? where id = ? (associate new child to parent) delete from child where id = ? (only if cascade delete orphan) 3. update child set parent = null where id = ? delete from parent where id = ? delete from child where id = ? If the join column is not nullable, then it seems that the sql is more efficient at first, but there are two problems. The first problem is that unless cascade delete orphan is specified, clearing the existing set of children on the parent object does not remove the children. The in-memory set won't contain them, but the next time that parent is loaded they will reappear. This makes sense in a way, since the way children are "deleted" is to set the parent reference to null, which can't happen if the column doesn't allow nulls. The persister doesn't issue the "update-to-null" delete, and the child stays attached to the parent. Now, assuming that the set-to-null-and-delete-later method of dealing with children is the best, then I think a documentation addition or warning in the code would be useful to help people avoid this situation. The second problem, and the one relating to the subject of the bug, has to do with the sql generated when the join column is not nullable. Assuming that cascade delete orphan is on for the relationship (which it has to be for it to be usable at all), sql like the following is generated: 1. insert into parent (id, prop) insert into child (id, parent_id, prop) update child set parent_id = ? where id = ? (redundant) 2. insert into child (id, parent_id, prop) (add new child) update child set parent_id = ? where id = ? (redundant) delete from child where id = ? (remove old child) 3. delete from child where id = ? delete from parent where id = ? Now, this is more appealing, firstly because the parent_id column is required in child. Secondly, instead of issuing an update and a delete to get rid of the child, just a delete is issued. However, while the insert has improved, the update to set the parent id is still issued. This doesn't hurt anything, but it is redundant since the parent_id is set on the insert. If this were removed, then the whole situation would be better: one insert to add instead of an insert and update, and one delete to remove instead of an update and a delete. Currently, OneToManyPersister has the following code: protected boolean isRowDeleteEnabled() { return keyIsUpdateable && keyIsNullable; } protected boolean isRowInsertEnabled() { return keyIsUpdateable; } My first thought at removing the redundant update is to add "&& keyIsNullable" to isRowInsertEnabled (so it matches isRowDeleteEnabled). Changing that will positively affect AbstractCollectionPersister.insertRows, removing the redundant sql. However, that value is also used in OneToManyPersister.doUpdateRows also AbstractCollectionPersister.recreate. I don't know exactly how these other uses would be affected by changing the one method. It could be that the change applies to all, or perhaps it should only affect insertRows. Either way, the change should be simple, but someone with deeper knowledge needs to evaluate the impact on the other uses of that value. Thanks John