fflib_SObjectSelectorTest failure with Lookup relationship and Person Type Accounts

[CSigelmann]

Describe the bug
The test toSOQL_When_SystemModeAndChildRelationship_Expect_WellFormedSOQL() relies on fflib_QueryFactory.getChildRelationship(SObjectType), which can get an unexpected relationship when Person Type Accounts are enabled. Opportunities is not guaranteed to be the first child relationship where childRow.getChildSObject() == Opportunity.sObjectType.

To Reproduce

https://github.com/CSigelmann/fflibSelectorTestBug

Steps to reproduce the behavior:

1. Create default scratch org
2. Deploy fflib
3. Enable Person Type Accounts
4. Create Lookup relationship to Contact on Opportunity with Field Name = "AFP" and Child Relationship Name = "OpportunitiesAFP"
5. Run anonymous apex

Schema.SObjectType table = Account.sObjectType;
for (Schema.ChildRelationship childRow : table.getDescribe().getChildRelationships())
{
    if (childRow.getChildSObject() == Opportunity.sObjectType)
    {
        System.debug(childRow.getRelationshipName());
    }
}

6. See that OpportunitiesAFP__pr comes before Opportunities
7. Run fflib_SObjectSelectorTest.toSOQL_When_SystemModeAndChildRelationship_Expect_WellFormedSOQL()
8. See that the test fails because OpportunitiesAFP__pr is being used instead of Opportunites

Note: I tried this with a different relationship name and did not experience the same results. OpportunitiesAgent__pr shows up below Opportunities every time. I am not sure how the order is determined, just that Opportunities coming first cannot be relied upon.

Expected behavior
The test should pass regardless of what custom relationships are in the org fflib is deployed to. It should either use fflib_QueryFactory.getChildRelationship(String) to get a specific relationship, or have a matcher that accepts other relationships in the assert.

Screenshots and text of error observed

fflib_SObjectSelectorTest.toSOQL_When_SystemModeAndChildRelationship_Expect_WellFormedSOQL()
System.AssertException: Assertion Failed: 
Expected: SELECT name, id, annualrevenue, accountnumber, (currencyisocode, )?\(SELECT name, id, amount, closedate(, currencyisocode)? FROM Opportunities ORDER BY Name ASC NULLS FIRST \)  FROM Account WITH SYSTEM_MODE ORDER BY Name ASC NULLS FIRST  
Actual:SELECT name, id, annualrevenue, accountnumber, (SELECT name, id, amount, closedate FROM OpportunitiesAFP__pr ORDER BY Name ASC NULLS FIRST )  FROM Account WITH SYSTEM_MODE ORDER BY Name ASC NULLS FIRST

Version
fflib-apex-common @ 41f92e9
fflib-apex-mocks @ b534ae8

[daveespo]

That's an EXCELLENT bug report. Thank you!

While I understand that the test failure is what you're reporting, but are you finding any problems with using Person Accounts in general with SObjectSelector or SObjectUnitOfWork?

[CSigelmann]

We are still in the very early stages of fflib adoption, and we just enabled Person Accounts a few days ago, so there hasn't been much opportunity to notice problems in general.

We noticed this because at the moment all of the fflib tests are being run when we deploy, so the first deployment after enabling Person Accounts failed with this test failure. We do have a manual workaround for now.

Ideally fflib tests wouldn't be run in our org, but I'm still researching if there's a good way to do that. Right now fflib is just in our source via git submodules.

[CSigelmann]

To add to this, the version of fflib_QueryFactory.getChildRelationship that is causing this issue is only used by the deprecated versions of fflib_QueryFactory.subselectQuery, so we should not have problems with this outside of the test failure so long as we don't use deprecated functions.

[daveespo]

@CSigelmann -- thank you again for such a perfect repro case and clear diagnosis of the problem. You are indeed correct that that one test method was using the deprecated addQueryFactorySubselect method (which relies on inferring the name using the SObject type and hopes that there is only one relationship)

I have submitted PR#493 with the fix and it should be merged shortly

[daveespo]

And for posterity, the list of relationship names that popped into existence when Person Accounts were enabled wasn't just the one that CSigelmann reported, there were two others which brought the list to:

08:44:15.1 (80775494)|USER_DEBUG|[6]|DEBUG|OpportunitiesAFP__pr
08:44:15.1 (80837740)|USER_DEBUG|[6]|DEBUG|Opportunities
08:44:15.1 (80894484)|USER_DEBUG|[6]|DEBUG|OpportunitiesAgent__pr
08:44:15.1 (80950490)|USER_DEBUG|[6]|DEBUG|PersonOpportunities