From 61e3b66a2a2eb704b029e72cd446f583135228de Mon Sep 17 00:00:00 2001 From: David Francoeur Date: Wed, 20 Dec 2023 14:59:08 -0500 Subject: [PATCH 1/2] Add a test to cover previous error --- .../tests/src/ModelPrePocessorSpec.scala | 82 +++++++++++++++++-- 1 file changed, 77 insertions(+), 5 deletions(-) diff --git a/modules/proto/tests/src/ModelPrePocessorSpec.scala b/modules/proto/tests/src/ModelPrePocessorSpec.scala index 238d0b2..369792e 100644 --- a/modules/proto/tests/src/ModelPrePocessorSpec.scala +++ b/modules/proto/tests/src/ModelPrePocessorSpec.scala @@ -320,6 +320,75 @@ class ModelPrePocessorSpec extends FunSuite { } } + test("apply PreventEnumConflicts - across namespace") { + val smithyTest = + """|$version: "2" + |namespace test + | + |enum Enum1 { + | VCONFLICT + |} + | + |enum Enum2 { + | VCONFLICT + |} + |""".stripMargin + + val other = + """|$version: "2" + |namespace a.ns + | + |enum OtherEnum { + | VCONFLICT + |} + |""".stripMargin + val original = buildModel(smithyTest, other) + val transformed = + process(original, ModelPreProcessor.transformers.PreventEnumConflicts) + def getEnumNames(m: Model, shapeId: ShapeId): List[String] = { + m.getShape(shapeId) + .toScala + .toList + .collect { + case shape: EnumShape => + shape.getMemberNames.asScala.toList + case shape: IntEnumShape => + shape.getMemberNames.asScala.toList + } + .flatten + } + + assertEquals( + getEnumNames(original, ShapeId.from("test#Enum1")), + List("VCONFLICT") + ) + + assertEquals( + getEnumNames(transformed, ShapeId.from("test#Enum1")), + List("ENUM1_VCONFLICT") + ) + + assertEquals( + getEnumNames(original, ShapeId.from("test#Enum2")), + List("VCONFLICT") + ) + + assertEquals( + getEnumNames(transformed, ShapeId.from("test#Enum2")), + List("ENUM2_VCONFLICT") + ) + + assertEquals( + getEnumNames(original, ShapeId.from(s"a.ns#OtherEnum")), + List("VCONFLICT") + ) + + assertEquals( + getEnumNames(transformed, ShapeId.from(s"a.ns#OtherEnum")), + List("VCONFLICT") + ) + } + private def checkTransformer(src: String, t: ProjectionTransformer)( check: (Model, Model) => Unit ): Unit = { @@ -328,13 +397,16 @@ class ModelPrePocessorSpec extends FunSuite { check(original, transformed) } - private def buildModel(src: String): Model = { - Model + private def buildModel(srcs: String*): Model = { + val assembler = Model .assembler() .discoverModels() - .addUnparsedModel("inlined-in-test.smithy", src) - .assemble() - .unwrap() + + srcs.zipWithIndex.foreach { case (s, i) => + assembler.addUnparsedModel(s"inlined-in-test.$i.smithy", s) + } + + assembler.assemble().unwrap() } private def process(m: Model, t: ProjectionTransformer): Model = { From 07cf64cfa33fff976eb9f57c587d9d2af6ca9352 Mon Sep 17 00:00:00 2001 From: David Francoeur Date: Wed, 20 Dec 2023 14:23:23 -0500 Subject: [PATCH 2/2] Fix the prevent enum conflict algorithm The gist of it is that we look for enum members are dupplicated inside one namespace but we use a map[string, boolean], (equivalent to a set[string]) to check for duplicates the map should be keyed by `$namespace-$memberName`, not just `memberName` the issue is that depending on the order of the processing, we can override some keys in the map if other namespace have enum members with the same name. in theory it should not be a problem, but my implementation was wrong so it turned out to be a problem --- .../proto3/ModelPreProcessor.scala | 103 ++++++++---------- 1 file changed, 45 insertions(+), 58 deletions(-) diff --git a/modules/proto/src/smithyproto/proto3/ModelPreProcessor.scala b/modules/proto/src/smithyproto/proto3/ModelPreProcessor.scala index 8ed178d..c73e86d 100644 --- a/modules/proto/src/smithyproto/proto3/ModelPreProcessor.scala +++ b/modules/proto/src/smithyproto/proto3/ModelPreProcessor.scala @@ -113,75 +113,62 @@ object ModelPreProcessor { def getName(): String = "prevent-enum-conflicts" def transform(x: TransformContext): Model = { val currentModel = x.getModel - val enumsShapes: List[EnumShape] = - currentModel.getEnumShapes.asScala.toList - .filter(s => !Prelude.isPreludeShape(s)) + val enumsShapes: List[EnumShape] = currentModel + .getEnumShapes() + .asScala + .filterNot(Prelude.isPreludeShape) + .toList - val intEnums: List[IntEnumShape] = - currentModel.getIntEnumShapes.asScala.toList.filter(s => - !Prelude.isPreludeShape(s) - ) + val intEnums: List[IntEnumShape] = currentModel + .getIntEnumShapes() + .asScala + .filterNot(Prelude.isPreludeShape) + .toList val allEnums: List[Shape] = enumsShapes ++ intEnums - val enumLabelsPerNamespace = allEnums - .groupMapReduce(_.getId.getNamespace)(es => - es.getMemberNames.asScala.toList - )(_ ++ _) - - val enumHasConflictMap: Map[String, Boolean] = { - allEnums.flatMap { es => - val id = es.getId - val enums = es.getMemberNames.asScala.toList - - val allEnumValues = - enumLabelsPerNamespace.getOrElse(id.getNamespace, List.empty) + val allCombos = for { + e <- allEnums + memberName <- e.getMemberNames().asScala.toList + } yield (e.getId().getNamespace(), memberName) - enums.map { enumName => - enumName -> (allEnumValues.count(_ == enumName) > 1) + val allRepeatedCombos = + allCombos + .groupBy(identity) + .view + .mapValues(_.size) + .collect { + case (k, v) if v > 1 => k } - }.toMap + .toSet - } - def enumHasConflict(enumValue: String): Boolean = { - enumHasConflictMap.getOrElse(enumValue, false) - } + def hasConflict(member: MemberShape): Boolean = allRepeatedCombos( + (member.getId().getNamespace(), member.getMemberName()) + ) - val newEnumShapes: List[Shape] = enumsShapes - .map { enumShape => - { - val b = enumShape.toBuilder - b.clearMembers() - enumShape.getAllMembers.asScala.foreach { - case (memberName, member) => - val newMember = if (enumHasConflict(memberName)) { - renameMember(member) - } else { - member - } - b.addMember(newMember) - } - b.build() - } + val newEnumShapes: List[Shape] = enumsShapes.map { enumShape => + val b = enumShape.toBuilder + b.clearMembers() + enumShape.members.asScala.foreach { + case member if hasConflict(member) => + b.addMember(renameMember(member)) + case member => + b.addMember(member) } + b.build() + } - val newIntEnumShapes = intEnums - .map { enumShape => - { - val b = enumShape.toBuilder - b.clearMembers() - enumShape.getAllMembers.asScala.foreach { - case (memberName, member) => - val newMember = if (enumHasConflict(memberName)) { - renameMember(member) - } else { - member - } - b.addMember(newMember) - } - b.build() - } + val newIntEnumShapes = intEnums.map { intEnumShape => + val b = intEnumShape.toBuilder + b.clearMembers() + intEnumShape.members.asScala.foreach { + case member if hasConflict(member) => + b.addMember(renameMember(member)) + case member => + b.addMember(member) } + b.build() + } val allShapes = newEnumShapes ++ newIntEnumShapes