apex-enterprise-patterns / fflib-apex-common

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

Override Default "Order By" Behavior #162

Closed DeviousBard closed 2 years ago

DeviousBard commented 7 years ago

Currently, a selector's default "order by" (from the "getOrderBy()" method) is always added first in the list of fields in the Query Factory's order list when the "newQueryFactory()" method is called.

Typically, a custom query is going to require its own unique ordering fields that do not include the defaults. There is no way to override the default "order by" behavior in the selector class, other than partially building a query factory, clearing the order list, and then adding the actual order fields to sort by -- which is extremely awkward, and makes the code difficult to read.

To make matters worse, even removing the "getOrderBy()" method does not fix the problem, because its absence causes a default ordering of "Name ASC NULLS FIRST" to be pre-pended to the order list.

The attached "AccountsSelector.cls" demonstrates the issue.

It has the following default "order by" method:

public override String getOrderBy() {
    return 'Id ASC';
}

It has the following selector method:

public List<Account> selectByBillingState(Set<String> billingStates, Integer recordLimit) {
    assertIsAccessible();
    String query = newQueryFactory()
        .setCondition('BillingState IN :billingStates')
        .setLimit(recordLimit)
        .addOrdering('BillingState', fflib_QueryFactory.SortOrder.ASCENDING)
        .toSOQL();
    System.debug('\n\n\nquery:\n' + query + '\n\n\n');
    return (List<Account>) Database.query(query);
}

I would expect the resulting query from the selector method to look like this:

SELECT BillingAddress, Description, Id, Name FROM Account WHERE BillingState IN :billingStates ORDER BY BillingState ASC NULLS FIRST LIMIT 20

But instead, due to the default "order by" in the selector, the query looks like this ("Id ASC NULLS FIRST" is pre-pended to the "ORDER BY" list):

SELECT BillingAddress, Description, Id, Name FROM Account WHERE BillingState IN :billingStates ORDER BY Id ASC NULLS FIRST, BillingState ASC NULLS FIRST LIMIT 20

Completely removing the "getOrderBy()" method results in the following query (the "Id" field is now replaced by the "Name" field at the beginning of the "ORDER BY"):

SELECT BillingAddress, Description, Id, Name FROM Account WHERE BillingState IN :billingStates ORDER BY Name ASC NULLS FIRST, BillingState ASC NULLS FIRST LIMIT 20

Neither of the alternative SOQL queries achieves the desired result, which would be to sort the account list by the "BillingState" field only.

DeviousBard commented 7 years ago

I'd like to have a discussion on a possible solution to this problem.

While attempting to fix this issue on my own, I realized most of the problem seems to be that the "fflib_SObjectSelector" class is not completely decoupled from the new fflib_QueryFactory class. The query factory's responsibility should be to generate the entire SOQL query. However, there is a good bit of logic that still resides in the "fflib_SObjectSelector" that is generating parts of the SOQL query.

I moved all of the logic out of the "fflib_SObjectSelector", and changed the query factory's constructor to take the selector as a parameter. I also moved all of the Boolean flags used to configure the query factory that affect the query generation up to the query factory class (i.e. CRUD assertion, FLS enforcement, sorting of select fields, inclusion of the selector's query fields, and inclusion of the selector's default "order by"). I also removed those parameters from the query factory constructors in favor of "applying" them to the query factory (in builder fashion). I think it was better to default them, and just let the caller change the ones that need to overridden. There are just too many combinations of parameters to make multiple constructors effective.

The changes I made also made it easier to change the way subselect queries can be added to a query factory.

This is the way a call to create a query with a subselect must be coded with the existing code base:

fflib_QueryFactory opportunitiesQueryFactory = newQueryFactory(true);

fflib_QueryFactory lineItemsQueryFactory = 
    new OpportunityLineItemsSelector().
        addQueryFactorySubselect(opportunitiesQueryFactory);

new PricebookEntriesSelector().
    configureQueryFactoryFields(lineItemsQueryFactory, 'PricebookEntry');

new ProductsSelector().
    configureQueryFactoryFields(lineItemsQueryFactory, 'PricebookEntry.Product2');

new PricebooksSelector().
    configureQueryFactoryFields(lineItemsQueryFactory, 'PricebookEntry.Pricebook2');

return (List<Opportunity>) Database.query(
    opportunitiesQueryFactory
    .setCondition('id in :idSet')
    .addOrdering('Type', fflib_QueryFactory.SortOrder.DESCENDING)
    .toSOQL());

With the changes that I've made, here's how the same query can be created:

String query = newQueryFactory()
    .withoutFLSEnforcement()
    .withCRUDAssertion()
    .withSelectorFieldsIncluded()
    .withoutSelectorOrderByIncluded()
    .withSelectFieldsSorted()
    .subselect(new OpportunityLineItemsSelector().newQueryFactory()
        .withSelectorFieldsIncluded()
        .withSelectFieldsSorted()
        .selectRelatedFields(new PricebookEntriesSelector(), 'PricebookEntry')
        .selectRelatedFields(new ProductsSelector(), 'PricebookEntry.Product2')
        .selectRelatedFields(new PricebooksSelector(), 'PricebookEntry.Pricebook2')
        )
    .setCondition('Id in :idSet')
    .addOrdering('Type', fflib_QueryFactory.SortOrder.ASCENDING)
    .toSOQL();
return (List<Opportunity>) Database.query(query);

Notice how the Boolean configuration flags are added using the ".with" or ".without" notation. I think it makes the code a bit more explicit as to what the intent is.

Attached are the three class files I changed to make this work as shown above.

fflib_ISObjectSelector.cls fflib_SObjectSelector.cls fflib_QueryFactory.cls

o-lexi commented 7 years ago

It seems like the same problem I ran into a while ago, however I didn't collaborate with Andy, and didn't have much time to investigate the root cause at that time. I was able to overcome it but overriding getOrderBy with the field that has all nulls or same values (I am using isDeleted).

afawcett commented 7 years ago

Ooooh @DeviousBard i love this, sooo much nicer!

I would love to get this into a PR and merged in, would you be open to contributing this further? If so what needs consideration though is backwards compatibility, the library is quite widely used now. Whats your thoughts on retaining this? Or should it just be in a brand new selector base class? fflib_SObjectSelecto2? Are you changes to fflib_QueryFactory mostly additional?

afawcett commented 7 years ago

@capeterson whats your thoughts on this btw? @DeviousBard has leveraged the fflib_ISObjectSelector interface with fflib_QueryFactory.

DeviousBard commented 7 years ago

@afawcett, I think I can incorporate my changes into the existing code set to make it backward compatible. I just removed a lot of code while I was developing it, to make it easier to keep of track of only what I was working on.

I've already made some changes to what I posted here earlier.

DeviousBard commented 7 years ago

@afawcett, I was able to incorporate my changes for the enhanced fflib_QueryFactory, and retain backward compatibility with the existing code. All of the current tests run successfully, and additional tests were added to exercise the new code.

Attached are the updated classes.

fflib-apex-common-QueryFactoryEnhancements.zip

thegogz commented 7 years ago

@DeviousBard why not submit a PR with your changes? They look great, I've run into this exact issue a few times and the current solution is less than ideal especially if you want different order by clauses for the same SObject types

afawcett commented 6 years ago

@DeviousBard yes please do submit a PR for this, it will be easier to see the changes and get a merge done! 👍

glenncoppens commented 6 years ago

We just had a major issue when using the current Ordering approach. Not knowing that the default Name-Ordering was added, so we just added extra Ordering on CreatedDate which wouldn't give us the most recent record (also, our tests should have covered this though, but unfortunately they didn't).

Anyway, @DeviousBard, it would be great to see your pull request submitted! 👍

rsoesemann commented 6 years ago

@DeviousBard I am also waiting for this desperately.

yurybond commented 6 years ago

Was the issue with Name/CreatedDate in ordering fixed?

afawcett commented 6 years ago

@rsoesemann it looks like we have the code from @DeviousBard, so could create our own PR.

afawcett commented 6 years ago

Oh i see we have a PR, excellent! https://github.com/financialforcedev/fflib-apex-common/pull/179

daveespo commented 2 years ago

Looks like PR #179 was merged eons ago and this issue should've been closed along with it