Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

PROC-1564: Validate rules for CRUD without context defined #174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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 @@ -1077,7 +1077,7 @@ public static void verifyInternallyConsistentDefinition(
@Nonnull final ConsumableTestDefinition testDefinition)
throws IncompatibleTestMatrixException {
verifyInternallyConsistentDefinition(
testName, matrixSource, testDefinition, ProvidedContext.nonEvaluableContext());
testName, matrixSource, testDefinition, ProvidedContext.evaluableContext());
}

/**
Expand Down Expand Up @@ -1216,7 +1216,7 @@ private static void verifyInternallyConsistentDefinition(
try {
RuleVerifyUtils.verifyRule(
rule,
providedContext.shouldEvaluate(),
providedContext.getEvaluationMode(),
expressionFactory,
elContext,
providedContext.getUninstantiatedIdentifiers());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,28 @@

/** May provide a map of identifiers to use for rule evaluation */
public class ProvidedContext {
enum ProvidedContextEvaluationMode {
EVALUATE_WITH_CONTEXT,
EVALUATE_WITHOUT_CONTEXT,
SKIP_EVALUATION;
}
/** @deprecated Use factory method nonEvaluableContext() */
@Deprecated
public static final Map<String, ValueExpression> EMPTY_CONTEXT = Collections.emptyMap();

private static final ProvidedContext NON_EVALUABLE =
new ProvidedContext(ProvidedContext.EMPTY_CONTEXT, false);
private static final ProvidedContext EMPTY_EVALUABLE =
new ProvidedContext(
ProvidedContext.EMPTY_CONTEXT,
ProvidedContextEvaluationMode.EVALUATE_WITHOUT_CONTEXT);
private final Map<String, ValueExpression> context;
private final Set<String> uninstantiatedIdentifiers;
/** if false, indicates that for rule verification, this context is unusable (empty) */
private final boolean shouldEvaluate;

private final ProvidedContextEvaluationMode mode;

/** @deprecated Use factory methods */
@Deprecated
public ProvidedContext(
Expand All @@ -27,6 +38,20 @@ public ProvidedContext(
this.context = context;
this.shouldEvaluate = shouldEvaluate;
this.uninstantiatedIdentifiers = uninstantiatedIdentifiers;
this.mode =
shouldEvaluate
? ProvidedContextEvaluationMode.EVALUATE_WITH_CONTEXT
: ProvidedContextEvaluationMode.SKIP_EVALUATION;
}

/** @deprecated Use factory methods */
@Deprecated
public ProvidedContext(
final Map<String, ValueExpression> context, final ProvidedContextEvaluationMode mode) {
this.context = context;
this.shouldEvaluate = mode == ProvidedContextEvaluationMode.SKIP_EVALUATION;
this.uninstantiatedIdentifiers = Collections.emptySet();
this.mode = mode;
}

/** @deprecated Use factory methods */
Expand All @@ -53,6 +78,10 @@ public static ProvidedContext nonEvaluableContext() {
return NON_EVALUABLE;
}

public static ProvidedContext evaluableContext() {
return EMPTY_EVALUABLE;
}

public Map<String, ValueExpression> getContext() {
return context;
}
Expand All @@ -61,6 +90,10 @@ public Set<String> getUninstantiatedIdentifiers() {
return uninstantiatedIdentifiers;
}

public ProvidedContextEvaluationMode getEvaluationMode() {
return mode;
}

public boolean shouldEvaluate() {
return shouldEvaluate;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,31 @@ private RuleVerifyUtils() {}
*
* @param shouldEvaluate if false, only checks syntax is correct
*/
@Deprecated
public static void verifyRule(
final String testRule,
final boolean shouldEvaluate,
final ExpressionFactory expressionFactory,
final ELContext elContext,
final Set<String> absentIdentifiers)
throws InvalidRuleException {
verifyRule(
testRule,
shouldEvaluate
? ProvidedContext.ProvidedContextEvaluationMode.EVALUATE_WITH_CONTEXT
: ProvidedContext.ProvidedContextEvaluationMode.SKIP_EVALUATION,
expressionFactory,
elContext,
absentIdentifiers);
}

public static void verifyRule(
final String testRule,
final ProvidedContext.ProvidedContextEvaluationMode evaluationMode,
final ExpressionFactory expressionFactory,
final ELContext elContext,
final Set<String> absentIdentifiers)
throws InvalidRuleException {
if (!isEmptyElExpression(testRule)) {
final ValueExpression valueExpression;
try {
Expand All @@ -51,7 +69,7 @@ public static void verifyRule(
testRule));
}

if (shouldEvaluate) {
if (evaluationMode != ProvidedContext.ProvidedContextEvaluationMode.SKIP_EVALUATION) {
/*
* must have a context to test against, even if it's "Collections.emptyMap()", how to
* tell if this method is used for ProctorBuilder or during load of the testMatrix.
Expand All @@ -73,17 +91,20 @@ public static void verifyRule(
testRule));
}

// Check identifiers in the AST and verify variable names
final Node undefinedIdentifier =
checkUndefinedIdentifier(root, elContext, absentIdentifiers);
if (undefinedIdentifier != null) {
throw new InvalidRuleException(
String.format(
"The variable %s is defined in rule %s, however it "
+ "is not defined in the application's test specification. Add the variable to your application's "
+ "providedContext.json or remove it from the rule, or if the application should not load your "
+ "test report the issue to the Proctor team.",
undefinedIdentifier.getImage(), testRule));
if (evaluationMode
== ProvidedContext.ProvidedContextEvaluationMode.EVALUATE_WITH_CONTEXT) {
// Check identifiers in the AST and verify variable names
final Node undefinedIdentifier =
checkUndefinedIdentifier(root, elContext, absentIdentifiers);
if (undefinedIdentifier != null) {
throw new InvalidRuleException(
String.format(
"The variable %s is defined in rule %s, however it "
+ "is not defined in the application's test specification. Add the variable to your application's "
+ "providedContext.json or remove it from the rule, or if the application should not load your "
+ "test report the issue to the Proctor team.",
undefinedIdentifier.getImage(), testRule));
}
}

// Evaluate rule with given context
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,22 @@ public void testELValidity_inProctorBuilderAllocationRules()
"-1:0.25,0:0.5,1:0.25"));
verifyInternallyConsistentDefinition(
"testELevalProctor", "test el recognition", testDefVal1);

// testing valid functions pass with proctor included functions (will throw exception if
// can't find) and backwards compatibility
final ConsumableTestDefinition testDefVal2 =
constructDefinition(
buckets,
fromCompactAllocationFormat(
"${proctor:version('213.0')=='2.0.0.0'}|-1:0.5,0:0.5,1:0.0",
"-1:0.25,0:0.5,1:0.25"));
assertThatThrownBy(
() ->
verifyInternallyConsistentDefinition(
"testELevalProctor", "test el recognition", testDefVal2))
.isInstanceOf(IncompatibleTestMatrixException.class)
.hasMessage(
"Invalid allocation rule in testELevalProctor: Failed to evaluate a rule ${proctor:version('213.0')=='2.0.0.0'}: Problems calling function [proctor:version]");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,22 @@ public void testVerifyRulesNestedMapBoolean() {
new String[] {});
}

@Test
public void testVerifyRulesProctorVersion() {
expectInvalidRule(
"${version != proctor:version('213.0')}",
new Object[][] {
{"version", ""},
},
new String[] {});
expectValidRule(
"${version != proctor:version('213.0.0.0')}",
new Object[][] {
{"version", ""},
},
new String[] {});
}

@Test
public void testValidRulesWithMethodCall() {
expectValidRule(
Expand Down
Loading