bugfix for Issue 221

[jprichter]

fix for #221 from @dsurfleet

---

This change is 

[stohn777]

Please add a test covering this scenario for regression-prevention sake.

[jprichter]

Please add a test covering this scenario for regression-prevention sake.

@stohn777 unit test added here

[stohn777]

@jprichter
The new test DOES NOT fail with the preceding version of fflib_SObjectUnitOfWork.

[jprichter]

@jprichter The new test DOES NOT fail with the preceding version of fflib_SObjectUnitOfWork.

Correct. One way to get it to fail before the change and pass after is to commit to the database instead of using the MockDML. Figured that wouldn't fly.

[ImJohnMDaniel]

@jprichter The new test DOES NOT fail with the preceding version of fflib_SObjectUnitOfWork
Correct. One way to get it to fail before the change and pass after is to commit to the database instead of using the MockDML. Figured that wouldn't fly.

@jprichter, I am a bit confused then by your comment. I would expect that the test case, applied to the current version of the codebase (not this PR branch) would fail. Is that a correct assumption or am I missing something?

[jprichter]

@jprichter, I am a bit confused then by your comment. I would expect that the test case, applied to the current version of the codebase (not this PR branch) would fail. Is that a correct assumption or am I missing something?

@ImJohnMDaniel , the scenario is relating an existing record to a new record. One way to test this is to commit to the database so we have an Id from the newly created record. If you run the test like this, we'll see that the unit test fails on the version that's in master and passes in the version that's in the PR.

The mocking framework does not create fake Ids after mocking the insert. I'm open to guidance on how to better test this scenario using mocking.

[stohn777]

Does this test add value then? My initial guess is that it does not since the errant code passes the test. Generally we try to avoid actual DML in tests, but some times, it's needed to properly execute the code being tested. However some gray exists between that and a test of Salesforce functionality exclusively.

If we can craft a test that properly recreates the problem, using actual DML to add value through testing this feature, without simply testing Salesforce functionality. Otherwise then adding a test that does NOT add value is pointless.

@jprichter @ImJohnMDaniel

[ImJohnMDaniel]

@ClayChipps and @jprichter -- Please add a null check on obj before trying to generate the mock Id.

[ClayChipps]

done

[ImJohnMDaniel]

@ClayChipps and @jprichter -- Any reason why you did not just simply modify the existing MockDML??

[ClayChipps]

Updated to use the existing MockDML.

[ClayChipps]

This bug only presents itself when the Unit of Work has SObjects registered in a certain order.

Here is a sample scenario where the current implementation fails:

Lets assume we have two SObjects that we are about to register to a Unit of Work.

Account: {
   Name: My New Account
}

Opportunity: {
  Id = 0063000000D8cuI,
  Name = 'Existing Opportunity',
  StageName = 'Closed',
  CloseDate = System.today()
}

We have registered the Unit of Work such that Opportunity is registered before Account.

fflib_SObjectUnitOfWork uow = new fflib_SObjectUnitOfWork(
     new List<Schema.SObjectType>{
         Opportunity.SObjectType,
         Account.SObjectType
     }
);

We then attempt to perform the following operation:

uow.registerNew(newAccount);
uow.registerDirty(existingOpportunity, Opportunity.AccountId, newAccount);
uow.commitWork();

This will fail as when the Unit of Work reaches the insertDmlByType, it will first process all Opportunity relationship resolutions (even though no Opportunity has been registered new), and then process the insert and resolution of all Account relationships.

Then when the Unit of Work reaches the updateDmlByType, the relationship resolution is not invoked again, and thus the relationship that was registered has never been resolved after the Account insert.

The initial fix was to add a call to m_relationships.get(sObjectType.getDescribe().getName()).resolve(); inside of the updateDmlByType method so that the relationships would be resolved once again once getting to the update phase, at which point the Account.Id will have been populated in the insertDmlByType.

This causes a separate issue that was exposed by the InvoicingServiceTest. The PricebookEntry.Product2Id field can only be set once, and after it is set it cannot be updated, even in memory. Thus, when the second call to m_relationships.get(sObjectType.getDescribe().getName()).resolve(); is made within the updateDmlByType method, the relationship resolution happens again and attempts to update the PricebookEntry.Product2Id field only to fail with an exception as that field is now read-only.

The solution here was to modify the Relationship resolver to remove relationships from the registered relationships list as they are successfully resolved, so that previously resolved relationships are not resolved a second time.

[ImJohnMDaniel]

@ClayChipps and @jprichter -- After more discussion on this PR, we would like to conduct a test of your changes in a real world series of codebases to assess any functional or performance impacts. I will reach out to you two directly to discuss this in detail. Thanks again for the help!