Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the prevent enum conflict algorithm #225

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

daddykotex
Copy link
Contributor

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.

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
@daddykotex daddykotex requested a review from lewisjkl December 20, 2023 20:06
@daddykotex daddykotex merged commit 2fd0853 into main Dec 20, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants