Looks good. I added a few minor comments. I'd like to see the Map sizing thing addressed and the 4.0 port done, but I have no problems with these changes.
On Fri 08 Jul 2011 07:32:25 AM CDT, Adam Warski wrote: > As Eric wrote this fix is needed to fix an Envers issue (HHH-6349), however I > don't feel competent to review the patch for hibernate-core. > So maybe someone else could try? :) > > Thanks, > Adam > > On Jul 4, 2011, at 3:20 PM, Scheper, Erik-Berndt wrote: > >> Hi, >> I'd like someone to review my proposed fix of HHH-6361 >> (https://github.com/hibernate/hibernate-core/pull/117) for the 3.6 branch. >> >>> From a Hibernate core perspective, this may seem like a minor issue, but >>> it is the cause of HHH-6349, where Envers forever loses the audit >>> information when objects were added to or removed from a collection. Since >>> there is no way to retrieve this information from the database at a later >>> moment, this is really bad from the Envers (auditing) perspective. >> >> The proposed fix causes both the provided testcase for HHH-6361 (the >> Hibernate core issue) and the testcase for HHH-6349 (the Envers issue) to >> pass by ensuring that after a merge() operation the snapshot value of the >> collection, as obtained by collectionEntry.getSnapshot(), corresponds with >> the database contents. This is a good thing, of course. >> >> However, apart from the possible performance implication (though I'm not >> sure if there's a remedy for this), I am a bit worried about the fact that I >> had to fix a unit test in the Hibernate testsuite >> (org.hibernate.test.manytomanyassociationclass.compositeid.ManyToManyAssociationClassCompositeIdTest) >> to make the 3.6 build succeed. As a rule, that's a bad sign. >> >> What happens is that after the patch this test crashes with an NPE in the >> hashCode() method of MembershipWithCompositeId.Id class. The reason of the >> NPE is that MembershipWithCompositeId now has a non-null "Id" property, with >> a null value of the userId and groupId properties. Even though it was easy >> to fix the test by overriding the deleteMembership()-method in >> ManyToManyAssociationClassCompositeIdTest by setting the "Id" property of >> MembershipWithCompositeId to null, this doesn't feel good to me. >> >> I'd really like to see a fix for HHH-6361 without these changes in >> ManyToManyAssociationClassCompositeIdTest, but I couldn't find one. Any help >> there would be appreciated of course. >> >> After an initial review, I'd be more than happy to provide a fix for the >> Hibernate 4.0.x series, which is also plagued by the same bug. >> >> Regards, >> Erik-Berndt >> >> Disclaimer: >> This message contains information that may be privileged or confidential and >> is the property of Sogeti Nederland B.V. or its Group members. It is >> intended only for the person to whom it is addressed. If you are not the >> intended recipient, you are not authorized to read, print, retain, copy, >> disseminate, distribute, or use this message or any part thereof. If you >> receive this message in error, please notify the sender immediately and >> delete all copies of this message. >> _______________________________________________ >> hibernate-dev mailing list >> hibernate-dev@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev -- st...@hibernate.org http://hibernate.org _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
