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

cl1337 · 2018-08-03T23:07:25Z

This PR addresses #362

CLAassistant · 2018-08-03T23:07:32Z

All committers have signed the CLA.

cl1337 · 2018-08-06T17:43:10Z

build failed due to

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

should be fixed by restart, which I don't have permission to do so

kriskowal

Thank you for proposing this change. I have some design and code nits I would like you to answer or address.

kriskowal · 2018-08-06T20:19:43Z

gen/enum.go

@@ -26,6 +26,11 @@ import (
 	"go.uber.org/thriftrw/compile"
 )

+const (
+	// GoWireLabel overrides name on the wire
+	GoWireLabel = "go.wire.label"


Let’s call this just "label" or "go.label" since it affects the JSON and log representation of the in memory struct and not the TBinary (wire) protocol.

kriskowal · 2018-08-06T20:26:27Z

gen/internal/tests/thrift/enums.thrift

+enum EnumWithWireLabel {
+    username (go.wire.label = "surname"),
+    password (go.wire.label = "hashed_password"),
+    salt (go.wire.label = "")


Can we also test an annotation without a value, for whether the behavior is equivalent to empty string? salt (label) is valid Thrift IDL.

kriskowal · 2018-08-06T20:31:36Z

gen/enum_test.go

+				"test roundtrip "+tt.wireValue,
+			)
+		})
+	}


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?

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

This is fine.

cl1337 · 2018-08-06T22:51:19Z

gen/enum_test.go

+	} {
+		invalidEnumItem := te.EnumWithLabel(tt.value)
+		b, err := json.Marshal(invalidEnumItem)
+		assert.NoError(t, err)


this is default marshal behavior: if not matching any key, just marshal as string representation of int value, without raising error

cl1337 · 2018-08-06T22:51:42Z

gen/enum_test.go

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


added marshal / unmarshal behavior

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(...)

cl1337 · 2018-08-06T22:52:04Z

gen/enum_test.go

+	}
+}
+
+func TestEnumLabelInvalid(t *testing.T) {


added test cases for invalid enum item marshal / unmarshal

kriskowal

This looks good to me. Please let @abhinav have a run at it.

cl1337 · 2018-08-07T00:42:17Z

thanks @kriskowal !

cl1337 · 2018-08-07T22:23:13Z

notes to myself:
1: add test to check label duplication enum Foo { x (label = "y"), y }
2: update the PR title and description and add an entry to the change-log

enum item label name introduced possibility of conflict enum item, that bypasses compile and idl parse, add error for such case in codegen, and add tests accordingly thriftrw#362

abhinav · 2018-08-09T18:24:24Z

gen/enum.go

+const (
+	// GoLabel overrides name on the wire
+	GoLabel = "go.label"
+)


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"

abhinav · 2018-08-09T22:12:04Z

gen/enum.go

@@ -265,6 +275,34 @@ func enumItemName(enumName string, spec *compile.EnumItem) (string, error) {
 	return enumName + name, err
 }

+// enumItemLabelName returns the actual name used for serialization/deserialization
+// default to EnumItem.Name, override by the value of GoLabel


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.)

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

abhinav · 2018-08-09T22:13:26Z

gen/enum.go

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


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 }

abhinav · 2018-08-09T22:14:55Z

gen/enum.go

+// duplicates in resolved enum item names
+func validateEnumUniqueNames(spec *compile.EnumSpec) error {
+	items := spec.Items
+	used := make(map[string]bool, len(items))


When using a map as a set, we prefer to use struct{} instead of bool so
that you don't accidentally rely on the value of the boolean. Plus struct{}
has zero runtime cost.

used := make(map[string]struct{}, len(items)) ... used[itemName] = struct{}{}

thanks! Pretty neat trick, and good to know about the zero runtime cost part :)

func BenchmarkBool(b *testing.B) { mp := make(map[string]bool) for i := 0; i < b.N; i++ { if _, ok := mp[string(i)]; ok { mp[string(i)] = true } } } func BenchmarkStruct(b *testing.B) { mp := make(map[string]struct{}) for i := 0; i < b.N; i++ { if _, ok := mp[string(i)]; ok { mp[string(i)] = struct{}{} } } }

▶ go test -bench=Benchmark -count=1 -benchmem bench_test.go goos: darwin goarch: amd64 BenchmarkBool-8 200000000 7.00 ns/op 0 B/op 0 allocs/op BenchmarkStruct-8 200000000 7.31 ns/op 0 B/op 0 allocs/op PASS

abhinav · 2018-08-09T22:19:22Z

gen/enum.go

+		itemName := enumItemLabelName(&i)
+		if _, isUsed := used[itemName]; isUsed {
+			return fmt.Errorf(
+				"duplicated item name %q found in enum %q",


nit: If we find a conflict, it would be nice to know what it's with. The above
map could be changed to a map[string]string, mapping label to original item.

map[string]EnumItem

Then this can be,

if conflict, isUsed := used[itemName]; isUsed { return fmt.Errorf( "item %q with label %q conflicts with item %q in enum %q", i.Name, itemName, conflict.Name, spec.Name) }

makes sense

abhinav · 2018-08-09T22:27:47Z

gen/enum_test.go

+		rep   string
+	}{
+		{-1, "-1"},
+		{1 << 10, "1024"},


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

abhinav · 2018-08-09T22:28:01Z

gen/enum_test.go

+		},
+		{
+			[]byte("\"; drop table users;\""),
+			"unknown enum value \"; drop table users;\" for \"EnumWithLabel\"",


nit: use backticks to avoid quoting "

abhinav · 2018-08-09T22:28:59Z

gen/enum_test.go

+					},
+				},
+			},
+			"duplicated item name \"A\" found in enum \"duplicate item name\"",


nit: backticks

abhinav · 2018-08-09T22:29:07Z

gen/enum_test.go

+	}
+}
+
+func TestGenInvalidEnumFailure(t *testing.T) {


nit: TestEnumLabelConflict

abhinav · 2018-08-09T22:30:38Z

gen/internal/tests/thrift/enums.thrift

+    username (go.label = "surname"),
+    password (go.label = "hashed_password"),
+    salt (go.label = ""),
+    sugar (go.label)


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.

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

cl1337 · 2018-08-10T04:20:43Z

Thanks @abhinav for the detailed review

abhinav

Great! Thanks for the change!

prashantv

Change looks good, would like to see more documentation (since this doesn't just affect the generated code but interoperability with external tools/systems that rely on the text output).

Also have some suggestions on tests

prashantv · 2018-08-13T17:36:05Z

CHANGELOG.md

- No changes yet.
+### Added
+- gen: Added `go.label` used in enum item to override enum
+  item name in codegen.


i think we should be more explicit about what exactly go.label affects here. as someone without much context on this change, it's unclear what "item name in codegen" actually means. does it mean the name of the Go identifier? Does it mean the name in String? What about JSON/YAML?

good point, what do you think of:

- gen: Added `go.label="<TAGGED_NAME>"` annotation for enum items. Corresponding items of the generated Go structs will be using <TAGGED_NAME> for JSON marshal/unmarshal instead of origin item name, this change does not apply to YAML

I think this change applies to all text outputs -- String(), JSON, YAML and anything else that uses TextMarshaler

I'll revise it to

- gen: Added `go.label="<TAGGED_NAME>"` annotation for enum items. Corresponding items of the generated Go structs will be using <TAGGED_NAME> for text marshal / unmarshal instead of its origin item name

prashantv · 2018-08-13T17:38:06Z

gen/enum.go

@@ -265,6 +273,32 @@ func enumItemName(enumName string, spec *compile.EnumItem) (string, error) {
 	return enumName + name, err
 }

+// enumItemLabelName returns the label we use for this enum item in the generated code.
+func enumItemLabelName(spec *compile.EnumItem) string {
+	labelName := spec.Name


optional: can be simplified:

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

prashantv · 2018-08-13T17:43:52Z

gen/enum_test.go

@@ -25,11 +25,12 @@ import (
 	"fmt"
 	"testing"

+	"github.com/stretchr/testify/assert"


think imports were grouped in this repo before -- can we keep the third party imports at the bottom?

ops, ide linter didnt' capture this

prashantv · 2018-08-13T17:45:16Z

gen/enum.go

 	"strings"

 	"go.uber.org/thriftrw/compile"
 )

+// GoLabel is the annotation expected on enum items to override their label.


similar to changelog, can we expand documentation here on what exactly this means and how this affects users relying on the JSON output from thriftrw-go

how about:

// GoLabel is the annotation expected on enum items to override their label. // Enum items tagged with `go.label=<TAGGED_NAME>` will generate go struct // using `TAGGED_NAME` for JSON Marshal/Unmarshal

There's no go struct involved for an enum, so I think it's a little confusing. I think it would be better as:

// GoLabel allows overriding the text formatting of an enum. // Enum items will use the annotation value when serialized as // a string. This affects String(), as well as text marshalling, used by JSON/YAML.

prashantv · 2018-08-13T17:46:29Z

gen/enum_test.go

+					},
+				},
+			},
+			`item "B" with label "A" conflicts with item "A" in enum "duplicate item name"`,


suggest an extra test where 2 different enums specify the same go.label

prashantv · 2018-08-13T17:47:57Z

gen/enum_test.go

+		errMsg string
+	}{
+		{
+			[]byte("some-random-str"),


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

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

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)

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

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

prashantv · 2018-08-13T17:48:43Z

gen/enum_test.go

+		errMsg string
+	}{
+		{
+			[]byte("some-random-str"),


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

prashantv

LGTM with comments

prashantv · 2018-08-14T18:59:34Z

CHANGELOG.md

+### Added
+- gen: Added `go.label="<TAGGED_NAME>"` annotation for enum items.
+  Corresponding items of the generated Go structs will be using
+  <TAGGED_NAME> for text marshal / unmarshal instead of its origin item


nit: some minor fixes, marshal -> marshalling, origin -> original

for text marshalling/unmarshaling instead of the original item name. This allows overriding the String/JSON/YAML output for an enum.

prashantv · 2018-08-14T19:01:44Z

gen/enum.go

 	"strings"

 	"go.uber.org/thriftrw/compile"
 )

+// GoLabel is the annotation expected on enum items to override their label.


There's no go struct involved for an enum, so I think it's a little confusing. I think it would be better as:

// GoLabel allows overriding the text formatting of an enum. // Enum items will use the annotation value when serialized as // a string. This affects String(), as well as text marshalling, used by JSON/YAML.

prashantv · 2018-08-14T19:04:00Z

gen/enum_test.go

+
+func TestEnumLabelInvalidUnmarshal(t *testing.T) {
+	tests := []struct {
+		errVal []byte


nit: since you are using strings for all the cases, make this string, and then convert to []byte when calling Unmarshal

removes some noise from tests

prashantv · 2018-08-14T19:04:33Z

gen/enum_test.go

+		t.Run(string(tt.errVal), func(t *testing.T) {
+			var expectedLabel te.EnumWithLabel
+			err := json.Unmarshal(tt.errVal, &expectedLabel)
+			assert.Equal(t, err.Error(), tt.errMsg)


use assert.EqualError, otherwise, if it doesn't return an error, this will panic

neat! Thanks for the advice

prashantv · 2018-08-14T19:04:59Z

gen/enum_test.go

+	for _, tt := range testCases {
+		t.Run(tt.spec.Name, func(t *testing.T) {
+			err := enum(nil, &tt.spec)
+			assert.Error(t, err)


use assert.EqualError instead of assert.Error + assert.Equal, which could panic if err is nil

This changes the generated MarshalLogObject for enums (thriftrw#366) to support the `go.label` annotation for enum items (thriftrw#363).

TextMarshaler cannot be round-tripped because of thriftrw#368.

🚀 add go.wire.label for enum to override on the wire name

62e78a7

cl1337 mentioned this pull request Aug 3, 2018

Support enum item renaming #362

Closed

kriskowal changed the title ~~add go.wire.label for enum item to override on the wire name~~ Add go.wire.label for enum item to override on the wire name Aug 6, 2018

kriskowal suggested changes Aug 6, 2018

View reviewed changes

♻️ address comments

c34fbf3

cl1337 commented Aug 6, 2018

View reviewed changes

kriskowal approved these changes Aug 6, 2018

View reviewed changes

cl1337 added 3 commits August 8, 2018 12:43

✅ adding duplicate check for name

50280ea

enum item label name introduced possibility of conflict enum item, that bypasses compile and idl parse, add error for such case in codegen, and add tests accordingly thriftrw#362

🔊 add enum go.label

adcc38e

⬆️ regen

50a01b6

cl1337 changed the title ~~Add go.wire.label for enum item to override on the wire name~~ Add go.label for enum item to override enum item name during code generation Aug 9, 2018

abhinav reviewed Aug 9, 2018

View reviewed changes

🔧 addressing comments, add case sensitive test cases

a77008f

abhinav approved these changes Aug 13, 2018

View reviewed changes

prashantv reviewed Aug 13, 2018

View reviewed changes

♻️ address comments: fix change log, add test cases for same label

dd737c3

prashantv reviewed Aug 14, 2018

View reviewed changes

prashantv approved these changes Aug 14, 2018

View reviewed changes

cl1337 and others added 5 commits August 14, 2018 14:27

✅ addressing commits in changelog, improve tests

906ce19

Merge branch 'dev' into cli/add-go-wire-label-for-enum

14241fc

CHANGELOG: collapse ADDED sections

dcefe66

enum: MarshalLogObject should support go.label

ba9c0a0

This changes the generated MarshalLogObject for enums (thriftrw#366) to support the `go.label` annotation for enum items (thriftrw#363).

enum: Test TextMarshaler and String

7d43886

TextMarshaler cannot be round-tripped because of thriftrw#368.

abhinav changed the title ~~Add go.label for enum item to override enum item name during code generation~~ gen/enum: Add go.label to override item name Aug 14, 2018

abhinav merged commit f686b85 into thriftrw:dev Aug 14, 2018

abhinav mentioned this pull request Aug 15, 2018

Release 1.13.0 #372

Closed

11 tasks

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

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

Conversation

cl1337 commented Aug 3, 2018

CLAassistant commented Aug 3, 2018 • edited Loading

cl1337 commented Aug 6, 2018

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl1337 Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

cl1337 commented Aug 7, 2018

cl1337 commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl1337 commented Aug 10, 2018

abhinav left a comment

Choose a reason for hiding this comment

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl1337 Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

CLAassistant commented Aug 3, 2018 •

edited

Loading

cl1337 Aug 6, 2018 •

edited

Loading

cl1337 Aug 13, 2018 •

edited

Loading