Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW (SFGE) @W-17915877@ PathStart functionality for SFGE #250

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ public abstract class AbstractMetaInfoCollector implements MetaInfoCollector {
private static final String APEX_FILE_EXTENSION = ".cls";
protected final TreeSet<String> collectedMetaInfo;
private final TreeSet<String> acceptedExtensions = getAcceptedExtensions();
private final TreeSet<String> pathsWalked;
private boolean projectFilesLoaded;

AbstractMetaInfoCollector() {
this.collectedMetaInfo = CollectionUtil.newTreeSet();
this.pathsWalked = CollectionUtil.newTreeSet();
}

/**
Expand Down Expand Up @@ -61,13 +63,21 @@ public TreeSet<String> getMetaInfoCollected() {

private void processSourceFolder(String sourceFolder) throws MetaInfoLoadException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, sourceFolders can now be a list of files.
But I didn't want to chase down and rename a million different variables, so I didn't do that. I can if people want, but I think I'd prefer to do it in a follow-up PR to keep this one readable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind it as a follow-up! Sounds like a good cleanup item 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in reading through the code, I think sourceFolder for the parameter is still right, isn't it? Later I see this, and the invocation reads right to me:

for (String sourceFolder : sourceFolders) {
       processSourceFolder(sourceFolder);
}

Path path = new File(sourceFolder).toPath();
// If the directory has any apex files, we should assume it's the class folder and that
// project files are in a sibling.
// So we'll go up a level before walking the file tree.
if (directoryContainsApex(path)) {
if (isDirectoryContainingApex(path)) {
// If the path is a directory with apex files in it, we should assume it's the class folder, and that project
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If the path is a directory with apex files in it, we should assume it's the class folder, and that project
// If the path is a directory with apex files in it, we should assume it's the classes folder, and that project

// files are in a sibling. So we'll go up a level before walking the file tree.
path = path.getParent();
} else if (isApexFile(path)) {
// If the path itself is an apex file, we should assume that it is contained in the class folder, and that
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If the path itself is an apex file, we should assume that it is contained in the class folder, and that
// If the path itself is an apex file, we should assume that it is contained in the classes folder, and that

// project files are in a sibling of its parent directory. So we'll go up two levels before walking the file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify for me what you mean by "project files" here?

// tree.
path = path.getParent().getParent();
}
final ProjectFileVisitor projectFileVisitor = new ProjectFileVisitor();
if (this.pathsWalked.contains(path.toString())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matters because when the sourceFolder paths are lists of individual files in the same folder, then path will end up being the same for all of them and we don't want to walk the same path trees repeatedly.

return;
}
this.pathsWalked.add(path.toString());
try {
Files.walkFileTree(path, projectFileVisitor);
} catch (IOException ex) {
Expand All @@ -78,7 +88,7 @@ private void processSourceFolder(String sourceFolder) throws MetaInfoLoadExcepti
}
}

private boolean directoryContainsApex(Path path) {
private boolean isDirectoryContainingApex(Path path) {
final File dir = path.toFile();
// Non-directories obviously don't have any apex.
if (!dir.isDirectory()) {
Expand All @@ -95,6 +105,15 @@ private boolean directoryContainsApex(Path path) {
return Arrays.stream(dirContents).anyMatch(f -> f.getName().endsWith(APEX_FILE_EXTENSION));
}

private boolean isApexFile(Path path) {
final File file = path.toFile();

if (!file.isFile()) {
return false;
}
return file.getName().endsWith(APEX_FILE_EXTENSION);
}

protected boolean pathMatches(Path path) {
final String pathExtension = getPathExtension(path);
return acceptedExtensions.contains(pathExtension);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import com.salesforce.rules.ApexFlsViolationRule;
import com.salesforce.rules.Violation;
import com.salesforce.rules.fls.apex.operations.FlsConstants;

import java.nio.file.Path;
import java.util.Collections;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -43,6 +45,11 @@ public void loadCustomSettings_testWithRootFolder(TestInfo testInfo) throws Exce
verifyBaseDirIsAvailable(testInfo, "root");
}

@Test
public void loadCustomSettings_testWithIndividualApexFiles(TestInfo testInfo) throws Exception {
verifyBaseDirIsAvailable(testInfo, Path.of("classes", "MyClass.cls").toString());
}

@Test
public void testCustomSettingRegistered(TestInfo testInfo) {
final MetaInfoCollector metaInfoCollector =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

import com.salesforce.TestUtil;
import com.salesforce.graph.ops.GraphUtil;

import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collections;
import java.util.Set;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
Expand Down Expand Up @@ -61,6 +64,28 @@ public void loadVisualForce_testWithClassFolder(TestInfo testInfo) throws Except
}
}

@Test
public void loadVisualForce_testWithIndividualApexFiles(TestInfo testInfo) throws Exception {
TestUtil.compileTestFiles(g, testInfo);
try {
MetaInfoCollectorTestProvider.setVisualForceHandler(new VisualForceHandlerImpl());
String[] sourceFiles = new String[]{
TestUtil.getTestFileDirectory(testInfo).resolve(Path.of("classes", "MyController.cls")).toString(),
TestUtil.getTestFileDirectory(testInfo).resolve(Path.of("classes", "MyNonController.cls")).toString()
};
MetaInfoCollector metaInfoCollector = MetaInfoCollectorProvider.getVisualForceHandler();
metaInfoCollector.loadProjectFiles(Arrays.asList(sourceFiles));
Set<String> referencedNames = metaInfoCollector.getMetaInfoCollected();
// When provided with a folder that contains apex, that folder and its siblings should
// be scanned for VF
// files.
MatcherAssert.assertThat(referencedNames, hasSize(equalTo(1)));
MatcherAssert.assertThat(referencedNames, contains("MyController"));
} finally {
MetaInfoCollectorTestProvider.removeVisualForceHandler();
}
}

@Test
public void loadVisualForce_testWithMalformedVf(TestInfo testInfo) throws Exception {
TestUtil.compileTestFiles(g, testInfo);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
public class MyClass {
@AuraEnabled
public static void foo(List<My_Custom_Set__c> toInsert) {
if (My_Custom_Set__c.SObjectType.getDescribe().isCreateable()) {
insert toInsert;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<CustomObject xmlns="http://soap.sforce.com/2006/04/metadata">
<customSettingsType>List</customSettingsType>
<description>Dummy Custom settings</description>
<enableFeeds>false</enableFeeds>
<label>My Cust Set</label>
<visibility>Public</visibility>
</CustomObject>
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?xml version="1.0" encoding="UTF-8"?>
<CustomObject xmlns="http://soap.sforce.com/2006/04/metadata">
<actionOverrides>
<actionName>Accept</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>CancelEdit</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>Clone</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>Delete</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>Edit</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>Follow</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>List</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>New</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>SaveEdit</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>Tab</actionName>
<type>Default</type>
</actionOverrides>
<actionOverrides>
<actionName>View</actionName>
<type>Default</type>
</actionOverrides>
<allowInChatterGroups>false</allowInChatterGroups>
<deploymentStatus>Deployed</deploymentStatus>
<description>Simple sobject to test</description>
<enableActivities>false</enableActivities>
<enableBulkApi>true</enableBulkApi>
<enableFeeds>false</enableFeeds>
<enableHistory>true</enableHistory>
<enableReports>true</enableReports>
<enableSearch>true</enableSearch>
<enableSharing>true</enableSharing>
<enableStreamingApi>true</enableStreamingApi>
<label>Simple SObject</label>
<nameField>
<displayFormat>N-{00}</displayFormat>
<label>Name</label>
<trackHistory>false</trackHistory>
<type>AutoNumber</type>
</nameField>
<pluralLabel>Simple SObjects</pluralLabel>
<searchLayouts/>
<sharingModel>ReadWrite</sharingModel>
</CustomObject>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
public class MyController {
public String someStringMethod() {
return 'beep';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
public class MyNonController {
public String someMethod() {
return 'beep';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<apex:page controller="MyController" layout="none">
</apex:page>
7 changes: 3 additions & 4 deletions packages/code-analyzer-sfge-engine/src/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,9 @@ export class SfgeEngine extends Engine {

public constructor(config: SfgeEngineConfig, clock: Clock = new RealClock()) {
super();
// TODO: When we support custom Java commands, we'll need to use the config property instead of the hardcoded string here.
const javaCommandExecutor: JavaCommandExecutor = new JavaCommandExecutor('java', this.emitLogEvent.bind(this));
this.sfgeWrapper = new RuntimeSfgeWrapper(javaCommandExecutor, clock, this.emitLogEvent.bind(this));
this.config = config;
const javaCommandExecutor: JavaCommandExecutor = new JavaCommandExecutor(this.config.java_command, this.emitLogEvent.bind(this));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to do this the first time around. Doing it now.

this.sfgeWrapper = new RuntimeSfgeWrapper(javaCommandExecutor, clock, this.emitLogEvent.bind(this));
}

public override getName(): string {
Expand Down Expand Up @@ -96,7 +95,7 @@ export class SfgeEngine extends Engine {

const sfgeResults: SfgeRunResult[] = await this.sfgeWrapper.invokeRunCommand(
selectedRuleInfoList,
relevantFiles, // TODO: WHEN WE ADD PATH-START TARGETING, THIS NEEDS TO CHANGE.
runOptions.pathStartPoints ?? [],
relevantFiles,
sfgeRunOptions,
(innerPerc: number, message?: string) => this.emitRunRulesProgressEvent(5 + 93*innerPerc/100, message) // 5%-98%
Expand Down
35 changes: 25 additions & 10 deletions packages/code-analyzer-sfge-engine/src/sfge-wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import fs from 'node:fs';
import {
getMessageFromCatalog,
LogLevel,
PathPoint,
SHARED_MESSAGE_CATALOG
} from '@salesforce/code-analyzer-engine-api';
import {createTempDir, JavaCommandExecutor} from '@salesforce/code-analyzer-engine-api/utils';
Expand Down Expand Up @@ -108,15 +109,15 @@ export class RuntimeSfgeWrapper {
}
}

public async invokeRunCommand(selectedRuleInfos: SfgeRuleInfo[], targetPaths: string[], projectFilePaths: string[], sfgeRunOptions: SfgeRunOptions, emitProgress: (percComplete: number) => void): Promise<SfgeRunResult[]> {
public async invokeRunCommand(selectedRuleInfos: SfgeRuleInfo[], pathStarts: PathPoint[], projectFilePaths: string[], sfgeRunOptions: SfgeRunOptions, emitProgress: (percComplete: number) => void): Promise<SfgeRunResult[]> {
const tmpDir: string = await this.getTemporaryWorkingDir();
emitProgress(2);

const inputFileName: string = path.join(tmpDir, 'sfgeInput.json');
const logFilePath: string = path.join(sfgeRunOptions.logFolder, this.logFileName);
const ruleNames: string[] = selectedRuleInfos.map(sri => sri.name);

await this.createSfgeInputFile(inputFileName, ruleNames, targetPaths, projectFilePaths);
await this.createSfgeInputFile(inputFileName, ruleNames, pathStarts, projectFilePaths);
const resultsOutputFile: string = path.join(tmpDir, 'resultsFile.json');
this.emitLogEvent(LogLevel.Debug, getMessage('LoggingToFile', 'run', logFilePath));
emitProgress(10);
Expand Down Expand Up @@ -163,15 +164,29 @@ export class RuntimeSfgeWrapper {
return this.temporaryWorkingDir;
}

private async createSfgeInputFile(filePath: string, rules: string[], targets: string[], allWorkspaceFiles: string[]): Promise<void> {
const sfgeTargets: SfgeTarget[] = targets.map(target => {
return {
targetFile: target,
targetMethods: []
};
});
private async createSfgeInputFile(filePath: string, rules: string[], pathStarts: PathPoint[], allWorkspaceFiles: string[]): Promise<void> {
const sfgeTargetsByFile: Map<string, SfgeTarget> = new Map();
if (pathStarts.length > 0) {
pathStarts.forEach(pathStart => {
const sfgeTarget: SfgeTarget = sfgeTargetsByFile.get(pathStart.file) ?? {
targetFile: pathStart.file,
targetMethods: []
};
if (pathStart.methodName) {
sfgeTarget.targetMethods = [...sfgeTarget.targetMethods, pathStart.methodName];
}
sfgeTargetsByFile.set(pathStart.file, sfgeTarget);
});
} else {
allWorkspaceFiles.map(file => {
sfgeTargetsByFile.set(file, {
targetFile: file,
targetMethods: []
});
});
}
const inputFileContents: SfgeInputFile = {
targets: sfgeTargets,
targets: [...sfgeTargetsByFile.values()],
projectDirs: allWorkspaceFiles,
rulesToRun: rules
};
Expand Down
Loading