disneystreaming / smithy-translate

Other
53 stars 12 forks source link

Fix the prevent enum conflict algorithm #225

Closed daddykotex closed 8 months ago

daddykotex commented 8 months ago

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


At HEAD, fixed:

> git log -1 --oneline 
a879ff9 (HEAD -> dfrancoeur/fix-enum-conflicts) Fix the prevent enum conflict algorithm
David.Francoeur@MAC-W1459R ~/w/d/smithy-translate (dfrancoeur/fix-enum-conflicts)> mill proto[2.13.12].tests.testOnly smithyproto.proto3.ModelPrePocessorSpec
[153/153] proto[2.13.12].tests.testOnly 
No shapes annotated with alloy.proto#protoEnabled were found.
smithyproto.proto3.ModelPrePocessorSpec:
  + apply Transitive on protoEnabled w/o an allowed namespace 0.183s
  + apply Transitive on protoEnabled w/ an allowed namespace 0.012s
  + apply Transitive does nothing if namespace is excluded 0.012s
  + smithytranslate UUID is not included if alloy#UUID is not used 0.013s
  + alloy#UUID is converted to smithytranslate#UUID 0.009s
  + PreludeReplacements - keep big int 0.01s
  + PreludeReplacements - keep timestamp 0.008s
  + PreludeReplacements - keep big decimal 0.007s
  + apply PreventEnumConflicts 0.019s
  + apply PreventEnumConflicts - across namespace 0.258s

Before the fix, broken:

> git co HEAD^
Note: switching to 'HEAD^'.
[...]
> mill proto[2.13.12].tests.testOnly smithyproto.proto3.ModelPrePocessorSpec
[115/153] proto[2.13.12].compile 
[info] compiling 1 Scala source to /Users/David.Francoeur/workspace/dev/smithy-translate/out/proto/2.13.12/compile.dest/classes ...
[info] done compiling
[153/153] proto[2.13.12].tests.testOnly 
No shapes annotated with alloy.proto#protoEnabled were found.
smithyproto.proto3.ModelPrePocessorSpec:
  + apply Transitive on protoEnabled w/o an allowed namespace 0.192s
  + apply Transitive on protoEnabled w/ an allowed namespace 0.013s
  + apply Transitive does nothing if namespace is excluded 0.009s
  + smithytranslate UUID is not included if alloy#UUID is not used 0.014s
  + alloy#UUID is converted to smithytranslate#UUID 0.011s
  + PreludeReplacements - keep big int 0.01s
  + PreludeReplacements - keep timestamp 0.008s
  + PreludeReplacements - keep big decimal 0.007s
  + apply PreventEnumConflicts 0.015s
==> X smithyproto.proto3.ModelPrePocessorSpec.apply PreventEnumConflicts - across namespace  0.246s munit.ComparisonFailException: /Users/David.Francoeur/workspace/dev/smithy-translate/modules/proto/tests/src/ModelPrePocessorSpec.scala:370
369:
370:    assertEquals(
371:      getEnumNames(transformed, ShapeId.from("test#Enum1")),
values are not the same
=> Obtained
List(
  "VCONFLICT"
)
=> Diff (- obtained, + expected)
 List(
-  "VCONFLICT"
+  "ENUM1_VCONFLICT"
 )
    at munit.Assertions.failComparison(Assertions.scala:274)
1 targets failed
proto[2.13.12].tests.testOnly 1 tests failed: 
  smithyproto.proto3.ModelPrePocessorSpec.apply PreventEnumConflicts - across namespace smithyproto.proto3.ModelPrePocessorSpec.apply PreventEnumConflicts - across namespace

In the previous implementation, we don't see the conflic because the value in the map for VCONFLICT is overriden by the VCONFLICT of enums in other namespace then the one that has problem.