Skip to content

Commit

Permalink
Merge pull request #1347 from forcedotcom/dev
Browse files Browse the repository at this point in the history
RELEASE: @W-14914244@: Merging `dev` to `release` for v3.21.0
rmohan20 authored Feb 6, 2024
2 parents c0b5f56 + 0c30039 commit bd3785d
Showing 28 changed files with 1,024 additions and 203 deletions.
4 changes: 0 additions & 4 deletions messages/SfgeEngine.md
Original file line number Diff line number Diff line change
@@ -5,7 +5,3 @@ Please wait
# messages.spinnerStart

Analyzing with Salesforce Graph Engine. See %s for details.

# errors.failedWithoutProjectDir

The --projectdir|-p flag is missing. Rerun your command with --projectdir|-p to allow Graph Engine to run, or with --engine|-e to exclude Graph Engine from execution.
16 changes: 14 additions & 2 deletions messages/run-common.md
Original file line number Diff line number Diff line change
@@ -32,11 +32,11 @@ Writes output to a file.

# flags.projectdirSummary

provide root directory of project
root directory of project

# flags.projectdirDescription

Provides the relative or absolute root project directory used to set the context for Graph Engine's analysis. Project directory must be a path, not a glob. Specify multiple values as a comma-separated list.
Provides the relative or absolute root project directories used to set the context for Graph Engine's analysis. Specify multiple values as a comma-separated list. Each project directory must be a path, not a glob. If --projectdir isn’t specified, a default value is calculated. The default value is a directory that contains all the target files.

# flags.sevthresholdSummary

@@ -81,3 +81,15 @@ The selected output format doesn't match the output file type. Output format: %s
# validations.projectdirMustExist

--projectdir must specify existing paths

# validations.noFilesFoundInTarget

No files were found in the target. --target must contain at least one file.

# info.resolvedTarget

The --target flag wasn't specified so the default target '.' will be used.

# info.resolvedProjectDir

The --projectdir flag wasn’t specified so the calculated project directory '%s' will be used.
10 changes: 3 additions & 7 deletions messages/run-dfa.md
Original file line number Diff line number Diff line change
@@ -33,7 +33,7 @@ Specifies number of rule evaluation threads, or how many entrypoints can be eval

# flags.rulethreadtimeoutSummary

specify timeout for individual rule threads in milliseconds. Alternatively, set the timeout value using environment variable `SFGE_RULE_THREAD_TIMEOUT`. Default: 90000 ms
specify timeout for individual rule threads in milliseconds. Alternatively, set the timeout value using environment variable `SFGE_RULE_THREAD_TIMEOUT`. Default: 900000 ms

# flags.rulethreadtimeoutDescription

@@ -49,11 +49,11 @@ Specifies Java Virtual Machine arguments to override system defaults while execu

# flags.targetSummary

return location of source code
source code location

# flags.targetDescription

Returns the source code location. Use glob patterns or specify individual methods with #-syntax. Multiple values are specified as a comma-separated list.
Specifies the source code location. Use glob patterns or specify individual methods with #-syntax. Multiple values are specified as a comma-separated list. Default is ".".

# flags.withpilotSummary

@@ -71,10 +71,6 @@ Method-level targets supplied to --target cannot be globs

Method-level target %s must be a real file

# validations.projectdirIsRequired

--projectdir is required for this command.

# examples

The paths specified for --projectdir must contain all files specified through --target cumulatively.
2 changes: 1 addition & 1 deletion messages/run-pathless.md
Original file line number Diff line number Diff line change
@@ -20,7 +20,7 @@ source code location

# flags.targetDescription

Source code location. May use glob patterns. Specify multiple values as a comma-separated list.
Specifies the source code location. May use glob patterns. Specify multiple values as a comma-separated list. Default is ".".

# flags.envSummary

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@salesforce/sfdx-scanner",
"description": "Static code scanner that applies quality and security rules to Apex code, and provides feedback.",
"version": "3.20.0",
"version": "3.21.0",
"author": "ISV SWAT",
"bugs": "https://github.com/forcedotcom/sfdx-scanner/issues",
"dependencies": {
584 changes: 551 additions & 33 deletions retire-js/RetireJsVulns.json

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@
import apex.jorje.semantic.ast.expression.SoqlExpression;
import apex.jorje.semantic.ast.expression.SoslExpression;
import apex.jorje.semantic.ast.expression.SuperMethodCallExpression;
import apex.jorje.semantic.ast.expression.SuperVariableExpression;
import apex.jorje.semantic.ast.expression.ThisMethodCallExpression;
import apex.jorje.semantic.ast.expression.ThisVariableExpression;
import apex.jorje.semantic.ast.expression.TriggerVariableExpression;
@@ -148,6 +149,8 @@ public static String getVertexLabel(Class<? extends AstNode> clazz) {
public static final String STANDARD_CONDITION = getVertexLabel(StandardCondition.class);
public static final String SUPER_METHOD_CALL_EXPRESSION =
getVertexLabel(SuperMethodCallExpression.class);
public static final String SUPER_VARIABLE_EXPRESSION =
getVertexLabel(SuperVariableExpression.class);
public static final String THIS_METHOD_CALL_EXPRESSION =
getVertexLabel(ThisMethodCallExpression.class);
public static final String THIS_VARIABLE_EXPRESSION =
82 changes: 55 additions & 27 deletions sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java
Original file line number Diff line number Diff line change
@@ -645,38 +645,66 @@ public static List<ApexPath> getPaths(
// current class or a static
// method on a another class
String definingType;
final String methodName = methodCallExpression.getMethodName();
String fullMethodName = methodCallExpression.getFullMethodName();
if (methodName.equals(fullMethodName)) {
// The method is being called on a class onto itself
definingType = vertex.getDefiningType();
List<String> potentialDefiningTypes = new ArrayList<>();
if (methodCallExpression.isThisReference()) {
// A call to `this.someMethod()` means the method must be on the defining type.
definingType = methodCallExpression.getDefiningType();
potentialDefiningTypes.add(
ApexStandardLibraryUtil.getCanonicalName(definingType));
} else if (methodCallExpression.isEmptyReference()) {
// For an empty reference (e.g., `someMethod()` as opposed to
// `whatever.someMethod()`), the first place we should look is
// the call's own defining type.
definingType = methodCallExpression.getDefiningType();
potentialDefiningTypes.add(
ApexStandardLibraryUtil.getCanonicalName(definingType));
// Inner classes can also invoke their outer class's static methods with an
// empty reference, so if this is an inner class, we should check the outer
// class too.
if (definingType.contains(".")) {
potentialDefiningTypes.add(definingType.split("\\.")[0]);
}
} else if (methodCallExpression.isSuperVariableReference()) {
// A call to `super.someMethod()` definitely means that the method is on the
// super-type.
definingType =
((ReferenceExpressionVertex)
(methodCallExpression.getReferenceExpression()))
.getSuperVariableExpression()
.orElseThrow(
() ->
new UnexpectedException(
"Missing SuperVariableExpression"))
.getCanonicalType();
potentialDefiningTypes.add(
ApexStandardLibraryUtil.getCanonicalName(definingType));
} else {
// TODO: Pass information to #getInvoked that this needs to be a static method
definingType = String.join(".", vertex.getChainedNames());
}
// The defining type could be an aliased reference to an inner class, so check that
// first.
String potentialInnerClassDefType =
ClassUtil.getMoreSpecificClassName(vertex, definingType).orElse(null);
if (potentialInnerClassDefType != null) {
invoked =
getInvoked(
g,
potentialInnerClassDefType,
(MethodCallExpressionVertex) vertex,
symbols)
.orElse(null);
// A method call on a non-empty, non-this reference (e.g.,
// `whatever.someMethod()`) lives on the thing being referenced.
definingType = String.join(".", methodCallExpression.getChainedNames());
potentialDefiningTypes.add(
ApexStandardLibraryUtil.getCanonicalName(definingType));
// A defining type without a period might be an aliased reference to a local
// inner class.
if (!definingType.contains(".")) {
// Check if an inner class exists, and if so, then check that before
// checking the outer class.
String potentialInnerClassDefType =
ClassUtil.getMoreSpecificClassName(vertex, definingType)
.orElse(null);
if (potentialInnerClassDefType != null) {
potentialDefiningTypes.add(0, potentialInnerClassDefType);
}
}
}
// If we found no inner classes, check outer classes.
if (invoked == null) {
definingType = ApexStandardLibraryUtil.getCanonicalName(definingType);
for (String potentialDefiningType : potentialDefiningTypes) {
invoked =
getInvoked(
g,
definingType,
(MethodCallExpressionVertex) vertex,
symbols)
getInvoked(g, potentialDefiningType, methodCallExpression, symbols)
.orElse(null);
if (invoked != null) {
break;
}
}
}
} else if (vertex instanceof NewObjectExpressionVertex) {
Original file line number Diff line number Diff line change
@@ -102,7 +102,7 @@ public Optional<ClassRefExpressionVertex> getClassRefExpression() {
if (abstractReferenceExpression instanceof ReferenceExpressionVertex) {
ReferenceExpressionVertex referenceExpression =
(ReferenceExpressionVertex) abstractReferenceExpression;
return referenceExpression.gtClassRefExpression();
return referenceExpression.getClassRefExpression();
}
return Optional.empty();
}
@@ -311,6 +311,20 @@ public boolean isEmptyReference() {
return referenceExpression.get() instanceof EmptyReferenceExpressionVertex;
}

/**
* @return - True if the method is qualified by a super expression. i.e., returns true for
* {@code super.someMethod()} but not {@code this.someMethod()}.
*/
public boolean isSuperVariableReference() {
if (referenceExpression.get() instanceof ReferenceExpressionVertex) {
return ((ReferenceExpressionVertex) referenceExpression.get())
.getSuperVariableExpression()
.isPresent();
} else {
return false;
}
}

private LazyVertex<AbstractReferenceExpressionVertex> _getReferenceVertex() {
return new LazyVertex<>(
() ->
Original file line number Diff line number Diff line change
@@ -16,13 +16,19 @@ public class ReferenceExpressionVertex extends AbstractReferenceExpressionVertex
* Presence of this vertex indicates that the reference was qualified with a 'this' reference
*/
private final LazyOptionalVertex<ThisVariableExpressionVertex> thisVariableExpression;
/**
* Presence of this vertex indicates that the reference was qualified with a {@code super}
* reference.
*/
private final LazyOptionalVertex<SuperVariableExpressionVertex> superVariableExpression;

ReferenceExpressionVertex(Map<Object, Object> properties) {
super(properties);
// TODO: Efficiency. This does 2 queries, is it safe to get the child and then look at the
// TODO: Efficiency. This does 3 queries, is it safe to get the child and then look at the
// type?
this.classRefExpressionVertex = _getClassRefExpression();
this.thisVariableExpression = _getThisVariableExpression();
this.superVariableExpression = _getSuperVariableExpression();
}

@Override
@@ -49,10 +55,14 @@ public Optional<ThisVariableExpressionVertex> getThisVariableExpression() {
return thisVariableExpression.get();
}

public Optional<ClassRefExpressionVertex> gtClassRefExpression() {
public Optional<ClassRefExpressionVertex> getClassRefExpression() {
return classRefExpressionVertex.get();
}

public Optional<SuperVariableExpressionVertex> getSuperVariableExpression() {
return superVariableExpression.get();
}

public String getReferenceType() {
return getString(Schema.REFERENCE_TYPE);
}
@@ -77,4 +87,12 @@ private LazyOptionalVertex<ClassRefExpressionVertex> _getClassRefExpression() {
.out(Schema.CHILD)
.hasLabel(ASTConstants.NodeType.CLASS_REF_EXPRESSION));
}

private LazyOptionalVertex<SuperVariableExpressionVertex> _getSuperVariableExpression() {
return new LazyOptionalVertex<>(
() ->
g().V(getId())
.out(Schema.CHILD)
.hasLabel(ASTConstants.NodeType.SUPER_VARIABLE_EXPRESSION));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.salesforce.graph.vertex;

import com.salesforce.exception.UnexpectedException;
import com.salesforce.graph.symbols.SymbolProvider;
import com.salesforce.graph.symbols.SymbolProviderVertexVisitor;
import com.salesforce.graph.visitor.PathVertexVisitor;
import java.util.Map;

public class SuperVariableExpressionVertex extends TODO_FIX_HIERARCHY_ChainedVertex
implements Typeable {
SuperVariableExpressionVertex(Map<Object, Object> properties) {
this(properties, null);
}

SuperVariableExpressionVertex(Map<Object, Object> properties, Object supplementalParam) {
super(properties, supplementalParam);
}

@Override
public boolean visit(PathVertexVisitor visitor, SymbolProvider symbols) {
return visitor.visit(this, symbols);
}

@Override
public boolean visit(SymbolProviderVertexVisitor visitor) {
return visitor.visit(this);
}

@Override
public void afterVisit(PathVertexVisitor visitor, SymbolProvider symbols) {
visitor.afterVisit(this, symbols);
}

@Override
public void afterVisit(SymbolProviderVertexVisitor visitor) {
visitor.afterVisit(this);
}

@Override
public String getCanonicalType() {
UserClassVertex parentClass =
getParentClass().orElseThrow(() -> new UnexpectedException("Missing parent class"));
return parentClass
.getSuperClassName()
.orElseThrow(() -> new UnexpectedException("Missing super class"));
}
}
Original file line number Diff line number Diff line change
@@ -37,7 +37,7 @@ public void setup() {
*/
@ValueSource(
strings = {"Boolean", "Id", "Integer", "List<String>", "Map<String, String>", "String"})
@ParameterizedTest(name = "{0}")
@ParameterizedTest(name = "{displayName}: variableType = {0}")
public void testUnresolvedUserMethodReturnIsIndeterminant(String variableType) {
String sourceCode =
"public class MyClass {\n"
@@ -61,7 +61,7 @@ public void testUnresolvedUserMethodReturnIsIndeterminant(String variableType) {
/** Method parameters have their type synthesized based on the variable declaration. */
@ValueSource(
strings = {"Boolean", "Id", "Integer", "List<String>", "Map<String, String>", "String"})
@ParameterizedTest(name = "{0}")
@ParameterizedTest(name = "{displayName}: variableType = {0}")
public void testUnresolvedUserMethodParameterIsIndeterminant(String variableType) {
String sourceCode =
"public class MyClass {\n"
@@ -94,7 +94,7 @@ public static Stream<Arguments> systemMethodSource() {
* return type of the method
*/
@MethodSource("systemMethodSource")
@ParameterizedTest(name = "variableType=({0}):method=({1})")
@ParameterizedTest(name = "{displayName}: variableType=({0}):method=({1})")
public void testUnresolvedSystemMethodReturnAssignmentIsIndeterminant(
String variableType, String method) {
String sourceCode =
@@ -124,7 +124,7 @@ public void testUnresolvedSystemMethodReturnAssignmentIsIndeterminant(
* return type of the method
*/
@MethodSource("systemMethodSource")
@ParameterizedTest(name = "variableType=({0}):method=({1})")
@ParameterizedTest(name = "{displayName}: variableType=({0}):method=({1})")
public void testUnresolvedSystemMethodReturnPassAsParameterIsIndeterminant(
String variableType, String method) {
String sourceCode =
Original file line number Diff line number Diff line change
@@ -142,7 +142,6 @@ public void instanceInvokedBySubclass_expectNoViolation(
* used.
*/
@Test
@Disabled // TODO: FIX AND ENABLE THIS TEST
public void instanceInvokedByOverridingSubclass_expectNoViolation() {
// Fill in the source code template.
String[] sourceCodes =
Original file line number Diff line number Diff line change
@@ -75,9 +75,9 @@ public void staticInvokedByOwnClass_expectNoViolation(
*/
@CsvSource({
// Every combination of private/public tested method and implicit/explicit type reference.
// "public, testedMethod()", // TODO: FIX AND ENABLE THIS TEST
"public, testedMethod()",
"public, MyClass.testedMethod()",
// "private, testedMethod()", // TODO: FIX AND ENABLE TEST
"private, testedMethod()",
"private, MyClass.testedMethod()",
})
@ParameterizedTest(name = "{displayName}: {0} static invoked as {1}")
@@ -176,11 +176,7 @@ public void staticInvokedBySubclass_expectNoViolation(
* references to outer type.
*/
@ValueSource(
strings = {
// "testedMethod()", // TODO: FIX AND ENABLE THIS TEST
"ParentClass.testedMethod()",
"ChildClass.testedMethod()"
})
strings = {"testedMethod()", "ParentClass.testedMethod()", "ChildClass.testedMethod()"})
@ParameterizedTest(name = "{displayName}: Invoked as {0}")
public void staticInvokedByInnerOfSubclass_expectNoViolation(String invocation) {
// spotless:off
2 changes: 1 addition & 1 deletion src/commands/scanner/rule/add.ts
Original file line number Diff line number Diff line change
@@ -37,6 +37,6 @@ export default class Add extends ScannerCommand {
};

protected createAction(logger: Logger, display: Display): Action {
return new RuleAddAction(logger, display, new InputProcessorImpl(this.config.version));
return new RuleAddAction(logger, display, new InputProcessorImpl(this.config.version, display));
}
}
2 changes: 1 addition & 1 deletion src/commands/scanner/rule/remove.ts
Original file line number Diff line number Diff line change
@@ -39,6 +39,6 @@ export default class Remove extends ScannerCommand {
};

protected createAction(logger: Logger, display: Display): Action {
return new RuleRemoveAction(logger, display, new InputProcessorImpl(this.config.version));
return new RuleRemoveAction(logger, display, new InputProcessorImpl(this.config.version, display));
}
}
5 changes: 2 additions & 3 deletions src/commands/scanner/run.ts
Original file line number Diff line number Diff line change
@@ -53,8 +53,7 @@ export default class Run extends ScannerRunCommand {
summary: getMessage(BundleName.Run, 'flags.targetSummary'),
description: getMessage(BundleName.Run, 'flags.targetDescription'),
delimiter: ',',
multiple: true,
required: true
multiple: true
})(),
// END: Targeting-related flags.
// BEGIN: Engine config flags.
@@ -89,7 +88,7 @@ export default class Run extends ScannerRunCommand {
};

protected createAction(logger: Logger, display: Display): Action {
const inputProcessor: InputProcessor = new InputProcessorImpl(this.config.version);
const inputProcessor: InputProcessor = new InputProcessorImpl(this.config.version, display);
const ruleFilterFactory: RuleFilterFactory = new RuleFilterFactoryImpl();
const engineOptionsFactory: EngineOptionsFactory = new RunEngineOptionsFactory(inputProcessor);
const resultsProcessorFactory: ResultsProcessorFactory = new ResultsProcessorFactoryImpl();
3 changes: 1 addition & 2 deletions src/commands/scanner/run/dfa.ts
Original file line number Diff line number Diff line change
@@ -41,7 +41,6 @@ export default class Dfa extends ScannerRunCommand {
char: 't',
summary: getMessage(BundleName.RunDfa, 'flags.targetSummary'),
description: getMessage(BundleName.RunDfa, 'flags.targetDescription'),
required: true,
delimiter: ',',
multiple: true
})(),
@@ -77,7 +76,7 @@ export default class Dfa extends ScannerRunCommand {
};

protected createAction(logger: Logger, display: Display): Action {
const inputProcessor: InputProcessor = new InputProcessorImpl(this.config.version);
const inputProcessor: InputProcessor = new InputProcessorImpl(this.config.version, display);
const ruleFilterFactory: RuleFilterFactory = new RuleFilterFactoryImpl();
const engineOptionsFactory: EngineOptionsFactory = new RunDfaEngineOptionsFactory(inputProcessor);
const resultsProcessorFactory: ResultsProcessorFactory = new ResultsProcessorFactoryImpl();
10 changes: 9 additions & 1 deletion src/lib/DefaultRuleManager.ts
Original file line number Diff line number Diff line change
@@ -289,12 +289,14 @@ export class DefaultRuleManager implements RuleManager {
const ruleTargetsInitialLength: number = ruleTargets.length;
// Positive patterns might use method-level targeting. We only want to do path evaluation against the part
// that's actually a path.
const targetPortions = target.split('#');
const targetPortions = splitOnMethodSpecifier(target)
// The array will always have at least one entry, since if there's no '#' then it will return a singleton array.
const targetPath = targetPortions[0];
if (globby.hasMagic(target)) {
// The target is a magic glob. Retrieve paths in the working directory that match it, and then filter against
// our pattern matcher.
// NOTE: We should consider in the future to resolve target paths based off of the projectdir instead of
// the present working directory.
const matchingTargets = await globby(targetPath);
// Map relative files to absolute paths. This solves ambiguity of current working directory
const absoluteMatchingTargets = matchingTargets.map(t => path.resolve(t));
@@ -346,3 +348,9 @@ export class DefaultRuleManager implements RuleManager {
return ruleTargets;
}
}

function splitOnMethodSpecifier(targetPath: string): string[] {
// Unlike targetPath.split('#'), this solution only splits on the last '#' instead of all of them
const lastHashPos: number = targetPath.lastIndexOf('#');
return lastHashPos < 0 ? [targetPath] : [targetPath.substring(0, lastHashPos), targetPath.substring(lastHashPos+1)]
}
21 changes: 14 additions & 7 deletions src/lib/EngineOptionsFactory.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Inputs, LooseObject, SfgeConfig} from "../types";
import {CUSTOM_CONFIG, INTERNAL_ERROR_CODE} from "../Constants";
import {CUSTOM_CONFIG, ENGINE, INTERNAL_ERROR_CODE} from "../Constants";
import {InputProcessor} from "./InputProcessor";
import {TYPESCRIPT_ENGINE_OPTIONS} from "./eslint/TypescriptEslintStrategy";
import {SfError} from "@salesforce/core";
@@ -22,18 +22,16 @@ abstract class CommonEngineOptionsFactory implements EngineOptionsFactory {
this.inputProcessor = inputProcessor;
}

protected abstract shouldSfgeRun(inputs: Inputs): boolean;

createEngineOptions(inputs: Inputs): EngineOptions {
const options: Map<string,string> = new Map();

// We should only add a GraphEngine config if we were given a --projectdir flag.
const projectDirPaths: string[] = this.inputProcessor.resolveProjectDirPaths(inputs);
if (projectDirPaths.length > 0) {
if (this.shouldSfgeRun(inputs)) {
const sfgeConfig: SfgeConfig = {
projectDirs: projectDirPaths
projectDirs: this.inputProcessor.resolveProjectDirPaths(inputs)
};
options.set(CUSTOM_CONFIG.SfgeConfig, JSON.stringify(sfgeConfig));
}

return options;
}

@@ -44,6 +42,10 @@ export class RunEngineOptionsFactory extends CommonEngineOptionsFactory {
super(inputProcessor);
}

protected shouldSfgeRun(inputs: Inputs): boolean {
return inputs.engine && (inputs.engine as string[]).includes(ENGINE.SFGE);
}

public override createEngineOptions(inputs: Inputs): EngineOptions {
const engineOptions: EngineOptions = super.createEngineOptions(inputs);

@@ -89,6 +91,11 @@ export class RunDfaEngineOptionsFactory extends CommonEngineOptionsFactory {
super(inputProcessor);
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected shouldSfgeRun(_inputs: Inputs): boolean {
return true;
}

public override createEngineOptions(inputs: Inputs): EngineOptions {
const engineOptions: EngineOptions = super.createEngineOptions(inputs);

104 changes: 94 additions & 10 deletions src/lib/InputProcessor.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import {Inputs} from "../types";
import normalize = require('normalize-path');
import path = require('path');
import untildify = require("untildify");
import {RunOptions} from "./RuleManager";
import {RunOutputOptions} from "./output/RunResultsProcessor";
import {inferFormatFromOutfile, OutputFormat} from "./output/OutputFormat";
import {SfError} from "@salesforce/core";
import {BundleName, getMessage} from "../MessageCatalog";
import {INTERNAL_ERROR_CODE} from "../Constants";
import {Display} from "./Display";
import normalize = require('normalize-path');
import path = require('path');
import fs = require('fs');
import untildify = require("untildify");
import globby = require('globby');
import {Tokens} from "@salesforce/core/lib/messages";

/**
* Service for processing inputs
@@ -23,9 +30,13 @@ export interface InputProcessor {

export class InputProcessorImpl implements InputProcessor {
private readonly sfVersion: string;
private readonly display: Display;
private readonly displayedKeys: Set<string>;

public constructor(sfVersion: string) {
public constructor(sfVersion: string, display: Display) {
this.sfVersion = sfVersion;
this.display = display
this.displayedKeys = new Set();
}

public resolvePaths(inputs: Inputs): string[] {
@@ -35,18 +46,35 @@ export class InputProcessorImpl implements InputProcessor {
}

public resolveProjectDirPaths(inputs: Inputs): string[] {
// TODO: Stop allowing an array of paths - move towards only 1 path (to resolve into 1 output path)
// If projectdir is provided, then return it since at this point it has already been validated to exist
if (inputs.projectdir && (inputs.projectdir as string[]).length > 0) {
return (inputs.projectdir as string[]).map(p => path.resolve(p));
return (inputs.projectdir as string[]).map(p => path.resolve(normalize(untildify(p))))
}
return [];

// If projectdir is not provided then:
// * We calculate the first common parent directory that includes all the target files.
// --> If none of its parent folders contain a sfdx-project.json file, then we return this first common parent.
// --> Otherwise we return the folder that contains the sfdx-project.json file.
const commonParentFolder = getFirstCommonParentFolder(this.getAllTargetFiles(inputs));
let projectFolder: string = findFolderThatContainsSfdxProjectFile(commonParentFolder);
projectFolder = projectFolder.length > 0 ? projectFolder : commonParentFolder
this.displayInfoOnlyOnce('info.resolvedProjectDir', [projectFolder])
return [projectFolder];
}

public resolveTargetPaths(inputs: Inputs): string[] {
// Turn the paths into normalized Unix-formatted paths and strip out any single- or double-quotes, because
// sometimes shells are stupid and will leave them in there.
const target: string[] = (inputs.target || []) as string[];
return target.map(path => normalize(untildify(path)).replace(/['"]/g, ''));
// Note that we do not do a path.resolve since the target input can be globby (which we handle elsewhere).

// If possible, in the future we should resolve all globs here instead of in the DefaultRuleManager.
// Also, I would recommend that we eventually resolve globs based on the projectdir (since it acts as a
// root directory) instead of the present working directory.
if (!inputs.target) {
this.displayInfoOnlyOnce('info.resolvedTarget')
}
const targetPaths: string[] = (inputs.target || ['.']) as string[];
return targetPaths.map(path => normalize(untildify(path)).replace(/['"]/g, ''));
}


@@ -62,11 +90,27 @@ export class InputProcessorImpl implements InputProcessor {
public createRunOutputOptions(inputs: Inputs): RunOutputOptions {
return {
format: outputFormatFromInputs(inputs),
verboseViolations: inputs["verbose-violations"] as boolean,
verboseViolations: inputs['verbose-violations'] as boolean,
severityForError: inputs['severity-threshold'] as number,
outfile: inputs.outfile as string
};
}

private getAllTargetFiles(inputs: Inputs): string[] {
const targetPaths: string[] = this.resolveTargetPaths(inputs).map(p => trimMethodSpecifier(p))
const allAbsoluteTargetFiles: string[] = globby.sync(targetPaths).map(p => path.resolve(p));
if (allAbsoluteTargetFiles.length == 0) {
throw new SfError(getMessage(BundleName.CommonRun, 'validations.noFilesFoundInTarget'), null, null, INTERNAL_ERROR_CODE);
}
return allAbsoluteTargetFiles;
}

private displayInfoOnlyOnce(messageKey: string, tokens?: Tokens) {
if (!this.displayedKeys.has(messageKey)) {
this.display.displayInfo(getMessage(BundleName.CommonRun, messageKey, tokens));
this.displayedKeys.add(messageKey);
}
}
}

function outputFormatFromInputs(inputs: Inputs): OutputFormat {
@@ -80,3 +124,43 @@ function outputFormatFromInputs(inputs: Inputs): OutputFormat {
return OutputFormat.TABLE;
}
}

function trimMethodSpecifier(targetPath: string): string {
const lastHashPos: number = targetPath.lastIndexOf('#');
return lastHashPos < 0 ? targetPath : targetPath.substring(0, lastHashPos)
}

function getFirstCommonParentFolder(targetFiles: string[]) {
const longestCommonStr: string = getLongestCommonPrefix(targetFiles);
const commonParentFolder = getParentFolderOf(longestCommonStr);
return commonParentFolder.length == 0 ? path.sep : commonParentFolder;
}

function getLongestCommonPrefix(strs: string[]): string {
// To find the longest common prefix, we first get the select the shortest string from our list of strings
const shortestStr = strs.reduce((s1, s2) => s1.length <= s2.length ? s1 : s2);

// Then we check that each string's ith character is the same as the shortest strings ith character
for (let i = 0; i < shortestStr.length; i++) {
if(!strs.every(str => str[i] === shortestStr[i])) {
// If we find a string that doesn't match the ith character, we return the common prefix from [0,i)
return shortestStr.substring(0, i)
}
}
return shortestStr;
}

function findFolderThatContainsSfdxProjectFile(folder: string): string {
let folderToCheck: string = folder;
while (folderToCheck.length > 0) {
if (fs.existsSync(path.resolve(folderToCheck, 'sfdx-project.json'))) {
return folderToCheck;
}
folderToCheck = getParentFolderOf(folderToCheck);
}
return '';
}

function getParentFolderOf(fileOrFolder: string): string {
return fileOrFolder.substring(0, fileOrFolder.lastIndexOf(path.sep))
}
7 changes: 3 additions & 4 deletions src/lib/ScannerRunCommand.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import {Flags} from '@salesforce/sf-plugins-core';
import {ScannerCommand} from './ScannerCommand';
import untildify = require('untildify');
import normalize = require('normalize-path');
import {BundleName, getMessage} from "../MessageCatalog";
import {OutputFormat} from "./output/OutputFormat";

@@ -48,11 +46,12 @@ export abstract class ScannerRunCommand extends ScannerCommand {
}),
// END: Flags related to results processing.
// BEGIN: Flags related to targeting.
projectdir: Flags.custom<string[]>({ // TODO: FIGURE OUT WHY WE NEED THIS ON BOTH "run" AND "run dfa"
projectdir: Flags.custom<string[]>({
char: 'p',
summary: getMessage(BundleName.CommonRun, 'flags.projectdirSummary'),
description: getMessage(BundleName.CommonRun, 'flags.projectdirDescription'),
parse: val => Promise.resolve(val.split(',').map(d => normalize(untildify(d))))
delimiter: ',',
multiple: true
})(),
// END: Flags related to targeting.
};
12 changes: 7 additions & 5 deletions src/lib/actions/AbstractRunAction.ts
Original file line number Diff line number Diff line change
@@ -19,6 +19,8 @@ import {inferFormatFromOutfile, OutputFormat} from "../output/OutputFormat";
import {ResultsProcessor} from "../output/ResultsProcessor";
import {ResultsProcessorFactory} from "../output/ResultsProcessorFactory";
import {JsonReturnValueHolder} from "../output/JsonReturnValueHolder";
import untildify = require('untildify');
import normalize = require('normalize-path');

/**
* Abstract Action to share a common implementation behind the "run" and "run dfa" commands
@@ -48,13 +50,13 @@ export abstract class AbstractRunAction implements Action {
const fh = new FileHandler();
// If there's a --projectdir flag, its entries must be non-glob paths pointing to existing directories.
if (inputs.projectdir) {
// TODO: MOVE AWAY FROM ALLOWING AN ARRAY OF DIRECTORIES HERE AND ERROR IF THERE IS MORE THAN ONE DIRECTORY
for (const dir of (inputs.projectdir as string[])) {
if (globby.hasMagic(dir)) {
for (const p of (inputs.projectdir as string [])) {
const projectDir: string = normalize(untildify(p))
if (globby.hasMagic(projectDir)) {
throw new SfError(getMessage(BundleName.CommonRun, 'validations.projectdirCannotBeGlob'));
} else if (!(await fh.exists(dir))) {
} else if (!(await fh.exists(projectDir))) {
throw new SfError(getMessage(BundleName.CommonRun, 'validations.projectdirMustExist'));
} else if (!(await fh.stats(dir)).isDirectory()) {
} else if (!(await fh.stats(projectDir)).isDirectory()) {
throw new SfError(getMessage(BundleName.CommonRun, 'validations.projectdirMustBeDir'));
}
}
8 changes: 5 additions & 3 deletions src/lib/actions/RunAction.ts
Original file line number Diff line number Diff line change
@@ -29,9 +29,11 @@ export class RunAction extends AbstractRunAction {
this.display.displayInfo(getMessage(BundleName.Run, 'output.filtersIgnoredCustom', []));
}
// None of the pathless engines support method-level targeting, so attempting to use it should result in an error.
for (const target of (inputs.target as string[])) {
if (target.indexOf('#') > -1) {
throw new SfError(getMessage(BundleName.Run, 'validations.methodLevelTargetingDisallowed', [target]));
if (inputs.target) {
for (const target of (inputs.target as string[])) {
if (target.indexOf('#') > -1) {
throw new SfError(getMessage(BundleName.Run, 'validations.methodLevelTargetingDisallowed', [target]));
}
}
}
}
26 changes: 11 additions & 15 deletions src/lib/actions/RunDfaAction.ts
Original file line number Diff line number Diff line change
@@ -24,22 +24,18 @@ export class RunDfaAction extends AbstractRunAction {
await super.validateInputs(inputs);

const fh = new FileHandler();
// The superclass will validate that --projectdir is well-formed,
// but doesn't require that the flag actually be present.
// So we should make sure it exists here.
if (!inputs.projectdir || (inputs.projectdir as string[]).length === 0) {
throw new SfError(getMessage(BundleName.RunDfa, 'validations.projectdirIsRequired'));
}
// Entries in the target array may specify methods, but only if the entry is neither a directory nor a glob.
for (const target of (inputs.target as string[])) {
// The target specifies a method if it includes the `#` syntax.
if (target.indexOf('#') > -1) {
if(globby.hasMagic(target)) {
throw new SfError(getMessage(BundleName.RunDfa, 'validations.methodLevelTargetCannotBeGlob'));
}
const potentialFilePath = target.split('#')[0];
if (!(await fh.isFile(potentialFilePath))) {
throw new SfError(getMessage(BundleName.RunDfa, 'validations.methodLevelTargetMustBeRealFile', [potentialFilePath]));
if (inputs.target) {
for (const target of (inputs.target as string[])) {
// The target specifies a method if it includes the `#` syntax.
if (target.indexOf('#') > -1) {
if (globby.hasMagic(target)) {
throw new SfError(getMessage(BundleName.RunDfa, 'validations.methodLevelTargetCannotBeGlob'));
}
const potentialFilePath = target.split('#')[0];
if (!(await fh.isFile(potentialFilePath))) {
throw new SfError(getMessage(BundleName.RunDfa, 'validations.methodLevelTargetMustBeRealFile', [potentialFilePath]));
}
}
}
}
15 changes: 2 additions & 13 deletions src/lib/sfge/SfgePathlessEngine.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import {SfError} from '@salesforce/core';
import {AbstractSfgeEngine, SfgeViolation} from "./AbstractSfgeEngine";
import {Rule, RuleGroup, RuleTarget, RuleViolation, SfgeConfig} from '../../types';
import {Rule, RuleGroup, RuleTarget, RuleViolation} from '../../types';
import {CUSTOM_CONFIG, RuleType} from '../../Constants';
import * as EngineUtils from "../util/CommonEngineUtils";
import {BundleName, getMessage} from "../../MessageCatalog";

export class SfgePathlessEngine extends AbstractSfgeEngine {
/**
@@ -32,16 +30,7 @@ export class SfgePathlessEngine extends AbstractSfgeEngine {
// For the non-DFA Graph Engine variant, we need to make sure that we have the
// necessary info to run the engine, since the relevant flags aren't required
// for `scanner run`.
if (engineOptions.has(CUSTOM_CONFIG.SfgeConfig)) {
const sfgeConfig: SfgeConfig = JSON.parse(engineOptions.get(CUSTOM_CONFIG.SfgeConfig)) as SfgeConfig;
if (sfgeConfig.projectDirs && sfgeConfig.projectDirs.length > 0) {
// If we've got a config with projectDirs, we're set.
return true;
}
}
// If we're here, it's because we're missing the necessary info to run this engine.
// We should throw an error indicating this.
throw new SfError(getMessage(BundleName.SfgeEngine, 'errors.failedWithoutProjectDir'));
return engineOptions.has(CUSTOM_CONFIG.SfgeConfig);
}

protected getSubVariantName(): string {
156 changes: 156 additions & 0 deletions test/lib/InputProcessor.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
import {InputProcessor, InputProcessorImpl} from "../../src/lib/InputProcessor";
import {assert, expect} from "chai";
import * as path from "path";
import {Inputs} from "../../src/types";
import {BundleName, getMessage} from "../../src/MessageCatalog";
import untildify = require("untildify");
import normalize = require("normalize-path");
import {FakeDisplay} from "./FakeDisplay";

describe("InputProcessorImpl Tests", async () => {
let display: FakeDisplay;
let inputProcessor: InputProcessor;
beforeEach(async () => {
display = new FakeDisplay();
inputProcessor = new InputProcessorImpl("2.11.8", display);
});

describe("resolveTargetPaths Tests", async () => {
it("Specified glob target stays as glob", async () => {
// Note that we may want to change this behavior in the future instead of waiting to resolve the globs
// in the DefaultRuleManager. But for now, adding this test.
const inputs: Inputs = {
target: ['test\\**\\*.page', '!test/code-fixtures/cpd', '~/*.class']
};
const resolvedTargetPaths: string[] = inputProcessor.resolveTargetPaths(inputs);
expect(resolvedTargetPaths).to.have.length(3);
expect(resolvedTargetPaths).to.contain('test/**/*.page');
expect(resolvedTargetPaths).to.contain('!test/code-fixtures/cpd');
expect(resolvedTargetPaths).to.contain(normalize(untildify('~/*.class')))
})

it("Specified target with method specifier", async () => {
const inputs: Inputs = {
target: ['test/code-fixtures/apex/SomeTestClass.cls#testMethodWithoutAsserts']
};
const resolvedTargetPaths: string[] = inputProcessor.resolveTargetPaths(inputs);
expect(resolvedTargetPaths).to.have.length(1);
expect(resolvedTargetPaths).to.contain('test/code-fixtures/apex/SomeTestClass.cls#testMethodWithoutAsserts');

expect(display.getOutputText()).to.equal('')
})

it("Unspecified target resolves to current directory", async () => {
const inputs: Inputs = {}
const resolvedTargetPaths: string[] = inputProcessor.resolveTargetPaths(inputs);
expect(resolvedTargetPaths).to.have.length(1);
expect(resolvedTargetPaths).to.contain('.');

expect(display.getOutputText()).to.equal('[Info]: ' +
getMessage(BundleName.CommonRun, 'info.resolvedTarget'))
})
})

describe("resolveProjectDirPath Tests", async () => {
it("Specified relative projectdir", async () => {
const inputs: Inputs = {
projectdir: ['test/code-fixtures']
};
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths(inputs);
expect(resolvedProjectDirs).to.contain(toAbsPath('test/code-fixtures'))
})

it("Specified absolute projectdir", async () => {
const inputs: Inputs = {
projectdir: [toAbsPath('test/code-fixtures')]
};
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths(inputs);
expect(resolvedProjectDirs).to.contain(toAbsPath('test/code-fixtures'))
})

it("Specified tildified projectdir", async () => {
const inputs: Inputs = {
projectdir: ['~/someFolder']
};
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths(inputs);
expect(resolvedProjectDirs).to.contain(toAbsPath(normalize(untildify('~/someFolder'))))
})

it("Unspecified projectdir and unspecified target", async() => {
const inputs: Inputs = {}
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths(inputs);
expect(resolvedProjectDirs).to.contain(toAbsPath('.'));

expect(display.getOutputArray()).to.have.length(2)
expect(display.getOutputArray()).to.contain('[Info]: ' +
getMessage(BundleName.CommonRun, 'info.resolvedTarget'))
expect(display.getOutputArray()).to.contain('[Info]: ' +
getMessage(BundleName.CommonRun, 'info.resolvedProjectDir', [toAbsPath('')]))
})

it("Unspecified projectdir with non-glob relative targets supplied", async () => {
const inputs: Inputs = {
target: ['test/code-fixtures/apex', 'test/catalog-fixtures/DefaultCatalogFixture.json']
};
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths(inputs);
expect(resolvedProjectDirs).to.contain(toAbsPath('test'));

expect(display.getOutputText()).to.equal('[Info]: ' +
getMessage(BundleName.CommonRun, 'info.resolvedProjectDir', [toAbsPath('test')]))
})

it("Unspecified projectdir with glob targets supplied (with sfdx-project.json in parents)", async () => {
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths({
target: ['test/**/*.page', '!test/code-fixtures/cpd']
});
// Note that test/code-fixtures/projects/app/force-app/main/default/pages is the first most common parent
// but test/code-fixtures/projects/app contains a sfdx-project.json and so we return this instead
expect(resolvedProjectDirs).to.contain(toAbsPath('test/code-fixtures/projects/app'));
})

it("Unspecified projectdir with glob targets supplied (with no sfdx-project.json in parents)", async () => {
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths({
target: ['test/code-fixtures/**/*.cls']
});
expect(resolvedProjectDirs).to.contain(toAbsPath('test/code-fixtures'));
})

it("Unspecified projectdir with target containing method specifiers", async () => {
const resolvedProjectDirs: string[] = inputProcessor.resolveProjectDirPaths({
target: [
'test/code-fixtures/apex/SomeTestClass.cls#testMethodWithoutAsserts',
'test/code-fixtures/apex/SomeOtherTestClass.cls#someTestMethodWithoutAsserts',
]
});
expect(resolvedProjectDirs).to.contain(toAbsPath('test/code-fixtures/apex'));
})

it("Unspecified projectdir with non-glob target that resolves to no files", async () => {
const inputs: Inputs = {
target: ['thisFileDoesNotExist.xml', 'thisFileAlsoDoesNotExist.json']
};
try {
inputProcessor.resolveProjectDirPaths(inputs);
assert.fail("Expected error to be thrown")
} catch (e) {
expect(e.message).to.equal(getMessage(BundleName.CommonRun, 'validations.noFilesFoundInTarget'));
}
})

it("Unspecified projectdir with glob target that resolves to no files", async () => {
const inputs: Inputs = {
target: ['**.filesOfThisTypeShouldNotExist']
};
try {
inputProcessor.resolveProjectDirPaths(inputs);
assert.fail("Expected error to be thrown")
} catch (e) {
expect(e.message).to.equal(getMessage(BundleName.CommonRun, 'validations.noFilesFoundInTarget'));
}
})
})
})

function toAbsPath(fileOrFolder: string): string {
return path.resolve(fileOrFolder)
}
49 changes: 1 addition & 48 deletions test/lib/sfge/SfgePathlessEngine.test.ts
Original file line number Diff line number Diff line change
@@ -4,7 +4,6 @@ import {SfgeConfig} from '../../../src/types';
import {CUSTOM_CONFIG} from '../../../src/Constants';
import {SfgePathlessEngine} from '../../../src/lib/sfge/SfgePathlessEngine';
import * as TestOverrides from '../../test-related-lib/TestOverrides';
import {BundleName, getMessage} from "../../../src/MessageCatalog";

TestOverrides.initializeTestSetup();

@@ -52,7 +51,7 @@ describe('SfgePathlessEngine', () => {
});

describe('#shouldEngineRun()', () => {
it('Returns true when SfgeConfig has non-empty projectdirs array', async () => {
it('Returns true when SfgeConfig has non-empty projectdir array', async () => {
// ==== SETUP ====
const engine = new SfgePathlessEngine();
await engine.init();
@@ -67,51 +66,5 @@ describe('SfgePathlessEngine', () => {
// ==== ASSERTIONS ====
expect(shouldEngineRun).to.be.true;
});

it('Throws error when SfgeConfig has empty projectdirs array', async () => {
// ==== SETUP ====
const engine = new SfgePathlessEngine();
await engine.init();
const sfgeConfig: SfgeConfig = {
projectDirs: []
};
const engineOptions: Map<string,string> = new Map();
engineOptions.set(CUSTOM_CONFIG.SfgeConfig, JSON.stringify(sfgeConfig));
// ==== TESTED METHOD ====
const invocationOfShouldEngineRun = () => {
// The only parameter that matters should be the engine options.
return engine.shouldEngineRun([], [], [], engineOptions);
};
// ==== ASSERTIONS ====
expect(invocationOfShouldEngineRun).to.throw(getMessage(BundleName.SfgeEngine, 'errors.failedWithoutProjectDir', []));
});

it('Throws error when SfgeConfig lacks projectdirs array', async () => {
// ==== SETUP ====
const engine = new SfgePathlessEngine();
await engine.init();
const engineOptions: Map<string,string> = new Map();
engineOptions.set(CUSTOM_CONFIG.SfgeConfig, "{}");
// ==== TESTED METHOD ====
const invocationOfShouldEngineRun = () => {
// The only parameter that matters should be the engine options.
return engine.shouldEngineRun([], [], [], engineOptions);
};
// ==== ASSERTIONS ====
expect(invocationOfShouldEngineRun).to.throw(getMessage(BundleName.SfgeEngine, 'errors.failedWithoutProjectDir', []));
});

it('Throws error when SfgeConfig is outright absent', async () => {
// ==== SETUP ====
const engine = new SfgePathlessEngine();
await engine.init();
// ==== TESTED METHOD ====
const invocationOfShouldEngineRun = () => {
// The only parameter that matters should be the engine options.
return engine.shouldEngineRun([], [], [], new Map());
};
// ==== ASSERTIONS ====
expect(invocationOfShouldEngineRun).to.throw(getMessage(BundleName.SfgeEngine, 'errors.failedWithoutProjectDir', []));
});
});
});

0 comments on commit bd3785d

Please sign in to comment.