#419 - implements a proposal for supporting native User Mode Database Operations introduced in Summer '22 #420

daveespo · 2022-07-13T21:15:20Z

This change is

… Operations introduced in Summer '22

daveespo

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

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 91 at r1 (raw file):

	private String getFieldPath(String fieldName, Schema.sObjectType relatedSObjectType){

		//Enforcing FLS using the legacy heuristic requires resolving the full field path to its respective

Here's how my reasoning went down:

We discussed keeping the "User Mode" and "Enforce Schema Validation" as separate concerns so that's where I started. But then I started writing up the Apex Doc comments on why you'd want to leave Schema Validation ON if you WEREN'T enabling FLS and it sounded silly ("Turn this feature on if you need us to validate that you have syntactical errors in your SOQL"). Why would you want to expend computation expense each time you run a query just because you're sloppy?

So I reasoned that we only need to do this heavy lifting if the legacy form of FLS enforcement was enabled.

This change causes many failing tests .. a few for reasons that are actually silly. But there are a few actual logic changes if you turn schema validation off. Notably, standard objects that have a lookup to a parent usually have an "Id" field

For example, this test fails:

13:35:41.189 (1191507628)|FATAL_ERROR|System.AssertException: Assertion Failed: Expected: SELECT CreatedBy.ManagerId, CreatedBy.Name, FirstName, Id, LastModifiedBy.Email, LastName FROM User, Actual: SELECT CreatedBy.Manager, CreatedBy.Name, FirstName, Id, LastModifiedBy.Email, LastName FROM User

Class.fflib_QueryFactoryTest.deterministic_toSOQL: line 555, column 1

We do black magic to transpose the "CreatedBy.Manager" into "CreatedBy.ManagerId"

See https://github.com/apex-enterprise-patterns/fflib-apex-common/blob/master/sfdx-source/apex-common/main/classes/fflib_SObjectDescribe.cls#L110

For that reason, we can't introduce this kind of change as backwards-compatible.

There are other problems too -- the Set of fields (private Set<String> fields) is a case-sensitive Set of strings .. so the Set doesn't implicitly de-dupe someone selecting a field using two different cases (ex "Name" and "name" are different elements in the Set and you end up with a 'duplicate field selected' SOQL error)

So now I'm back at trying to figure out how to accomplish the goal of increasing efficiency of query construction (i.e. avoiding the Describe loops) without creating immense challenges for people adopting this change.

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

    }

    public fflib_SObjectSelector(Boolean includeFieldSetFields, Boolean userMode)

We talked about not adding yet-another-overload of the constructor and instead using a Config builder pattern. But we have all of these fluent style state mutation methods (ignoreCRUD(), enforceFLS()) and once I started trying to add a builder object, it got so messy with trying to retain backwards compat given that "there is more than one way to do it"

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

    }

	public virtual class UserModeDML extends SimpleDML{

And we need to talk about if/how we support this from fflib_Application.

There is already a newInstance method to construct a SOUOW and pass it an IDML implementation. Adding a "userMode" overload of newInstance() felt wrong. If you want a UserMode version of SOUOW, you should just use the existing newInstance() method and pass it an instance of this class.

tfuda

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @daveespo, @ImJohnMDaniel, and @stohn777)

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls line 119 at r1 (raw file):

    public fflib_SObjectSelector(Boolean includeFieldSetFields, Boolean userMode)
    {
        this(includeFieldSetFields, false, false, false, true);

Shouldn't you be passing userMode as the last arg here?

afawcett · 2022-10-20T20:46:23Z

I think the challenge here is this PR is attempting to do to many things - its done an excellent job of exposing the user mode capability - but it also challenged itself with optimizing the field validation and related features. I took a copy of this code and commented out the code in getFieldPath and all tests pass just fine.

	/**if(mFlsEnforcement != FLSEnforcement.LEGACY){
		return fieldName;
	}**/

Personally I would conceded to fight that battle over optimizing for another day and run with these changes without the above code. I'll also be transparent and say that it would greatly help my work on the next edition of the book as well if this code code merged in the next few months! ;-)

afawcett · 2022-10-20T23:59:06Z

I also think we should add... explicit SYSTEM_MODE, as using NONE will still enforce sharing depending on the sharing clause on the outer class.

public enum FLSEnforcement{NONE, LEGACY, USER_MODE, SYSTEM_MODE}

afawcett · 2022-10-21T00:36:52Z

I also found I needed to change where the WITH USER_MODE was emitted during the SOQL string build - per docs it needs to happen after the WHERE clause...

	result += ' FROM ' + (relationship != null ? relationship.getRelationshipName() : table.getDescribe().getName());
	if(conditionExpression != null)
		result += ' WHERE '+conditionExpression;

	if(mFlsEnforcement == FLSEnforcement.USER_MODE){
		result += ' WITH USER_MODE';
	}
	if(mFlsEnforcement == FLSEnforcement.SYSTEM_MODE){
		result += ' WITH SYSTEM_MODE';
	}

…GACY option and the NONE option in order to preserve th expected behavior with regard to the normalization of the generated SOQL

…; Also, reintroduces the check for LEGACY FLS enforcement before actually asserting FLS isAccessible() (was removed in the prior commit on this feature)

… Mode (both CRUD and FLS) simple Selector use cases -- also introduces a formal SYSTEM_MODE which is subtly different than the legacy behavior in that Sharing is not enforced in SYSTEM_MODE regardless of the class declaration

tfuda

Reviewed 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @daveespo, @ImJohnMDaniel, and @stohn777)

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 301 at r2 (raw file):

	**/
	public fflib_QueryFactory selectFields(Set<Schema.SObjectField> fields){
		for(Schema.SObjectField token:fields){

Couldn't / shouldn't this just be:

public fflib_QueryFactory selectFields(Set<Schema.SObjectField> fields) {
   for (Schema.SObjectField token : fields) {
      selectField(token);
   }
   return this;
}

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 314 at r2 (raw file):

	/**
	 * Selects multiple fields. This acts the same as calling {@link #selectField(Schema.SObjectField)} multiple times.
	 * @param fields the set of {@link Schema.SObjectField}s to select.

Should be "the LIST of {@link Schema.SObjectField}s to select."

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 318 at r2 (raw file):

	**/
	public fflib_QueryFactory selectFields(List<Schema.SObjectField> fields){
		for(Schema.SObjectField token : fields){

Same comment as above; can't this delegate to the selectField(Schema.SObjectField) method to DRY the code a bit?

sfdx-source/apex-common/test/classes/fflib_SObjectSelectorTest.cls line 376 at r3 (raw file):

	@IsTest
	static void toSoql_When_UserModeAndUserCannnotReadObject_Expect_QueryException(){

Minor nit; the method under test is called toSOQL, but your test methods are called toSoql_XXX

daveespo

Addressing @afawcett 's concerns:

Yes, we're mixing concerns, but as my comment on the PR suggests, trying to keep them separate was almost more absurd than foregoing some backwards compatibility. I fixed up the bug in logic so that the legacy test cases pass and introduced some new test cases (more are still necessary)
I added SYSTEM_MODE which required redoing the interfaces on SObjectSelector as well. I didn't introduce an equivalent "NONE" because the default behavior of SObjectSelector is to enforce CRUD but not FLS so "none" would be a misnomer. I only introduced a LEGACY
Yes, my new test cases identified the malformed SOQL .. that's all been fixed up now

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ImJohnMDaniel, @stohn777, and @tfuda)

a discussion (no related file):
I added test coverage to this PR to illustrate the expected behavior and actually exercise the new SYSTEM_MODE and USER_MODE SOQL generation

I need more test coverage to cover relationship queries and some of those special snowflake relationships where we craft the relationship field lookup fields (Account -> AccountId on Contact)

Also, test coverage of the new UserModeDML class needs to be added

And I want to add overloaded/parallel versions of a bunch of the test methods that are using LEGACY mode selectors to verify the behavior when SYSTEM or USER mode is specified (notably that all fields are downcased in order to dedupe)

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 301 at r2 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Couldn't / shouldn't this just be:

public fflib_QueryFactory selectFields(Set<Schema.SObjectField> fields) {
   for (Schema.SObjectField token : fields) {
      selectField(token);
   }
   return this;
}

Yes, will do

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls line 119 at r1 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Shouldn't you be passing userMode as the last arg here?

Good eyes, yes, fixed

sfdx-source/apex-common/test/classes/fflib_SObjectSelectorTest.cls line 32 at r3 (raw file):

	@TestSetup
	static void testSetup(){
		fflib_SecurityUtilsTest.testSetup();

The fixture data (notably the Perm Set) is something we need here too so rather than copy and paste, I'm just calling over there to reuse the same test setup method we use for SecurityUtils tests

sfdx-source/apex-common/test/classes/fflib_SObjectSelectorTest.cls line 370 at r3 (raw file):

	}

	private static void assertEqualsSelectFields(String expectedSelectFields, String actualSelectFields)

This private method was unused so I deleted it

stohn777

Cloned and pushed to a scratch org for some playing and testing. Looks good.

stohn777 · 2022-10-26T19:01:07Z

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls

@@ -367,9 +402,22 @@ public abstract with sharing class fflib_SObjectSelector
    public fflib_QueryFactory newQueryFactory(Boolean assertCRUD, Boolean enforceFLS, Boolean includeSelectorFields)


Admittedly, indent characters are not standard throughout the framework, but since we're touching this function, let's ensure the indent characters are the same for these related functions.

stohn777 · 2022-10-26T19:05:25Z

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls

@@ -408,7 +456,8 @@ public abstract with sharing class fflib_SObjectSelector
    		subSelectQueryFactory, 


Sync indent character.

afawcett · 2022-11-13T19:47:58Z

Thanks all - all this makes sense - look forward to seeing this merged. I currently have a copy of fflib and mocks in my book sample app repo. @ImJohnMDaniel and I are keen to remove this and simply pull latest from these repos via setup scripts. @ImJohnMDaniel is working on those scripts for me (thanks dude!). Are we able to get this merged in the next month or so max? Is there any tasks I can help with?

… Child Queries; fixes bug with (lack of) field deduplication when mixing SObjectField tokens and String field paths by normalizing the list of fields as downcased field paths; DRYed up the manipulation of the fields collection and looping over collections due to the dual overloads for both Set and List argument types; added test coverage for all of these changes

daveespo

I believe this PR is now merge-ready. I added more test cases based on the bugs I found while running this branch against my largest project (1900 test methods) which exposed bugs with how namespaces are handled (our project is a managed package).

The legacy test methods and coverage need some love -- mostly to clean up naming conventions and make it clear what the method is actually testing -- and at the same time, run some of the existing test methods against "SYSTEM_MODE" versions of the same Selectors to prove out that the assertions that those test methods make are valid in both cases -- but that's too much churn for this PR.

We can't actually integrate this branch into my production managed package until User Mode in Apex is GA -- presumably with Spring '23 (Feb 2023) -- so I can't justify the expense to QA the app with the new SYSTEM_MODE and USER_MODE variants -- the best I can do in the short term is to confirm that test coverage passes (which it does with a few minor asterisks which are problems on my end with how we're enumerating fields for "SELECT *" scenarios)

Reviewable status: 3 of 5 files reviewed, 11 unresolved discussions (waiting on @ImJohnMDaniel and @tfuda)

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 91 at r1 (raw file):

Previously, daveespo (David Esposito) wrote…

Here's how my reasoning went down:

We discussed keeping the "User Mode" and "Enforce Schema Validation" as separate concerns so that's where I started. But then I started writing up the Apex Doc comments on why you'd want to leave Schema Validation ON if you WEREN'T enabling FLS and it sounded silly ("Turn this feature on if you need us to validate that you have syntactical errors in your SOQL"). Why would you want to expend computation expense each time you run a query just because you're sloppy?

So I reasoned that we only need to do this heavy lifting if the legacy form of FLS enforcement was enabled.

This change causes many failing tests .. a few for reasons that are actually silly. But there are a few actual logic changes if you turn schema validation off. Notably, standard objects that have a lookup to a parent usually have an "Id" field

For example, this test fails:
13:35:41.189 (1191507628)|FATAL_ERROR|System.AssertException: Assertion Failed: Expected: SELECT CreatedBy.ManagerId, CreatedBy.Name, FirstName, Id, LastModifiedBy.Email, LastName FROM User, Actual: SELECT CreatedBy.Manager, CreatedBy.Name, FirstName, Id, LastModifiedBy.Email, LastName FROM User

Class.fflib_QueryFactoryTest.deterministic_toSOQL: line 555, column 1
We do black magic to transpose the "CreatedBy.Manager" into "CreatedBy.ManagerId"

See https://github.com/apex-enterprise-patterns/fflib-apex-common/blob/master/sfdx-source/apex-common/main/classes/fflib_SObjectDescribe.cls#L110

For that reason, we can't introduce this kind of change as backwards-compatible.

There are other problems too -- the Set of fields (private Set<String> fields) is a case-sensitive Set of strings .. so the Set doesn't implicitly de-dupe someone selecting a field using two different cases (ex "Name" and "name" are different elements in the Set and you end up with a 'duplicate field selected' SOQL error)

So now I'm back at trying to figure out how to accomplish the goal of increasing efficiency of query construction (i.e. avoiding the Describe loops) without creating immense challenges for people adopting this change.

By making the condition more correct (i.e. only bypass the SObjectDescribe stuff if you are opting into the USER_MODE and SYSTEM_MODE), this is now a backwards compatible change. I consider this thread resolved.

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 301 at r2 (raw file):

Previously, daveespo (David Esposito) wrote…

Yes, will do

Done (see above and below for DRYing)

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 318 at r2 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Same comment as above; can't this delegate to the selectField(Schema.SObjectField) method to DRY the code a bit?

Done.

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 164 at r4 (raw file):

			throw new InvalidFieldException('Invalid field: null');		
		}
		return field.getDescribe().getLocalName();

ATTENTION: This should be a no-op change. If you're running a SOQL query in a managed package, getting the namespaced name of a field in your namespace is unnecessary

Without this change, you can end up with duplicate fields because you might select the Amount__c field on your custom SObject using the SObjectField token (ex. MyObject__c.Amount__c) which would return MyPackage__Amount__c which would duplicate the same field if you just called selectField('Amount__c'); (the String overload)

Like I said above, this change is benign in my namespaced scratch org so I have high confidence in this change

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 266 at r4 (raw file):

	**/
	public fflib_QueryFactory selectField(Schema.SObjectField field){
		return selectFields(new Set<Schema.SObjectField>{field});

the introduction of the addField() private method below called attention to how much duplicate code we had. Rather than layer it on thicker, I DRYed this up

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 354 at r4 (raw file):

		 */
		if(mFlsEnforcement == FLSEnforcement.SYSTEM_MODE || mFlsEnforcement == FLSEnforcement.USER_MODE){
			fieldPath = fieldPath.toLowerCase();

As called out above in the now-resolved comment thread in getFieldPath(), we need to normalize the field path to dedupe the Set -- and rather than sprinkle 'toLowerCase()' all over the place, I created this method and then DRYed things up above so we only call it from a couple of places

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

Previously, daveespo (David Esposito) wrote…

And we need to talk about if/how we support this from fflib_Application.

There is already a newInstance method to construct a SOUOW and pass it an IDML implementation. Adding a "userMode" overload of newInstance() felt wrong. If you want a UserMode version of SOUOW, you should just use the existing newInstance() method and pass it an instance of this class.

This thread is unresolved but it may not be a blocker. Being able to override the IDML implementation via the constructor overload is existing behavior so for folks that already implemented CRUD/FLS enforcement in SOUOW, they already have a solution that works for generating an SOUOW instance with their IDML instance

I considered adding a setDML(IDML dmlImpl) to SOUOW but I don't like introducing the ability to change behavior of an instance after it's constructed and potentially loaded with records

Another option would be to pass an IDML instance to commitWork() via a new override (ex commitWork(IDML dmlImpl))

stohn777 · 2022-12-05T21:47:41Z

As expected, unit test "fflib_SObjectSelectorTest.testPolymorphicSelectWithRelatedType" still fails when multi-currency enabled. PR 429 should resolve.

tfuda

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @daveespo and @ImJohnMDaniel)

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 314 at r2 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Should be "the LIST of {@link Schema.SObjectField}s to select."

You missed this comment on the ApexDoc header block. The param specification is incorrect. The parameter is a List, not a Set.

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 308 at r4 (raw file):

	}

	private fflib_QueryFactory selectSObjectField(Iterator<Schema.SObjectField> iter){

Shouldn't this be named "selectSObjectFields" (plural)?

sfdx-source/apex-common/test/classes/fflib_SObjectSelectorTest.cls line 376 at r3 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Minor nit; the method under test is called toSOQL, but your test methods are called toSoql_XXX

Seems like you skimmed over my "non-blocking" comments, so I'm making them blockers.

…ests on the PR

daveespo

Reviewable status: 2 of 5 files reviewed, 10 unresolved discussions (waiting on @ImJohnMDaniel, @stohn777, and @tfuda)

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 314 at r2 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

You missed this comment on the ApexDoc header block. The param specification is incorrect. The parameter is a List, not a Set.

Done.

sfdx-source/apex-common/main/classes/fflib_QueryFactory.cls line 308 at r4 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Shouldn't this be named "selectSObjectFields" (plural)?

Done.

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls line 402 at r3 (raw file):

Previously, stohn777 (John Storey) wrote…

Admittedly, indent characters are not standard throughout the framework, but since we're touching this function, let's ensure the indent characters are the same for these related functions.

Done.

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls line 456 at r3 (raw file):

Previously, stohn777 (John Storey) wrote…

Sync indent character.

Done.

sfdx-source/apex-common/test/classes/fflib_SObjectSelectorTest.cls line 376 at r3 (raw file):

Previously, tfuda (Thomas Fuda) wrote…

Seems like you skimmed over my "non-blocking" comments, so I'm making them blockers.

Done.

… the polymorphic Selector test to make sure it still produces valid SOQL in SYSTEM_MODE

tfuda

Reviewed 1 of 3 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ImJohnMDaniel and @stohn777)

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls

daveespo

Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @ImJohnMDaniel, @stohn777, and @tfuda)

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

Previously, ImJohnMDaniel (John M. Daniel) wrote…

@daveespo -- Would it not be better to set UserModeDML to implement IDML instead of extending SimpleDML??

The reason we created SimpleDML was to provide an easy way to extend default functionality without needing to reimplement method bodies (i.e. DRY)

Each time we added a new method to IDML and consumers of this library upgraded, they were forced to copy and paste a default implementation into their custom IDML implementations. By extending SimpleDML and just overriding the methods you want to modify, it keeps the intent of your changes clear.

Unrelated: Rather than call this "UserModeDML" and have it only support USER_MODE, should we instead change this class to accept a DataAccess argument like the other classes touched in this PR so that you can specify SYSTEM_MODE or USER_MODE (the difference between SYSTEM_MODE and the default DML operation is that sharing rules are not enforce in SYSTEM_MODE)

ImJohnMDaniel · 2023-01-04T15:22:25Z

@daveespo said:

The reason we created SimpleDML was to provide an easy way to extend default functionality without needing to reimplement method bodies (i.e. DRY)
Each time we added a new method to IDML and consumers of this library upgraded, they were forced to copy and paste a default implementation into their custom IDML implementations. By extending SimpleDML and just overriding the methods you want to modify, it keeps the intent of your changes clear.

Ok. We will leave it that question of inheritance alone.

ImJohnMDaniel · 2023-01-04T15:23:10Z

@daveespo said:

Unrelated: Rather than call this "UserModeDML" and have it only support USER_MODE, should we instead change this class to accept a DataAccess argument like the other classes touched in this PR so that you can specify SYSTEM_MODE or USER_MODE (the difference between SYSTEM_MODE and the default DML operation is that sharing rules are not enforce in SYSTEM_MODE)

Now this suggestion I like. Any suggestions on how this would be done??

cc: @stohn777

…tional constructor to UserModeDML to permit explicit SYSTEM_MODE

daveespo

Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions (waiting on @ImJohnMDaniel, @stohn777, and @tfuda)

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

Previously, daveespo (David Esposito) wrote…

This thread is unresolved but it may not be a blocker. Being able to override the IDML implementation via the constructor overload is existing behavior so for folks that already implemented CRUD/FLS enforcement in SOUOW, they already have a solution that works for generating an SOUOW instance with their IDML instance

I considered adding a setDML(IDML dmlImpl) to SOUOW but I don't like introducing the ability to change behavior of an instance after it's constructed and potentially loaded with records

Another option would be to pass an IDML instance to commitWork() via a new override (ex commitWork(IDML dmlImpl))

And further, the fflib_Application.UnitOfWorkFactory has an overload that takes an IDML implementation:

public virtual fflib_ISObjectUnitOfWork newInstance(List<SObjectType> objectTypes, fflib_SObjectUnitOfWork.IDML dml)

This would be the recommended way to pass in a UserModeDML for consumers that are using fflib_Application pattern

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

Previously, daveespo (David Esposito) wrote…

The reason we created SimpleDML was to provide an easy way to extend default functionality without needing to reimplement method bodies (i.e. DRY)

Each time we added a new method to IDML and consumers of this library upgraded, they were forced to copy and paste a default implementation into their custom IDML implementations. By extending SimpleDML and just overriding the methods you want to modify, it keeps the intent of your changes clear.

Unrelated: Rather than call this "UserModeDML" and have it only support USER_MODE, should we instead change this class to accept a DataAccess argument like the other classes touched in this PR so that you can specify SYSTEM_MODE or USER_MODE (the difference between SYSTEM_MODE and the default DML operation is that sharing rules are not enforce in SYSTEM_MODE)

@ImJohnMDaniel per our chat, I added an overloaded constructor for UserModeDML that lets you specify SYSTEM_MODE

(and rather than define a new enumeration, I was able to use the Apex AccessLevel enumeration because we are only supporting SYSTEM_MODE and USER_MODE with the new UserModeDML class; no need to have a LEGACY enumerated value)

clebrsonn · 2023-05-10T16:29:37Z

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls line 33 at r8 (raw file):

	implements fflib_ISObjectSelector
{
    public enum DataAccess{LEGACY, USER_MODE, SYSTEM_MODE}

Why not use the "FLSEnforcement" variable from the class:
"fflib_QueryFactory" here?

daveespo

Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions

sfdx-source/apex-common/main/classes/fflib_SObjectSelector.cls line 33 at r8 (raw file):

Previously, clebrsonn (Cleberson) wrote…

Why not use the "FLSEnforcement" variable from the class:
"fflib_QueryFactory" here?

There are 4 enumerated values in fflib_QueryFactory.FLSEnforcement -- i.e. there is a NONE option that isn't present in this enumeration

That was to support the somewhat incongruous prior behavior of only enforcing CRUD and not FLS via the two different boolean flags on the legacy Selector constructors (enforceCRUD and enforceFLS arguments)

With native User Mode support, we don't have the option to selectively enforce CRUD but not FLS so we wanted to make it clear to the user of the library that their only options going forward are USER_MODE or SYSTEM_MODE -- with LEGACY being a shim to remain backwards compatible.

#419 - implements a proposal for supporting native User Mode Database…

3cf0788

… Operations introduced in Summer '22

daveespo requested review from ImJohnMDaniel and stohn777 July 13, 2022 21:15

daveespo commented Jul 13, 2022

View reviewed changes

tfuda reviewed Jul 15, 2022

View reviewed changes

ImJohnMDaniel and others added 4 commits October 24, 2022 00:30

Merge branch 'master' into 419-user-mode-proposal

4c180b1

#419 -- we should continue to enforce legacy behavior both for the LE…

8cb7b05

…GACY option and the NONE option in order to preserve th expected behavior with regard to the normalization of the generated SOQL

#419 - wasn't properly passing the passed in argument to the overload…

899c325

…; Also, reintroduces the check for LEGACY FLS enforcement before actually asserting FLS isAccessible() (was removed in the prior commit on this feature)

tfuda reviewed Oct 26, 2022

View reviewed changes

daveespo commented Oct 26, 2022

View reviewed changes

stohn777 previously approved these changes Oct 27, 2022

View reviewed changes

Merge branch 'master' into 419-user-mode-proposal

07cd4b5

daveespo dismissed stohn777’s stale review via 1341481 November 30, 2022 20:20

daveespo commented Nov 30, 2022

View reviewed changes

daveespo marked this pull request as ready for review November 30, 2022 21:44

daveespo mentioned this pull request Dec 2, 2022

Add fflib_QueryFactory database operation mode support #434

Closed

stohn777 previously approved these changes Dec 5, 2022

View reviewed changes

chazwatkins mentioned this pull request Dec 6, 2022

How to get a selector without sharing apex-enterprise-patterns/at4dx#62

Open

tfuda reviewed Dec 7, 2022

View reviewed changes

#419 - whitespace and test method naming convention tchanges per requ…

e411f7c

…ests on the PR

daveespo dismissed stohn777’s stale review via e411f7c December 20, 2022 13:59

daveespo commented Dec 20, 2022

View reviewed changes

#419 - updates README to announce new feature and adds an overload of…

081a014

… the polymorphic Selector test to make sure it still produces valid SOQL in SYSTEM_MODE

daveespo added 2 commits December 20, 2022 09:55

#419 - apparently my wiki syntax was wrong; updates the README further

958aca3

#419 - apparently relative links don't work; changing to absolute

e347715

stohn777 previously approved these changes Dec 20, 2022

View reviewed changes

tfuda reviewed Dec 20, 2022

View reviewed changes

ImJohnMDaniel reviewed Dec 31, 2022

View reviewed changes

sfdx-source/apex-common/main/classes/fflib_SObjectUnitOfWork.cls Show resolved Hide resolved

Merge branch 'master' into 419-user-mode-proposal

3b4fa46

daveespo commented Jan 4, 2023

View reviewed changes

#419 - updates README to call attention to UserModeDML and adds an op…

d04b6d7

…tional constructor to UserModeDML to permit explicit SYSTEM_MODE

daveespo dismissed stohn777’s stale review via d04b6d7 January 9, 2023 16:00

daveespo commented Jan 9, 2023

View reviewed changes

ImJohnMDaniel approved these changes Jan 11, 2023

View reviewed changes

stohn777 approved these changes Jan 11, 2023

View reviewed changes

daveespo merged commit 5422faa into master Jan 11, 2023

daveespo deleted the 419-user-mode-proposal branch January 11, 2023 23:55

daveespo commented May 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#419 - implements a proposal for supporting native User Mode Database Operations introduced in Summer '22 #420

#419 - implements a proposal for supporting native User Mode Database Operations introduced in Summer '22 #420

daveespo commented Jul 13, 2022 •

edited

Loading

daveespo left a comment

tfuda left a comment

afawcett commented Oct 20, 2022 •

edited

Loading

afawcett commented Oct 20, 2022

afawcett commented Oct 21, 2022 •

edited

Loading

tfuda left a comment

daveespo left a comment

stohn777 left a comment

stohn777 Oct 26, 2022

stohn777 Oct 26, 2022

afawcett commented Nov 13, 2022 •

edited

Loading

daveespo left a comment

stohn777 commented Dec 5, 2022

tfuda left a comment

daveespo left a comment

tfuda left a comment

daveespo left a comment

ImJohnMDaniel commented Jan 4, 2023

ImJohnMDaniel commented Jan 4, 2023

daveespo left a comment

clebrsonn commented May 10, 2023

daveespo left a comment

		@@ -367,9 +402,22 @@ public abstract with sharing class fflib_SObjectSelector
		public fflib_QueryFactory newQueryFactory(Boolean assertCRUD, Boolean enforceFLS, Boolean includeSelectorFields)

		@@ -408,7 +456,8 @@ public abstract with sharing class fflib_SObjectSelector
		subSelectQueryFactory,

#419 - implements a proposal for supporting native User Mode Database Operations introduced in Summer '22 #420

#419 - implements a proposal for supporting native User Mode Database Operations introduced in Summer '22 #420

Conversation

daveespo commented Jul 13, 2022 • edited Loading

daveespo left a comment

Choose a reason for hiding this comment

tfuda left a comment

Choose a reason for hiding this comment

afawcett commented Oct 20, 2022 • edited Loading

afawcett commented Oct 20, 2022

afawcett commented Oct 21, 2022 • edited Loading

tfuda left a comment

Choose a reason for hiding this comment

daveespo left a comment

Choose a reason for hiding this comment

stohn777 left a comment

Choose a reason for hiding this comment

stohn777 Oct 26, 2022

Choose a reason for hiding this comment

stohn777 Oct 26, 2022

Choose a reason for hiding this comment

afawcett commented Nov 13, 2022 • edited Loading

daveespo left a comment

Choose a reason for hiding this comment

stohn777 commented Dec 5, 2022

tfuda left a comment

Choose a reason for hiding this comment

daveespo left a comment

Choose a reason for hiding this comment

tfuda left a comment

Choose a reason for hiding this comment

daveespo left a comment

Choose a reason for hiding this comment

ImJohnMDaniel commented Jan 4, 2023

ImJohnMDaniel commented Jan 4, 2023

daveespo left a comment

Choose a reason for hiding this comment

clebrsonn commented May 10, 2023

daveespo left a comment

Choose a reason for hiding this comment

daveespo commented Jul 13, 2022 •

edited

Loading

afawcett commented Oct 20, 2022 •

edited

Loading

afawcett commented Oct 21, 2022 •

edited

Loading

afawcett commented Nov 13, 2022 •

edited

Loading