jamessimone / apex-rollup

Fast, configurable, elastically scaling custom rollup solution. Apex Invocable action, one-liner Apex trigger/CMDT-driven logic, and scheduled Apex-ready.
MIT License
210 stars 30 forks source link

Issues with OR in Where clause #494

Closed Avinava closed 10 months ago

Avinava commented 11 months ago

We have been using apex-rollup for setting up rolls and it has been working great overall. However, we have encountered an issue where the filter criteria and rollup values are incorrect when OR statements are added in the WHERE clauses.

Example:

(
  (
      Status = 'Escalated'
      AND Type IN ('System', 'Issue')
  )
  OR
  (
      Type IN ('Maintenance', 'Break Fix')
      AND Status != 'New'
  )
  OR
  (
      Status = 'In-Progress'
      AND Type = 'Upgrade'
  )
)

I am currently debugging this issue to identify the root cause, but I wanted to bring it to your attention as a heads up in case you were already aware of it

Screenshot 2023-09-13 at 6 28 56 PM

version : v1.5.84

jamessimone commented 11 months ago

I would need to see a log generated by the rollup running without successfully updating things. I will say that you have a workaround at present, which is creating a formula field to contain the logic in this where clause and then using that formula field in the where clause instead.

Avinava commented 11 months ago

@jamessimone yeah formula fields seems to be only work around, tried adding extra brackets around the clause doesn't seem to help.

Saw you added some fixes around OR clause, I am trying to replicate this using test class.

Will update you once I have something

Avinava commented 11 months ago
  @IsTest
  static void shouldWorkMultipleOrInWhereClause() {
    Account acc = [SELECT Id, Name FROM Account];

    insert new List<Opportunity>{
      new Opportunity(StageName = 'one', CloseDate = System.today(), AccountId = acc.Id, Name = 'one', Amount = 2),
      new Opportunity(StageName = 'two', CloseDate = System.today(), AccountId = acc.Id, Name = 'two', Amount = 2),
      new Opportunity(StageName = 'three', CloseDate = System.today(), AccountId = acc.Id, Name = 'three', Amount = 2),
      new Opportunity(StageName = 'four', CloseDate = System.today().addDays(1), AccountId = acc.Id, Name = 'four', Amount = 2)
    };

    Test.startTest();
    Rollup__mdt mdt = new Rollup__mdt(
      CalcItem__c = 'Opportunity',
      LookupObject__c = 'Account',
      RollupFieldOnCalcItem__c = 'StageName',
      LookupFieldOnCalcItem__c = 'AccountId',
      LookupFieldOnLookupObject__c = 'Id',
      RollupFieldOnLookupObject__c = 'SicDesc',
      RollupOperation__c = 'LAST',
      CalcItemWhereClause__c = '(StageName = \'one\' AND CloseDate = Today) OR (StageName = \'four\' AND CloseDate = Today)'
    );
    Rollup.performFullRecalculation(
      mdt 
    );
    Test.stopTest();

    List<Opportunity> opps = Database.query('SELECT Id, StageName FROM Opportunity WHERE ' + mdt.CalcItemWhereClause__c);
    System.assertEquals('one', opps[opps.size() - 1].StageName);
    acc = [SELECT SicDesc FROM Account];
    System.assertEquals('one', acc.SicDesc);
  }

I believe this should be able to replicate the issue

jamessimone commented 11 months ago

@Avinava thanks for providing this repro - I must have missed the notification about your last comment. I'll have a look and let you know.

jamessimone commented 11 months ago

@Avinava I was able to reproduce this issue and have fixed it. It will be go out in the next version of Apex Rollup to be released, v1.5.86. Please note that you'll need to update your where clause - it should be (StageName = \'one\' AND CloseDate = TODAY) OR (StageName = \'four\' AND CloseDate = TODAY) - in other words, the date literals for TODAY should be capitalized.

With your current version, this will already work, but at the moment it might trigger a full recalc every time due to a related issue with query parsing. That's the fix that will be going out in the next version.

Avinava commented 11 months ago

@jamessimone thanks for the quick fix ! I will try to get the latest commit in my dev box and test it !

Avinava commented 11 months ago

@jamessimone regarding the Date literal, I am just curious does capitalisation really matter with soql and dynamic query ? or it needs to be capitalised for apex-rollups internal classes ?

jamessimone commented 11 months ago

@Avinava SOQL is indeed case insensitive but apex rollup's internal classes are only partially SOQL-compliant with regards to sensitivity. I haven't expanded out the date literal support to be case insensitive.

Avinava commented 11 months ago

@jamessimone I tested lil bit more but seems like there is still an issue.

Below is a test condition that should work

@IsTest
  static void shouldWorkWithINandOR() {
    Account acc = [SELECT Id, Name FROM Account];

    insert new List<Opportunity>{
      new Opportunity(StageName = 'one', CloseDate = System.today(), AccountId = acc.Id, Name = 'one', Amount = 2, LeadSource = 'Web'),
      new Opportunity(StageName = 'two', CloseDate = System.today(), AccountId = acc.Id, Name = 'two', Amount = 2, LeadSource = 'Phone'),
      new Opportunity(StageName = 'three', CloseDate = System.today(), AccountId = acc.Id, Name = 'three', Amount = 2, LeadSource = 'Portal'),
      new Opportunity(StageName = 'four', CloseDate = System.today().addDays(1), AccountId = acc.Id, Name = 'four', Amount = 2, LeadSource = 'Walk In')
    };

    Test.startTest();
    Rollup__mdt mdt = new Rollup__mdt(
      CalcItem__c = 'Opportunity',
      LookupObject__c = 'Account',
      RollupFieldOnCalcItem__c = 'StageName',
      LookupFieldOnCalcItem__c = 'AccountId',
      LookupFieldOnLookupObject__c = 'Id',
      RollupFieldOnLookupObject__c = 'SicDesc',
      RollupOperation__c = 'LAST',
      CalcItemWhereClause__c = '(StageName = \'one\' AND LeadSource IN (\'Web\', \'Phone\') ) OR (LeadSource IN (\'Web\', \'Phone\') AND StageName != \'one\')'
    );
    Rollup.performFullRecalculation(mdt);
    Test.stopTest();

    List<Opportunity> opps = Database.query('SELECT Id, StageName FROM Opportunity WHERE ' + mdt.CalcItemWhereClause__c);
    System.assertEquals('two', opps[opps.size() - 1].StageName);
    acc = [SELECT SicDesc FROM Account];
    System.assertEquals('two', acc.SicDesc);
     // Validate that job ran as queueable - that the where clause was formatted correctly, in other words
    System.assertEquals('Completed', [SELECT Status FROM AsyncApexJob WHERE JobType = 'Queueable' LIMIT 1]?.Status);
  }
jamessimone commented 11 months ago

I will have a look! Thanks for bringing to my attention

jamessimone commented 11 months ago

@Avinava I've identified where this issue is occurring and I'll be working on a fix for it. However, it's possible that in the meantime you can get this working yourself:

CalcItemWhereClause__c = ' LeadSource IN (\'Web\', \'Phone\') AND (StageName = \'one\' OR StageName != \'one\')'

works, for instance. I am assuming these are simply example values, though? In any case, part of the current issue is that the LeadSource part of the clause is duplicated, and refactoring that out helps massively. I am going to continue to try to fix the actual underlying issue - just pointing out that depending on what your actual values are, you may have a path forward even before a fix is introduced. Thanks!