Fields that are polymorphic fail due to first type being used. #137

squattingdog · 2016-09-01T08:49:38Z

When a polymorphic field is being used and the type chosen does not match the sObjectType set in the selector, the configureQueryFactoryFields method will fail.

Add the ability to set the sObjectType when validating the field exists on the object. Use the type set in the selector to determine which polymorphic type to use.

Example: Lead.Owner.UserRoleID
Owner is GROUP | USER
Group is the first type returned and does not contain UserRoleID. The method then fails when the getField result is null.

This change is

…he type specified in the selector. Add the ability to set the sObjectType when validating the field exists on the object. Use the type set in the selector to determine which polymorphic type to use. Example: Lead.Owner.UserRoleID Owner is GROUP | USER Group does not contain UserRoleID and fails when the getField call sets the sObjectField token to null.

dfruddffdc · 2017-01-19T18:55:14Z

Actually, polymorphic fields in Salesforce are a special case.

Lead.Owner can relate to a Queue or a User.
But crucially, the queryable fields don't map to either Queue or User.

With a bit of experimenting and googling, it turns out that the related object type is an SObjectType called Name. It's a coincidence that there are fields with the same name on the User and Name SObjectTypes. It would be a dangerous assumption to say all fields on the User SObjectType will also exist on Name.

So I think the fix is smaller, and shouldn't require any changes to method signatures. Just when trying to get the describe for the lookup referenceTo, it should be something like:
SObjectType relatedSObjectType = relatedObjs.size() == 1 ? relatedObjs[0] : Name.SObjectType;

squattingdog · 2017-02-13T01:53:55Z

In this case of Lead.Owner, the getDescribe method returns 2 values, Queue and User. Name would not match the values returned. The validation is performed using the defined selector's getSObjectType method to validate that the field exists on the specified sObjectType. The UserSelector.getSObjectType returns User.getSObjectType if it were another selector, it would return the type specific to it.

For the above reason, I am not following the "dangerous assumption" statement. As stated, all the fields on User do not exist on Name. If the relatedSObjectType is set to Name, the validation will still fail when looping the fields to find a User or Queue specific field. For example, the field User.BadgeText does not exist on Name. While this proposed solution is simpler on the surface, it still contains the same error this solution solves. That is, when the process loops the fields for a given SObjectType, it may not find a valid match because the wrong type is being used for the matching process.

dfruddffdc · 2017-02-14T17:23:48Z

Hi again, I didn't entirely follow this:

The validation is performed using the defined selector's getSObjectType method to validate that the field exists on the specified sObjectType. The UserSelector.getSObjectType returns User.getSObjectType if it were another selector, it would return the type specific to it.

But hopefully your own example will clarify things here.

For example, the field User.BadgeText does not exist on Name

You are correct, User has a field called BadgeText, which Name does not.
That is why this SOQL works: SELECT BadgeText FROM User
But this SOQL fails: SELECT Owner.BadgeText FROM Lead
And this SOQL passes: SELECT Owner.UserRoleId FROM Lead - because of the Name.UserRoleId field, not because of the User.UserRoleId field.

While a specific Lead record's (i.e. a single DB row) Owner is either a USER or a GROUP, the Lead SObjectType's (i.e. the whole DB table) Owner lookup is related to the Name SObjectType, not User or Group.

dfruddffdc · 2017-02-14T17:47:24Z

Ah, I follow a bit more now. You are using the UserSelector to ensure you select all interesting fields about the Lead's Owner. In fact, you need a NameSelector to configure the fields on the main QueryFactory instance.

//new Testfflib_UserSelector().configureQueryFactoryFields(qf, 'Lead.Owner');
new MyNameSelector().configureQueryFactoryFields(qf, 'Lead.Owner');

So I still feel the fix would be:
SObjectType relatedSObjectType = relatedObjs.size() == 1 ? relatedObjs[0] : Name.SObjectType;

# Conflicts: # fflib/src/classes/fflib_QueryFactory.cls

squattingdog · 2017-03-07T20:25:13Z

Separate note here, the SF org user creds seem to be out of sync which is causing the validation deploy to fail. @afawcett

squattingdog · 2017-03-07T20:47:29Z

Hi David,
The issue that is being addressed is with the describe call. It fails due to the incorrect SObjectType being used; the actual soql that is generated and ultimately executed is not in question as the error happens prior to any soql being generated. If the field doesn't exist on Name then the describe still fails.

For example, if we are looping the values for Lead.Owner.BadgeText we get an array of [Owner, BadgeText] for the specific Lead sObjectType. The first iteration through we get Owner. On the describe we get back the array: [ Group, User]. Let's set the SObjectType to Name because there is more than 1 item in the array. The next iteration we are describing Name to find BadgeText, however, BadgeText does not exist on Name. This results in the same error as arbitrarily setting the sObjectType to the first item in the array.

This PR addresses the issue by passing in the sObjectType from the concrete selector that is using the QueryFactory instance. I understand that this creates a tighter coupling between the 2 classes. Perhaps, passing an instance of the concrete SObjectSelector to the QueryFactory constructor and then subsequently calling the SObjectSelector instance's getSObjectType method is a bit better??

Interested to hear any proposals for this issue.

afawcett · 2018-10-15T02:24:24Z

@squattingdog i think its best to avoid tighter coupling so passing in the sobjectype feels fine to me. @dfruddffdc do you agree?

afawcett · 2018-11-23T17:38:16Z

@squattingdog i appreciate this is an old PR by now, wondered if you still had a few on my last question on the coupling? I think we are close to getting agreement and merging.

squattingdog · 2019-01-17T17:40:13Z

@afawcett hi, sorry for the delay, I lost track of this over the holidays. Anyway, the current implementation of this PR passes the sObjectType. Are you saying that is the best option at this time?

…pex-common into merge-from-fork-source

afawcett · 2019-02-04T01:25:44Z

@squattingdog indeed it does! Sorry i misread your last para from an earlier comment. @agarciaffdc this is good in my view to merge.

agarciaffdc · 2019-02-05T06:19:15Z

fflib/src/classes/fflib_QueryFactory.cls

 			this.setMessage( 'Invalid field \''+fieldName+'\' for object \''+objectType+'\'' );
 		}
-	}
+	}public class InvalidRelationshipException extends Exception{


@squattingdog sorry for asking this when your PR is opened since so long ago. But could you clean format a bit? This class should be at least one line below the } And we usually add open brace below the line instead of beside. For instance:

}public class InvalidRelationshipException extends Exception{

should be:

} public class InvalidRelationshipException extends Exception {

Don't worry about the code it is not yours. We will merge it as soon as you format it a bit.

yup, not sure how that happened...addressing those.

agarciaffdc · 2019-02-05T06:20:56Z

@dbtavernerffdc FYI

squattingdog · 2019-02-05T22:48:58Z

FYI, there still seems to be an issue with the creds being used for the Travis CI builds.

/home/travis/build/financialforcedev/fflib-apex-common/lib/exec_anon.xml:147: INVALID_LOGIN: Invalid username, password, security token; or user locked out.

agarciaffdc · 2019-02-07T06:16:01Z

Thanks for the hands up @squattingdog We will work around it in order to be able to merge it without Travis confirmation on your fork. Then Travis works properly on the lib repo

dbtavernerffdc · 2019-02-08T16:08:59Z

fflib/src/classes/fflib_QueryFactory.cls

 		}
 	}
+
+	public class InvalidRelationshipException extends Exception


I can't find anywhere else in this pull request where this exception is referenced. Perhaps it should be thrown if the passed in object type is not in the reference to list.

Good one!! @squattingdog could you check this one?

@squattingdog once you check this one we will merge it quickly.

dbtavernerffdc · 2019-02-08T16:25:40Z

fflib/src/classes/fflib_QueryFactory.cls

+					if(relatedObjs.size() > 1) {
+						String relatedSObjectName = fflib_SObjectDescribe.getDescribe(relatedSObjectType).getDescribe().getName();
+						for(Schema.sObjectType sot : relatedObjs) {
+							if(fflib_SObjectDescribe.getDescribe(sot).getDescribe().getName() == relatedSObjectName){


Out of interest why are we comparing the names and not just the sobject type objects?

JAertgeerts · 2022-02-25T09:07:39Z

@afawcett @ImJohnMDaniel can this be looked at? This is a pretty significant issue for any field (and increasing with the increase of polymorphic fields added with Industry clouds) that can have multiple objects as a related object.

…ans up the couple of issues outlined on the prior PR comment thread (removes unused Exception and uses the SObjectType for the comparison vs. the String name)

daveespo

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @agarciaffdc, @daveespo, @dbtavernerffdc, and @squattingdog)

a discussion (no related file):
I created a new PR for this change request over in #402 since this PR is against the old mdapi format project structure and has conflicts that are too hard to resolve

Additionally, I addressed several comment threads below in that other branch so I'm marking them as resolved here and closing this PR without merging it.

fflib/src/classes/fflib_QueryFactory.cls, line 118 at r4 (raw file):

Previously, dbtavernerffdc wrote…

Out of interest why are we comparing the names and not just the sobject type objects?

I agree -- I changed this to compare the SObjectType and it properly compiled and all tests passed

fflib/src/classes/fflib_QueryFactory.cls, line 794 at r4 (raw file):

Previously, agarciaffdc wrote…

@squattingdog once you check this one we will merge it quickly.

This Exception seems to be unused so I removed it from the code base

Redux of PR #137 - rebased against latest master and merge conflicts resolved along with the two outstanding comments on the prior PR

tstone added 2 commits March 7, 2017 13:16

Merge remote-tracking branch 'fflib-origin/master' into merger

9f7dede

# Conflicts: # fflib/src/classes/fflib_QueryFactory.cls

Merge branch 'merger' into master

c78614a

squattingdog force-pushed the master branch from 44d8813 to c78614a Compare March 7, 2017 18:43

afawcett added the discussion label Mar 8, 2017

Merge branch 'master' of https://github.com/financialforcedev/fflib-a…

6fa7ff2

…pex-common into merge-from-fork-source

agarciaffdc reviewed Feb 5, 2019

View reviewed changes

squattingdog added 2 commits February 5, 2019 17:33

formatting changes

83508ff

additional formatting changes

823b772

agarciaffdc approved these changes Feb 7, 2019

View reviewed changes

dbtavernerffdc reviewed Feb 8, 2019

View reviewed changes

dbtavernerffdc approved these changes Feb 8, 2019

View reviewed changes

JAertgeerts approved these changes Feb 25, 2022

View reviewed changes

daveespo reviewed Apr 5, 2022

View reviewed changes

daveespo closed this Apr 5, 2022

daveespo added a commit that referenced this pull request Apr 5, 2022

Merge pull request #402 from apex-enterprise-patterns/rebase-pr-137

1e5eb56

Redux of PR #137 - rebased against latest master and merge conflicts resolved along with the two outstanding comments on the prior PR

This was referenced Jun 8, 2023

How to get Owner.XXX fields for custom objects? fflib_QueryFactory #131

Closed

Selector classes on custom object errored out on parent field "OwnerId" #388

Closed

Fields that are polymorphic fail due to first type being used. #137

Fields that are polymorphic fail due to first type being used. #137

Uh oh!

Conversation

squattingdog commented Sep 1, 2016 • edited by daveespo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfruddffdc commented Jan 19, 2017

Uh oh!

squattingdog commented Feb 13, 2017

Uh oh!

dfruddffdc commented Feb 14, 2017

Uh oh!

dfruddffdc commented Feb 14, 2017

Uh oh!

squattingdog commented Mar 7, 2017

Uh oh!

squattingdog commented Mar 7, 2017

Uh oh!

afawcett commented Oct 15, 2018

Uh oh!

afawcett commented Nov 23, 2018

Uh oh!

squattingdog commented Jan 17, 2019

Uh oh!

afawcett commented Feb 4, 2019

Uh oh!

agarciaffdc Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squattingdog Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

agarciaffdc commented Feb 5, 2019

Uh oh!

squattingdog commented Feb 5, 2019

Uh oh!

agarciaffdc commented Feb 7, 2019

Uh oh!

dbtavernerffdc Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

agarciaffdc Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarciaffdc Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

dbtavernerffdc Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

JAertgeerts commented Feb 25, 2022

Uh oh!

daveespo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

squattingdog commented Sep 1, 2016 •

edited by daveespo

Loading

agarciaffdc Feb 5, 2019 •

edited

Loading

agarciaffdc Feb 10, 2019 •

edited

Loading