Add support for upsert by external Id

[jonathanwiesel]

I'm afraid that since there's no external id field in the standard data model I found no way to create a test for this.

Closes #269

---

This change is 

[afawcett]

@jonathanwiesel in your test you can register an IDML mock impl and assert on the parameters passed to its methods. I have seen this done in another PR recently.

[jonathanwiesel]

@afawcett thank you! Will take a look to locate it and see how it goes

[jonathanwiesel]

@afawcett Instead of using the isExternalId from the DescribeFieldResult class, I changed to use the isIdLookup which is specifically for upsert operations to be more consistent and also supports the standard Id, which allowed me to create a test for it.

[wimvelzeboer]

I think you are missing here the resolving of relationships. You do register them in the registerupsert method

[jonathanwiesel]

Seems like it, I based my update in the registetDirty method that seems to also miss the resolving, is it a bug in the dirty relationship resolution or are the dirty relationships resolved elsewhere?

[wimvelzeboer]

Resolving of relationships is only for new records, but since an upsert also handles new records we should resolve the relationships here.

[jonathanwiesel]

Working on that I noted the following:

Since the insertDmlByType method iterates over all sObjects for the Unit of Work (not necessarily the records that were registered with registerNew) the insertDmlByType will always resolve all relationships (even those registered using one of the registerDirty methods).

Adding another resolver in the upsertDmlByType method will result in resolving again the relationships which could lead to some errors like unable to set again creatable but not updateable fields (such as the Product2Id in a PricebookEntry).

That's why I need to modify the resolve method on the Relationship class, in order to resolve whats missing resolution. What are your thoughts about it?

[jonathanwiesel]

Looking again, that change I made on the resolver is not a good one since it will ignore initial resolvers if the field is already filled before registering and therefore producing unexpected results.

An idea comes to my head that maybe each xByType method should be responsible to resolve the relationships related only for the records registered for that operation, although the risk exists that a greater change in the implementation may be needed.

[ClayChipps]

@jonathanwiesel If you want any help on this, please let me know. My current implementation is needing this functionality and I was about to resurrect this PR for my own purposes, but happy to help you instead. 👍

[jonathanwiesel]

Thank you for the support @ctchipps 👍 I think I got it, I'll let you know in in case it get's nasty

[jonathanwiesel]

Ok so what I ended up doing to maintain most of the logic intact to avoid refactoring was in fact adding that second resolver to the upsert method, but adding a conditional in the resolver logic to check if a relationship has already been resolved to not resolve it again (and therefore mitigate risks of double resolving on creatable but no updatable fields).

What do you guys think?

[ClayChipps]

@jonathanwiesel Awesome work! I am going to try to get to reviewing this sometime this week.

@wimvelzeboer Do you mind to take a review pass on this as well?

[jonathanwiesel]

@ctchipps @wimvelzeboer I was wondering if you had the opportunity to take a look at this?

[jonathanwiesel]

Any feedbackcon this? Is been quite a while 💔

[ClayChipps]

Any feedbackcon this? Is been quite a while 💔

Apologies, this fell off my radar. Let me take a deep review and give this some testing. Thank you very much for the work on this. :)

[stohn777]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jonathanwiesel and @stohn777)

a discussion (no related file):
The change branch has a compile error. See the Github Actions unit test execution log.

Error  fflib_SObjectUnitOfWorkTest  Class fflib_SObjectUnitOfWorkTest.MockDML must implement the method: void fflib_SObjectUnitOfWork.IDML.dmlUpsert(List<SObject>, Schema.SObjectField) (917:19)

---

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls, line 506 at r3 (raw file):

        Boolean externalIdFieldIsValid = externalIdField.getDescribe().isIdLookup();

        if (record.Id != null && externalIdFieldName != 'Id')

I wouldn't expect the case of 'Id' to cause problems, although would a case-insensitive comparison be a tad safer?

[jonathanwiesel]

@stohn777 after almost 1.5 year of silence I convinced myself to tackle this again, I hope it is still useful

[chazwatkins]

I'm interested in this functionality as my team has some upsert use cases and would like to use the unit of work. Any chance it can get reviewed and merged?

[nwcm]

Also looking for this.

Happy to assit, who is leading this work and how do we get it merged?

[viliusDC]

Also looking for this. What is the status? Any chance it can get reviewed and merged any time soon?

[karlsauter]

I'm also interested in this functionality. I'm using the IDoWork for this case currently, but a method would be much nicer. Can I contribute to this?

[jonathanwiesel]

Anything I can do to push this forward to see it merged after 4 years 😁

[ImJohnMDaniel]

G'day @jonathanwiesel and @karlsauter -- Thanks for reaching out. While this is still on the team's radar, we are working on other issues at the moment. Rest assured we will get to this as soon as we can.