Merge pull request #1437 from disneystreaming/guarantee-enum-ordering

Respect enum values ordering

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 0.18.13
+
+* modify logic to guarantee that rendered and dynamic enum values respect the ordering from the specification.
+
 # 0.18.12
 
 * fix issue where schemas for fields of generated big structs (over 22 fields in size) would not be ordered correctly

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

@@ -342,13 +342,14 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
         })
 
       override def enumShape(shape: EnumShape): Option[Decl] = {
+        val enumValues = shape.getEnumValues()
         val values = shape
-          .getEnumValues()
+          .members()
           .asScala
           .zipWithIndex
-          .map { case ((name, value), index) =>
-            val member = shape.getMember(name).get()
-
+          .map { case (member, index) =>
+            val name = member.getMemberName()
+            val value = enumValues.get(name)
             EnumValue(
               value = value,
               intValue = index,
@@ -372,12 +373,13 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
       }
 
       override def intEnumShape(shape: IntEnumShape): Option[Decl] = {
+        val enumValues = shape.getEnumValues()
         val values = shape
-          .getEnumValues()
+          .members()
           .asScala
-          .map { case (name, value) =>
-            val member = shape.getMember(name).get()
-
+          .map { member =>
+            val name = member.getMemberName()
+            val value = enumValues.get(name)
             EnumValue(
               value = name,
               intValue = value,

modules/codegen/test/src/smithy4s/codegen/internals/RendererSpec.scala

@@ -16,7 +16,13 @@
 
 package smithy4s.codegen.internals
 
-final class RendererSpec extends munit.FunSuite {
+import org.scalacheck.Gen
+import org.scalacheck.Prop
+import software.amazon.smithy.model.shapes.EnumShape
+import software.amazon.smithy.model.Model
+import software.amazon.smithy.model.shapes.StructureShape
+
+final class RendererSpec extends munit.ScalaCheckSuite {
   import TestUtils._
 
   test("list member hints should be preserved") {
@@ -484,4 +490,34 @@ final class RendererSpec extends munit.FunSuite {
       )
     )
   }
+
+  property("enumeration order is preserved") {
+
+    // custom input to avoid scalacheck shrinking
+    case class Input(identifiers: List[String])
+    Prop.forAll(
+      Gen
+        .nonEmptyListOf(Gen.identifier.map(_.toUpperCase()))
+        .map(_.distinct)
+        .map(Input(_))
+    ) { input =>
+      import input.identifiers
+      val builder = EnumShape.builder().id("input#MyEnum")
+      identifiers.foreach(id => builder.addMember(id, id))
+      val enumShape = builder.build()
+      val unitShape = StructureShape.builder().id("smithy.api#Unit").build()
+
+      val model = Model.builder().addShapes(unitShape, enumShape).build()
+
+      val allContents = generateScalaCode(model)
+      allContents.values.exists { fileContent =>
+        val cleanLines = fileContent.linesIterator
+          .map(_.trim().replace(",", "")) // removing whitespace and commas
+          .toList
+
+        cleanLines.containsSlice(identifiers)
+      }
+    }
+  }
+
 }

modules/dynamic/src/smithy4s/dynamic/internals/DynamicModelCompiler.scala

@@ -220,7 +220,6 @@ private[dynamic] object Compiler {
         shape.members.toList
           // Introducing arbitrary ordering for reproducible rendering.
           // Needs to happen before zipWithIndex so that intValue is also predictable.
-          .sortBy(_._1)
           .flatMap { case (k, m) =>
             getTrait[smithy.api.EnumValue](m.traits).toList.map {
               _.value match {
@@ -276,7 +275,7 @@ private[dynamic] object Compiler {
     }
 
     override def intEnumShape(id: ShapeId, shape: IntEnumShape): Unit = {
-      val values: Map[Int, EnumValue[Int]] = shape.members.toList
+      val values: List[EnumValue[Int]] = shape.members.toList
         .flatMap { case (k, m) =>
           getTrait[smithy.api.EnumValue](m.traits).toList.map {
             _.value match {
@@ -291,22 +290,20 @@ private[dynamic] object Compiler {
           }
         }
         .map { case (name, intValue, traits) =>
-          intValue -> EnumValue(
+          EnumValue(
             stringValue = name,
             intValue = intValue,
             value = intValue,
             name = name,
             hints = allHints(traits)
           )
         }
-        .toMap
-
-      val valueList = values.map(_._2).toList.sortBy(_.intValue)
 
+      val valueMap = values.map(v => v.intValue -> v).toMap
       if (shape.traits.contains(IdRef("alloy#openEnum"))) {
         val theEnum = enumeration[Int](
           v =>
-            values.getOrElse(
+            valueMap.getOrElse(
               v,
               EnumValue(
                 stringValue = v.toString,
@@ -317,15 +314,15 @@ private[dynamic] object Compiler {
               )
             ),
           EnumTag.OpenIntEnum(identity),
-          valueList
+          values
         )
 
         update(id, shape.traits, theEnum)
       } else {
         update(
           id,
           shape.traits,
-          intEnumeration(values, valueList)
+          intEnumeration(valueMap, values)
         )
       }
     }

modules/dynamic/test/src-jvm/smithy4s/dynamic/EnumSpec.scala

@@ -23,6 +23,10 @@ import smithy4s.schema.EnumValue
 import smithy4s.Hints
 import smithy4s.Document
 import smithy4s.schema.Schema
+import smithy4s.schema.SchemaVisitor
+import smithy4s.schema.EnumTag
+import org.scalacheck.Prop
+import org.scalacheck.Gen
 
 class EnumSpec extends DummyIO.Suite {
   val model = """
@@ -154,17 +158,17 @@ class EnumSpec extends DummyIO.Suite {
       ShapeId("example", "Smithy20Enum"),
       expectedValues = List(
         EnumValue(
-          stringValue = "Fire",
+          stringValue = "Ice",
           intValue = 0,
           value = 0,
-          name = "FIRE",
+          name = "ICE",
           hints = Hints.empty
         ),
         EnumValue(
-          stringValue = "Ice",
+          stringValue = "Fire",
           intValue = 1,
           value = 1,
-          name = "ICE",
+          name = "FIRE",
           hints = Hints.empty
         )
       )
@@ -175,19 +179,19 @@ class EnumSpec extends DummyIO.Suite {
     assertEnum(
       ShapeId("example", "MyIntEnum"),
       expectedValues = List(
-        EnumValue(
-          stringValue = "FIRE",
-          intValue = 10,
-          value = 10,
-          name = "FIRE",
-          hints = Hints.empty
-        ),
         EnumValue(
           stringValue = "ICE",
           intValue = 42,
           value = 42,
           name = "ICE",
           hints = Hints.empty
+        ),
+        EnumValue(
+          stringValue = "FIRE",
+          intValue = 10,
+          value = 10,
+          name = "FIRE",
+          hints = Hints.empty
         )
       )
     )
@@ -204,7 +208,7 @@ class EnumSpec extends DummyIO.Suite {
         )
         .encode(1)
 
-      assertEquals(actual, Document.DString("Ice"))
+      assertEquals(actual, Document.DString("Fire"))
     }
   }
 
@@ -230,20 +234,20 @@ class EnumSpec extends DummyIO.Suite {
       ShapeId("example", "EnumWithTraits"),
       expectedValues = List(
         EnumValue(
-          stringValue = "FIRE",
+          stringValue = "ICE",
           intValue = 0,
           value = 0,
-          name = "FIRE",
-          hints = Hints.empty
-        ),
-        EnumValue(
-          stringValue = "ICE",
-          intValue = 1,
-          value = 1,
           name = "ICE",
           hints = Hints(
             ShapeId("smithy.api", "deprecated") -> Document.obj()
           )
+        ),
+        EnumValue(
+          stringValue = "FIRE",
+          intValue = 1,
+          value = 1,
+          name = "FIRE",
+          hints = Hints.empty
         )
       )
     )
@@ -279,6 +283,66 @@ class EnumSpec extends DummyIO.Suite {
     }
   }
 
+  property("Enums members retain the ordering from the smithy specification") {
+    // custom input to avoid scalacheck shrinking
+    case class Input(identifiers: List[String])
+    Prop.forAll(
+      Gen
+        .nonEmptyListOf(Gen.identifier.map(_.toUpperCase()))
+        .map(_.distinct)
+        .map(Input(_))
+    ) { input =>
+      import input.identifiers
+      val stringEnumBuilder = software.amazon.smithy.model.shapes.EnumShape
+        .builder()
+        .id("input#MyStringEnum")
+
+      val intEnumBuilder = software.amazon.smithy.model.shapes.IntEnumShape
+        .builder()
+        .id("input#MyIntEnum")
+      identifiers.foreach(id => stringEnumBuilder.addMember(id, id))
+      identifiers.zipWithIndex.foreach { case (name, value) =>
+        intEnumBuilder.addMember(name, identifiers.size - value)
+      }
+      val stringEnumShape = stringEnumBuilder.build()
+      val intEnumShape = intEnumBuilder.build()
+      val unitShape = software.amazon.smithy.model.shapes.StructureShape
+        .builder()
+        .id("smithy.api#Unit")
+        .build()
+
+      val model = software.amazon.smithy.model.Model
+        .builder()
+        .addShapes(unitShape, stringEnumShape, intEnumShape)
+        .build()
+
+      val index: DynamicSchemaIndex = DynamicSchemaIndex.loadModel(model)
+      val stringEnumSchemaNames = index
+        .getSchema(ShapeId("input", "MyStringEnum"))
+        .toList
+        .flatMap(EnumNamesSchemaVisitor(_))
+
+      val intEnumSchemaNames = index
+        .getSchema(ShapeId("input", "MyIntEnum"))
+        .toList
+        .flatMap(EnumNamesSchemaVisitor(_))
+      assertEquals(stringEnumSchemaNames, identifiers)
+      assertEquals(intEnumSchemaNames, identifiers)
+    }
+  }
+
+  type ConstList[A] = List[String]
+  object EnumNamesSchemaVisitor extends SchemaVisitor.Default[ConstList] {
+    def default[A]: List[String] = List.empty
+    override def enumeration[E](
+        shapeId: ShapeId,
+        hints: Hints,
+        tag: EnumTag[E],
+        values: List[EnumValue[E]],
+        total: E => EnumValue[E]
+    ): ConstList[E] = values.map(_.name)
+  }
+
   private def decodeEncodeCheck[A](schema: Schema[A])(input: Document) = {
     val decoded = Document.Decoder
       .fromSchema(schema)

modules/dynamic/test/src/smithy4s/dynamic/DummyIO.scala

@@ -27,7 +27,7 @@ object DummyIO {
     def raiseError[A](e: Throwable): IO[A] = Left(e)
   }
 
-  trait Suite extends munit.FunSuite {
+  trait Suite extends munit.ScalaCheckSuite {
     override def munitValueTransforms: List[ValueTransform] =
       ioValueTransform :: super.munitValueTransforms