Repository: ariga/atlas
Pull request #315 (2021-12-18 16:54:48)

sql/internal/specutil: sanity tests for all drivers
Adding a sanity test for all driver type registries and fixing some bugs along the way. 

postgres enum needs some more love, but ill do that separately. 


Patch:
diff --git a/schema/schemaspec/schemahcl/opts.go b/schema/schemaspec/schemahcl/opts.go
index cef28505..6fb53816 100644
--- a/schema/schemaspec/schemahcl/opts.go
+++ b/schema/schemaspec/schemahcl/opts.go
@@ -77,7 +77,7 @@ func typeFuncSpec(typeSpec *schemaspec.TypeSpec) function.Function {
 		case reflect.String:
 			p.Type = cty.String
 			spec.Params = append(spec.Params, p)
-		case reflect.Int, reflect.Float32:
+		case reflect.Int, reflect.Float32, reflect.Int64:
 			p.Type = cty.Number
 			spec.Params = append(spec.Params, p)
 		case reflect.Bool:
diff --git a/schema/schemaspec/spec.go b/schema/schemaspec/spec.go
index 6653611d..58accd2e 100644
--- a/schema/schemaspec/spec.go
+++ b/schema/schemaspec/spec.go
@@ -72,6 +72,10 @@ type (
 		// T is the database identifier for the type.
 		T          string
 		Attributes []*TypeAttr
+		// RType is the reflect.Type of the schema.Type used to describe the TypeSpec.
+		// This field is optional and used to determine the TypeSpec in cases where the
+		// schema.Type does not have a `T` field.
+		RType reflect.Type
 	}
 
 	// TypeAttr describes an attribute of a TypeSpec, for example `varchar` fields
diff --git a/sql/internal/specutil/types.go b/sql/internal/specutil/types.go
index 5f29ecfa..f6de78c3 100644
--- a/sql/internal/specutil/types.go
+++ b/sql/internal/specutil/types.go
@@ -16,7 +16,7 @@ import (
 // PrintType returns the string representation of a column type which can be parsed
 // by the driver into a schema.Type.
 func (r *TypeRegistry) PrintType(typ *schemaspec.Type) (string, error) {
-	spec, ok := r.Find(typ.T)
+	spec, ok := r.findT(typ.T)
 	if !ok {
 		return "", fmt.Errorf("specutil: type %q not found in registry", typ.T)
 	}
@@ -72,10 +72,10 @@ type TypeRegistry struct {
 // Register adds one or more TypeSpec to the registry.
 func (r *TypeRegistry) Register(specs ...*schemaspec.TypeSpec) error {
 	for _, s := range specs {
-		if _, exists := r.Find(s.T); exists {
+		if _, exists := r.findT(s.T); exists {
 			return fmt.Errorf("specutil: type with T of %q already registered", s.T)
 		}
-		if _, exists := r.FindByName(s.Name); exists {
+		if _, exists := r.findName(s.Name); exists {
 			return fmt.Errorf("specutil: type with name of %q already registered", s.T)
 		}
 		r.r = append(r.r, s)
@@ -93,8 +93,8 @@ func NewRegistry(specs ...*schemaspec.TypeSpec) *TypeRegistry {
 	return r
 }
 
-// FindByName searches the registry for types that have the provided name.
-func (r *TypeRegistry) FindByName(name string) (*schemaspec.TypeSpec, bool) {
+// findName searches the registry for types that have the provided name.
+func (r *TypeRegistry) findName(name string) (*schemaspec.TypeSpec, bool) {
 	for _, current := range r.r {
 		if current.Name == name {
 			return current, true
@@ -103,8 +103,8 @@ func (r *TypeRegistry) FindByName(name string) (*schemaspec.TypeSpec, bool) {
 	return nil, false
 }
 
-// Find searches the registry for types that have the provided T.
-func (r *TypeRegistry) Find(t string) (*schemaspec.TypeSpec, bool) {
+// findT searches the registry for types that have the provided T.
+func (r *TypeRegistry) findT(t string) (*schemaspec.TypeSpec, bool) {
 	for _, current := range r.r {
 		if current.T == t {
 			return current, true
@@ -115,7 +115,6 @@ func (r *TypeRegistry) Find(t string) (*schemaspec.TypeSpec, bool) {
 
 // Convert converts the schema.Type to a *schemaspec.Type.
 func (r *TypeRegistry) Convert(typ schema.Type) (*schemaspec.Type, error) {
-	s := &schemaspec.Type{}
 	rv := reflect.ValueOf(typ)
 	if rv.Kind() == reflect.Ptr {
 		rv = rv.Elem()
@@ -123,31 +122,25 @@ func (r *TypeRegistry) Convert(typ schema.Type) (*schemaspec.Type, error) {
 	if !rv.IsValid() {
 		return nil, errors.New("specutil: invalid schema.Type on Convert")
 	}
-	tf := rv.FieldByName("T")
-	if !tf.IsValid() {
-		return nil, fmt.Errorf("specutil: cannot convert schema.Type without T field for %T", typ)
-	}
-	if tf.Kind() != reflect.String {
-		return nil, fmt.Errorf("specutil: cannot convert non-string T field for %T", typ)
-	}
-	s.T = tf.String()
-	typeSpec, ok := r.Find(s.T)
+	typeSpec, ok := r.findType(rv)
 	if !ok {
-		return nil, fmt.Errorf("specutil: type %q not found in registry", s.T)
+		return nil, fmt.Errorf("specutil: cannot find type spec for %s", rv.Type())
 	}
+	s := &schemaspec.Type{T: typeSpec.T}
 	// Iterate the attributes in reverse order, so we can skip zero value and optional attrs.
 	for i := len(typeSpec.Attributes) - 1; i >= 0; i-- {
 		attr := typeSpec.Attributes[i]
 		n := inflect.Camelize(attr.Name)
 		field := rv.FieldByName(n)
+		// If TypeSpec has an attribute that isn't mapped to a field on the schema.Type skip it.
 		if !field.IsValid() {
-			return nil, fmt.Errorf("invalid field name %q for attr %q on type spec %q", n, attr.Name, typeSpec.T)
+			continue
 		}
 		if field.Kind() != attr.Kind {
 			return nil, errors.New("incompatible kinds on typespec attr and typefield")
 		}
 		switch attr.Kind {
-		case reflect.Int:
+		case reflect.Int, reflect.Int64:
 			v := int(field.Int())
 			if v == 0 && len(s.Attrs) == 0 {
 				break
@@ -178,6 +171,29 @@ func (r *TypeRegistry) Convert(typ schema.Type) (*schemaspec.Type, error) {
 	return s, nil
 }
 
+func (r *TypeRegistry) findType(rv reflect.Value) (*schemaspec.TypeSpec, bool) {
+	tf := rv.FieldByName("T")
+	if tf.IsValid() && tf.Kind() == reflect.String {
+		name := tf.String()
+		if typeSpec, ok := r.findT(name); ok {
+			return typeSpec, true
+		}
+	}
+	if typeSpec, ok := r.findRType(rv.Type()); ok {
+		return typeSpec, true
+	}
+	return nil, false
+}
+
+func (r *TypeRegistry) findRType(rt reflect.Type) (*schemaspec.TypeSpec, bool) {
+	for _, ts := range r.Specs() {
+		if ts.RType != nil && ts.RType == rt {
+			return ts, true
+		}
+	}
+	return nil, false
+}
+
 // Specs returns the TypeSpecs in the registry.
 func (r *TypeRegistry) Specs() []*schemaspec.TypeSpec {
 	return r.r
@@ -185,7 +201,7 @@ func (r *TypeRegistry) Specs() []*schemaspec.TypeSpec {
 
 // Type converts a *schemaspec.Type into a schema.Type.
 func (r *TypeRegistry) Type(typ *schemaspec.Type, extra []*schemaspec.Attr, parser func(string) (schema.Type, error)) (schema.Type, error) {
-	typeSpec, ok := r.Find(typ.T)
+	typeSpec, ok := r.findT(typ.T)
 	if !ok {
 		return nil, fmt.Errorf("specutil: typespec not found for %s", typ.T)
 	}
diff --git a/sql/mysql/sqlspec.go b/sql/mysql/sqlspec.go
index 9c7ddf80..b563a433 100644
--- a/sql/mysql/sqlspec.go
+++ b/sql/mysql/sqlspec.go
@@ -440,8 +440,22 @@ func hasCollate(attr []schema.Attr, parent []schema.Attr) (string, bool) {
 
 // TypeRegistry contains the supported TypeSpecs for the mysql driver.
 var TypeRegistry = specutil.NewRegistry(
-	specutil.TypeSpec(tEnum, &schemaspec.TypeAttr{Name: "values", Kind: reflect.Slice, Required: true}),
-	specutil.TypeSpec(tSet, &schemaspec.TypeAttr{Name: "values", Kind: reflect.Slice, Required: true}),
+	&schemaspec.TypeSpec{
+		Name: tEnum,
+		T:    tEnum,
+		Attributes: []*schemaspec.TypeAttr{
+			{Name: "values", Kind: reflect.Slice, Required: true},
+		},
+		RType: reflect.TypeOf(schema.EnumType{}),
+	},
+	&schemaspec.TypeSpec{
+		Name: tSet,
+		T:    tSet,
+		Attributes: []*schemaspec.TypeAttr{
+			{Name: "values", Kind: reflect.Slice, Required: true},
+		},
+		RType: reflect.TypeOf(SetType{}),
+	},
 	specutil.TypeSpec(tBit, specutil.SizeTypeAttr(false)),
 	specutil.TypeSpec(tInt, unsignedTypeAttr(), specutil.SizeTypeAttr(false)),
 	specutil.TypeSpec(tTinyInt, unsignedTypeAttr(), specutil.SizeTypeAttr(false)),
@@ -458,7 +472,7 @@ var TypeRegistry = specutil.NewRegistry(
 	specutil.TypeSpec(tTime),
 	specutil.TypeSpec(tDateTime),
 	specutil.TypeSpec(tYear),
-	specutil.TypeSpec(tVarchar, specutil.SizeTypeAttr(false)),
+	specutil.TypeSpec(tVarchar, specutil.SizeTypeAttr(true)),
 	specutil.TypeSpec(tChar, specutil.SizeTypeAttr(false)),
 	specutil.TypeSpec(tVarBinary, specutil.SizeTypeAttr(false)),
 	specutil.TypeSpec(tBinary, specutil.SizeTypeAttr(false)),
diff --git a/sql/postgres/sqlspec.go b/sql/postgres/sqlspec.go
index 91776111..40f97376 100644
--- a/sql/postgres/sqlspec.go
+++ b/sql/postgres/sqlspec.go
@@ -369,8 +369,8 @@ func arrayType(t string) (string, bool) {
 
 // TypeRegistry contains the supported TypeSpecs for the postgres driver.
 var TypeRegistry = specutil.NewRegistry(
-	specutil.TypeSpec(tBit, specutil.SizeTypeAttr(true)),
-	specutil.AliasTypeSpec("bit_varying", tBitVar, specutil.SizeTypeAttr(true)),
+	specutil.TypeSpec(tBit, &schemaspec.TypeAttr{Name: "len", Kind: reflect.Int64}),
+	specutil.AliasTypeSpec("bit_varying", tBitVar, &schemaspec.TypeAttr{Name: "len", Kind: reflect.Int64}),
 	specutil.TypeSpec(tVarChar, specutil.SizeTypeAttr(true)),
 	specutil.TypeSpec(tChar, specutil.SizeTypeAttr(true)),
 	specutil.TypeSpec(tCharacter, specutil.SizeTypeAttr(true)),


Test patch:
diff --git a/sql/internal/spectest/spectest.go b/sql/internal/spectest/spectest.go
new file mode 100644
index 00000000..3045fcac
--- /dev/null
+++ b/sql/internal/spectest/spectest.go
@@ -0,0 +1,66 @@
+// Copyright 2021-present The Atlas Authors. All rights reserved.
+// This source code is licensed under the Apache 2.0 license found
+// in the LICENSE file in the root directory of this source tree.
+
+package spectest
+
+import (
+	"reflect"
+	"testing"
+
+	"ariga.io/atlas/schema/schemaspec"
+	"ariga.io/atlas/sql/internal/specutil"
+	"ariga.io/atlas/sql/schema"
+	"github.com/stretchr/testify/require"
+)
+
+// RegistrySanityTest runs a sanity for a TypeRegistry, generated a dummy *schemaspec.Type
+// then converting it to a schema.Type and back to a *schemaspec.Type.
+func RegistrySanityTest(t *testing.T, registry *specutil.TypeRegistry, parser func(string) (schema.Type, error), skip []string) {
+	for _, ts := range registry.Specs() {
+		if contains(ts.Name, skip) {
+			continue
+		}
+		t.Run(ts.Name, func(t *testing.T) {
+			spec := dummyType(t, ts)
+			styp, err := registry.Type(spec, nil, parser)
+			require.NoError(t, err)
+			require.NoErrorf(t, err, "failed formatting: %styp", err)
+			convert, err := registry.Convert(styp)
+			require.NoError(t, err)
+			after, err := registry.Type(convert, nil, parser)
+			require.NoError(t, err)
+			require.EqualValues(t, styp, after)
+		})
+	}
+}
+
+func contains(s string, l []string) bool {
+	for i := range l {
+		if s == l[i] {
+			return true
+		}
+	}
+	return false
+}
+
+func dummyType(t *testing.T, ts *schemaspec.TypeSpec) *schemaspec.Type {
+	spec := &schemaspec.Type{T: ts.T}
+	for _, attr := range ts.Attributes {
+		var a *schemaspec.Attr
+		switch attr.Kind {
+		case reflect.Int, reflect.Int64:
+			a = specutil.LitAttr(attr.Name, "2")
+		case reflect.String:
+			a = specutil.LitAttr(attr.Name, `"a"`)
+		case reflect.Slice:
+			a = specutil.ListAttr(attr.Name, `"a"`, `"b"`)
+		case reflect.Bool:
+			a = specutil.LitAttr(attr.Name, "false")
+		default:
+			t.Fatalf("unsupported kind: %s", attr.Kind)
+		}
+		spec.Attrs = append(spec.Attrs, a)
+	}
+	return spec
+}
diff --git a/sql/internal/specutil/types_test.go b/sql/internal/specutil/types_test.go
index dc51d555..f4f44673 100644
--- a/sql/internal/specutil/types_test.go
+++ b/sql/internal/specutil/types_test.go
@@ -62,7 +62,7 @@ func TestRegistry(t *testing.T) {
 	require.NoError(t, err)
 	err = r.Register(text)
 	require.EqualError(t, err, `specutil: type with T of "text" already registered`)
-	spec, ok := r.FindByName("text")
+	spec, ok := r.findName("text")
 	require.True(t, ok)
 	require.EqualValues(t, spec, text)
 }
diff --git a/sql/mysql/sqlspec_test.go b/sql/mysql/sqlspec_test.go
index a0643d46..f4392dd7 100644
--- a/sql/mysql/sqlspec_test.go
+++ b/sql/mysql/sqlspec_test.go
@@ -7,6 +7,7 @@ import (
 
 	"ariga.io/atlas/schema/schemaspec"
 	"ariga.io/atlas/schema/schemaspec/schemahcl"
+	"ariga.io/atlas/sql/internal/spectest"
 	"ariga.io/atlas/sql/internal/specutil"
 	"ariga.io/atlas/sql/schema"
 	"ariga.io/atlas/sql/sqlspec"
@@ -900,6 +901,10 @@ func TestTypes(t *testing.T) {
 	}
 }
 
+func TestRegistrySanity(t *testing.T) {
+	spectest.RegistrySanityTest(t, TypeRegistry, parseRawType, nil)
+}
+
 func hclEqual(t *testing.T, expected, actual []byte) {
 	require.EqualValues(t, string(hclwrite.Format(expected)), string(hclwrite.Format(actual)))
 }
diff --git a/sql/postgres/sqlspec_test.go b/sql/postgres/sqlspec_test.go
index 76105766..51c8523c 100644
--- a/sql/postgres/sqlspec_test.go
+++ b/sql/postgres/sqlspec_test.go
@@ -7,6 +7,7 @@ import (
 
 	"ariga.io/atlas/schema/schemaspec"
 	"ariga.io/atlas/schema/schemaspec/schemahcl"
+	"ariga.io/atlas/sql/internal/spectest"
 	"ariga.io/atlas/sql/internal/specutil"
 	"ariga.io/atlas/sql/schema"
 	"ariga.io/atlas/sql/sqlspec"
@@ -747,3 +748,7 @@ func TestTypes(t *testing.T) {
 		})
 	}
 }
+
+func TestRegistrySanity(t *testing.T) {
+	spectest.RegistrySanityTest(t, TypeRegistry, parseRawType, []string{"enum"})
+}