From c3d5506bdb47b2d380dc5f8007165ba929b1ca36 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Mon, 1 Jul 2024 17:36:40 -0400 Subject: [PATCH] PROC-1564: Validate rules for CRUD without context defined --- .../indeed/proctor/common/ProctorUtils.java | 4 +- .../proctor/common/ProvidedContext.java | 33 ++++++++++++++ .../proctor/common/RuleVerifyUtils.java | 45 ++++++++++++++----- .../proctor/common/TestProctorUtils.java | 16 +++++++ .../proctor/common/TestRuleVerifyUtils.java | 16 +++++++ 5 files changed, 100 insertions(+), 14 deletions(-) diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/ProctorUtils.java b/proctor-common/src/main/java/com/indeed/proctor/common/ProctorUtils.java index 076fbd6c3..d2827efbe 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/ProctorUtils.java +++ b/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()); diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/ProvidedContext.java b/proctor-common/src/main/java/com/indeed/proctor/common/ProvidedContext.java index 53958e0b0..6e5e44808 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/ProvidedContext.java +++ b/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 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 context; private final Set 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 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 getContext() { return context; } @@ -61,6 +90,10 @@ public Set getUninstantiatedIdentifiers() { return uninstantiatedIdentifiers; } + public ProvidedContextEvaluationMode getEvaluationMode() { + return mode; + } + public boolean shouldEvaluate() { return shouldEvaluate; } diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java b/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java index 09cfcdd76..aeef530e6 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java @@ -29,6 +29,7 @@ private RuleVerifyUtils() {} * * @param shouldEvaluate if false, only checks syntax is correct */ + @Deprecated public static void verifyRule( final String testRule, final boolean shouldEvaluate, @@ -36,6 +37,23 @@ public static void verifyRule( final ELContext elContext, final Set 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 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 diff --git a/proctor-common/src/test/java/com/indeed/proctor/common/TestProctorUtils.java b/proctor-common/src/test/java/com/indeed/proctor/common/TestProctorUtils.java index c986d9aea..2e7a69353 100644 --- a/proctor-common/src/test/java/com/indeed/proctor/common/TestProctorUtils.java +++ b/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 diff --git a/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleVerifyUtils.java b/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleVerifyUtils.java index bfa8f763f..a55196926 100644 --- a/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleVerifyUtils.java +++ b/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(