#199 removed 'with sharing' keyword from base classes to respect call…

[yurybond]

removed 'with sharing' keyword from base classes to respect caller sharing context

According to trailhead module about SOQ. The Selector should respect the context of the caller. It is not happening when base class has with/without sharing

Security - Avoid using the with sharing or without sharing keywords on Selector Apex classes to ensure that the calling context inherits this context. Typically, this is the Service class, which, as per its design guidelines, must specify with sharing... see more

@afawcett, @afawcettffdc

[yurybond]

@dfruddffdc Could you please review this pull request?

[afawcett]

So this is a correct change that addresses a historic oversight in the original library. The challenge is some code maybe reliant on it now, so this could be a breaking changing. Ideally, this would actually use "inherit sharing" (new for Winter'19), but that is still a potential breaking change for some.

One thought i have is this...

public inherit sharing fflib_SObjectSelectorBase
public with sharing fflib_SObjectSelector extends fflib_SObjectSelectorBase
public inherit sharing fflib_SObjectSelector2 extends fflib_SObjectSelectorBase

I would also use this as an opportunity to clarify the FLS/CRUD security by having fflib_SObjectSelect2 disable both

public fflib_SObjectSelector2() { this(false, false, false); }

[yurybond]

@afawcett See your point about backward compatibility ) So I have made changes in selector according to your recommendations.

But, for the domain class, I even not sure how to deal with it. fflib_SObjectDomain has a lot of subtypes and static variables which make it almost impossible to move something to the new Base class.

P.S. Is there somebody who can merge changes from pull requests?

[jstorey-milcorp-ita]

I like it. We should consider looking at the whole library from an "open, closed" principle.

[jstorey-milcorp-ita]

Suggested change

	<apiVersion>43.0</apiVersion>
	<apiVersion>44.0</apiVersion>

[afawcett]

@yurybond this looks good from a backwards comparability perspective for Selector. For Domain base class i agree thats a bigger challenge. I am honestly not sure what to do here, other than duplicate the statics and subtypes but that i feel will get quite out of control. I am wondering if we can do something like wrap the method handler invocations in a "with sharing" context dynamically based on a constructor arg (this could default to true or if the fflib_SObjectDomain2 is used default to false). Something placed in the triggerHandler logic might do the trick? Thoughts?

Also can we please retain API 37 for the new classes for now. The lib should be updated as a whole to a new API level and for that matter all classes (that don't want to explicit state it) leverage the new "inhirted sharing" keywords.... but thats a difference story/PR...

[afawcett]

@yurybond any thoughts on the above?

[yurybond]

@afawcett,

1. I can roll-back version upgrade for package.xml and fflib_SObjectSelector2.cls. But for fflib_SObjectSelectorBase.cls I am afraid we won't be able to save this class because inherited sharing was introduced in 43.

2. In terms of domain class:

• I guess it is better to make a changes in it in a separate branch
• I spent a couple of hours trying to create 2nd version of domain with support of all constants and backward compatibility but it was pain - there are too many cross-references with other classes.

Andy, what do you think about making a fork of all project where we can re-design domain which is now looks messy and maintaining of it is a real pain?

Also I have an idea to remove query builder from library. Query Builder was not applicable in projects with complex calculations because of high consumption of CPU time. The only benefit I saw there is checking access level to objects and fields, but shortly it will be supported by platform
https://releasenotes.docs.salesforce.com/en-us/spring19/release-notes/rn_apex_select_with_security_enforced.htm

[daveespo]

@yurybond -- can you say more about the CPU issues with QueryFactory? A merge was made in 2017 that resolved a bunch of the overhead with using QueryFactory. It was resolved in #105 with PR #107 and Chris Peterson notes that QueryFactory is used not just for string concatenation but for handling Field Sets where there may be overlapping lists of fields across multiple FieldSets (#105 (comment))

I agree that QueryFactory should be retrofitted to use the new WITH SECURITY_ENFORCED but that sounds like a different issue :-)

[resample-admin]

Can anyone comment on the status of this pull request?

I would like the 'inherited sharing' behavior. Wondering if I should modify my local files or if a a new version of fflib with them is coming soon.

[daveespo]

@afawcett -- I appreciate the breaking change that just changing SObjectSelector would cause, but with the changes to the Winter '20 Sites Guest User sharing model, many teams are now forced to more often run Apex in a Without Sharing context. I think we should either remove the sharing declaration (or use 'inherited sharing') for all classes.

A quick grep of the other Apex classes in this project and Apex Mocks shows that we have some 'with sharing' applied to classes that most certainly don't need it (fflib_StringBuilder, etc.) ... trying to exorcise those demons in a non-breaking-change way is much more complex.

I think that we just need to explicitly call this out as a breaking change in the README -- we've made some behavioral changes back in 2014

CC: @agarciaffdc @dfruddffdc

[daveespo]

One more comment on fflib_SObjectSelector: It's an abstract class ... it should really be up to the concrete implementation class to define the sharing model. Perhaps we should remove the sharing declaration altogether.

[daveespo]

We're going to close this for now given the conversation over on #262