Fix small smithy-rules-engine issues (#1681)

* Expose errorRule and endpoint rule functions for override in RuleEvaluator * fix bug function endpoints fn construction A copy-paste error in built in function construction functions meant that the JVM class was incorrect. This produces unexpected results when visitors are invoked because the wrong visitor branch will be called. This doesn't happen in general because `fromNode` produces the correct behavior. * Fix toString of test expectation * Fix another unordered source & fix lint * Fix return type of StringEquals.ofExpressions
smithy-lang · Apr 6, 2023 · cbe14b3 · cbe14b3
1 parent 95817a5
commit cbe14b3
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 22 deletions.
diff --git a/...-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/RuleEvaluator.java b/...-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/RuleEvaluator.java
@@ -120,7 +120,6 @@ public Value visitLibraryFunction(FunctionDefinition definition, List<Expression
     }
 
     private Value handleRule(Rule rule) {
-        RuleEvaluator self = this;
         return scope.inScope(() -> {
             for (Condition condition : rule.getConditions()) {
                 Value value = evaluateCondition(condition);
@@ -143,28 +142,37 @@ public Value visitTreeRule(List<Rule> rules) {
 
                 @Override
                 public Value visitErrorRule(Expression error) {
-                    return error.accept(self);
+                    return RuleEvaluator.this.visitErrorRule(error);
                 }
 
                 @Override
                 public Value visitEndpointRule(Endpoint endpoint) {
-                    Value.Endpoint.Builder builder = Value.Endpoint.builder()
-                            .sourceLocation(endpoint)
-                            .url(endpoint.getUrl()
-                                    .accept(RuleEvaluator.this)
-                                    .expectString());
-                    endpoint.getProperties()
-                            .forEach((key, value) -> builder.addProperty(key.toString(),
-                                    value.accept(RuleEvaluator.this)));
-                    endpoint.getHeaders()
-                            .forEach((name, expressions) -> expressions.forEach(expr -> builder.addHeader(name,
-                                    expr.accept(RuleEvaluator.this).expectString())));
-                    return builder.build();
+                    return RuleEvaluator.this.visitEndpointRule(endpoint);
                 }
             });
         });
     }
 
+    public Value visitErrorRule(Expression error) {
+        return error.accept(this);
+    }
+
+    public Value visitEndpointRule(Endpoint endpoint) {
+        Value.Endpoint.Builder builder = Value.Endpoint.builder()
+                .sourceLocation(endpoint)
+                .url(endpoint.getUrl()
+                        .accept(RuleEvaluator.this)
+                        .expectString());
+        endpoint.getProperties()
+                .forEach((key, value) -> builder.addProperty(key.toString(),
+                        value.accept(RuleEvaluator.this)));
+        endpoint.getHeaders()
+                .forEach((name, expressions) -> expressions.forEach(expr -> builder.addHeader(name,
+                        expr.accept(RuleEvaluator.this).expectString())));
+        return builder.build();
+
+    }
+
     public Value evaluateCondition(Condition condition) {
         Value value = condition.getFn().accept(this);
         if (!value.isNone()) {

diff --git a/...ngine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/BooleanEquals.java b/...ngine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/BooleanEquals.java
@@ -48,8 +48,8 @@ public BooleanEquals(FunctionNode functionNode) {
      * @param right the right hand-side expression.
      * @return the function defining the BooleanEquals of the left and right expressions.
      */
-    public static Function ofExpressions(Expression left, Expression right) {
-        return LibraryFunction.ofExpressions(DEFINITION, left, right);
+    public static BooleanEquals ofExpressions(Expression left, Expression right) {
+        return new BooleanEquals(FunctionNode.ofExpressions(DEFINITION.getId(), left, right));
     }
 
     @Override

diff --git a/...engine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/StringEquals.java b/...engine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/StringEquals.java
@@ -48,8 +48,8 @@ public StringEquals(FunctionNode functionNode) {
      * @param right the right expression.
      * @return a function instance representing the StringEquals comparison.
      */
-    public static Function ofExpressions(Expression left, Expression right) {
-        return LibraryFunction.ofExpressions(DEFINITION, left, right);
+    public static StringEquals ofExpressions(Expression left, Expression right) {
+        return new StringEquals(FunctionNode.ofExpressions(DEFINITION.getId(), left, right));
     }
 
     @Override

diff --git a/...hy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/traits/ContextIndex.java b/...hy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/traits/ContextIndex.java
@@ -16,6 +16,7 @@
 package software.amazon.smithy.rulesengine.traits;
 
 import java.lang.ref.WeakReference;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -25,7 +26,6 @@
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.OperationShape;
 import software.amazon.smithy.model.shapes.Shape;
-import software.amazon.smithy.utils.MapUtils;
 import software.amazon.smithy.utils.Pair;
 import software.amazon.smithy.utils.SmithyUnstableApi;
 
@@ -77,11 +77,14 @@ public Map<MemberShape, ContextParamTrait> getContextParams(Shape operation) {
                 .orElseThrow(() -> new IllegalArgumentException(operation.toShapeId()
                                                                 + " is not an operation shape"));
 
-        return getModel().expectShape(operationShape.getInputShape())
+        LinkedHashMap<MemberShape, ContextParamTrait> out = new LinkedHashMap<>();
+
+        getModel().expectShape(operationShape.getInputShape())
                 .members().stream()
                 .map(memberShape -> Pair.of(memberShape, memberShape.getTrait(ContextParamTrait.class)))
                 .filter(pair -> pair.right.isPresent())
-                .collect(MapUtils.toUnmodifiableMap(pair -> pair.left, pair -> pair.right.get()));
+                .forEach(pair -> out.put(pair.left, pair.right.get()));
+        return out;
     }
 
     private Model getModel() {

diff --git a/...ules-engine/src/main/java/software/amazon/smithy/rulesengine/traits/ExpectedEndpoint.java b/...ules-engine/src/main/java/software/amazon/smithy/rulesengine/traits/ExpectedEndpoint.java
@@ -104,7 +104,11 @@ public String toString() {
         }
         if (!properties.isEmpty()) {
             sb.append("properties:\n");
-            properties.forEach((k, v) -> sb.append(StringUtils.indent(String.format("%s: %s", k, v), 2)));
+            properties.forEach((k, v) -> sb
+                    .append(
+                            StringUtils.indent(
+                                    String.format("%s: %s", k, Node.prettyPrintJson(v)), 2)
+                    ));
         }
         return sb.toString();
     }

diff --git a/...ine/src/test/java/software/amazon/smithy/rulesengine/language/fn/FunctionOfExprsTest.java b/...ine/src/test/java/software/amazon/smithy/rulesengine/language/fn/FunctionOfExprsTest.java
@@ -0,0 +1,69 @@
+package software.amazon.smithy.rulesengine.language.fn;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+import software.amazon.smithy.rulesengine.language.syntax.expr.Expression;
+import software.amazon.smithy.rulesengine.language.syntax.fn.FunctionDefinition;
+import software.amazon.smithy.rulesengine.language.visit.ExpressionVisitor;
+
+public class FunctionOfExprsTest {
+
+    @Test
+    void boolEquals() {
+        Expression boolEquals = Expression.of(true).equal(true);
+        assertEquals(boolEquals.accept(new ExpectLibFunction()), false);
+        assertEquals(boolEquals.accept(new ExpectBuiltInFunction()), "bool");
+    }
+
+
+    @Test
+    void stringEquals() {
+        Expression stringEquals = Expression.of("asdf").equal("asdf");
+        assertEquals(stringEquals.accept(new ExpectLibFunction()), false);
+        assertEquals(stringEquals.accept(new ExpectBuiltInFunction()), "string");
+    }
+
+    @Test
+    void not() {
+        Expression not = Expression.of("asdf").equal("asdf").not();
+        assertEquals(not.accept(new ExpectLibFunction()), false);
+        assertEquals(not.accept(new ExpectBuiltInFunction()), "not");
+    }
+
+    static class ExpectLibFunction extends ExpressionVisitor.Default<Boolean> {
+        @Override
+        public Boolean visitLibraryFunction(FunctionDefinition fn, List<Expression> args) {
+            return true;
+        }
+
+        @Override
+        public Boolean getDefault() {
+            return false;
+        }
+    }
+
+    static class ExpectBuiltInFunction extends ExpressionVisitor.Default<String> {
+        @Override
+        public String getDefault() {
+            return "other";
+        }
+
+        @Override
+        public String visitBoolEquals(Expression left, Expression right) {
+            return "bool";
+        }
+
+
+        @Override
+        public String visitStringEquals(Expression left, Expression right) {
+            return "string";
+        }
+
+        @Override
+        public String visitNot(Expression not) {
+            return "not";
+        }
+    }
+}