[Mifos-developer] issue 1512 persistent object equals/hashCode - data model clarification needed

[Sam Lee]

Hi,

I'm looking into issue 1512, the equals/hashCode issue identified from findbug.

To ensure I understand the business requirement / data model, could
someone clarify whether the following is true or not?

1. For each loan product, it may be associated with multiple fees. (I
guess fees the product could apply)
2. However, each loan product will be associated with the same fee at
most once. In other words, in mysql,
table prd_offering_fees should have a unique constraint on
(prd_offering_id, fee_id)

You can also see the attached patch on LoanOfferingBOTest that
reflects the above assumption. Or you can see the actual test right
below (rather than going through the patching).

- sam

A new test method in LoanOfferingBOTest.java (equivalent to the attached patch)

public void testBuildloanOfferingWithDuplicateFeeXXX() throws SystemException,
ApplicationException {
createIntitalObjects();
Date startDate = offSetCurrentDate(0);
LoanOfferingBO loanOffering = new LoanOfferingBO(TestObjectFactory
.getContext(), "Loan Offering", "LOAN", productCategory,
prdApplicableMaster, startDate, interestTypes,
new Money("1000"), new Money("3000"), 12.0, 2.0, 3.0,
(short) 20, (short) 1, (short) 12, false, true, false,
frequency, principalglCodeEntity, intglCodeEntity);

FeeBO fee = TestObjectFactory.createOneTimeAmountFee("Loan One time ",
FeeCategory.LOAN, "100", FeePayment.UPFRONT);

LoanOfferingFeesEntity loanOfferingFees1 =
new LoanOfferingFeesEntity(loanOffering, fee);
loanOffering.addPrdOfferingFee(loanOfferingFees1);

// another fee for the proudct: which refers to the same fee
// as the one above
LoanOfferingFeesEntity loanOfferingFees2 =
new LoanOfferingFeesEntity(loanOffering, fee);
loanOffering.addPrdOfferingFee(loanOfferingFees2);

assertEquals("sam: I believe the business requirement is that the
loan should not have fees that are essentially the same.",
1, loanOffering.getLoanOfferingFees().size());

assertEquals("sam: I belive these two offering fees are considered
identical from business requirement",
true, loanOfferingFees1.equals(loanOfferingFees2));
}

[Tom Bostelmann]

Sam, just wanted to let you know that this is on my radar - I'm a bit swamped right now with Van away.  But it's on my list to respond to - probably Monday before I get to it, though.  Sorry for the delay and it's great to see you digging into the code :)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[Tom Bostelmann]

This is a great question.  The additional test fails - which it should because LoanOfferingFeesEntity (bad use of plural 'Fees' in name aside) represents an instance of a fee that is associated with the loan product.

Here's my attempt at a business example (Aliya/Emily/Beth/Amy/... please step in to correct me if I'm wrong):

An example situation could be where the loan officer sets up a fee to cover the costs of providing a consultation visit (this is represented by the FeeBO).  The loan officer has decided that a certain loan product requires two such visits (two instances of LoanOfferingFeesEntity using the same FeeBO).  Therefore there should be two different instances of the same fee associated with this loan product.

Does that make sense?

On Nov 14, 2007 11:12 PM, Sam Lee <orio...@gmail.com> wrote:

[Sam Lee]

Tom,

I think I know what you mean but just to make sure: are you saying

1. A relationship such as the following is valid? (a poorman ascii art)
                     |---LoanOfferFeesEntity(id6)---|
LoanOfferingBO(id1) --                              ---- FeeBO(id2)
                     |---LoanOfferFeesEntity(id5)---|
  
2. If  (1) is correct, are we also saying that (in ER term), the LoanOfferFeesEntity (backed by  prd_offering_fees table) has no business key?

Thanks.


- sam

> 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/
>

[Aliya Walji]

>> Here's my attempt at a business example (Aliya/Emily/Beth/Amy/... please step in to correct me if I'm wrong):

 

I think you’re correct, Tom, on the functionality.  According to the FS “A particular fee instance can be applied multiple times to a customer account”.  Checking out this functionality in the product also verifies this behavior.  I can create a loan account with two of the same fee type applied to the account.

 

---

[Sam Lee]

Tom / Aliya,

 

Let me try again. What I'd like to get a clarity is whether LoanOfferFeesEntity has a business key or not. The answer will impact the correct equals() / hashCode() implementation for the Java object .

 

I'll try to map the Java class in question to the actual UI and database tables to make sure we're on the same page.  To the extent I could map, it looks like LoanOfferingBO refers to a Loan Product (rather than individual loan account). The UI (and the current ER data model) seems to imply a LoanOfferingBO (aka loan product on UI) can be associated with a FeeBO (aka Fee type on UI) at most once.

 

 

The java classes in this context include:

                     |---LoanOfferFeesEntity(id6)---|
LoanOfferingBO(id1) --                              ---- FeeBO(id2)
                     |---LoanOfferFeesEntity(id5)---|

 

1. Could someone reconfirm if my understanding below is correct?

1a. A LoanOfferingBO (maps to prd_offering table + loan_offering table) instance maps to a Loan Product (rather than a Loan account) as in the the main object to be created in the Loan product definition screen:

http://test.mifos.org:8085/mifos/loanproductaction.do?method=load&recordOfficeId=1&recordLoanOfficerId=1&randomNUm=-1022391167983022447

 

1b. A LoanOfferFeesEntity (map to prd_offering_fees table) instance maps to the association between a fee type and the loan product (basically the fees section of the Loan Product definition screen above)

 

1c. A FeeBO (maps to fees table) instance maps to individual Fee Type.

 

 

2. If my understanding as in (1) is correct,

2a. Mifos web UI does not allow a loan product to be associated with a fee multiple times (it's a simple multi-selection form).

2b. Using the example Tom provided earlier on, I don't see anywhere in the loan product definition screen (loanproductaction.do) allowing the loan officers to define that a certain loan product require two consultation visits (and hence two consultation visit fees).

2c. in ER term, the LoanOfferFeesEntity (backed by  prd_offering_fees table) has a business key of (product_id, fee_id).

 

 

Thanks for the clarification in advance. Thanks.

 

 

- sam

[Tom Bostelmann]

Ah, okay.  So basically, the UI doesn't allow you to create multiple "relationships" [LoanOfferFeesEntity] to the same fee.  So, you're wondering if that means that changes whether or not two instances of LoanOfferFeesEntity with the same Fee are, in fact, distinct.  Is this correct?

Aliya, you may correct me on this, but I think that it doesn't.  Even though the UI doesn't fully support the data model, I think the data model needs to support two distinct relationships to the same fee.

Does that answer your question, Sam?

(good question btw ;)
-Tom

[Sam Lee]

I think the ball is now back to Aliya to clarify the data model.

Tom - you're correct  on what I'm asking. I'm asking for clarification because Aliya's previous reply seems to refer to actual Loan Accounts, rather than Loan Products. So I want to double check just to make sure.

[Amy Bensinger (Contractor)]

Just a reminder—Aliya is in the field (without any internet connectivity)  and will not be able to respond for a few more days.

 

---

[Emily Tucker]

Hi Folks,

 

I'll try to take a crack at this while Aliya's out.  I wasn't able to follow the entire thread, and I think some of this was already figured out, but hopefully my response will be of some use:

 

(*) Loan products, through the UI, can only have 1 fee type assigned to them (not sure how this is structured on the back-end).  Back when building 1.0, we discussed whether/not to allow for the same fee to be attached twice to a loan product-- and decided to prevent this thinking that it might lead to errors.  If there are two consultation visits, and a fee charged for each visit, then the MFI would create two fee types:  Consultation Fee 1, Consultation Fee 2-- and then apply those individually to the loan product.  [But I actually dont think that this is a common scenario]  From a functional perspective-- I don't think it really matters whether/not we limit the number of times a fee can be attached to a loan product.  If there are good tech reasons to allow a fee to be attached multiple times to a loan product-- then that's fine.  Sounds like you want to do this in the datamodel, even if we don't enable in the UI, and that's fine.

 

(*) Loan accounts:

A loan account can have the same fee applied to it multiple times.  For example: an MFI might define a "late fee" and apply this fee to the loan account whenever the loan is late.

 

Hope this helps,

Emily.

 

 

---

From: mifos-devel...@lists.sourceforge.net [mailto:mifos-devel...@lists.sourceforge.net] On Behalf Of Sam Lee
Sent: Tuesday, November 27, 2007 6:59 PM

[Tom Bostelmann]

Thanks Emily, that's the background we needed.

Sam, there's a couple ways to deal with this.

1.) Refactor the equals/hashCode to assert that only one fee can be associated with a Loan Product - after thinking about this, I'm concerned that this could be confusing since the data model implies that you can have multiple "relationships" to the same fee.

2.) Remove the LoanOfferFeesEntity or have it be an extension of FeeBO (maybe adding a Loan Product Id column and putting a unique constraint on it?) - i'm not sure I like this idea either since we don't have a specific reason for constraining Loan Products in this way - it's a lot fo change for no specific reason.

3.) Leave the data model as is with the understanding that you can associate the same FeeBO to a loan product.

Unless others object, I'd like to stick with #3 (especially since it doesn't require any work! :)

-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper
from Novell.  From the desktop to the data center, Linux is going
mainstream.  Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4

[Sam Lee]

Thanks Emily for the background as well!

 

Tom: Option 3 sounds reasonable to me too, with one caveat that the current code as-is still has a (lower-priority) bug on hashCode(). The issue is that a LoanOfferingFeesEntity entity (as in a row in DB) could have different hashCode, say, when loaded from different hibernate sessions. The bug is unlikely to cause any real problem at this point, but could lead to nasty issues at some point as the codes evolve.

 

I can get to it at some point in December: I'm also traveling now so it's harder to do any real code change.

 

To close on issue 1512,

1.  I can fix the LoanOfferingFeesEntity hashCode()

2. Apply similar fix to LoanOfferingFundEntity and PrdOfferingMeetingEntity : I believe they are analogous to LoanOfferingFeesEntity

3.  The remain open question is OfficeBO - does OfficeBO have a natural business key? Again the decision will affect the correct equals() / hashCode() implementation.

 

- sam

[Sam Lee]

Hi,

Attached please see a in-progress patch for issue 1512. To bring you back to the context, I think what we need to do is:

1. Given the understanding of LoanOfferingFeesEntity's unique constraint, fix the hashCode() impl.

Status: the patch tries to expose the existing flaw on the hashCode. It does not contain a bug fix yet.


What is not covered in this patch:

2. Apply similar fix to LoanOfferingFundEntity and PrdOfferingMeetingEntity : I believe they are analogous to LoanOfferingFeesEntity

3.  The remain open question is OfficeBO - does OfficeBO have a natural business key? Again the decision will affect the correct equals() / hashCode() implementation.

- sam

P.S. The patch is not done against the latest trunk code as I still have issues in trying to getting an update. The meat is really a brand new test method in LoanOfferingBOTest.java, copied out here for inspection convenience

    public void testUpdateWithDuplicateFeeXXX() throws SystemException,
        ApplicationException {
   
        // test setup: save a loan with one fee associated. 
        //

        createIntitalObjects();
        Date startDate = offSetCurrentDate(0);

        LoanOfferingBO product = new LoanOfferingBO(TestObjectFactory

                        .getContext(), "Loan Offering", "LOAN", productCategory,
                        prdApplicableMaster, startDate, interestTypes,
                        new Money("1000"), new Money("3000"), 12.0, 2.0, 3.0,
                        (short) 20, (short) 1, (short) 12, false, true, false,
                        frequency, principalglCodeEntity, intglCodeEntity);
   
        FeeBO fee = TestObjectFactory.createOneTimeAmountFee("Loan One time ",

                                FeeCategory.LOAN , "100", FeePayment.UPFRONT);
   
        LoanOfferingFeesEntity loanOfferingFees1 =
                new LoanOfferingFeesEntity(product, fee);
        product.addPrdOfferingFee(loanOfferingFees1);
   
       
        product.save();
        HibernateUtil.commitTransaction();
        HibernateUtil.closeSession();
       
        // test setup done
       
        // test body: load the same loan obejct from two different hibernate session
       
        LoanOfferingBO productLoad1 =
            (LoanOfferingBO) TestObjectFactory.getObject(
                LoanOfferingBO.class, product.getPrdOfferingId());
        // try to get the one-and-only-one loanOfferingFee
        LoanOfferingFeesEntity loanOfferingFeeLoad1 = null;
        for (LoanOfferingFeesEntity lofe : productLoad1.getLoanOfferingFees()) {
            loanOfferingFeeLoad1 = lofe; // should run only once.
        }
        HibernateUtil.commitTransaction(); // nothing to be commited but just to be safe
        HibernateUtil.closeSession();
       
       
        LoanOfferingBO productLoad2 =
            (LoanOfferingBO) TestObjectFactory.getObject(
                LoanOfferingBO.class, product.getPrdOfferingId());
        // try to get the one-and-only-one loanOfferingFee
        LoanOfferingFeesEntity loanOfferingFeeLoad2 = null;
        for (LoanOfferingFeesEntity lofe : productLoad2.getLoanOfferingFees()) {
            loanOfferingFeeLoad2 = lofe; // should run only once.
        }
        HibernateUtil.commitTransaction(); // nothing to be commited but just to be safe
        HibernateUtil.closeSession();
       
        // expectation: the two LoanOfferingFeesEntity objects
        // 1. should be the same (as of equals),
        // 2. should have the same hashCode
        // 2a. operation using hash-based set should work normally.
       
        assertEquals("The two loanOfferingFeesEntity objects should have the same identity", loanOfferingFeeLoad1, loanOfferingFeeLoad2);       
 
        // sam: failing with current hashCode impl
        assertEquals("Since the two are equal, their hashCode should be equal per contract", loanOfferingFeeLoad1.hashCode(), loanOfferingFeeLoad2.hashCode ());
       
        final int numLoanOfferingsFeeBefore = productLoad2.getLoanOfferingFees().size();
        productLoad2.addPrdOfferingFee(loanOfferingFeeLoad1);       
        final int numLoanOfferingsFeeAfter = productLoad2.getLoanOfferingFees().size();
        // sam: failing with current hashCode impl
        assertEquals("Since the two are equal, adding one to one to the other's parent (effectively adding to a set) should be a no-op, ie, not changing the size",
                numLoanOfferingsFeeBefore, numLoanOfferingsFeeAfter);
   
    }

   

    public void testUpdateWithDuplicateFeeXXX() throws SystemException,
        ApplicationException {
   
        final short pid = 229;
        product = (LoanOfferingBO) TestObjectFactory.getObject(LoanOfferingBO.class, new Short(pid));
        TestObjectFactory.removeObject(product);
        product = null;
   
        // test setup: save a loan with one fee associated. 
        //

        createIntitalObjects();
        Date startDate = offSetCurrentDate(0);

        LoanOfferingBO product = new LoanOfferingBO(TestObjectFactory

                        .getContext(), "Loan Offering", "LOAN", productCategory,
                        prdApplicableMaster, startDate, interestTypes,
                        new Money("1000"), new Money("3000"), 12.0, 2.0, 3.0,
                        (short) 20, (short) 1, (short) 12, false, true, false,
                        frequency, principalglCodeEntity, intglCodeEntity);
   
        FeeBO fee = TestObjectFactory.createOneTimeAmountFee("Loan One time ",

                                FeeCategory.LOAN , "100", FeePayment.UPFRONT);
   
        LoanOfferingFeesEntity loanOfferingFees1 =
                new LoanOfferingFeesEntity(product, fee);
        product.addPrdOfferingFee(loanOfferingFees1);
   
       
        product.save();
        HibernateUtil.commitTransaction();
        HibernateUtil.closeSession();
       
        // test setup done
       
        // test body: load the same loan obejct from two different hibernate session
       
        LoanOfferingBO productLoad1 =
            (LoanOfferingBO) TestObjectFactory.getObject(
                LoanOfferingBO.class, product.getPrdOfferingId());
        // try to get the one-and-only-one loanOfferingFee
        LoanOfferingFeesEntity loanOfferingFeeLoad1 = null;
        for (LoanOfferingFeesEntity lofe : productLoad1.getLoanOfferingFees()) {
            loanOfferingFeeLoad1 = lofe; // should run only once.
        }
        HibernateUtil.commitTransaction(); // nothing to be commited but just to be safe
        HibernateUtil.closeSession();
       
       
        LoanOfferingBO productLoad2 =
            (LoanOfferingBO) TestObjectFactory.getObject(
                LoanOfferingBO.class, product.getPrdOfferingId());
        // try to get the one-and-only-one loanOfferingFee
        LoanOfferingFeesEntity loanOfferingFeeLoad2 = null;
        for (LoanOfferingFeesEntity lofe : productLoad2.getLoanOfferingFees()) {
            loanOfferingFeeLoad2 = lofe; // should run only once.
        }
        HibernateUtil.commitTransaction(); // nothing to be commited but just to be safe
        HibernateUtil.closeSession();
       
        // expectation: the two LoanOfferingFeesEntity objects
        // 1. should be the same (as of equals),
        // 2. should have the same hashCode
        // 2a. operation using hash-based set should work normally.
       
        assertEquals("The two loanOfferingFeesEntity objects should have the same identity", loanOfferingFeeLoad1, loanOfferingFeeLoad2);       
   
        assertEquals("Since the two are equal, their hashCode should be equal per contract", loanOfferingFeeLoad1.hashCode(), loanOfferingFeeLoad2.hashCode());
       
        final int numLoanOfferingsFeeBefore = productLoad2.getLoanOfferingFees().size();
        productLoad2.addPrdOfferingFee(loanOfferingFeeLoad1);       
        final int numLoanOfferingsFeeAfter = productLoad2.getLoanOfferingFees().size();
        assertEquals("Since the two are equal, adding one to one to the other's parent (effectively adding to a set) should be a no-op, ie, not changing the size",
                numLoanOfferingsFeeBefore, numLoanOfferingsFeeAfter);

[Tom Bostelmann]

Quick response:

1.) I think we agreed that you're fixing something that isn't broken :P  We aren't going to assert that you can have only one instance of LoanOfferingFees with the same Fee.  So, I'm afraid this patch is moot.

2.) In the future make sure you apply patches in the correct form.  I understand that you're having trouble with checking out code and I appreciate your time.  Unfortunately we can't sustain patch review without this criteria.

3.) In reviewing the code, you should remove HibernateUtil.commitTransaction() and HibernateUtil.closeSession(). Add a try/catch block with a final statement that rolls the transaction back instead.  These processes add a significant amount of time to the test and aren't necessary (yes, I know it's wide-spread in the code).  See 'TestLoanBORedoDisbursal' for an example of how to do this.

Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[Sam Lee]

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