NEW @W-16360876@ Implemented describeRules() functionality for FlowTest #93

jfeingold35 · 2024-09-25T19:32:07Z

This PR does the following:

Deletes a bunch of files from the locally stored FlowTest folder that never should have been committed in the first place.
Updates several files within the locally stored FlowTest folder to provide the new API for rule description.
Utilizes that API to get descriptions for the FlowTest queries, and converts those into the CodeAnalyzer house style.

packages/code-analyzer-flowtest-engine/src/engine.ts

jfeingold35 · 2024-09-25T19:41:57Z

packages/code-analyzer-flowtest-engine/src/engine.ts

+        return flowTestRules.map(flowTestRule => {
+            return {
+                // The name maps directly over.
+                name: flowTestRule.query_name,


The query_name field has spaces in it, and the query_id field has periods in it. So I chose to use the one with spaces in it for now. We can have a discussion about what postprocessing might be needed to bring the names more into line with our best practices.

jfeingold35 · 2024-09-25T19:43:11Z

packages/code-analyzer-flowtest-engine/src/python/FlowTestCommandWrapper.ts

        ];

        let stdout: string = '';
        const processStdout = (stdoutMsg: string) => {
            stdout += stdoutMsg;
        }
        await this.pythonCommandExecutor.exec(pythonArgs, processStdout);
-        return stdout;
+        return (JSON.parse(stdout) as FlowTestRuleDescriptor[]).sort((a, b) => a.query_name.localeCompare(b.query_name));


The sorting is necessary because the output printed to stdout isn't sorted at this time. Robert intends to update it with sorting, and when he does I will change it here too.

packages/code-analyzer-flowtest-engine/src/python/FlowTestCommandWrapper.ts

...nalyzer-flowtest-engine/test/test-data/goldfiles/FlowTestCommandWrapper.test.ts/catalog.json

stephen-carter-at-sf · 2024-09-30T18:04:39Z

packages/code-analyzer-flowtest-engine/test/engine.test.ts

I see that code coverage for this flowtest package is no longer 100% like all other packages

code-analyzer-flowtest-engine/src | 100 | 100 | 100 | 100 | config.ts | 100 | 100 | 100 | 100 | engine.ts | 100 | 100 | 100 | 100 | messages.ts | 100 | 100 | 100 | 100 | plugin.ts | 100 | 100 | 100 | 100 | code-analyzer-flowtest-engine/src/python | 90.56 | 44.44 | 77.77 | 92.15 | FlowTestCommandWrapper.ts | 100 | 100 | 100 | 100 | PythonCommandExecutor.ts | 78.26 | 28.57 | 70 | 80.95 | 35-45 PythonVersionIdentifier.ts | 100 | 100 | 80 | 100 |

If possible, can we either add in code coverage for anything to be submitted or add in the ignore comments where code coverage isn't easily able to be covered?

Or will these be hit when the run method is implemented?

Improved: Now I see

------------------------------------------|---------|----------|---------|---------|------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ------------------------------------------|---------|----------|---------|---------|------------------- PythonVersionIdentifier.ts | 100 | 100 | 80 | 100 |

Maybe there is 1 function that isn't fully hit.

stephen-carter-at-sf · 2024-09-30T18:09:36Z

packages/code-analyzer-flowtest-engine/src/engine.ts

@@ -23,24 +24,60 @@ export class FlowTestEngine extends Engine {

    public async describeRules(_describeOptions: DescribeOptions): Promise<RuleDescription[]> {


The describe command should be looking at the workspace if it is provided to see if the workspace even contains relevant files (in this case, flow based xml files). If there is a workspace and there are no relevant files, then no rules should be returned. That is, we shouldn't be doing any work if there are no flow files in the workspace.

It looks like the extensions the FlowTest tool looks for are .flow-meta.xml and .flow. So I'll make the plugin look for those too.

stephen-carter-at-sf · 2024-09-30T18:11:46Z

packages/code-analyzer-flowtest-engine/src/engine.ts

        this.emitRunRulesProgressEvent(100);
        return {
            violations: []
        };
    }
+
+    private convertFlowTestRulesToCodeAnalyzerRules(flowTestRules: FlowTestRuleDescriptor[]): RuleDescription[] {


Nitpick: Can these private methods instead be functions?

Also maybe call this function toRuleDescription(flowTestRule: FlowTestRuleDiscriptor) and then simply make your line 29 to be

const ruleDescriptions: RuleDescription[] = flowTestRules.map(toRuleDescription);

packages/code-analyzer-flowtest-engine/src/python/FlowTestCommandWrapper.ts

stephen-carter-at-sf · 2024-10-01T13:11:15Z

packages/code-analyzer-flowtest-engine/src/engine.ts

-        return [];
+        const flowTestRules: FlowTestRuleDescriptor[] = await this.commandWrapper.getFlowTestRuleDescriptions();
+        this.emitDescribeRulesProgressEvent(75);
+        const convertedRules = flowTestRules.map(r => toRuleDescription(r));


Since it is now a function (and not tied to a class instance), I believe you could have also just done flowTestRules.map(toRuleDescription)

stephen-carter-at-sf · 2024-10-01T13:20:59Z

packages/code-analyzer-flowtest-engine/src/engine.ts

+function toRuleDescription(flowTestRule: FlowTestRuleDescriptor): RuleDescription {
+    return {
+        // The name maps directly over.
+        name: flowTestRule.query_name,


It looks like in the catalog.goldfile.json that an example query_name is Flow: SystemModeWithSharing recordUpdates data and here it looks like we are using this as the rule name. This does not seem good to me.

Robert closed the Enhancement Request for rules having shorter names (see https://git.soma.salesforce.com/SecurityTools/FlowSecurityLinter/issues/39) ... but it looks like he maintained spaces and colons in the names. For example I see

query_name = f"Flow Security: User access to {elem_type} {check_labels_val} in mode {run_mode}"
got turned into

query_name = f"Flow: {run_mode} {elem_type} {check_labels_val}"

which is still too long.

So either we need to go back to him and make these names even shorter without spaces and colons, or we'll need to create a mapping on our end (either fixed or dynamic).

For example on our end we could probably just remove the prefix "Flow: " and then convert all spaces to dashes for now or something. So instead of
Flow: SystemModeWithSharing recordUpdates data maybe the name could for now just be SystemModeWithSharing-recordUpdates-data

But I created https://git.soma.salesforce.com/SecurityTools/FlowSecurityLinter/issues/47 to ask that we may receive better names.

stephen-carter-at-sf · 2024-10-02T15:01:53Z

packages/code-analyzer-flowtest-engine/src/engine.ts

+            const workspaceFiles: string[] = await describeOptions.workspace.getExpandedFiles();
+            // If a workspace is provided but it contains no flow files, then return no rules.
+            if (!workspaceFiles.some(fileIsFlowFile)) {
+                this.emitLogEvent(LogLevel.Fine, 'Workspace contains no Flow files; returning no rules');


This seems relevant to possible user debugging... so maybe make this to LogLevel.Debug instead of LogLevel.Fine.

jfeingold35 force-pushed the d/W-16360876 branch from 5ae8f6f to 89833b5 Compare September 25, 2024 19:40

jfeingold35 commented Sep 25, 2024

View reviewed changes

jag-j reviewed Sep 26, 2024

View reviewed changes

packages/code-analyzer-flowtest-engine/src/python/FlowTestCommandWrapper.ts Show resolved Hide resolved

jag-j approved these changes Sep 26, 2024

View reviewed changes

stephen-carter-at-sf requested changes Sep 30, 2024

View reviewed changes

stephen-carter-at-sf requested changes Oct 1, 2024

View reviewed changes

jfeingold35 added 9 commits October 1, 2024 11:23

NEW @W-16360876@ Updated Flowtest with latest changes

c2d4066

NEW @W-16360876@ FlowTest supports describeRules method

97134b9

NEW @W-16360876@ Pulled in latest FlowTest changes

2f516bc

NEW @W-16360876@ Feedback from code review.

466022a

NEW @W-16360876@ Added test coverage for PythonCommandExecutor.

d0f6321

CHANGE @W-16360876@ Implemented feedback from code review.

b46b0cc

CHANGE @W-16360876@ FlowTest checks Workspace for flow files

3be2d99

CHANGE @W-16360876@ Increasing timeout for tests

4dc5c5f

CHANGE @W-16360876@ Feedback from code review.

d2ceb4c

jfeingold35 force-pushed the d/W-16360876 branch from 7b8eed0 to d2ceb4c Compare October 1, 2024 16:23

CHANGE @W-16360876@ Fixed failing Windows test

ca3c4f6

jfeingold35 force-pushed the d/W-16360876 branch from 0b5373f to ca3c4f6 Compare October 1, 2024 21:30

stephen-carter-at-sf reviewed Oct 2, 2024

View reviewed changes

CHANGE @W-16360876@ Feedback from code review

04e32eb

stephen-carter-at-sf approved these changes Oct 2, 2024

View reviewed changes

jfeingold35 marked this pull request as ready for review October 2, 2024 16:29

jfeingold35 merged commit abf6cd8 into dev Oct 2, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW @W-16360876@ Implemented describeRules() functionality for FlowTest #93

NEW @W-16360876@ Implemented describeRules() functionality for FlowTest #93

jfeingold35 commented Sep 25, 2024 •

edited

Loading

jfeingold35 Sep 25, 2024

jfeingold35 Sep 25, 2024

stephen-carter-at-sf Sep 30, 2024

stephen-carter-at-sf Oct 1, 2024 •

edited

Loading

stephen-carter-at-sf Sep 30, 2024

jfeingold35 Sep 30, 2024

stephen-carter-at-sf Sep 30, 2024

stephen-carter-at-sf Oct 1, 2024

stephen-carter-at-sf Oct 1, 2024

stephen-carter-at-sf Oct 2, 2024

		@@ -23,24 +24,60 @@ export class FlowTestEngine extends Engine {

		public async describeRules(_describeOptions: DescribeOptions): Promise<RuleDescription[]> {

NEW @W-16360876@ Implemented describeRules() functionality for FlowTest #93

NEW @W-16360876@ Implemented describeRules() functionality for FlowTest #93

Conversation

jfeingold35 commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-carter-at-sf Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfeingold35 commented Sep 25, 2024 •

edited

Loading

stephen-carter-at-sf Oct 1, 2024 •

edited

Loading