Improve ability to customize functionality about fflib-apex-common HOT 7 CLOSED

Thanks for this @jondavis9898, do you have some use cases or examples that prompted this thought?

from fflib-apex-common.

Why yes I do :) I'm working on some requirements that need to expand the base functionality for specific scenarios. I could fork and customize but I'm trying to keep underlying details "abstracted" to ease support/maintenance in the future and since the changes I need to make are relatively minor. By being able to override methods (e.g. commitWork), I can tweak things just slightly. Currently, the only viable approach is to directly modify fflib classes which makes upgrade pathing as fflib adds new features difficult.

Here's some use cases I've worked on just in the last couple of days:

1. Automatic "Forced (developer mode) rollups" - In fflib_SObjectUnitOfWork, implement automatic rollup triggering (using DLRS Developer mode) after DML but before IDoWork is processed in commitWork. To do this, need to add a new member variable to class and override commitWork, howevre unable to do either due to protection levels. Additionally, check validation errors from domain classes and throw exception (see #2 for more details). We have record counts of more than 200 so doing rollups this way (instead of using triggerHandler keeps DML statements, queries, etc. to a minimum).
2. Keep Track of "Validation Errors" based on Record Id - In fflib_SObjectDomain, implement a "ValidationErrors" static variable similar to Errors. Given our record counts, validation must occur after all DML. This class is virtual but all inner pieces are private and/or not virtual (e.g. can't re-use re-use the FieldError, ObjectError, etc.).
3. "Before Validations" - In fflib_SObjectDomain, need to do some "before validations" and currently no method exposed for this (before only calls onApplyDefaults/onBeforeInsert/Update, etc.). All that's needed is to override handleBeforeInsert & handleBeforeUpdate but protection levels prohibit.

I have some more use cases but hopefully that gives you a feel for what I'm thinking. As time permits, I'm hoping to get a fork created and issue a pull request. In the meantime, I need to get this POC working :) I think opening up the library would make extending functionality fairly trivial and would drive a lot of value when "customization" is needed that the wider fflib audience would likely not value much from.

from fflib-apex-common.

Yeah thanks for these, and indeed would be good to see some more use cases, to see if any extensibility commonalities manifest. The problem with opening up all methods in a more 'liberal' sense without use cases like is it becomes a bit of nightmare to evolve the library once such methods are out in the field, without risk of breaking backwards compatibility, simply because you never know who has used what and in what combination. Its a balance between keeping implementation private and thus upgradable easily vs exposing enough customisation of behaviour.

So if we need more hooks like IDoWork and/or it to fire at different stages, i'd like to see use cases drive extensibility via interface approaches or specific method overrides rather than just open up more virtual or protected methods (which would also warrant more unit tests for each of the increasing amount of potential uses cases).

RE: 1) Yes, perhaps some new preCommitWork and postCommitWork new methods to override?

RE: 2) Have you seen the error method in the base class currently, this might do what you want? Its actually there to help with a unit test scenario (in a time where DMLException was light on detail), but still present and might work for you?

RE: 3) can you elaborate on the type of validations you mean by "do some before validations", why are the current hooks not enabling this?

Please keep me posted on more use cases and your POC very interested, this is great feedback!

from fflib-apex-common.

Agreed that it's a double-edge sword to open up the library for extensibility/customizing behavior. I think it comes down to whatever the primary goals of the project are. If providing a "canned" package (e.g. similar to a binary distribution or managed package install in the SFDC world) that supports extensibility through interfaces is the goal, then changing to less protected methods would go against that objective. If the goal is to provide a general base framework with the expectation that most will extend it, then opening up protection levels makes sense. I think it's fair to expect that as new versions come out, anyone extending the framework through derivation would be responsible for making sure the base framework changes don't break their customizations. That said, while I don't think it would add significantly more unit tests, etc. to the base, opening up protection levels would require some additional coverage and would make you think twice before implementing a change. For example, in a recent release the m_dirtyListByType was changed from a List to Map<Id, SObject>. If this member variable was protected, doing this would almost certain to break derived classes that might be accessing that member directly. There are both pros/cons to both alternatives and at the end of the day, it is your library :) I know in my case, having it more open would be much easier but I can definitely see why keeping the internals private makes a ton of sense and is likely the better option for the broader fflib audience.

RE: 1) pre/postCommitWork methods would allow order of operations to be controlled. The current IDoWork doesn't provide a means for controlling order of execution of the work so that's another option as well (exposing methods that can control the order) but I like the pre/postCommit methods to override. However, for the rollups I need to have a member variable that maps SObjectType to Map<Id, SObject> just like dirty/deleted/etc. I would do this in a derived class (assuming pre/postCommitWork is added) but then I need to override constructor and loop through objecttypes to build the map initially. This looping would be then be performed again in the base constructor. Possibly the solution here is a registerType method so that the default constructor loops through the types passed in and calls registerType on each one. The base implementation of that sets all the "m_" members while a derived version can use the opportunity to do it's own registration.
Recommendations: Could get really robust with methods at each "stage" of commitWork (e.g. preDMLOperations, postDMLOperations, preWork, postWork, etc.). At a minimum, I think preCommitWorkt (before any DML), postCommitWork (end of try block), commitWorkFail (inside of catch), and commitWorkComplete(inside of a finally block - in our use case, we must always decrement a reference counter). Also, registerSObjectType for registering a type that the UOW is aware of.

2. I've seen the base Error class. What I needed to build here is a Map<Id, List> instead of just a flat error list. I also need to add some additional metadata that needs to be tracked and wanted to make "Validation" issues different than "Errors." To do this, I created a new factory in exactly same manner as ErrorFactory along with helper methods in SObjectDomain directly. The challenge here is that I'm unable to instantiate FieldError & ObjectError directly in a derived class because the default constructor is marked private.
   Recommendations: Possibly make the FieldError, ObjectError and Error classes public & virtual since any derived use would not effect base usage?

3. The current onValidate hooks occur in after events. In our application, we have conditionally required fields (SFDC Validations won't work for these), extremely custom FLS checks, etc. I'd like to do all these up-front to improve performance so having a before validation hook would be desirable. The alternative is that the onBeforeInsert/onBeforeUpdate in the derived class checks these but having a discrete onBeforeValidate keeps the concerns separate. One interesting note here is that handleBeforeInsert is marked virtual but all other handleXXX methods are not. Not sure why one is and the others aren't but would be great if they all were virtual. Assuming those methods and their signatures aren't anticipated to change, I don't think this would require a lot of extra unit tests or considerations moving forward.
   Recommendation: Add onBeforeValidate methods or mark handleXXX methods virtual

As you requested, more use cases:

1. We have implemented a custom security solution since we cannot rely on Roles/Profiles for security (long story - let me know if you're interested). FFLib has the "EnforcingTriggerCRUDSecurity" but these checks are tightly bound to the handleXXX methods. Possibly having a method checkPermission that could be overridden with the base class implementation using what is currently in the handleXXX methods.
2. Similar to #1, would like to implement custom solution for Selector FLS checks. Currently SecurityUtils is used but it's tightly coupled and no ability to override or inject a different securityutils class.
3. In fflib_SObjectSelector, 99.9% of our select statements will always have an IsActive__c = true on them. Would be nice if we could override default functionality to include this condition by default.

Sorry for the lengthy post, hope this is helpful. Appreciate your consideration and thoughts!

from fflib-apex-common.

Sorry for the delay in getting back to you here...

Regarding your points above...

RE: 1, thanks for your submission on the event interface, i've now merged this!
RE: 2, happy to make those public, please submit a PR.
RE: 3, yes, odd one has virtual and others do not, happy with your recommendation here please submit a PR.

Regarding your use cases...

RE: 1, i'm not clear if your saying, you cannot do what you need with the current library or if its a case that you can do what you want, its just not as elegant as you'd want?
RE: 2, ok, so sounds like we need an interface between security utils and selector, so that a selector implementation or custom selector base class can inject different handling, be interested in seeing your ideas on what methods would be on such an interface? Maybe open a new issue to discuss?
RE: 3, i think there is extensibility for this, how about this... Could you do this by having your own common application selector base class, that implements the getSObjectFIeldList method, calls your own virtual method say getAdditionalSObjectFields and merges the result with your standard fields?

from fflib-apex-common.

Thanks Andrew, no worries on the delay!

Regarding the recommendations:
RE: 1 - Awesome, thank you!
RE: 2 - Created PR #68
RE: 3 - Created PR #69

Regarding use cases:
RE: 1 - I created PR #70 to demonstrate what I was thinking. To answer your question, I can do what's needed it's just not as elegant as the permission check concern isn't separated. By separating, the default implementation can leverage the libraries EnforcingTriggerCRUDSecurity while a derived class can implement its own security checks without having to override other methods that would otherwise not be necessary to override.
RE: 2 - Spot on with what I was thinking. I've opened #71 to further discuss.
RE: 3 - Your approach works for determining the list of fields but I was focused on the condition (WHERE) clause. My apologies on the confusion here, I should have said fflib_QueryFactory in addition to fflib_SObjectSelector. What I was thinking here is that we'd have a base Selector that derives from SObjectSelector and it would have an abstract that would indicate if it supports "IsActive" field. Then, through the chain of getting a new QueryFactory, the QF would know to automatically append the "AND (IsActive__c = true)" unless told not to. To do this the fflib classes would need to be "opened" up in some way to customize functionality. What we ended up doing for this is having discrete selectActiveXXX methods. For our selectors, we would very rarely call the non-selectActive flavor of a selector method so originally I was thinking to do the inverse and automatically add in the IsActive__c = true condition. After thinking this through some more, I think trying to do that complicates the library and even our code so having a discrete selectActiveXXX and leaving the API as-is makes the most sense to me.

from fflib-apex-common.

Great, i think we are getting into a flow and sync on what works and what doesn't here. So I'll close this issue as it seems its run its course and we have some good outputs. Thanks again for all your work here and elsewhere! 👍

from fflib-apex-common.