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(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev

Ok, thanks for looking into this, Steve. Actually I was already working on the 4.0 port and I'll definitely look into the Map sizing and take that into account. Unfortunately I won't have time to fix that this weekend, so I'll probably submit updated / new pull requests for master and 3.6 either on Monday or Tuesday. Regards, Erik-Berndt ________________________________________ Van: hibernate-dev-bounces(a)lists.jboss.org [hibernate-dev-bounces(a)lists.jboss.org] namens Steve Ebersole [steve(a)hibernate.org] Verzonden: vrijdag 8 juli 2011 17:59 Aan: hibernate-dev(a)lists.jboss.org Onderwerp: Re: [hibernate-dev] Patch for HHH-6361 / HHH-6349 : Envers loses audit information due to a Hibernate core bug 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(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev -- steve(a)hibernate.org http://hibernate.org _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev 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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

Hi, I've updated the original pull request (https://github.com/hibernate/hibernate-core/pull/118) to address the Map sizing issue. I've also removed a few debug log statements which could provoke a NPE. I've also done the 4.0 port and added a new pull request for the current master (https://github.com/hibernate/hibernate-core/pull/132) . Regards, Erik-Berndt Scheper ________________________________________ Van: hibernate-dev-bounces(a)lists.jboss.org [hibernate-dev-bounces(a)lists.jboss.org] namens Steve Ebersole [steve(a)hibernate.org] Verzonden: vrijdag 8 juli 2011 17:59 Aan: hibernate-dev(a)lists.jboss.org Onderwerp: Re: [hibernate-dev] Patch for HHH-6361 / HHH-6349 : Envers loses audit information due to a Hibernate core bug 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(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev -- steve(a)hibernate.org http://hibernate.org _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev 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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev