fflib-apex-common icon indicating copy to clipboard operation
fflib-apex-common copied to clipboard

Allow for different implementations of Application factories

Open wimvelzeboer opened this issue 4 years ago • 10 comments

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 Reviewable

wimvelzeboer avatar Sep 25 '20 13:09 wimvelzeboer

@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

ImJohnMDaniel avatar Sep 25 '20 14:09 ImJohnMDaniel

@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.

wimvelzeboer avatar Sep 25 '20 14:09 wimvelzeboer

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?

ImJohnMDaniel avatar Sep 25 '20 14:09 ImJohnMDaniel

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.

wimvelzeboer avatar Sep 25 '20 15:09 wimvelzeboer

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.

ImJohnMDaniel avatar Sep 25 '20 15:09 ImJohnMDaniel

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

JAertgeerts avatar Oct 26 '21 21:10 JAertgeerts

@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'.

stohn777 avatar Apr 10 '22 19:04 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'.

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 avatar Apr 11 '22 06:04 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

wimvelzeboer avatar Apr 11 '22 10:04 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.

wimvelzeboer avatar Apr 19 '22 12:04 wimvelzeboer