Skip to content
New issue

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

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

Already on GitHub? # to your account

gen/enum: Add go.label to override item name #363

Merged
merged 12 commits into from
Aug 14, 2018
25 changes: 21 additions & 4 deletions gen/enum.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ import (
"go.uber.org/thriftrw/compile"
)

const (
// GoLabel overrides name on the wire
GoLabel = "go.label"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

styling nit: const group unnecessary if it's just one constant.

// GoLabel is the annotation expected on enum items to override their label.
const GoLabel = "go.label"


// enumGenerator generates code to serialize and deserialize enums.
type enumGenerator struct{}

Expand Down Expand Up @@ -98,7 +103,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
switch string(value) {
<- $enum := .Spec ->
<range .Spec.Items ->
case "<.Name>":
case "<enumItemWireName .>":
*<$v> = <enumItemName $enumName .>
return nil
<end ->
Expand All @@ -118,7 +123,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
switch int32(<$v>) {
<range .UniqueItems ->
case <.Value>:
return []byte("<.Name>"), nil
return []byte("<enumItemWireName .>"), nil
<end ->
}
<end ->
Expand Down Expand Up @@ -165,7 +170,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
switch <$w> {
<range .UniqueItems ->
case <.Value>:
return "<.Name>"
return "<enumItemWireName .>"
<end ->
}
<end ->
Expand All @@ -190,7 +195,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
switch int32(<$v>) {
<range .UniqueItems ->
case <.Value>:
return ([]byte)("\"<.Name>\""), nil
return ([]byte)("\"<enumItemWireName .>\""), nil
<end ->
}
<end ->
Expand Down Expand Up @@ -246,6 +251,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
UniqueItems: items,
},
TemplateFunc("enumItemName", enumItemName),
TemplateFunc("enumItemWireName", enumItemWireName),
)

return wrapGenerateError(spec.Name, err)
Expand All @@ -265,6 +271,17 @@ func enumItemName(enumName string, spec *compile.EnumItem) (string, error) {
return enumName + name, err
}

// enumItemWireName returns the actual name used for serialization/deserialization
// default to EnumItem.Name, override by the value of GoLabel
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

// enumItemLabelName returns the label we use for this enum item in the generated code.

(I'm trying to make it clear that the label actually has nothing to do with
serialization for the use cases that ThriftRW supports. Using ThriftRW objects
as JSON is technically unsupported; it just happens to work.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, the serialization/deserialization is pretty misleading given ThriftRW's context here

func enumItemWireName(spec *compile.EnumItem) string {
labelName := spec.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: can be simplified:

if val := spec.Annotations[GoLabel]; len(val) > 0 {
  return val
}
return spec.Name

val, ok := spec.Annotations[GoLabel]
if ok && len(val) > 0 {
labelName = val
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If you use the single assignment form for a map, it returns the zero
value for that type (which is an empty string in this case).

So this can be simplified to,

if val := spec.Annotations[GoLabel]; len(val) > 0 {
  labelName = val
}

}
return labelName
}

// enumUniqueItems returns a subset of the given list of enum items where
// there are no value collisions between items.
func enumUniqueItems(items []compile.EnumItem) []compile.EnumItem {
Expand Down
66 changes: 66 additions & 0 deletions gen/enum_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -399,3 +399,69 @@ func TestEnumAccessors(t *testing.T) {
})
})
}

func TestEnumLabelLegit(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/Legit/Valid

tests := []struct {
enumItem te.EnumWithLabel
wireValue string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: jsonValue not wireValue

}{
{te.EnumWithLabelUsername, "\"surname\""},
{te.EnumWithLabelPassword, "\"hashed_password\""},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can use backticks to avoid having to escape "

`"surname"`
`"hashed_password"`

{te.EnumWithLabelSalt, "\"salt\""},
{te.EnumWithLabelSugar, "\"sugar\""},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a case for unknown enums here. (They're still valid.)

{te.EnumWithLabel(42), "42"}

}
for _, tt := range tests {
t.Run("compare label:"+tt.wireValue, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just do t.Run(tt.enumItem.String()

// marshal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added marshal / unmarshal behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the JSON behavior check in a subtest since this is also testing roundtripping.

t.Run("JSON round trip", func(t *testing.T) {
  ...
})
assertRoundTrip(...)

label := te.EnumWithLabel(tt.enumItem)
b, err := json.Marshal(label)
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do require.NoError here so that the test doesn't proceed if it failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

assert.Equal(t, tt.wireValue, string(b))

// unmarshal
var expectedLabel te.EnumWithLabel
err = json.Unmarshal(b, &expectedLabel)
Copy link
Contributor

Choose a reason for hiding this comment

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

require.NoError

assert.Equal(t, expectedLabel, label)
Copy link
Contributor

Choose a reason for hiding this comment

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

With assert.Equal, the call pattern is assert.Equal(t, expected, got). Here, label is what we expect and expectedLabel is what we got (from deserializing). So this should be,

assert.Equal(t, label, expectedLabel)

(We should fix naming.)


assertRoundTrip(
t, &tt.enumItem,
wire.NewValueI32(int32(tt.enumItem)),
"test roundtrip "+tt.wireValue,
)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also confirm through an integration test that this effects the desired behavior for JSON marshaling?

Can we also test the behavior of serializing an unrecognized enum case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

@cl1337 cl1337 Aug 6, 2018

Choose a reason for hiding this comment

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

just want to make sure I understand your comment correctly, are you suggesting something here like:

...
    // marshal
    label := te.EnumWithWireLabel(te.EnumWithWireLabelUsername)
    b, err := json.Marshal(label)
    assert.Equal(t, "username", string(b))

    //  unmarlshal      
    var testLabel te.EnumWithWireLabel
    err = json.Unmarshal(b, testLabel)
    assert.Equal(t, testLabel, label)
... 

or you're suggesting a specific kind of integration tests in this repo? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine.

}

func TestEnumLabelInvalid(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added test cases for invalid enum item marshal / unmarshal


for _, tt := range []struct {
value te.EnumWithLabel
rep string
}{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for table tests, we prefer the following form.

tests := []struct{
  ...
}{
  ...
}

for _, tt := range tests {
  ..
}

Inlining the test cases with the range statement hurts readability IMO.

{-1, "-1"},
{1 << 10, "1024"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't technically invalid. These are valid enum items. They're just unknown. You can fold these cases into the valid case above.

} {
invalidEnumItem := te.EnumWithLabel(tt.value)
b, err := json.Marshal(invalidEnumItem)
assert.NoError(t, err)
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 is default marshal behavior: if not matching any key, just marshal as string representation of int value, without raising error

assert.Equal(t, tt.rep, string(b))
}

for _, tt := range []struct {
errVal []byte
errMsg string
}{
{
[]byte("some-random-str"),
Copy link
Contributor

Choose a reason for hiding this comment

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

this test isn't testing the UnmarshalText at all -- this is testing the json.Unmarshal. Not sure that's useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/thriftrw/thriftrw-go/blob/dev/gen/internal/tests/enums/types.go#L98
json.Unmarshal will call UnmarshalText in this test case.

I was testing against UnmarshalText, and was suggested to test json.Unmarshal in previous revision, let me know if we want to also add UnmarshalText here

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that this code never hits UnmarshalText.
https://play.golang.org/p/B1SIZYDGr_4

Using json.Unmarshal is fine, but the test case is incorrect since it probably should be "some-random-str" (e.g., the quotes should be part of the value passed to Unmarshal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good catch! Thanks for this comment, I'll fix the test cases here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually looked at it again I added that first test case intentionally, you are right it didn't hit UnmarshalText at all like the rest of test cases, I added it as an invalid case for json Unmarshal, but has nothing to do with this label PR (generic invalid case), I'll make it "some-randome-str" so it hits unmarshalText

Copy link
Contributor

Choose a reason for hiding this comment

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

we should have a test for the original enum name, and ensure that it's no longer recognized. e.g., "USERNAME"

"invalid character 's' looking for beginning of value",
},
{
[]byte("\"; drop table users;\""),
"unknown enum value \"; drop table users;\" for \"EnumWithLabel\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use backticks to avoid quoting "

},
} {
var expectedLabel te.EnumWithLabel
err := json.Unmarshal(tt.errVal, &expectedLabel)
assert.Equal(t, err.Error(), tt.errMsg)
}
}
2 changes: 0 additions & 2 deletions gen/internal/tests/collision/constants.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions gen/internal/tests/enums/idl.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

173 changes: 173 additions & 0 deletions gen/internal/tests/enums/types.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions gen/internal/tests/thrift/enums.thrift
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,13 @@ enum lowerCaseEnum {
containing, lower_case, items
}

// EnumWithLabel use label name in serialization/deserialization
enum EnumWithLabel {
username (go.label = "surname"),
password (go.label = "hashed_password"),
salt (go.label = ""),
sugar (go.label)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the items be upper-case (the labels can be lower-case)? That matches our
Thrift Style Guide.

USERNAME (go.label = "surname")

And we should have one case where the label is a Thrift keyword since that's
the motivation behind this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good advice, enum name and its label is actually case sensitive, I added upper/lower cases for item name and label, also added one reserved keyword label

}

// collision with RecordType_Values() function.
enum RecordType_Values { FOO, BAR }