Supporting inheritance for query factory

[tahsinz]

In certain projects, the FFLIB library can be used as a seperated package (locked, unlocked or managed). Other package or library might have a need to inherit query factory to add or modify certain functionality and need the core library intact. Currently, the class is not abstract or virtual so the class can be inherited and also variables are private so can't be accessed by the inherited class. So this class makes the class virtual and change the access modifiers (private to projected).

[tahsinz]

@afawcett , just wondering why the build failed? Can you also review the pull request?

[daveespo]

I don't know if I agree with the spirit of this PR.

Exposing internal implementation details for override by subclasses makes for brittle, unmaintainable subclasses. A minor change in internal base class implementation will break your subclass' expectation of behavior/lifecycle, etc.

Instead, you should propose methods with clear arguments and return values for manipulating internal state details.

[tahsinz]

Thanks, @daveespo for raising the concern. I'm not sure how much familiar are you with the recent modular deployment changes in apex through SFDX. Going forward in the apex development route, this PR will be critical to support FFLIB library for modular deployments.

Let me give you an example. In an SFDX modular deployment environment, you can create folders and create dependency among the folders to that development and deployment can be modular and easy to manage in the long run. For example, in a sample project, we can have folders: FFLIB, core (providing functionalities to all clouds), service (only service cloud), community (only community cloud). FFLIB code comes directly from Financial force repo, and other folders have custom code. If community cloud developers want to enhance the query factory to include few more methods (such as adding IN, LIKE, etc filters) which they frequently use, why they will include it? FFLIB folder is not the option as all other developers worldwide might not need it, core folder is not a good option as other clouds won't need that. Hence community folder will be the appropriate one. But, there is the problem, the community cloud cant inherit query factory as it is not abstract or virtual. I hope it clarifies why this inheritance is so crucial in the modular SFDX development.

In regards to addressing your point, the subclasses are not included in the FFLIB library hence, there is no possibility of breaking the build. In the future, we might add subclass with essential functionality, but the unit test classes will ensure that the old functionality does not break. Im not sure when the code is going through reviews, how can an unmaintainable subclass be added to the repo? This inheritance PR does provide an object-oriented trajectory and align very well with software development lifecycle.

[daveespo]

I do understand the development model of SFDX -- we are using it too. However, I'm not sure the changes you're proposing have much to do with SFDX -- even in a monolithic MDAPI development project, the same thing applies.

I'm not objecting to declaring QueryFactory as virtual, that's fine -- Most of the other fflib classes are marked as virtual (SObjectUnitOfWork and SObjectDomain)

I'm objecting to marking all of the members as protected so that they can be manipulated by subclasses.

If you have a specific needs for overriding functionality in a subclass, a more formal interface should be proposed. If you take a look at #19 as an example, it's a formal interface for overriding DML behavior. Possibly coupled with some form of dependency injection.

[stohn777]

I'm in support of more openly extensible classes for flexibility and maintainability. At some point one's needs will diverge from another's with a common class meeting neither's.

I'm encouraged that FFLIB will evolve well and meet the needs of most teams minimizing the overall need for customization, yet when some team needs that peculiar wrinkle of functionality that is not appropriate for inclusion in a canon class, then design flexibility options should exist.

I also agree that dependency injection is another powerful tool that could be leveraged more in FFLIB. Both DI and extensibility are principles in SOLID development.

1. SOLID (wikipedia)

[ImJohnMDaniel]

@tahsinz -- I am with @daveespo on this. I am not a big fan of exposing the inner workings of the query factory.

To handle your questions around SFDX adoption, I would recommend you to the look at Force-DI and the AT4DX repos. They demonstrate the utilization of the FFLIB repo and dependency injection techniques to effectively provide the separation and modularization that you are looking for. Here are two talks from TDX18 and DF18 that explain the approach.

[stohn777]

As to the degree this change opens the fflib_QueryFactory, I'm a bit wary too. More accessibility points could be warranted, although raising every method and property to Protected access is concerning.

[afawcett]

RE: DX

I can see how you might associate this requirement with SFDX as it encourages modularization further. Your desire to follow the best practice of not modifying an open source code base is for sure solid, though it does apply regardless if said source file is in the root of your project (pre-DX) or a lib folder.

RE: Extensibility Needs

So putting SFDX aside for a moment, I also think extensibility is a key need of any library. Whats important as well though is how its exposed per @daveespo comments above. Extensibility is a feature request and thus has to be backed by known uses cases that allow for an explicit exposure rather than a blanket exposure that cannot be tracked or supported in every possible use case going forward.

RE: The requirement

@tahsinz you mention that you want the ability to add additional filter methods for IN and LIKE etc. Thats a good requirement and i would consider either a) actually making the change to the fflib_QueryFactory to support these (recommend sharing a brief spec first) and then of course raising a PR to have it reviewed or b) make just the class virtual (per suggestions above) and perhaps add onBeforeToSOQL virtual method that is called at the start of the toSOQL method to allow final state of the factory to be set by an extending class prior to the SOQL query string being constructed.

To further elaborate on b). With the above hooks in place, you would then extend the fflib_QueryFactory with your own class and add more advanced WHERE clause building methods. To ensure that your extension class had a chance to call the setCondition method just prior to the caller calling the toSOQL you would override the onBeforeToSOQL method and use this as your moment to compile the WHERE clause string.

Thoughts on Next Steps....

In the end option b) done generically is the same as a), accept that everyone gets to see your fine work! That said, the reason why the where clause part of the SOQL has not had the method treatment as its more advanced given the richness of this part of the SOQL syntax. So... you might want to go with option a) for now and incubate/evolve your extension class containing the WHERE clause methods in your own code base and then check back with another PR in a few months (or whenever) with how things have ended up.

Andy

[tahsinz]

Updates


Thanks, @afawcett for the thorough feedback. It was really helpful. For now, I have decided to go through the option b for this pull request. I have made the classes virtual, changed access modifiers for some private variables to protected, added the onBeforeSOQL virtual method so that the extension classes can override it with advanced where clauses or filters and utilize the setCondition method. Finally, the onBeforeSOQL is called at the start of the toSOQL method to allow the final state of the factory to be set by an extending class prior to the SOQL query string being constructed.

Thought for Next Steps

In the near future, I will work to come with a brief spec for adding additional filters (LIKE, IN, etc) to the fflib_Querfactory so that everyone can use them.

@dbtavernerffdc , it will be highly appreciated if you can review the pull request.

[daveespo]

I think these recent commits miss the point of the objections that Andy, the Johns and I have.

By exposing internal properties for manipulation by a subclass, the behavior of QueryFactory could change or become inconsistent depending on order of operations.

If the subclass needs to read the values, you should expose protected getters but by just marking the property as protected, it allows a subclass to modify the value.

Example:

public virtual class fflib_QueryFactory{
 ...
  private Boolean enforceFLS;

  ...

  protected Boolean isEnforcingFLS(){return enforceFLS;}
}

I think that this PR should only include the following changes:

• Make fflib_QueryFactory virtual
• Add the onBeforeSOQL() hook -- I think it should be protected, not public since it should never be called from outside
• For any of the existing internal private variables that don't have getter methods defined, define a protected getter for them

Notably, the limitCount and offsetCount variables already have public getters/setters defined so I don't know what the point of making the variables themselves marked as public is trying to accomplish. Also, there are public setEnforceFLS and setCondition but no equivalent getters for those; that seems like an oversight and they should be added.

[daveespo]

@tahsinz -- you're welcome to reopen this thread if you want to propose a new implementation for your desired extensibility needs