@Oskar, @Patrick First - thanks for the feedback.
After reading Patrick's reply I said to myself "No? The hash code isn't changing during the test!". After looking at it again I saw I was wrong... Can't believe I didn't see the gethashcode impl for the KeyEntity I was using in the test (that I sort of copied from Envers test base which was ported from Hibernate Envers code base). Now it makes sense what you were saying. To summarize... * Really not an issue. When hash code (and/or equals) doesn't change, everything works as expected. * IF changing the key (and its hash changes), in NH4 it throws while NH3 it doesn't. Even though it's sort of a breaking change, it's an illegal op so need to fix it (maybe it would be better with a more explicit exception though). I'll rewrite red Envers tests instead. (I'm a bit surprised these ported tests are there in the first place though - AFAIK also in Java, hash of a key is not allowed to be changed after put in a dic.) With great shame I feel sorry wasting your time :(, even though I really appreciate the feedback. Hopefully an Envers version targeting NH 4 now can be released soon. Feel free to reject Jira issue and pull request. All the best, Roger From: [email protected] [mailto:[email protected]] On Behalf Of Patrick Earl Sent: den 22 november 2013 16:57 To: nhibernate-development Subject: Re: [nhibernate-development] NH-3555 In the pull request, the hash code is changing while the entry is in the dictionary. However, I'd agree that aside from the insanity of changing a key inside of a dictionary, the more expected behaviour would be to only end up with one entry. While I'm quite unfamiliar with the dictionary side of NHibernate, I'd suggest a couple minor changes to the code: 1. Use xmapvalue instead of looking up the value again in the second if. 2. In the unit test, assert that the number ends up being 20. Regards, Patrick Earl On Wed, Nov 20, 2013 at 5:54 AM, Roger Kratz <[email protected]<mailto:[email protected]>> wrote: Yes, forgot to mention... In this case the key instance is the very same instance, just modify its data so no changes that affects Equals or GetHashcode. I just pushed my proposal of fix to the old pull request https://github.com/nhibernate/nhibernate-core/pull/229 If you don't agree I can fix Envers tests (there's lot of old ported Java tests currently failing) instead, but I'll be happy to get some input first. From: [email protected]<mailto:[email protected]> [mailto:[email protected]<mailto:[email protected]>] On Behalf Of Oskar Berggren Sent: den 20 november 2013 13:39 To: [email protected]<mailto:[email protected]> Subject: Re: [nhibernate-development] NH-3555 Changing something that affects Equals()/GetHashCode() of an object while that object is the key of a dictionary isn't allowed by the IDictionary contract. What behavior should we expect? /Oskar 2013/11/20 Roger Kratz <[email protected]<mailto:[email protected]>> Hi I reported regression bug 3555 some time ago. This issue currently prevents releasing an Envers version targeting NH 4. I'm sitting trying to fixing the issue myself, but run into an issue whether old behavior is correct or not. What the issue is all about is... var key = new KeyEntity(); var value = new ValueEntity(); var entity = new MapEntity(); entity.SomeMapProp[key] = value; (1) [open session] [begin tran] [save entity] [commit tran] [begin tran] Key.SomeProperty = newvalue; (2) [commit tran] [/close session] ....in other words; changing the key object of an entry in a map/dictionary. In current NH 4 code base, code above throws. I've fixed the problem locally but noticed that my "fix" got another result than NH 3. In NH 3 code above resulted in _two_ entries in the map - both (1) and (2), while my local fix resulted in one entry (only 2). I personally think the behavior in NH 3 was wrong. I'll be glad to fix this issue so a new Envers version could be released together with NH 4 alpha 2 but I need to double check with you first what the "correct" behavior is in this case. Any opinions? All the best, Roger -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]<mailto:nhibernate-development%[email protected]>. For more options, visit https://groups.google.com/groups/opt_out. -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]<mailto:[email protected]>. For more options, visit https://groups.google.com/groups/opt_out. -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]<mailto:nhibernate-development%[email protected]>. For more options, visit https://groups.google.com/groups/opt_out. -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]<mailto:[email protected]>. For more options, visit https://groups.google.com/groups/opt_out. -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/groups/opt_out.
