Repository: yarpc/yab
Pull request #263 (2019-05-15 03:09:03)

Introduce yamlalias to simplify aliases in templates
For yab templates, we support a few aliases to simplify
usage for users. E.g., we support shardKey, shardkey, shard-key or sk.

This requires a lot of manual wiring up to work. Instead, introduce
a yamlalias package which will use reflection to generate a new struct
that contains additional fields for each alias.

codecov[bot]: # [Codecov](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=h1) Report
> Merging [#263](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=desc) into [dev](https://codecov.io/gh/yarpc/yab/commit/efe1b0c0ec02d608a32983346b46ae4c18c17d97?src=pr&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/yarpc/yab/pull/263/graphs/tree.svg?width=650&token=yEgrqUTCuh&height=150&src=pr)](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##              dev     #263      +/-   ##
==========================================
+ Coverage   92.42%   92.47%   +0.05%     
==========================================
  Files          43       44       +1     
  Lines        2364     2380      +16     
==========================================
+ Hits         2185     2201      +16     
  Misses        121      121              
  Partials       58       58
```


| [Impacted Files](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [options.go](https://codecov.io/gh/yarpc/yab/pull/263/diff?src=pr&el=tree#diff-b3B0aW9ucy5nbw==) | `94.59% <ø> (-0.28%)` | :arrow_down: |
| [internal/yamlalias/wrap.go](https://codecov.io/gh/yarpc/yab/pull/263/diff?src=pr&el=tree#diff-aW50ZXJuYWwveWFtbGFsaWFzL3dyYXAuZ28=) | `100% <100%> (ø)` | |
| [template.go](https://codecov.io/gh/yarpc/yab/pull/263/diff?src=pr&el=tree#diff-dGVtcGxhdGUuZ28=) | `86.66% <100%> (-2.47%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=footer). Last update [efe1b0c...829e76e](https://codecov.io/gh/yarpc/yab/pull/263?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/internal/yamlalias/wrap.go b/internal/yamlalias/wrap.go
new file mode 100644
index 0000000..a70a5ec
--- /dev/null
+++ b/internal/yamlalias/wrap.go
@@ -0,0 +1,123 @@
+// Package yamlalias adds support for aliases when parsing YAML.
+//
+// Aliases are specified as a comma-separated list specified via
+// "yaml-aliases" struct tag.
+//
+// Example:
+//
+//   type Config struct {
+//     UserName string `yaml:"username" yaml-aliases:"userName,user-name"`
+//   }
+//
+// This will parse the following YAML examples in the same way:
+//   {"username": "John Doe"}
+//   {"userName": "John Doe"}
+//   {"user-name": "John Doe"}
+//
+// Note: It only adds aliases for top-level fields in the struct.
+package yamlalias
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+
+	"gopkg.in/yaml.v2"
+)
+
+// Unmarshal is a wrapper for yaml.Unmarshal but adds
+// support for any yaml-aliases specified in out.
+func Unmarshal(in []byte, out interface{}) error {
+	return yaml.Unmarshal(in, addAliases(out))
+}
+
+// UnmarshalStrict is a wrapper for yaml.UnmarshalStrict but adds
+// support for any yaml-aliases specified in out.
+func UnmarshalStrict(in []byte, out interface{}) error {
+	return yaml.UnmarshalStrict(in, addAliases(out))
+}
+
+// addAliases generates a new struct for unmarshalling that contains all exported
+// fields from the passed in struct, but also adds additional fields for any
+// aliases.
+//
+// Given a struct such as:
+//
+//   type Config struct {
+//     UserName string `yaml:"username" yaml-aliases:"userName,user-name"`
+//     TTL time.Duration `yaml:"ttl"`
+//   }
+//
+// It will create a new struct with a pointer field for each field in the
+// original struct and a new field for each alias.
+//
+//   struct {
+//     UserName *string `yaml:"username" yaml-aliases:"userName,user-name"`
+//     UserNameYamlAlias1 *string `yaml:"userName"`
+//     UserNameYamlAlias2 *string `yaml:"user-name"`
+//     TTL *time.Duration `yaml:"ttl"`
+//   }
+//
+// A value of that struct is returned with the pointer fields pointing to
+// fields of the original struct.
+//
+// out.UserName = &dest.UserName
+// out.UserNameYamlAlias1 = &dest.UserName
+// out.UserNameYamlAlias2 = &dest.UserName
+// out.TTL = &dest.TTL
+// return out
+//
+// The YAML library respects existing pointers when unmarshalling, and
+// does not replace them:
+// https://gist.github.com/prashantv/fa4f92b4b95f936d68495be250ed3506
+func addAliases(dest interface{}) interface{} {
+	rv := reflect.ValueOf(dest).Elem()
+	rt := rv.Type()
+
+	fields := make([]reflect.StructField, 0, rt.NumField())
+	dests := make([]reflect.Value, 0, rt.NumField())
+	for i := 0; i < rt.NumField(); i++ {
+		f := rt.Field(i)
+
+		// Ignore unexported fields.
+		if f.PkgPath != "" {
+			continue
+		}
+
+		// Add a pointer field that matches the original field in the struct.
+		fields = append(fields, reflect.StructField{
+			Name: f.Name,
+			Type: reflect.PtrTo(f.Type),
+			Tag:  f.Tag,
+		})
+		dests = append(dests, rv.Field(i).Addr())
+
+		allAliases, ok := f.Tag.Lookup("yaml-aliases")
+		if !ok {
+			// No aliases specified. No other fields to add.
+			continue
+		}
+
+		aliases := strings.Split(allAliases, ",")
+		for j, alias := range aliases {
+			// Add a new field that is a pointer to the original field.
+			// We generate a name that is unlikely to clash. Clashes will cause panics.
+			fields = append(fields, reflect.StructField{
+				Name: fmt.Sprintf("%vYamlAlias%v", f.Name, j),
+				Type: reflect.PtrTo(f.Type),
+				Tag:  reflect.StructTag(fmt.Sprintf(`yaml:%q`, alias)),
+			})
+			dests = append(dests, rv.Field(i).Addr())
+		}
+	}
+
+	// The struct we generated has pointers to the original fields.
+	// Set all the pointers to the fields in the original struct.
+	generated := reflect.StructOf(fields)
+	out := reflect.New(generated)
+	for i, dest := range dests {
+		out.Elem().Field(i).Set(dest)
+	}
+
+	return out.Interface()
+}
diff --git a/options.go b/options.go
index d594ec6..f11657c 100644
--- a/options.go
+++ b/options.go
@@ -174,10 +174,6 @@ func (s *stringAlias) UnmarshalFlag(value string) error {
 	return nil
 }
 
-func (s *stringAlias) UnmarshalYAML(unmarshal func(interface{}) error) error {
-	return unmarshal(s.dest)
-}
-
 func setEncodingOptions(opts *Options) {
 	if opts.ROpts.Aliases.JSON {
 		opts.ROpts.Encoding = encoding.JSON
diff --git a/template.go b/template.go
index 18a0f28..c2a3710 100644
--- a/template.go
+++ b/template.go
@@ -27,41 +27,25 @@ import (
 	"strings"
 	"time"
 
+	"github.com/yarpc/yab/internal/yamlalias"
 	"github.com/yarpc/yab/templateargs"
 
 	"gopkg.in/yaml.v2"
 )
 
 type template struct {
-	Peers                     []string    `yaml:"peers"`
-	Peer                      string      `yaml:"peer"`
-	PeerList                  string      `yaml:"peerList"`
-	Peerlist                  stringAlias `yaml:"peerlist"`
-	PeerDashList              stringAlias `yaml:"peer-list"`
-	Caller                    string      `yaml:"caller"`
-	Service                   string      `yaml:"service"`
-	Thrift                    string      `yaml:"thrift"`
-	Procedure                 string      `yaml:"procedure"`
-	Method                    stringAlias `yaml:"method"`
-	DisableThriftEnvelope     *bool       `yaml:"disableThriftEnvelope"`
-	DisableDashThriftEnvelope **bool      `yaml:"disable-thrift-envelope"`
-	Disablethriftenvelope     **bool      `yaml:"disablethriftenvelope"`
-
-	ShardKey        string `yaml:"shardKey"`
-	RoutingKey      string `yaml:"routingKey"`
-	RoutingDelegate string `yaml:"routingDelegate"`
-
-	Shardkey        stringAlias `yaml:"shardkey"`
-	Routingkey      stringAlias `yaml:"routingkey"`
-	Routingdelegate stringAlias `yaml:"routingdelegate"`
-
-	ShardDashKey        stringAlias `yaml:"shard-key"`
-	RoutingDashKey      stringAlias `yaml:"routing-key"`
-	RoutingDashDelegate stringAlias `yaml:"routing-delegate"`
-
-	SK stringAlias `yaml:"sk"`
-	RK stringAlias `yaml:"rk"`
-	RD stringAlias `yaml:"rd"`
+	Peers                 []string `yaml:"peers"`
+	Peer                  string   `yaml:"peer"`
+	PeerList              string   `yaml:"peerList" yaml-aliases:"peerlist,peer-list"`
+	Caller                string   `yaml:"caller"`
+	Service               string   `yaml:"service"`
+	Thrift                string   `yaml:"thrift"`
+	Procedure             string   `yaml:"procedure" yaml-aliases:"method"`
+	DisableThriftEnvelope *bool    `yaml:"disableThriftEnvelope" yaml-aliases:"disablethriftenvelope,disable-thrift-envelope"`
+
+	ShardKey        string `yaml:"shardKey" yaml-aliases:"shardkey,shard-key,sk"`
+	RoutingKey      string `yaml:"routingKey" yaml-aliases:"routingkey,routing-key,rk"`
+	RoutingDelegate string `yaml:"routingDelegate" yaml-aliases:"routingdelegate,routing-delegate,rd"`
 
 	Headers map[string]string           `yaml:"headers"`
 	Baggage map[string]string           `yaml:"baggage"`
@@ -96,11 +80,12 @@ func readYAMLFile(yamlTemplate string, templateArgs map[string]string, opts *Opt
 }
 
 func readYAMLRequest(base string, contents []byte, templateArgs map[string]string, opts *Options) error {
-	t, err := unmarshalTemplate(contents)
-	if err != nil {
+	var t template
+	if err := yamlalias.UnmarshalStrict(contents, &t); err != nil {
 		return err
 	}
 
+	var err error
 	t.Request, err = templateargs.ProcessMap(t.Request, templateArgs)
 	if err != nil {
 		return err
@@ -166,33 +151,6 @@ func overrideParam(s *string, newS string) {
 	}
 }
 
-func unmarshalTemplate(bytes []byte) (*template, error) {
-	t := &template{}
-
-	t.Method.dest = &t.Procedure
-
-	t.Peerlist.dest = &t.PeerList
-	t.PeerDashList.dest = &t.PeerList
-
-	t.Shardkey.dest = &t.ShardKey
-	t.Routingkey.dest = &t.RoutingKey
-	t.Routingdelegate.dest = &t.RoutingDelegate
-
-	t.ShardDashKey.dest = &t.ShardKey
-	t.RoutingDashKey.dest = &t.RoutingKey
-	t.RoutingDashDelegate.dest = &t.RoutingDelegate
-
-	t.SK.dest = &t.ShardKey
-	t.RK.dest = &t.RoutingKey
-	t.RD.dest = &t.RoutingDelegate
-
-	t.Disablethriftenvelope = &t.DisableThriftEnvelope
-	t.DisableDashThriftEnvelope = &t.DisableThriftEnvelope
-
-	err := yaml.Unmarshal(bytes, &t)
-	return t, err
-}
-
 type headers map[string]string
 
 // In these cases, the existing item (target, from flags) overrides the source


Test patch:
diff --git a/internal/yamlalias/wrap_test.go b/internal/yamlalias/wrap_test.go
new file mode 100644
index 0000000..d1b4ea4
--- /dev/null
+++ b/internal/yamlalias/wrap_test.go
@@ -0,0 +1,164 @@
+package yamlalias
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v2"
+)
+
+func stringPtr(s string) *string {
+	return &s
+}
+
+func TestWrap(t *testing.T) {
+	type Simple struct {
+		Foo string `yaml:"foo" yaml-aliases:"bar,baz"`
+	}
+
+	type ManyTypes struct {
+		Str   string            `yaml-aliases:"string"`
+		Int   int               `yaml-aliases:"Int"`
+		Slice []string          `yaml-aliases:"list"`
+		Map   map[string]string `yaml-aliases:"dict"`
+	}
+
+	type AliasToPtr struct {
+		Foo *string `yaml:"foo" yaml-aliases:"bar"`
+	}
+
+	tests := []struct {
+		msg  string
+		data string
+		want interface{}
+	}{
+		{
+			msg:  "No aliases",
+			data: `{"foo": "foo", "bar": "bar"}`,
+			want: &struct {
+				Foo string
+				Bar string
+			}{"foo", "bar"},
+		},
+		{
+			msg:  "set field by normal name",
+			data: `{"foo": "foo"}`,
+			want: &Simple{"foo"},
+		},
+		{
+			msg:  "set field by first alias",
+			data: `{"bar": "bar"}`,
+			want: &Simple{"bar"},
+		},
+		{
+			msg:  "set field by second alias",
+			data: `{"baz": "baz"}`,
+			want: &Simple{"baz"},
+		},
+		{
+			msg:  "set field with multiple aliases, last one wins",
+			data: `{"bar": "bar", "foo": "foo", "baz": "baz"}`,
+			want: &Simple{"baz"},
+		},
+		{
+			msg:  "pointer field that is not set",
+			data: `{}`,
+			want: &AliasToPtr{},
+		},
+		{
+			msg:  "pointer field set directly",
+			data: `{"foo": "foo"}`,
+			want: &AliasToPtr{stringPtr("foo")},
+		},
+		{
+			msg:  "pointer field that is set by alias",
+			data: `{"bar": "bar"}`,
+			want: &AliasToPtr{stringPtr("bar")},
+		},
+		{
+			msg: "Unexported fields are ignored",
+			want: &struct {
+				Foo string `yaml:"foo" yaml-aliases:"bar"`
+				bar string `yaml:"bar" yaml-aliases:"baz"`
+			}{
+				Foo: "bar",
+				bar: "",
+			},
+			data: `{"bar": "bar"}`,
+		},
+		{
+			msg: "Alias with different case",
+			want: &struct {
+				Foo string `yaml:"foo" yaml-aliases:"Foo"`
+			}{
+				Foo: "Foo",
+			},
+			data: `{"Foo": "Foo"}`,
+		},
+		{
+			msg: "Alias with dashes",
+			want: &struct {
+				Foo string `yaml:"foo" yaml-aliases:"foo-bar"`
+			}{
+				Foo: "foo-bar",
+			},
+			data: `{"foo-bar": "foo-bar"}`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.msg, func(t *testing.T) {
+			// To allow use of anonymous structs, we use reflection to create
+			// a new empty struct.
+			v := reflect.New(reflect.TypeOf(tt.want).Elem()).Interface()
+			require.NoError(t, Unmarshal([]byte(tt.data), v))
+			assert.Equal(t, tt.want, v)
+		})
+	}
+}
+
+func TestWrapPointerField(t *testing.T) {
+	var dest string
+	type StructWithPointer struct {
+		Foo *string `yaml:"foo" yaml-aliases:"bar"`
+	}
+
+	s := StructWithPointer{Foo: &dest}
+	require.NoError(t, yaml.Unmarshal([]byte(`{"foo": "initial"}`), &s), "failed to unmarshal")
+	assert.Equal(t, "initial", dest, "dest expected to be updated")
+
+	require.NoError(t, Unmarshal([]byte(`{"bar": "updated"}`), &s), "failed to unmarshal with alias")
+	assert.Equal(t, "updated", dest, "expected dest to be updated via alias")
+}
+
+func TestWrapInvalid(t *testing.T) {
+	tests := []struct {
+		msg string
+		t   interface{}
+	}{
+		{
+			msg: "Alias clashes with other field",
+			t: &struct {
+				Foo string `yaml:"foo" yaml-aliases:"bar,baz"`
+				Bar string `yaml:"bar"`
+			}{},
+		},
+		{
+			msg: "Alias clashes with another alias",
+			t: &struct {
+				Foo string `yaml:"foo" yaml-aliases:"baz"`
+				Bar string `yaml:"bar" yaml-aliases:"baz"`
+			}{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.msg, func(t *testing.T) {
+			assert.Panics(t, func() {
+				Unmarshal([]byte(`{}`), &tt.t)
+			})
+		})
+	}
+}
diff --git a/template_test.go b/template_test.go
index cffe7c3..5a3f773 100644
--- a/template_test.go
+++ b/template_test.go
@@ -29,6 +29,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"github.com/yarpc/yab/internal/yamlalias"
 )
 
 func mustReadYAMLFile(t *testing.T, yamlTemplate string, opts *Options) {
@@ -292,9 +293,10 @@ func TestTemplateAlias(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		for _, template := range tt.templates {
-			t.Run(template, func(t *testing.T) {
-				templ, err := unmarshalTemplate([]byte(template))
+		for _, in := range tt.templates {
+			t.Run(in, func(t *testing.T) {
+				var templ template
+				err := yamlalias.Unmarshal([]byte(in), &templ)
 				require.NoError(t, err)
 				assert.Equal(t, tt.wantShardKey, templ.ShardKey, "shard key aliases expanded")
 				assert.Equal(t, tt.wantRoutingKey, templ.RoutingKey, "routing key aliases expanded")
@@ -312,23 +314,23 @@ func TestDisableThriftEnvelopeOverride(t *testing.T) {
 		wantDisableThriftEnvelope bool
 	}{
 		{
-			initial: false,
-			yaml:    "method: foo",
+			initial:                   false,
+			yaml:                      "method: foo",
 			wantDisableThriftEnvelope: false,
 		},
 		{
-			initial: true,
-			yaml:    "method: foo",
+			initial:                   true,
+			yaml:                      "method: foo",
 			wantDisableThriftEnvelope: true,
 		},
 		{
-			initial: false,
-			yaml:    "disable-thrift-envelope: true",
+			initial:                   false,
+			yaml:                      "disable-thrift-envelope: true",
 			wantDisableThriftEnvelope: true,
 		},
 		{
-			initial: true,
-			yaml:    "disable-thrift-envelope: true",
+			initial:                   true,
+			yaml:                      "disable-thrift-envelope: true",
 			wantDisableThriftEnvelope: true,
 		},
 	}