apex-enterprise-patterns / fflib-apex-common

Common Apex Library supporting Apex Enterprise Patterns and much more!
BSD 3-Clause "New" or "Revised" License
911 stars 517 forks source link

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

Closed daveespo closed 1 year ago

daveespo commented 2 years ago

This change is Reviewable

afawcett commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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';
    }
afawcett commented 1 year ago

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?

stohn777 commented 1 year ago

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

ImJohnMDaniel commented 1 year ago

@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 commented 1 year ago

@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

clebrsonn commented 1 year ago

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?