apex-enterprise-patterns / fflib-apex-common

Common Apex Library supporting Apex Enterprise Patterns and much more!
BSD 3-Clause "New" or "Revised" License
913 stars 517 forks source link

fflib_ISObjectSelector is missing method signatures. #333

Closed wimvelzeboer closed 3 years ago

wimvelzeboer commented 3 years ago

Hi, When I was going through the fflib-apex-common-examples repository I found an incompatibility with the way selector classes are instantiated and some of the functionality in the classes.

Selectors are supposed to be instantiated via the static newInstance method on the main implementation of the selector.

public interface ICasesSelector extends fflib_ISObjectSelector
public inherited sharing class CasesSelector extends fflib_SObjectSelector implements ICasesSelector
{
    ...
    public static ICasesSelector newInstance()
    {
        return (ICasesSelector) Application.Selector.newInstance(Schema.Case.SObjectType);
    }
    ...
}

So that you can create an instance via:

List<Case> records = CasesSelector.newInstance().selectById(...);

But if you create a selector method that needs to do a SubSelect, then this approach doesn't work. You would expect that the following would work:

public interface IContactsSelector extends fflib_ISObjectSelector 
{
   List<Contact> selectByIdWithCases(Set<Id> idSet);
}
public inherited sharing class ContactsSelector extends fflib_SObjectSelector implements IContactsSelector
{
    ...
    public List<Contact> selectByIdWithCases(Set<Id> idSet)
    {
        fflib_QueryFactory queryFactory = newQueryFactory();

        CasesSelector.newInstance()
                .addQueryFactorySubselect(queryFactory);

        return (List<Contact>) Database.query(queryFactory.setCondition('Id in :idSet').toSOQL());
    }
    ...
}

This fails on the call to addQueryFactorySubselect, as that method is not part of the fflib_ISObjectSelector interface, which is returned by the newInstance() method.

It would work if the newInstance method would return an instance of CasesSelector, but that would mean that we do not have the flexibility of dependency injection anymore.

Another solution would be to add those methods to the interface class of the selector (ICasesSelector), but that doesn't really make sense.

The only solution I see is to include the methods (addQueryFactorySubselect, configureQueryFactoryFields) to the fflib_ISObjectSelector interface.

Do you see any other solution for this?

ImJohnMDaniel commented 3 years ago

@wimvelzeboer why would you not instantiate the other selector class directly as demonstrated here?

wimvelzeboer commented 3 years ago

@ImJohnMDaniel I think that the whole point of returning an interface by the newInstance method is to allow for different implementations. If you implement the selector method in the way it is showed in the example, then it creates a hard reference between two different selector classes. There is no way to replace the implementation anymore.

This is very limiting when you want to do things like A-B testing and feature switching, where you are heavily depended on dependency injection.

I have a scenario where we have a new feature that is dynamically enabled for a selected group of users. In the current application we have a ContactsSelector referencing the CasesSelector. The new updated feature is not touching the ContactsSelector but it does make a change to the CasesSelector method getSObjectFieldList, then the ContactsSelector needs to be able to dynamically route to the new CasesSelector implementation.

public inherited sharing class ContactsSelector extends fflib_SObjectSelector implements IContactsSelector
{
    ...
    public List<Contact> selectByIdWithCases(Set<Id> idSet)
    {
        fflib_QueryFactory queryFactory = newQueryFactory();
        CasesSelector.newInstance()
                .addQueryFactorySubselect(queryFactory);
        return (List<Contact>) Database.query(queryFactory.setCondition('Id in :idSet').toSOQL());
    }
    ...
}
public inherited sharing class CasesSelectorA extends fflib_SObjectSelector implements ICasesSelector
{
    ...
    public List<Schema.SObjectField> getSObjectFieldList()
    {
        return new List<Schema.SObjectField> {
                Case.Id,
                Case.Subject,
                Case.OriginalField__c
        };
    }
}
public inherited sharing class CasesSelectorB extends fflib_SObjectSelector implements ICasesSelector
{
    ...
    public List<Schema.SObjectField> getSObjectFieldList()
    {
        return new List<Schema.SObjectField> {
                Case.Id,
                Case.Subject,
                Case.SomeOtherField__c
        };
    }
}

The Application class is modified to return the correct implementation for the user based on Custom Permissions which enables the (new/updated) feature.

daveespo commented 3 years ago

Couldn't you cast the return value of the newInstance() method to fflib_SObjectSelector? Example:

((fflib_SObjectSelector)CasesSelector.newInstance()).addQueryFactorySubselect(queryFactory)

I'm not against adding addQueryFactorySubselect and configureQueryFactoryFields to fflib_ISObjectSelector but it sounds like the use case is a little exotic. And if we add those 2 methods, should we be adding all of the public methods on fflib_SObjectSelector?

wimvelzeboer commented 3 years ago

That is Indeed a way to do it. Although I am never that fond of casting.