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. :)

[john-storey-devops]

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

[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.

[afawcett]

I have started looking at this. The challenge is a change that might erode code coverage and thus break backwards compatibility as such. I plan to compare to what degree this would be the case and come back to this thread. I also have a few possible ideas on how to get more coverage if needed....

[afawcett]

I did a compare with main and this appears to have increased coverage: now 92.62% (Previous: 92.56%, Change: +0.06%)

I will approve on this basis.

=== Coverage Summary ===
fflib_AnyOrder                               37/    37 lines  100.00%
fflib_ApexMocks                             108/   108 lines  100.00%
fflib_ApexMocksUtils                         89/    89 lines  100.00%
fflib_Application                           100/   114 lines   87.72%
fflib_ArgumentCaptor                         20/    20 lines  100.00%
fflib_IDGenerator                             6/     6 lines  100.00%
fflib_InOrder                               133/   133 lines  100.00%
fflib_InvocationOnMock                       18/    18 lines  100.00%
fflib_Match                                 242/   242 lines  100.00%
fflib_MatcherDefinitions                    369/   369 lines  100.00%
fflib_MatchersReturnValue                     3/     3 lines  100.00%
fflib_MethodArgValues                        12/    12 lines  100.00%
fflib_MethodCountRecorder                    16/    16 lines  100.00%
fflib_MethodReturnValueRecorder              33/    33 lines  100.00%
fflib_MethodVerifier                         48/    50 lines   96.00%
fflib_Objects                                37/    37 lines  100.00%
fflib_QualifiedMethod                        29/    29 lines  100.00%
fflib_QualifiedMethodAndArgValues            12/    12 lines  100.00%
fflib_QueryFactory                          262/   277 lines   94.58%
fflib_SecurityUtils                         108/   110 lines   98.18%
fflib_SObjectDescribe                       136/   150 lines   90.67%
fflib_SObjectDomain                         336/   368 lines   91.30%
fflib_SObjects                              142/   150 lines   94.67%
fflib_SObjectSelector                       126/   171 lines   73.68%
fflib_SObjectUnitOfWork                     299/   387 lines   77.26%
fflib_StringBuilder                          37/    41 lines   90.24%
fflib_System                                 14/    14 lines  100.00%
fflib_VerificationMode                       41/    41 lines  100.00%

=== Comparison with Previous Run (coverage-2.json) ===

✅ No regressions detected

📈 IMPROVEMENTS (Coverage Increased):
Class Name                                 Previous    Current     Change
---------------------------------------- ---------- ---------- ----------
fflib_SObjectUnitOfWork                       74.25%      77.26%      +3.01%

Previous run: /Users/andrewfawcett/Documents/fflib/.coverage/coverage-2.json

Current run: /Users/andrewfawcett/Documents/fflib/.coverage/coverage-3.json

=== Overall Coverage ===
Total Lines Covered:   2813
Total Lines Uncovered: 224
Total Lines:           3037
Overall Coverage:      92.62% (Previous: 92.56%, Change: +0.06%)

✅ Overall coverage meets Salesforce minimum requirement (75%)

════════════════════════════════════════════════════════════════
📊 SUMMARY
════════════════════════════════════════════════════════════════
Files compared:
  Previous: coverage-2.json
  Current:  coverage-3.json

Coverage changes:
  Regressions:  0
  Improvements: 1

Overall coverage:
  Current:  92.62%
  Previous: 92.56%
  Change:   ⬆️  +0.06%
════════════════════════════════════════════════════════════════

[afawcett]

IDML2 interface was added as a new interface extending IDML to support upsert by external ID, allowing existing IDML implementations to continue working without changes while new implementations can opt into IDML2 for the additional functionality. Fix upsertDmlByType to only process records when upsert records exist to avoid requiring IDML2 needlessly. Added a test to cover the exception path for custom IDML implementations that don't support IDML2.

cc @john-storey-devops @ImJohnMDaniel @daveespo

[daveespo]

@daveespo made 7 comments.
Reviewable status: 0 of 3 files reviewed, 10 unresolved discussions (waiting on @ImJohnMDaniel, @jonathanwiesel, and @wimvelzeboer).

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 117 at r6 (raw file):

            if (!objList.isEmpty()) {

                Type objListType = Type.ForName('List<' + objList[0].getSObjectType() + '>');

This recreation of the List seems unnecessary? Database.upsert works properly even if the list is a generic List<SObject>

I tested the following anon Apex and it inserts and updates as expected:

List<Sobject> cList = new List<SObject>();
cList.add(new Contact(LastName = 'Esposito',ExtId__c='ESPO'));
cList.add(new Contact(LastName = 'Daniels',ExtId__c='DANIELS'));
Database.upsert(cList,Contact.ExtId__c);

Let's remove this so that people don't copy/paste bad patterns when creating their own IDML implementations

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 148 at r6 (raw file):

    }

	public virtual class UserModeDML extends SimpleDML{

UserModeDML needs to override dmlUpsert and use the overload to Database.upsert that takes the AccessLevel param (and passing true for the allOrNone param)

https://developer.salesforce.com/docs/atlas.en-us.apexref.meta/apexref/apex_methods_system_database.htm#apex_System_Database_upsert_3

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 534 at r6 (raw file):

		assertForSupportedSObjectType(m_upsertRecordsPerType, sObjName);

        if (externalIdField == null) 

I know there's lots of different patterns in this code base but we've been using curly braces for all conditionals in recent work so let's add them here (all of the conditionals below need them as well)

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 537 at r6 (raw file):

            throw new UnitOfWorkException('Invalid argument: externalIdField. If you want to upsert by id, use the registerUpsert method that has only one argument');

        String externalIdFieldName = externalIdField.getDescribe().getName();

Per the comment below, let's see if we've already registered this externalIdField in the m_externalExternalIdToUpsertPerType and if we have, let's skip doing these validations

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 554 at r6 (raw file):

        {
            throw new UnitOfWorkException(String.format(
                'SObject type {0} has already registered an upsert by external id {1}, you cannot use another is this unit of work.',

Typo: s/is/in

Additionally, I think this check should be hoisted up higher in the method because if we get past this condition, we will know that we've already validated the external ID field and can short-circuit all of the Describes for future invocations

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 562 at r6 (raw file):

        m_externalIdToUpsertPerType.put(sObjName, externalIdField);

        if (relatedToParentRecord!=null && relatedToParentField!=null)

Need curly braces

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 804 at r6 (raw file):

            sobjName = sObjectType.getDescribe().getName();
            List<SObject> upsertRecords = m_upsertRecordsPerType.get(sobjName);
            if (upsertRecords != null && !upsertRecords.isEmpty()) {

Should we do this validation in the registerUpsert() so that the stack trace that the developer sees is where they're actually calling the registerUpsert rather than some entirely different body of code where commitWork() is called?

[afawcett]

Feedback has been addressed in the latest commit:

fflib_SObjectUnitOfWork: Simplified SimpleDML.dmlUpsert to use Database.upsert(objList, externalId) directly; added UserModeDML.dmlUpsert with the access-level overload; in registerUpsert hoisted the already-registered check, short-circuit when the same external ID is reused, validate IDML2 at registration (so the stack trace points at the call site), added braces throughout, and fixed the typo (is→in).

Tests: Updated so the IDML2 exception is expected from registerUpsert; added tests covering UserModeDML upsert and the new exception paths (null external ID, field not on target, field not valid for upsert, different external ID already registered for same type). New code paths are covered and no code coverage regression was found (overall coverage improved).

cc @daveespo please take a look. :-)

[daveespo]

@daveespo made 6 comments and resolved 4 discussions.
Reviewable status: 0 of 3 files reviewed, 12 unresolved discussions (waiting on @afawcett, @jonathanwiesel, and @wimvelzeboer).

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 99 at r7 (raw file):

    }

    public interface IDML2 extends IDML

Upon discussion internally, we would prefer to make this a composable pattern. That is, this new interface should not extend IDML and should instead just declare the new functionality.

SimpleDML would then implement both IDML and the new interface.

And we'd like to have the new interface describe the aspect it's implementing. So we propose IDMLUpsertable

@afawcett thoughts?

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 116 at r7 (raw file):

        public virtual void dmlUpsert(List<SObject> objList, Schema.SObjectField externalId)
        {
            if (!objList.isEmpty())

Remove isEmpty check since the upsert should be a no-op with an empty list

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 547 at r7 (raw file):

                registerRelationship(record, relatedToParentField, relatedToParentRecord);
            }
            return;

Not a huge fan of this short-circuit return since it means we have to repeat the 4 lines of code above down at the bottom of this method to cover the 'else' case.

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 562 at r7 (raw file):

        String externalIdFieldName = externalIdField.getDescribe().getName();
        Boolean relatedHasExternalIdField = sObjectType.getDescribe().fields.getMap().keySet().contains(externalIdFieldName.toLowerCase());

Can just just replace with sObjectType == externalIdField.getDescribe().getSobjectType()

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 565 at r7 (raw file):

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

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

I'm skeptical of this use case -- using the actual Salesforce Id as the External ID?

I propose that we throw an Exception here until we find out that there is a valid use case for this sort of pattern.

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 831 at r7 (raw file):

                    dml2.dmlUpsert(upsertRecords, m_externalIdToUpsertPerType.get(sobjName));
                } else {
                    throw new UnitOfWorkException('Upsert by external ID requires IDML2 implementation. Current DML implementation does not support this feature.');

We already checked this condition before registering the record in UOW so we can remove this instanceof check

[daveespo]

@daveespo reviewed 3 files, made 2 comments, and resolved 9 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on afawcett, jonathanwiesel, and wimvelzeboer).

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 547 at r7 (raw file):

Previously, daveespo (David Esposito) wrote…

Not a huge fan of this short-circuit return since it means we have to repeat the 4 lines of code above down at the bottom of this method to cover the 'else' case.

DRY'd this up by calling a private method that pushes the record and relationship

---

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls line 565 at r7 (raw file):

Previously, daveespo (David Esposito) wrote…

I'm skeptical of this use case -- using the actual Salesforce Id as the External ID?

I propose that we throw an Exception here until we find out that there is a valid use case for this sort of pattern.

This use case is probably unrealistic in the real world but it helps with having test coverage here in this project. We don't have any Standard Objects with external IDs but we can use this trickery of using the standard ID field as an external ID in order to cover all of this new code. Very clever @jonathanwiesel

The review request is now outdated