Default rules dont run when providing a custom rule configuration

[filiprafalowicz]

I have created following flow that is violating a bunch of Flow Scanner rules:

<?xml version="1.0" encoding="UTF-8"?>
<Flow xmlns="http://soap.sforce.com/2006/04/metadata">
    <apiVersion>49.0</apiVersion>
    <environments>Default</environments>
    <interviewLabel>BadFlow {!$Flow.CurrentDateTime}</interviewLabel>
    <label>BadFlow</label>
    <loops>
        <name>Loop_through_Contacts</name>
        <label>Loop through Contacts</label>
        <locationX>176</locationX>
        <locationY>431</locationY>
        <collectionReference>Get_Contacts</collectionReference>
        <iterationOrder>Asc</iterationOrder>
        <nextValueConnector>
            <targetReference>Update_Contact</targetReference>
        </nextValueConnector>
    </loops>
    <processMetadataValues>
        <name>BuilderType</name>
        <value>
            <stringValue>LightningFlowBuilder</stringValue>
        </value>
    </processMetadataValues>
    <processMetadataValues>
        <name>CanvasMode</name>
        <value>
            <stringValue>AUTO_LAYOUT_CANVAS</stringValue>
        </value>
    </processMetadataValues>
    <processMetadataValues>
        <name>OriginBuilderType</name>
        <value>
            <stringValue>LightningFlowBuilder</stringValue>
        </value>
    </processMetadataValues>
    <processType>AutoLaunchedFlow</processType>
    <recordLookups>
        <name>Get_Contacts</name>
        <label>Get Contacts</label>
        <locationX>176</locationX>
        <locationY>323</locationY>
        <assignNullValuesIfNoRecordsFound>false</assignNullValuesIfNoRecordsFound>
        <connector>
            <targetReference>Loop_through_Contacts</targetReference>
        </connector>
        <filterLogic>and</filterLogic>
        <filters>
            <field>AccountId</field>
            <operator>EqualTo</operator>
            <value>
                <elementReference>$Record.Id</elementReference>
            </value>
        </filters>
        <getFirstRecordOnly>false</getFirstRecordOnly>
        <object>Contact</object>
        <storeOutputAutomatically>true</storeOutputAutomatically>
    </recordLookups>
    <recordUpdates>
        <name>Copy_1_of_Update_Contact</name>
        <label>Copy 1 of Update Contact</label>
        <locationX>264</locationX>
        <locationY>647</locationY>
        <connector>
            <targetReference>Loop_through_Contacts</targetReference>
        </connector>
        <inputReference>Loop_through_Contacts</inputReference>
    </recordUpdates>
    <recordUpdates>
        <name>Update_Contact</name>
        <label>Update Contact</label>
        <locationX>264</locationX>
        <locationY>539</locationY>
        <connector>
            <targetReference>Copy_1_of_Update_Contact</targetReference>
        </connector>
        <inputReference>Loop_through_Contacts</inputReference>
    </recordUpdates>
    <start>
        <locationX>50</locationX>
        <locationY>0</locationY>
        <connector>
            <targetReference>Get_Contacts</targetReference>
        </connector>
        <object>Account</object>
        <recordTriggerType>Create</recordTriggerType>
        <triggerType>RecordAfterSave</triggerType>
    </start>
    <status>Draft</status>
</Flow>

When I run sf flow scan command without any additional parameters and with no .flow-scanner.json file defined (so I am using default rules only) I am getting following output:

=== Flow: BadFlow (9 results)
Type: AutoLaunchedFlow

RULE                      TYPE           NAME
────────────────────────  ─────────────  ────────────────────────
Copy API Name             recordUpdates  Copy_1_of_Update_Contact
DML Statement In A Loop   recordUpdates  Update_Contact
DML Statement In A Loop   recordUpdates  Copy_1_of_Update_Contact
Missing Flow Description  description    undefined
Flow Naming Convention    name           BadFlow
Missing Fault Path        recordLookups  Get_Contacts
Missing Fault Path        recordUpdates  Update_Contact
Missing Fault Path        recordUpdates  Copy_1_of_Update_Contact
Missing Null Handler      recordLookups  Get_Contacts

This looks as expected. However, if I run the Flow with my custom config like this:

{
    "rules": {
        "FlowName": {
            "severity": "error",
            "expression": "[A-Za-z0-9]+_(SCR|SFL|SCH|EVT|EML|BeforeCreate|BeforeUpdate|BeforeDelete|BeforeCreateBeforeUpdate|AfterCreate|AfterUpdate|AfterDelete|AfterCreateAfterUpdate)_[A-Za-z0-9]+"
        }
    }
}

Where I only update what should be the pattern for the Flow Naming Convention, then if I run sf flow scan command again, I am getting following output:

=== Flow: BadFlow (1 results)
Type: AutoLaunchedFlow

RULE                    TYPE  NAME
──────────────────────  ────  ───────
Flow Naming Convention  name  BadFlow

So all of the other rules, that I have not defined in my flow-scanner.json file (because I don't want to override them) are not being checked. Not sure if it is working this way as designed, but this is definitely because of the logic in GetRuleDefinitions where custom config does not use DefaultRuleStore. I think ideally selectedRules should be initialized with default rules and then custom config should be processed and provided configuration should be overidden.

[RubenHalman]

@filiprafalowicz Thank you for this detailed feedback. The core module is indeed intended to execute all default rules when no rules are explicitly specified. Your suggestion for initializing selectedRules with default rules and then processing custom configurations to override them sounds like a sensible approach and could enhance the functionality of the too, but would one still be able to deselect default rules?

In the current setup, users can specify rules without providing an "Expression" attribute value, effectively avoiding any unintended overrides of default rules. In any case please let us know if this sufficiently supports your use case and if perhaps we could improve on the documentation to make this more clear!

[filiprafalowicz]

I guess this is the matter of how this plugin should be used. I can list all of the rules in my configuration and not provide any additional details like this:

{
    "rules": {
        "APIVersion": {},
        "CopyAPIName": {},
        "DMLStatementInLoop": {},
        "DuplicateDMLOperation": {},
        "FlowDescription": {},
        "HardcodedId": {},
        "MissingFaultPath": {},
        "MissingNullHandler": {},
        "SOQLQueryInLoop": {},
        "UnconnectedElement": {},
        "UnusedVariable": {},
        "FlowName": {
            "severity": "error",
            "expression": "[A-Za-z0-9]+_(SCR|SFL|SCH|EVT|EML|BeforeCreate|BeforeUpdate|BeforeDelete|BeforeCreateBeforeUpdate|AfterCreate|AfterUpdate|AfterDelete|AfterCreateAfterUpdate)_[A-Za-z0-9]+"
        }
    }
}

And that will do the trick for me. However it is a little unintuitive for me that when I want to override a single rule, I need to specify all of them. I was thinking more of how configuration works in eslint where you extend default configuration and then you can provide your own overrides and keep default / recommended configuration. Current approach in lightning-flow-scanner is more similar to PMD where you just list all of the rules you want to use with additional configuration if necessary, but if you don't list anything, it will use default configuration.

I guess this is where the decision has to be made on what is expected and how it should be used, and documentation should be updated to mention these things.

[RubenHalman]

I think your suggestion makes a lot of sense going forward when introducing more rules and custom rules. But as is, we are not there yet, so please bear with us! If you feel comfortable working this out further we'd be more than happy to connect and take your help on making this more intuitive! For now I'll keep this open, thank you for the valuable feedback!

[nvuillam]

@filiprafalowicz that's a good suggestion, usually linters have a set of default rules, and the local configuration file is "merged" in this default set of rules, meaning that the day new rules appear, you don't have to update your locally defined config to benefit of them :)