@W-10759090@: Implemented method-level targeting for SFGE, and message-passing system to allow for proper logging. #710

jfeingold35 · 2022-06-01T19:06:11Z

This PR does the following:

Adds method-level targeting to scanner:run:dfa's --target flag. Syntax is path/to/file.cls#method1;method2.
Adds a new cli-messaging module that duplicates the functionality of the messaging package in pmd-cataloger.
Uses the new module to log messages when a method doesn't exist or has multiple matches.
Refactors CommandLineSupport and its child classes slightly.
Moves OutputProcessor.ts from pmd to services.

What it does NOT do:

Integrate the new module into pmd-cataloger. This will be done in a subsequent pull request. It was omitted this time because it lowers test coverage beyond acceptable minimum.

jfeingold35 · 2022-06-01T20:05:24Z

cli-messaging/src/main/java/com/salesforce/messaging/CliMessager.java

@@ -0,0 +1,99 @@
+package com.salesforce.messaging;


Basically a copy of pmd-cataloger/src/main/java/sfdc/sfdx/scanner/messaging/SfdxMessager.java.

jfeingold35 · 2022-06-01T20:06:02Z

cli-messaging/src/main/java/com/salesforce/messaging/EventKey.java

@@ -0,0 +1,59 @@
+package com.salesforce.messaging;


Basically a copy of pmd-cataloger/src/main/java/sfdc/sfdx/messaging/EventKey.java.

jfeingold35 · 2022-06-01T20:06:22Z

cli-messaging/src/main/java/com/salesforce/messaging/Message.java

@@ -0,0 +1,47 @@
+package com.salesforce.messaging;


Basically a copy of pmd-cataloger/src/main/java/sfdc/sfdx/messaging/Message.java.

jfeingold35 · 2022-06-01T20:07:01Z

cli-messaging/src/main/java/com/salesforce/messaging/MessagePassableException.java

@@ -0,0 +1,47 @@
+package com.salesforce.messaging;


Basically a copy of pmd-cataloger/src/main/java/sfdc/sfdx/messaging/SfdxScannerException.java.

jfeingold35 · 2022-06-01T20:07:42Z

cli-messaging/src/test/java/com/salesforce/messaging/EventKeyTest.java

@@ -0,0 +1,126 @@
+package com.salesforce.messaging;


Basically a copy of pmd-cataloger/src/test/java/sfdc/sfdx/messaging/EventKeyTest.java.

jfeingold35 · 2022-06-01T20:41:30Z

src/lib/services/CommandLineSupport.ts

-		cp.stderr.on('data', data => {
-			stderr += data;
-		});
+	protected abstract handleResults(args: ResultHandlerArgs): void;


New abstract method for doing any weird stuff that must be done with results.

jfeingold35 · 2022-06-01T20:41:46Z

src/lib/services/CommandLineSupport.ts

-				rej(stderr);
-			}
-		});
+	protected defaultResultHandler(args: ResultHandlerArgs): void {


Default implementation of abstract method. Most of the implementers will just call super.defaultResultHandler().

This feels a little awkward to me since this abstract method's implementations are invoking another protected non-final method, which is at the same level as the abstract method's definition. Could possibly lead to cyclic invocations if defaultResultHandler() is overridden.

A few common patterns we could consider in this case:

(Composition over inheritance)[https://en.wikipedia.org/wiki/Composition_over_inheritance] - this default implementation could be in a class outside of this hierarchy

Creating hooks to call additional steps before/after the default steps are done

jfeingold35 · 2022-06-01T20:42:15Z

src/lib/sfge/SfgeEngine.ts


 		try {
-			const output = await SfgeWrapper.runSfge(targetPaths, rules, JSON.parse(engineOptions.get(CUSTOM_CONFIG.SfgeConfig)) as SfgeConfig);
+			const output = await SfgeWrapper.runSfge(targets, rules, JSON.parse(engineOptions.get(CUSTOM_CONFIG.SfgeConfig)) as SfgeConfig);


Passing the targets instead of just the path components.

jfeingold35 · 2022-06-01T20:42:48Z

src/lib/util/EventCreator.ts

-			type: 'INFO', 
-			handler: 'UX', 
-			verbose: false, 
+			messageKey: eventTemplateKey,


Formatting changes done by IntelliJ without my consent.

jfeingold35 · 2022-06-01T20:43:13Z

test/lib/RuleManager.test.ts

 				expect(results[0].paths.length).to.equal(2, 'Wrong number of paths matched');
 			});

+			it('Positive method-level targets are properly matched', async () => {


New unit test for method-level targeting.

jfeingold35 · 2022-06-01T21:31:43Z

sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java

+	}
+
+	@Test
+	public void getTargetMethods_targetMethodInInnerAndOuterClass() {


Test case: A method that exists in both inner and outer class. QUESTION: The syntax here would be path/to/file.cls#methodName, and it would return both the inner and outer class methods. Are we okay with that? I'm okay with that for now, and we can change it if people complain.

jfeingold35 · 2022-06-01T21:32:13Z

sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java

+    }
+
+	@Test
+	public void getTargetMethods_targetMethodInInnerClass() {


Test case: Targeting a method in an inner class. As noted below, the syntax would be path/to/file.cls#methodName, without including the inner class.

rmohan20 · 2022-06-06T23:32:33Z

src/lib/sfge/SfgeWrapper.ts

 		const classpath = await this.buildClasspath();

 		const targetListFile = await this.createInputFile([this.createTargetJsons()]);
 		const sourceListFile = await this.createInputFile(this.projectDirs);


While QA'g this as an internal implementation, we had a discussion around passing a single file with both target and source instead of separate files but didn't have an iteration to make the changes. Could you address it with these changes? A single input file would help us move towards a configuration of sorts for executing SFGE.

I'll see what I can do.

…e-passing system to allow for proper logging.

…aloger and increase test coverage as necessary.

…etter readability.

jfeingold35 commented Jun 1, 2022

View reviewed changes

jfeingold35 marked this pull request as ready for review June 1, 2022 20:56

jfeingold35 commented Jun 1, 2022

View reviewed changes

rmohan20 reviewed Jun 6, 2022

View reviewed changes

rmohan20 approved these changes Jun 7, 2022

View reviewed changes

jfeingold35 added 6 commits June 7, 2022 13:54

@W-10759090@: Implemented method-level targeting for SFGE, and messag…

5a0c065

…e-passing system to allow for proper logging.

@W-10759090@: Added unit test coverage for inner classes.

f526cb5

@W-10759090@: Added help text documenting new feature.

6980d4b

@W-10759090@: Supplemental PR to integrate cli-messaging into pmd-cat…

9735b1d

…aloger and increase test coverage as necessary.

@W-10759090@: Refactored CLI result handler into separate class for b…

539e265

…etter readability.

@W-10759090@: Consolidated SFGE input into single file.

8882d41

jfeingold35 force-pushed the d/W-10759090-c branch from d8b895a to 8882d41 Compare June 7, 2022 18:54

jfeingold35 merged commit 5912f12 into dev-3 Jun 7, 2022

jfeingold35 mentioned this pull request Jun 7, 2022

D/w 10759090 b #709

Closed

jfeingold35 deleted the d/W-10759090-c branch June 23, 2022 15:39

@W-10759090@: Implemented method-level targeting for SFGE, and message-passing system to allow for proper logging. #710

@W-10759090@: Implemented method-level targeting for SFGE, and message-passing system to allow for proper logging. #710

Uh oh!

Conversation

jfeingold35 commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jfeingold35 commented Jun 1, 2022 •

edited

Loading