Hi,
Is there a reason why n.s.h.collection.Bag does not implements the equals and hashcode methods (as j.u.List implementations does in jdk) ?

Emmanuel

A bag is not really a list, hence we don't need to implement the whole contract.

Ok but is it inapropriate to compare 2 bags and saying that having a proper implementation of bag.equals and bag.hashcode ?

My underlying question is "Can I implement an equals and hashcode method, with respect to the bag semantic w/o breaking hibernate way of working ?"

I'm ready to do it.

I would rather that bag had no equals()/hashCode(), since I don't like the idea that equals() forces the collection to be initialized.

But you did it on the set implementation, probably for the same purpose, no ?



I don't wan't to disturb you ;-), I just think using bag equals right now can be very confusing, and try to convince you or being convinced by you.

Emmanuel

In the case of a Set, we need it to satisfy the Set contract.

OK Gavin, take a big icy coke and answer this post ;-)
Bag.equals method isn't used that much (assuming the current one is buggy - at least have a strange behavior).
Don't you think a programmer calling myCollection.equals(anotherOne) understand that he needs to load it and thus assumes that perf ussue ?

OK I did the patch for the 2.0.3 version (can't have access to head right now)

http://opensource.atlassian.com/projects/hibernate/secure/ViewIssue.jspa?key=HB-375

I don't think bag.equals is widely used, so the read() call won't be a issue in common usage, and Object.equals semantic is now more appropriate.

Hope you guys will accept it.

Emmanuel

Well, I'm not absolutely certain.

I always thought it was a really crap to have persistent collections getting initialized when they were used in other collections, etc.

I -could- be convinced to change my mind on this, but other people would also need to request it. (I don't make changes that break existing code lightly.)

Actually, I agree with you. Blind usage of collection comparison is bad thing in persistent fwk, and I advise not to do it (the 'understand what yo're doing' advice). But the proper answer of the underlying framework is to:
- avoid it by throwing an exception during equals call
- implementing it properly and document it as a bad usage.

Really, it is. My collegue just spent the better part of an afternoon figuring out why removal of an object from a list was failing and we found that Bag indeed does not implement equals, and, unfortunately, nowhere in Javadocs is it documented that Bag partially implements the Collections framework contracts.

Apart from the fact that documentation does not make this clear, the issue is that Bag is silently failing its contract, which is never good behavior. I agree with Emmanuel; this should either be overriden to throw an exception (in which case Bag would still be failing its contract but at least making some noise about it so hapless newbies like ourselves can work around quickly) or implemented with a note of caution.

The bottom line is, this omission breaks required behavior for the sake of an optimization, with no way of overriding it and no documentation to warn inexperienced users.

BTW, the performance reasoning you give only applies to lazily loaded collections. If the collection is not lazy loading (the *default* behavior) there is no additional overhead.

Note that this is legacy and we advise you to use lazy loading for collections by default whenever possible.

If it is intended functionality, it is, by definition, not a bug.

I think it is a stretch to call not implementing Bag.equals() intended functionality; it has to be actually useful before we can do that, not to mention being documented. It seems to me that comparing two fully initialized collections in your entity model should always just work, you don't expect the meaning of collection equality to be broken when you have fully fetched all necessary data. A sensible implementation of Bag.equals() can throw an exception when either collection is not initialized (or return false to keep the current behavior) but it should work correctly and as expected otherwise.

Just Apply my patch, it should still work even on Hibernate 2.1.
I'll add a comment on the Javadoc to make it more clear.