Skip to content

feat(generator/rust): skip duplicate enum values #1538

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 19, 2025
Merged

feat(generator/rust): skip duplicate enum values #1538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions generator/internal/rust/annotate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package rust

import (
"fmt"
"log/slog"
"strings"

"github.com/googleapis/google-cloud-rust/generator/internal/api"
Expand Down Expand Up @@ -187,10 +188,10 @@ type fieldAnnotations struct {
}

type enumAnnotation struct {
Name string
ModuleName string
DocLines []string
DefaultValueName string
Name string
ModuleName string
DocLines []string
UniqueNames []*api.EnumValue
// The fully qualified name, including the `codec.modulePath`
// (typically `crate::model::`) prefix. For external enums this is prefixed
// by the external crate name.
Expand Down Expand Up @@ -491,22 +492,37 @@ func (c *codec) annotateEnum(e *api.Enum, state *api.APIState, sourceSpecificati
for _, ev := range e.Values {
c.annotateEnumValue(ev, e, state)
}
defaultValueName := ""
// For BigQuery (and so far only BigQuery), the enum values conflict when
// converted to the Rust style [1]. Basically, there are several enum values
// in this service that differ only in case, such as `FULL` vs. `full`.
//
// We create a list with the duplicates removed to avoid conflicts in the
// generated code.
//
// [1]: Both Rust and Protobuf use `SCREAMING_SNAKE_CASE` for these, but
// some services do not follow the Protobuf convention.
seen := map[string]*api.EnumValue{}
var unique []*api.EnumValue
for _, ev := range e.Values {
if ev.Number == 0 {
defaultValueName = enumValueName(ev)
break
name := enumValueName(ev)
if existing, ok := seen[name]; ok {
if existing.Number != ev.Number {
slog.Warn("conflicting names for enum values", "enum.ID", e.ID)
}
} else {
unique = append(unique, ev)
seen[name] = ev
}
}
qualifiedName := fullyQualifiedEnumName(e, c.modulePath, sourceSpecificationPackageName, c.packageMapping)
relativeName := strings.TrimPrefix(qualifiedName, c.modulePath+"::")
e.Codec = &enumAnnotation{
Name: enumName(e),
ModuleName: toSnake(enumName(e)),
DocLines: formatDocComments(e.Documentation, e.ID, state, c.modulePath, e.Scopes(), c.packageMapping),
DefaultValueName: defaultValueName,
QualifiedName: qualifiedName,
RelativeName: relativeName,
Name: enumName(e),
ModuleName: toSnake(enumName(e)),
DocLines: formatDocComments(e.Documentation, e.ID, state, c.modulePath, e.Scopes(), c.packageMapping),
UniqueNames: unique,
QualifiedName: qualifiedName,
RelativeName: relativeName,
}
}

Expand Down
69 changes: 60 additions & 9 deletions generator/internal/rust/annotate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,15 @@ func TestEnumAnnotations(t *testing.T) {
}
annotateModel(model, codec, "")

if diff := cmp.Diff(&enumAnnotation{
Name: "TestEnum",
ModuleName: "test_enum",
QualifiedName: "crate::model::TestEnum",
RelativeName: "TestEnum",
DocLines: []string{"/// The enum is documented."},
DefaultValueName: "VALUE",
}, enum.Codec); diff != "" {
want := &enumAnnotation{
Name: "TestEnum",
ModuleName: "test_enum",
QualifiedName: "crate::model::TestEnum",
RelativeName: "TestEnum",
DocLines: []string{"/// The enum is documented."},
UniqueNames: []*api.EnumValue{v0, v1, v2},
}
if diff := cmp.Diff(want, enum.Codec, cmpopts.IgnoreFields(api.EnumValue{}, "Codec", "Parent")); diff != "" {
t.Errorf("mismatch in enum annotations (-want, +got)\n:%s", diff)
}

Expand All @@ -315,7 +316,6 @@ func TestEnumAnnotations(t *testing.T) {
}, v1.Codec); diff != "" {
t.Errorf("mismatch in enum annotations (-want, +got)\n:%s", diff)
}

if diff := cmp.Diff(&enumValueAnnotation{
Name: "VALUE",
EnumType: "TestEnum",
Expand All @@ -325,6 +325,57 @@ func TestEnumAnnotations(t *testing.T) {
}
}

func TestDuplicateEnumValueAnnotations(t *testing.T) {
// Verify we can handle values that are not in SCREAMING_SNAKE_CASE style.
v0 := &api.EnumValue{
Name: "full",
ID: ".test.v1.TestEnum.full",
Number: 1,
}
v1 := &api.EnumValue{
Name: "FULL",
ID: ".test.v1.TestEnum.FULL",
Number: 1,
}
v2 := &api.EnumValue{
Name: "partial",
ID: ".test.v1.TestEnum.partial",
Number: 2,
}
// This does not happen in practice, but we want to verify the code can
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gave me pause. Maybe the generator should panic so we can go yell at the service team?

If we do receive the value 3, I guess we will just treat it as an UNKNOWN_VALUE:3 (or whatever). Which should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gave me pause. Maybe the generator should panic so we can go yell at the service team?

I thought about that, but then we would stuck: sidekick update fails for all services, not just the offending one. We would need to hack something.

If we do receive the value 3, I guess we will just treat it as an UNKNOWN_VALUE:3 (or whatever). Which should be fine.

Exactly. It is very unlikely to happen (https://google.aip.dev/126 says enum values must be SCREAMING_SNAKE_CASE, the deviations are service bugs). And if it does happen we still can handle the incoming enums.

// handle it if it ever does.
v3 := &api.EnumValue{
Name: "PARTIAL",
ID: ".test.v1.TestEnum.PARTIAL",
Number: 3,
}
enum := &api.Enum{
Name: "TestEnum",
ID: ".test.v1.TestEnum",
Values: []*api.EnumValue{v0, v1, v2, v3},
}

model := api.NewTestAPI(
[]*api.Message{}, []*api.Enum{enum}, []*api.Service{})
codec, err := newCodec(true, map[string]string{})
if err != nil {
t.Fatal(err)
}
annotateModel(model, codec, "")

want := &enumAnnotation{
Name: "TestEnum",
ModuleName: "test_enum",
QualifiedName: "crate::model::TestEnum",
RelativeName: "TestEnum",
UniqueNames: []*api.EnumValue{v0, v2},
}

if diff := cmp.Diff(want, enum.Codec, cmpopts.IgnoreFields(api.EnumValue{}, "Codec", "Parent")); diff != "" {
t.Errorf("mismatch in enum annotations (-want, +got)\n:%s", diff)
}
}

func TestJsonNameAnnotations(t *testing.T) {
parent := &api.Field{
Name: "parent",
Expand Down
4 changes: 2 additions & 2 deletions generator/internal/rust/templates/common/enum.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ limitations under the License.
pub struct {{Codec.Name}}(i32);

impl {{Codec.Name}} {
{{#Values}}
{{#Codec.UniqueNames}}

{{#Codec.DocLines}}
{{{.}}}
{{/Codec.DocLines}}
pub const {{Codec.Name}}: {{Codec.EnumType}} = {{Codec.EnumType}}::new({{Number}});
{{/Values}}
{{/Codec.UniqueNames}}

/// Creates a new {{Codec.Name}} instance.
pub(crate) const fn new(value: i32) -> Self {
Expand Down
Loading