Allow for different implementations of Application factories

[wimvelzeboer]

This PR is to create the possibility for different implementations of the Application factories.
It is fully backwards compatible with the static maps, as methods are replaced and directing to the new classes while showing a deprecated message with instruction on refactoring to the new classes.

This change is mainly done to allow for an implementation similar to force-di, or even make a direct link to it so that fflib can be used in combination with force-di.

The code is not yet entirely finished, but I mainly raised it now to open up the discussion to see the response on this idea.

---

This change is 

[ImJohnMDaniel]

@wimvelzeboer -- Can you compare how this approach is any different than the Application class/approach found in the AT4DX project? That Application already uses the Force-DI project and does not introducing a dependency from fflib-apex-common to Force-DI

• AT4DX Application class

[wimvelzeboer]

@ImJohnMDaniel It is kinda similar in what is tries to do, except this design is more on a higher level. AT4DX tries to wrap around some of the interface structure limitations.
For example; you have framework specific source-code in the Application class (e.g. Application.SelectorFactory). In my opinion the Application class should belong to the application and the selectorFactory is something of the framework. Combining the two different pieces of code is confusing for developers.
In this PR, the issue is solved by improving the interface structure if the framework itself, and allowing for different implementations on a higher level of abstraction.
The lack of that improved interface structure results into a lot of complexity as you can see in AT4DX.

I also prefer to have everything in one framework and not having to install too many sub-libraries. So, I think either AT4DX should be added to apex-common or something like this PR.

Using maps for class routing or force-di, that should just be another implementation. That's why I rather resolve the interface structure first and then work on the implementation like AT4DX.
The whole point of using Interfaces is that the implementation can be replaced, AT4DX is adding a second structure to the framework which makes things confusing.

[ImJohnMDaniel]

In my opinion the Application class should belong to the application and the selectorFactory is something of the framework.

I would agree with you. In the traditional implementation of the Application class (non-DX/2GP concerns), it does have that distinct separation.

Take a look at the Application.cls found in the fflib-apex-common-samplecode project. In that version, the Application.cls is completely specific to the application you are building and relies on the framework pieces from the fflib_Application.cls (found in this repo).

The AT4DX approach does move the Application class under the "framework" umbrella but every mapping is now dynamic in the ApplicationFactory CMDT objects. Each application (aka 2GP) inserts their own mappings into the CMDT object so that every application in the org can use the same, common convention of a "Application" class without the need to modify the mappings in the Application class directly (as is the situation in the classic implementation of the Application factory).

I also prefer to have everything in one framework and not having to install too many sub-libraries. So, I think either AT4DX should be added to apex-common or something like this PR.

While I hear you on this point, you are advocating the merging of potentially four frameworks -- Apex Mocks, Apex Common, Force-DI, and AT4DX. For those using Apex Common in the traditional way, they already have the sub-library of Apex Mocks and if we were to merge, there would be backward-compatibility issues around AT4DX's introduction of the Application.cls verses developers own, classic implementation of Application.cls

Granted, I need to take a deeper dive into the changes you are suggesting in this PR, so please give me some time to digest that. I should have some time in the coming days to do this.

Also, can you elaborate further on why the AT4DX Application class implementation confuses developers?

[wimvelzeboer]

Also, can you elaborate further on why the AT4DX Application class implementation confuses developers?

I think its mainly the different levels of abstraction. AT4DX is basically introducing a complete new Application factory / feature, duplicating the one from apex-common. While the OO principles would be more to keep the single feature / interface but with different implementations. Similar as the design principle of Force-DI, one feature-interface with multiple implementations.

Another example is the use of UnitOfWork in DomainProcessAbstractAction. Both are complete different features, but DomainProcessAbstractAction is using IApplicationSObjectUnitOfWork instead of an implementation of fflib_ISObjectUnitOfWork. IApplicationSObjectUnitOfWork is a wrapper around the original adding new features, but it would be more clear for developers if those additions are added to the original. I think they make a useful addition.

[ImJohnMDaniel]

Ok, I see what you are saying. It was the decision to not alter the fflib-apex-common UOW and other base classes to ensure backwards compatibility for the classic fflib-apex-common implementations.

@afawcett, @daveespo, and @stohn777 -- I would like you three to also take a deep look at this PR and add your comments, please.

[JAertgeerts]

@daveespo @afawcett @stohn777 can you please review this?

[stohn777]

@wimvelzeboer
I'm going to take a look at this, but for starters, could you please address the conflicts as you see fit respecting other changes to 'master'.

[wimvelzeboer]

@wimvelzeboer I'm going to take a look at this, but for starters, could you please address the conflicts as you see fit respecting other changes to 'master'.

Yes, it is indeed a bit outdated looking at the changes after I raised this PR. Let me work on rebasing and updating the code!

[wimvelzeboer]

@stohn777 I saw that there was a lot of overlap with other changes that were already accepted. So, I did some refactoring of those. Now this PR mainly includes the following:

Adds more documentation to Interface classes.

Adding of replaceWith method to factories
With this you can replace implementations in real-time. Very useful when you are doing AB testing or dynamic feature toggling.

Implement SOLID principles in the Application class,
which only references interfaces and not real implementations to make it easy to replace any implementation.

Adds extra setMock overloads to make mocking easier.
Instead of doing this:

fflib_ApexMocks mocks = new fflib_ApexMocks();
AccountsSelector selectorMock = (AccountsSelector) mocks.mock(AccountsSelector.class);
mocks.startStubbing();
mocks.when(selectorMock.sObjectType()).thenReturn(Schema.Account.SObjectType);
mocks.stopStubbing();
Application.Selector.setMock(selectorMock);

You can now do the same without the need to stub it:

fflib_ApexMocks mocks = new fflib_ApexMocks();
AccountsSelector selectorMock = (AccountsSelector) mocks.mock(AccountsSelector.class);
Application.Selector.setMock(Schema.Account.SObjectType, selectorMock);

This PR also replaces #403

[stohn777]

I'm not persuaded with the need for Application.Selector.setMock(SObjectType, fflib_ISObjectSelector). What is the benefit for adding a mock thing that hasn't been properly mocked, other than saving 3 lines of code which is important to the functionality of the Application's thing factories' internal maps?

Reviewable status: 0 of 25 files reviewed, 1 unresolved discussion (waiting on @afawcett, @daveespo, @dbtavernerffdc, @ImJohnMDaniel, @kjonesffdc, @swillcockffdc, and @wimvelzeboer)

a discussion (no related file):
Let me see if I got this right. This PR intends to offer a way to register and get a thing (Selector, Domain, etc.) from a SObjectType that is different from the SObjectType related to the Ids in the parameter. For instance, one might provide a list of Ids for child SObjects to the parent's thing which might select the related parents. Is my understanding up to par?

If so, I'd like the java-doc comments to explain that more explicitly.

If not, then please help me to understand what the intent of these methods.

The intent of the replaceWith methods is understandable, the others are less clear.

---

[wimvelzeboer]

I'm not persuaded with the need for Application.Selector.setMock(SObjectType, fflib_ISObjectSelector). What is the benefit for adding a mock thing that hasn't been properly mocked, other than saving 3 lines of code which is important to the functionality of the Application's thing factories' internal maps?

I did find a lot of code duplication in many unit-test where the only thing that was being stubbed was the SObjectType. Maybe the use-case is mainly with Domains than Selectors, as Domains do not always have anything to return. You then basically only want to verify that methods are invoked.

a discussion (no related file):
Let me see if I got this right. This PR intends to offer a way to register and get a thing (Selector, Domain, etc.) from a SObjectType that is different from the SObjectType related to the Ids in the parameter. For instance, one might provide a list of Ids for child SObjects to the parent's thing which might select the related parents. Is my understanding up to par?

The intend of the List<SObject> selectById(Set<Id> recordIds, SObjectType sObjectType) method, is to provide the SObjectType of the Ids. So that fflib doesn't need to determine the SObjectType and validate the Ids again.
Like in the context of a domain class:

public class Accounts extends fflib_SObjects implements IAccounts {
  public static IAccount newInstance(Set<Id> ids) {
    return (IAccounts) Application.newInstance(ids, Schema.Account.SObjectType);
  }
}

The main reason for this was to avoids run-time errors, in the case where the Id set is empty, when you do constructs like:

IContacts contactsDomain = Contacts.newInstance(contactRecordsWithoutAccount);
IAccounts domain = Accounts.newInstance(
    contactsDomain.getAccountIds()
);

Adding isEmpty() checks all over the place might become cumbersome, therefore we came up with this method providing that SObjectType to avoid the run-time exceptions.

[stohn777]

Continuing in line.

Reviewable status: 0 of 25 files reviewed, 4 unresolved discussions (waiting on @afawcett, @daveespo, @dbtavernerffdc, @ImJohnMDaniel, @kjonesffdc, @swillcockffdc, and @wimvelzeboer)

a discussion (no related file):

Previously, wimvelzeboer (William Velzeboer) wrote…

Let me see if I got this right. This PR intends to offer a way to register and get a thing (Selector, Domain, etc.) from a SObjectType that is different from the SObjectType related to the Ids in the parameter. For instance, one might provide a list of Ids for child SObjects to the parent's thing which might select the related parents. Is my understanding up to par?

The intend of the List<SObject> selectById(Set<Id> recordIds, SObjectType sObjectType) method, is to provide the SObjectType of the Ids. So that fflib doesn't need to determine the SObjectType and validate the Ids again.
Like in the context of a domain class:

public class Accounts extends fflib_SObjects implements IAccounts {
  public static IAccount newInstance(Set<Id> ids) {
    return (IAccounts) Application.newInstance(ids, Schema.Account.SObjectType);
  }
}

The main reason for this was to avoids run-time errors, in the case where the Id set is empty, when you do constructs like:

IContacts contactsDomain = Contacts.newInstance(contactRecordsWithoutAccount);
IAccounts domain = Accounts.newInstance(
    contactsDomain.getAccountIds()
);

Adding isEmpty() checks all over the place might become cumbersome, therefore we came up with this method providing that SObjectType to avoid the run-time exceptions.

Continuing in line.

---

---

sfdx-source/apex-common/main/classes/fflib_Application.cls, line 283 at r2 (raw file):

		 */
		public virtual List<SObject> selectById(Set<Id> recordIds, SObjectType sObjectType)
		{

This if statement seems to block the intended use case, instantiating a Selector with a blank set.

---

sfdx-source/apex-common/main/classes/fflib_Application.cls, line 339 at r2 (raw file):

		 *                    avoids the need to stub the mock to return its SObjectType
		 * @param selectorInstance The instance of the mocked selector
		 */

With "fflib_Application" being virtual, I believe a convenience method may exist in the concrete implementation, if needed.

---

sfdx-source/apex-common/main/classes/fflib_Application.cls, line 420 at r2 (raw file):

		 * @return Instance of a Domain containing the queried records
		 * @exception Throws an exception via the Selector Factory if the Ids are not all of the same SObjectType
		 **/

In our conversation above, the use case is defined as "calling this newInstance method with an empty set and SObjectType. The benefit is avoiding a runtime exception. However as I follow from this entry point, a runtime exception would still be generated on line 285 because the set is empty. The code on line 287, using the SObjectType, would not be reached.

FATAL_ERROR|fflib_Application.DeveloperException: Invalid record Id's set

Class.fflib_Application.SelectorFactory: line 285, column 1
Class.fflib_Application.DomainFactory: line 423, column 1
AnonymousBlock: line 1, column 1

Executed with:
    AccountsDomain accounts = (AccountsDomain) Application.Domains.newInstance(new Set<Id>(), Account.SObjectType);

Seemingly the if statement is unnecessary in that context, at line 284.