Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove 'with sharing' from fflib_SObjectSelector and fflib_SObjectDomain #199

Closed
yurybond opened this issue Sep 14, 2018 · 5 comments · Fixed by #262
Closed

Remove 'with sharing' from fflib_SObjectSelector and fflib_SObjectDomain #199

yurybond opened this issue Sep 14, 2018 · 5 comments · Fixed by #262

Comments

@yurybond
Copy link
Contributor

There is a big inconsistency in definition of selector base class and trailhead documentation of how it shoud be used.

  1. According to trailhead we need to avoid using of 'with/without sharing' on selectors to allow to apply calling context (for instance respect of controller's context when use selector method) See more:
    https://trailhead.salesforce.com/en/modules/apex_patterns_dsl/units/apex_patterns_dsl_learn_selector_l_principles

  2. For some base class for all selector classes fflib_SObjectSelector has 'with sharing' which is inherited and applied to child class (in other words selector that inherit base class fflib_SObjectSelector will ignore caller 'sharing' context). See more
    https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_classes_keywords_sharing.htm

  3. I am afraid removing of 'with sharing' keyword from base selector class and base domain class will affect a lot of people around the world, but we have to do it to make it work properly.

@yurybond yurybond changed the title Remove 'with sharing' from fflib_SObjectSelector Remove 'with sharing' from fflib_SObjectSelector and fflib_SObjectDomain Sep 14, 2018
yurybond added a commit to yurybond/fflib-apex-common that referenced this issue Sep 14, 2018
… classes to respect caller sharing context
yurybond added a commit to yurybond/fflib-apex-common that referenced this issue Oct 19, 2018
yurybond added a commit to yurybond/fflib-apex-common that referenced this issue Oct 19, 2018
yurybond added a commit to yurybond/fflib-apex-common that referenced this issue Oct 19, 2018
@jjulicher
Copy link

I totally agree with this.
Although with the latest update perhaps this should be inherited sharing?

@yurybond
Copy link
Contributor Author

@jjulicher indeed! According to @afawcett recommendation I created a new base class with inherited sharing. Andrew's comment in on my pull request: #200

@wimvelzeboer
Copy link
Contributor

wimvelzeboer commented Jan 7, 2020

I also agree that the base class should be set to inherited sharing so that the caller can decide how to use it.

It would be nice to use the following pattern that I also use for Service classes.

Application.cls

public with sharing class Application
{
    public static final fflib_Application.SelectorFactory Selector =
            new fflib_Application.SelectorFactory(
                    new Map<SObjectType, Type>
                    {
                            Account.SObjectType => AccountsSelectorImp.class
                    }
            );

    public static final fflib_Application.SelectorFactory ElevatedSelector =
                new fflib_Application.SelectorFactory(
                        new Map<SObjectType, Type>
                        {
                                Account.SObjectType => AccountsSelectorElevatedImp.class
                        }
                );
}

AccountsSelector.cls
This inherited sharing class contains all the selector methods.

public abstract inherited sharing class AccountsSelector extends sflib_SObjectSelector
{
    ...
    public AccountsSelector()
    {
        super();
    }
    public AccountsSelector(Boolean includeFieldSetFields, Boolean enforceCRUD, Boolean enforceFLS)
    {
        super(includeFieldSetFields,enforceCRUD,enforceFLS);
    }
    ...
    public List<Account> selectById(Set<Id> idSet)
    {
        return (List<Account>) selectSObjectsById(idSet);
    }
    ...
}

AccountsSelectorImp.cls
This is an empty class that is extended from the AccountsSelector and add the with sharing

public with sharing class AccountsSelectorImp extends AccountsSelector {}

AccountsSelectorElevatedImp.cls
This without sharing class only contains a constructor telling the framework to bypass CRUD and FLS checks.

public without sharing class AccountsSelectorElevatedImp extends AccountsSelector
{
    public AccountsSelector()
    {
        super(false, false, false);
    }
}

With this you can get a with or without sharing instance directly from the Application class.

@ImJohnMDaniel
Copy link
Contributor

@daveespo, @afawcett, @yurybond, @wimvelzeboer, and @jjulicher -- With the introduction of PR #262, are we agreed that we can close this PR??

Any questions or concerns on this course of action?

@daveespo
Copy link
Contributor

The subject of this ticket is about SObjectDomain and SObjectSelector however all of the talk and pull request is about SObjectSelector. Yes, #262 provides a viable workaround to the work proposed on this ticket so this ticket and its related pull request should be closed/wontfix

(there are some words I may want to say about SObjectDomain soon but I don't want to muddy up this ticket with those)

daveespo added a commit that referenced this issue Jan 30, 2020
…thods

#199 - mark fflib_SObjectSelector methods as virtual
jsardello pushed a commit to lampo/salesforce-apex-common that referenced this issue Jul 10, 2020
* Added new overload to registerRelationship to allow reference by External Id. The changes cannot be covered by a Unit Test due to lack of such references on standard sObjects. Changes were tested manually in a scratch org in execute anonymous using custom objects.

* Default Sorting considering invalid field and shield

Financial Force library (FFLIB) by default sorts record based on CreatedDate. Some object (ie. account share) might not have the field and hence will through error.  This change checks the field encryption (taken from @wimvelzeboer ) and if the object does not have created date field, assigns Id as default sorting field.

* Cleaned up the code based on pull request feedback.

* Update fflib_SObjectSelector.cls

Fixing the comment-blocking indenting issues (caused by IDE)

* Update fflib_SObjectSelector.cls

Updated the access modifiers from protected to private.

* correct spelling of word updates within comment and exception message

* Adding new method to mocks

* Rebrand as FFLib Apex Common & correct copyright notices to match git history

* ChildRelationship -> Schema.ChildRelationship

* upgrade api version

* Allow mocking for getChangedRecords

The hardcoded reference to the readonly Trigger.oldMap is replace so that it can be mocked in unit-tests

* Add mocking for getChangedRecords via Test.Database

* Add empty recycle bin to UnitOfWork

* Update README.md

* Add fluent design to the class constructor

With this change a selector can be configured via AccountsSelector selector = new AccountsSelector().enforceFLS();

* Add missing return statement for fluent design to the class constructor

With this change a selector can be configured via AccountsSelector selector = new AccountsSelector().enforceFLS();

* Add the option to extend the Application class.

This will allow an extension that could combine the force-di repo with the fflib-apex-commons.

* Updated to 47.0

* Fix for failing unit test by adding required methods to implementations

* 186 - consistent license version across source

* apex-enterprise-patterns#199 - mark fflib_SObjectSelector methods as virtual that do actual SOQL -- permits override in a 'without sharing' subclass

* Fixes apex-enterprise-patterns#263 - test assertion using string with typo in it (caused by apex-enterprise-patterns#234)

* Update fflib_SObjectUnitOfWork.cls

typo

* Fixes apex-enterprise-patterns#259

Changes made:
* Migrated all code to Salesforce DX Source format

* Update to document this change

* Update README.md

Updated Deploy button to not hard code the repo name. This means branch deploys work.

* Updating API version of Apex Commons (#1)

* Updating API version of Apex Commons

Updating API version of Apex Commons

* Adding Code Owners

Adding Code Owners

* Update from apex enterprise patterns master of fflib_Apex_Common (#3)

* Added new overload to registerRelationship to allow reference by External Id. The changes cannot be covered by a Unit Test due to lack of such references on standard sObjects. Changes were tested manually in a scratch org in execute anonymous using custom objects.

* Default Sorting considering invalid field and shield

Financial Force library (FFLIB) by default sorts record based on CreatedDate. Some object (ie. account share) might not have the field and hence will through error.  This change checks the field encryption (taken from @wimvelzeboer ) and if the object does not have created date field, assigns Id as default sorting field.

* Cleaned up the code based on pull request feedback.

* Update fflib_SObjectSelector.cls

Fixing the comment-blocking indenting issues (caused by IDE)

* Update fflib_SObjectSelector.cls

Updated the access modifiers from protected to private.

* Adding new method to mocks

* Rebrand as FFLib Apex Common & correct copyright notices to match git history

* upgrade api version

* Allow mocking for getChangedRecords

The hardcoded reference to the readonly Trigger.oldMap is replace so that it can be mocked in unit-tests

* Add mocking for getChangedRecords via Test.Database

* Update README.md

* Add fluent design to the class constructor

With this change a selector can be configured via AccountsSelector selector = new AccountsSelector().enforceFLS();

* Add missing return statement for fluent design to the class constructor

With this change a selector can be configured via AccountsSelector selector = new AccountsSelector().enforceFLS();

* Add the option to extend the Application class.

This will allow an extension that could combine the force-di repo with the fflib-apex-commons.

* Updated to 47.0

Co-authored-by: Rick Parker <45234060+TheRickParker@users.noreply.github.com>
Co-authored-by: Tahsin Zulkarnine <tahsinz@gmail.com>
Co-authored-by: dbtavernerffdc <dbtaverner@financialforce.com>
Co-authored-by: kjonesffdc <31822118+kjonesffdc@users.noreply.github.com>
Co-authored-by: Jan Aertgeerts <jan@janaertgeerts.be>
Co-authored-by: William Velzeboer <wgj.velzeboer@gmail.com>
Co-authored-by: John M. Daniel <ImJohnMDaniel@users.noreply.github.com>
Co-authored-by: Andrew Fawcett <andy@andyinthecloud.com>

* Add idea to git ignore

Co-authored-by: Rick Parker <rick.lee.parker@gmail.com>
Co-authored-by: Tahsin Zulkarnine <tahsinz@gmail.com>
Co-authored-by: dbtavernerffdc <dbtaverner@financialforce.com>
Co-authored-by: Connor Jerow <cgjerow@gmail.com>
Co-authored-by: Kevin Jones <kjones@financialforce.com>
Co-authored-by: Stefano Pallicca <stefano.pallicca@imagicle.com>
Co-authored-by: Jan Aertgeerts <jaertgee@its.jnj.com>
Co-authored-by: William Velzeboer <wgj.velzeboer@gmail.com>
Co-authored-by: William Velzeboer <wvelzebo@its.jnj.com>
Co-authored-by: Wim Velzeboer <wimvelzeboer@outlook.com>
Co-authored-by: John M. Daniel <ImJohnMDaniel@users.noreply.github.com>
Co-authored-by: Andrew Fawcett <andy@andyinthecloud.com>
Co-authored-by: David Esposito <dave@patrontechnology.com>
Co-authored-by: David Esposito <233309+daveespo@users.noreply.github.com>
Co-authored-by: Marek Madejski <marekmadejski@yandex.com>
Co-authored-by: John M. Daniel <imjohnmdaniel@ce-v.com>
Co-authored-by: Rick Parker <45234060+TheRickParker@users.noreply.github.com>
Co-authored-by: kjonesffdc <31822118+kjonesffdc@users.noreply.github.com>
Co-authored-by: Jan Aertgeerts <jan@janaertgeerts.be>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants