PROC-1564: Validate rules for CRUD without context defined

Author: zacharygoodwin

proctor-common/src/main/java/com/indeed/proctor/common/ProctorUtils.java

@@ -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());
     }
 
     /**
@@ -1216,7 +1216,7 @@ private static void verifyInternallyConsistentDefinition(
             try {
                 RuleVerifyUtils.verifyRule(
                         rule,
-                        providedContext.shouldEvaluate(),
+                        providedContext.getEvaluationMode(),
                         expressionFactory,
                         elContext,
                         providedContext.getUninstantiatedIdentifiers());

proctor-common/src/main/java/com/indeed/proctor/common/ProvidedContext.java

@@ -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(
@@ -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 */
@@ -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;
     }
@@ -61,6 +90,10 @@ public Set<String> getUninstantiatedIdentifiers() {
         return uninstantiatedIdentifiers;
     }
 
+    public ProvidedContextEvaluationMode getEvaluationMode() {
+        return mode;
+    }
+
     public boolean shouldEvaluate() {
         return shouldEvaluate;
     }

proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java

@@ -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 {
@@ -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.
@@ -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

proctor-common/src/test/java/com/indeed/proctor/common/TestProctorUtils.java

@@ -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

proctor-common/src/test/java/com/indeed/proctor/common/TestRuleVerifyUtils.java

@@ -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(