CrowdStrike / csproto

CrowdStrike's Protocol Buffers library
MIT License
122 stars 13 forks source link

Using of the correct name for oneof message and options #124

Closed biosvs closed 1 year ago

biosvs commented 1 year ago

Example of the input protos, for which csProto generates uncompilable code:

message I18nVariable {
  oneof one_of_values {
    string opt_one = 1;
    string opt_two = 2;
  }
}

And another example:

message Msg {
  oneof one_of_values {

  Msg.Tags tags = 1;

  message Tags {
    repeated string tags = 1;
  }
}

In the first case, I18nVariable will be used in generated code, while protobuf-go changes name to I18NVariable (n -> N). In the second case, Msg_Tags will be used, while protobuf-go changes name of the oneof filed to MsgTags (Msg_Tags also exists - kinda tricky, I know).

This PR fixes generated code.

Once again, I didn't find unit tests for such thing, so I didn't add my either, sorry.

dylan-bourque commented 1 year ago

Thanks for the submission.

... didn't find unit tests ...

Yeah. The gotcha is that the test matrix involves generating code for each permutation of (proto2, proto3) X (gogo, google V1, google V2) and I didn't want to have the "main" module depend on all 3 underlying runtimes.

You can run make generate to build protoc-gen-fastmarshal to ./bin and use that to generate code in each of the example/* sub-folders. Ideally, we would also add a new example message in each of those variants for the edge cases you've uncovered.

dylan-bourque commented 1 year ago

@biosvs can you run make generate test to regenerate the .pb.go files under the example/ folder for each permutation and validate these changes?

biosvs commented 1 year ago

@dylan-bourque sure, made it.

dylan-bourque commented 1 year ago

v0.25.0 was tagged this morning and includes this change. Let me know if you find any other issues.

Thank you for the contribution.