Hi Tom, Thanks for the review. Two different types of responses, one on the process clarification, and one on the issues itself.
1. Process clarification: 1a. Could you clarify what you mean by "appling patches in the correct form?" Is it a matter of following the general submission process (in my case, I know I bent the rule of syncing with the latest code, which I'll avoid in the future)? http://www.mifos.org/developers/technical-orientation/code-submission-process-1 1b. Thanks for the clarification on using HibernateUtil in the test. In general, do we have somewhere (wiki) that says "if you want to do XXX, see this as examples?" 1c. Sending code to solicit discussion: The work-in-progress patch I sent earlier is NOT meant to be a full patch (in the sense that it's meant to be integrated with the trunk code pending review). Instead, it's meant to be communicating more precisely on where I am for feedback. Is sendng patches the agreed way to do it? 2. Now regarding the issue itself. >From earlier conversation, we all agreed that the business logic should not assert a LoanOfferringFees with the same Fee. That is NOT what the patch is about. What the patch is about is on a more subtle (arguably not urgent for v1.1) issue that the identical (as in java equals) objects do not have "reasonable" hashCode() impl. I'll try to explain it again here: So let's say both LoanOfferingFeeEntity objects lofe1 and lofe2 are both referring to the same persistent entity in DB, with id (aka primary key) 12345. If somehow (unlikely but possible) the caller code tries to add both objects lofe1 and lofe2 in to the same parent product instance: ... loanOfferingBO.addPrdOfferingFee(lofe1); loanOfferingBO.addPrdOfferingFee(lofe2); ... (then somehow persist the product instance) The current behavior is very fragile for two reasons: 1a.With the current loanOfferingBO, both lofe1 and lofe2 will be added to the internal set (which is a hash-based) despite they have lofe1.equals(lofe2), violating the java Set contract. As a case in point, if somehow the loanOfferingBO switches to a non-hash based set implementation (such as TreeSet I think), the behavior will be different in the sense that lofe2 will be NOT be added to the internal set. 1b. Again, based on the current behavior, where both lofe1 and lofe2 are added to the internal set, when hibernate tries to persist it, the behavior is undefined (i.e., could change from releases to releases) because the internal set actually contains two elements referring to the same DB row. Again, the behavior is undefined - It might work for now, but could be a source of subtle nasty bugs in the future, say, when one tries to upgrade hibernate. Related info: http://www.hibernate.org/hib_docs/v3/reference/en/html/persistent-classes.html#persistent-classes-equalshashcode
------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
