Modifications to registerDirty( SObject, List<SObjectField> ) to only register dirtyFields

[foxysolutions]

Modifications to registerDirty( SObject, List ) to enable only registering the specified fields.

• Method (name) implies to only register the fields specified, but at first-method-call the complete record is registered without any notice
• Current method was only beneficial when having multiple memory references, with same recordId; Note, due to kept reference, any update on originally registered record will get updated and no later registerDirty is needed
• New implementation will allow registration of only specified fields; reference is lost to original record, but is not necessary when only updating per field.
• Additional Exception is provided when mixing registerDirty with and without dirtyFields to avoid system overwriting specific field updates
• Test class added to clarify a bit more on the scenario covered by this commit

---

This change is 

[afawcett]

These seem like good clarification changes to me. My only concern is breaking backward compatibility, even though the original behavior was likely not expected code could be inadvertently depending on it. Thoughts?

[cropredyHelix]

@foxcreation - I know that my org's implementation calls registerDirty multiple times on same sobject in memory for some use cases -- would that be broken by this PR?

[afawcett]

@ImJohnMDaniel whats your views on this one?

[afawcett]

So have studied this PR further again and am warming to just going ahead with it. I would like @ImJohnMDaniel opinion and @Autobat if he has time?

[cropredyHelix]

so, currentmethod registerDirty(Sobject) is unchanged?

e.g.

Account a = new Account(Id = x, Website = 'w');
registerDirty(a);
a.Rating = 'foo';
commitWork();

will update the Account id=x with Website='w' and Rating = 'foo' ?

and new method would work as follows

Account a0 = new Account(Id = 'x, Website='W');
registerDirty(a0,new List<SObjectField>{Account.Website});
Account a1 = new Account(Id='x',Rating='foo');
registerDirty(a1,new List<SObjectField>{Account.Rating}); // diff sobject, same Id
commitWork();

will update the Account id=x with Website='w' and Rating = 'foo' ?

[foxysolutions]

@afawcett @cropredyHelix Apologies for the late response. Please find the clarification below.

@cropredyHelix all your examples will indeed stay as they were before

• Providing the same ID but with a different object reference will still be working as well (as long as both method-calls contain dirtyFields, or both lack dirtyFields - see example below)
• References are still in place for the registrations WITHOUT fields provided. Then a changes to the record after registration without fields will be reflected in the database call at commitWork
• Registrations WITH fields specified will only copy those fields at that moment and will not be updated after changes

The scenarios which are tend to be fixed with this scenario are:
Account acc = new Account( Id = x, Website = 'W', Rating='foo', Name='Dummy' );
uow.registerDirty( acc, new List<SObjectField>{ Account.Website } );
acc.Name = 'Failure';
uow.commitWork();

Initially, this would insert an Account with Website, Name and Rating, where Name = 'Failure'; However, in my perspective the desire for only registering certain fields is to avoid the reference to the object and the update of additional fields specified on the provided record. We only want to store the values which are in the defined Fields at that time (so only the Website);

As of now, I can think of two scenarios which will NOT NOT be backward compatible:

1. When registerDirty( record, dirtyFields ) was used to register a full record with reference, instead of using registerDirty( record ); Then only the fields are registered which are specified, while initially all fields were updated (hard to track).

2. When registerDirty( record, dirtyFields ) is called first and afterwards registerDirty( record ) is called. for the same record Then the reference is not the same and the system will not know which fields to overwrite and which to keep. Then an Exception is thrown (easy to detect and easy to fix).

I do acknowledge backward compatibility should be the goal, however, in my perspective both scenarios are undesirable and should be prevented in the first place. They do not make sense to be applied. However, I do fully understand the disadvantage of such impact.

In case of any questions or comments, feel free to reach out. My aim is to respond quicker from now on :).

[cropredyHelix]

@foxcreation - I don't understand your example code. What is a0? Did you mean acc?

If I follow, the essence of this is that if one uses the new second arg List, then one should use it consistently up until commitWork. If that is the intent, then, you could throw an error if the caller tried to mix use cases (e.g. registerDirty(x) vs registerDirty(x,fieldList) for the same sobject.

[foxysolutions]

@cropredyHelix you're absolutely right, I copied from an initiation question, but didn't replace all. I've edited my post to correctly refer to acc. Thanks for your sharpness!

I think I can only agree with your summary partially, as the second arg list method already existed, although it did the exact same as registerDirty( record ) when they were called for the first time. Hence, this PR changes a bit more than only restricting that user to mix the use cases.

It is correct that one should not mix. From my perspective it also wouldn't make sense:

• when registerDirty( record ) is called, there is no need to register that record again, since the reference is registered.
• when registerDirty( record, fieldList ) is called, then one should indeed only enrich the record to be registered with fieldList-methods till commitWork. Else, the system would not be able to determine which fields to, and which not to overwrite from the full record provided. For this scenario, there is already an exception in place as can be seen on line 287.
• when registerDirty( record ) is called, but one continues to register per fieldList, one should be aware it will be saved as expected, but that the original record variable will be changed as well (as of the referencing).

I hope this elaborates a bit better on the impact of this PR, but please do challenge me to discuss the benefits of this change.

[ImJohnMDaniel]

@foxcreation -- I have read the discussion above and please forgive me if I am missing something in that description but can you answer why registering only field changes becomes a benefit verses simply registering the entire record as dirty?

[foxysolutions]

but can you answer why registering only field changes becomes a benefit verses simply registering the entire record as dirty?

@ImJohnMDaniel no problem, but please do note this method already exists in the current code base and my pull request only applies some changes for a more logical behaviour.

My understanding to allow a developer to only explicitly register certain fields is to give control of what is updated. One could image the following situation:

• User A fetches a record with all N default selector fields to update only one field (note, 'for update' is kept out of this example)
• Between record retrieval and commitWork() of user A, user B adjusts one of the queried fields
• Then, when registering the full record, the change of user B will be overwritten unintentionally because the full record of user A is registered
  While, when only registering the field which requires update, all changes made to the record in between processes are untouched.

Please let me know whether this elaborates the need for the function and the need for this change sufficiently.

[foxysolutions]

@ImJohnMDaniel @afawcett @cropredyHelix what would be the next steps for this Pull Request to be further reviewd? Please let me know in case any additional input is needed. Thanks!

[ImJohnMDaniel]

@foxcreation -- I just downloaded the code and tested it. I am getting a failed test execution. Here is the output:

fflib_SObjectUnitOfWorkTest.testRegisterDirty_ExpectReplacement  fflib_SObjectUnitOfWork.UnitOfWorkException: Cannot determine what fields to update on record Opportunity:{Id=0062F000009KOLbQAO, Name=Expected}
                                                                 /Users/john/workspace/_aep/fflib-apex-common/sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls:251:1
                                                                 /Users/john/workspace/_aep/fflib-apex-common/sfdx-source/apex-common/test/classes/fflib_SObjectUnitOfWorkTest.cls:358:1

Can you investigate please and let me know what you are seeing?

Thanks for the help

[afawcett]

@foxcreation I'm aligned with this, but we do need to get the test failure @ImJohnMDaniel points out above fixed.

[foxysolutions]

@ImJohnMDaniel could you please retry, I tested it now a couple times, but on my dev environment all methods pass smoothly. Might it accidentally be some local changes in your environment? Please let me know when the issue remains, then I'll further investigate. Thanks!

[ImJohnMDaniel]

G'day @foxcreation -- at the moment, this PR is probably in a bad state. Not because of your changes (which I believe have merit), but because the main branch of the project has changed directory structure to DX Source Format since mid-April.

My advice is to capture the changes to UOW files, cancel this PR, fork the main repo again (which is in DX Source Format) and begin the PR again. Either that, or update your forked branch with all changes from the main repo and let's see where that gets us.