Fix deterministic order of properties (#1555)

* Add failing test * Fix failing test
smithy-lang · Jan 9, 2023 · 78ab8dc · 78ab8dc
1 parent d2f9c0f
commit 78ab8dc
Show file tree

Hide file tree

Showing 7 changed files with 29 additions and 13 deletions.
diff --git a/...hy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/Scope.java b/...hy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/Scope.java
@@ -17,7 +17,7 @@
 
 import java.util.ArrayDeque;
 import java.util.Deque;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Supplier;
@@ -81,7 +81,7 @@ public <U> U inScope(Supplier<U> func) {
 
     @Override
     public String toString() {
-        HashMap<Identifier, T> toPrint = new HashMap<>();
+        Map<Identifier, T> toPrint = new LinkedHashMap<>();
         for (ScopeLayer<T> layer : scope) {
             toPrint.putAll(layer.getTypes());
         }

diff --git a/smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/Type.java b/smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/Type.java
@@ -15,6 +15,7 @@
 
 package software.amazon.smithy.rulesengine.language.eval;
 
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -389,7 +390,7 @@ final class Record implements Type {
         private final Map<Identifier, Type> shape;
 
         public Record(Map<Identifier, Type> shape) {
-            this.shape = shape;
+            this.shape = new LinkedHashMap<>(shape);
         }
 
         @Override

diff --git a/...hy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/Value.java b/...hy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/eval/Value.java
@@ -20,6 +20,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -83,7 +84,7 @@ public Value numberNode(NumberNode node) {
 
             @Override
             public Value objectNode(ObjectNode node) {
-                HashMap<Identifier, Value> out = new HashMap<>();
+                HashMap<Identifier, Value> out = new LinkedHashMap<>();
                 node.getMembers().forEach((name, member) -> out.put(Identifier.of(name), Value.fromNode(member)));
                 return Value.record(out);
             }

diff --git a/...engine/src/main/java/software/amazon/smithy/rulesengine/language/syntax/expr/Literal.java b/...engine/src/main/java/software/amazon/smithy/rulesengine/language/syntax/expr/Literal.java
@@ -16,7 +16,9 @@
 package software.amazon.smithy.rulesengine.language.syntax.expr;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -142,7 +144,7 @@ public ILiteral numberNode(NumberNode numberNode) {
 
             @Override
             public ILiteral objectNode(ObjectNode objectNode) {
-                Map<Identifier, Literal> obj = new HashMap<>();
+                Map<Identifier, Literal> obj = new LinkedHashMap<>();
                 objectNode.getMembers().forEach((k, v) -> {
                     obj.put(Identifier.of(k), new Literal(v.accept(this), v));
                 });
@@ -245,7 +247,7 @@ public Type visitString(Template value) {
 
             @Override
             public Type visitRecord(Map<Identifier, Literal> members) {
-                Map<Identifier, Type> tpe = new HashMap<>();
+                Map<Identifier, Type> tpe = new LinkedHashMap<>();
                 ((Record) value).members.forEach((k, v) -> {
                     tpe.put(k, v.typeCheck(scope));
                 });
@@ -543,7 +545,7 @@ static final class Record implements ILiteral {
         private final Map<Identifier, Literal> members;
 
         Record(Map<Identifier, Literal> members) {
-            this.members = members;
+            this.members = Collections.unmodifiableMap(new LinkedHashMap<>(members));
         }
 
         public Map<Identifier, Literal> members() {

diff --git a/...in/resources/software/amazon/smithy/rulesengine/testutil/valid-rules/minimal-ruleset.json b/...in/resources/software/amazon/smithy/rulesengine/testutil/valid-rules/minimal-ruleset.json
@@ -1,15 +1,16 @@
 {
+  "version": "1.3",
   "parameters": {
     "Region": {
-      "type": "string",
       "builtIn": "AWS::Region",
-      "required": true
+      "required": true,
+      "type": "String"
     }
   },
   "rules": [
     {
-      "documentation": "base rule",
       "conditions": [],
+      "documentation": "base rule",
       "endpoint": {
         "url": "https://{Region}.amazonaws.com",
         "properties": {
@@ -20,10 +21,10 @@
               "signingRegion": "{Region}"
             }
           ]
-        }
+        },
+        "headers": {}
       },
       "type": "endpoint"
     }
-  ],
-  "version": "1.3"
+  ]
 }
diff --git a/smithy-rules-engine/src/test/java/software/amazon/smithy/rulesengine/RulesetTestUtil.java b/smithy-rules-engine/src/test/java/software/amazon/smithy/rulesengine/RulesetTestUtil.java
@@ -2,9 +2,11 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.InputStreamReader;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.rulesengine.language.EndpointRuleSet;
+import software.amazon.smithy.utils.IoUtils;
 
 public class RulesetTestUtil {
     public static EndpointRuleSet loadRuleSet(String resourceId) {

diff --git a/...engine/src/test/java/software/amazon/smithy/rulesengine/language/EndpointRuleSetTest.java b/...engine/src/test/java/software/amazon/smithy/rulesengine/language/EndpointRuleSetTest.java
@@ -33,6 +33,7 @@
 import software.amazon.smithy.rulesengine.language.syntax.parameters.ParameterType;
 import software.amazon.smithy.rulesengine.language.syntax.parameters.Parameters;
 import software.amazon.smithy.rulesengine.language.syntax.rule.Rule;
+import software.amazon.smithy.utils.IoUtils;
 import software.amazon.smithy.utils.MapUtils;
 
 class EndpointRuleSetTest {
@@ -54,6 +55,14 @@ void testRuleEval() {
         assertEquals(expected, result.expectEndpoint());
     }
 
+    @Test
+    void testDeterministicSerde() {
+        String resourceId = "software/amazon/smithy/rulesengine/testutil/valid-rules/minimal-ruleset.json";
+        EndpointRuleSet actual = RulesetTestUtil.loadRuleSet(resourceId);
+        String asString = IoUtils.readUtf8Resource(RulesetTestUtil.class.getClassLoader(), resourceId);
+        assertEquals(Node.prettyPrintJson(Node.parseJsonWithComments(asString)), Node.prettyPrintJson(actual.toNode()));
+    }
+
     @Test
     void testMinimalRuleset() {
         EndpointRuleSet actual = RulesetTestUtil.minimalRuleSet();