Repository: ariga/atlas
Pull request #1478 (2023-02-24 21:05:03)

schemahcl: limit type variables scope to specific location
Limiting the variable's scope to specific attributes provides type-safety to the schema. i.e., Eval fails on "parsing" and not on the decoding stage, which improves the error reporting. 

From:
```
Error: schemahcl: failed reading spec as *postgres.doc: set field "type": converting cty.Value to *schemahcl.Type: incorrect type type
```

To:
```
Error: schema1.hcl:6,12-18: Unknown column.type; There is no type named "string".
```


Patch:
diff --git a/schemahcl/hcl.go b/schemahcl/hcl.go
index 40d395da..66595428 100644
--- a/schemahcl/hcl.go
+++ b/schemahcl/hcl.go
@@ -17,6 +17,7 @@ import (
 	"github.com/hashicorp/hcl/v2/hclsyntax"
 	"github.com/hashicorp/hcl/v2/hclwrite"
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/function"
 )
 
 // Marshal returns the Atlas HCL encoding of v.
@@ -237,27 +238,47 @@ func (s *State) resource(ctx *hcl.EvalContext, file *hcl.File) (*Resource, error
 	return res, nil
 }
 
-// mayExtendVars gets the current scope context, and extend it with additional
-// variables if it was configured this way using WithScopedEnums.
-func (s *State) mayExtendVars(ctx *hcl.EvalContext, scope []string) *hcl.EvalContext {
-	vars, ok := s.config.pathVars[strings.Join(scope, ".")]
-	if !ok {
+// mayScopeContext returns a new limited context for the given scope with access only
+// to variables defined by WithScopedEnums and WithTypes and references in the document.
+func (s *State) mayScopeContext(ctx *hcl.EvalContext, scope []string) *hcl.EvalContext {
+	path := strings.Join(scope, ".")
+	vars, ok1 := s.config.pathVars[path]
+	funcs, ok2 := s.config.pathFuncs[path]
+	if !ok1 && !ok2 {
 		return ctx
 	}
-	ctx = ctx.NewChild()
-	ctx.Variables = vars
-	return ctx
+	nctx := &hcl.EvalContext{
+		Variables: make(map[string]cty.Value),
+		Functions: make(map[string]function.Function),
+	}
+	for n, v := range vars {
+		nctx.Variables[n] = v
+	}
+	for n, f := range funcs {
+		nctx.Functions[n] = f
+	}
+	// A patch from the past. Should be moved
+	// to specific scopes in the future.
+	nctx.Functions["sql"] = rawExprImpl()
+	for p := ctx; p != nil; p = p.Parent() {
+		for k, v := range p.Variables {
+			if isRef(v) {
+				nctx.Variables[k] = v
+			}
+		}
+	}
+	return nctx
 }
 
 func (s *State) toAttrs(ctx *hcl.EvalContext, hclAttrs hclsyntax.Attributes, scope []string) ([]*Attr, error) {
 	var attrs []*Attr
 	for _, hclAttr := range hclAttrs {
-		ctx := s.mayExtendVars(ctx, append(scope, hclAttr.Name))
-		at := &Attr{K: hclAttr.Name}
-		value, diag := hclAttr.Expr.Value(ctx)
+		scope := append(scope, hclAttr.Name)
+		value, diag := hclAttr.Expr.Value(s.mayScopeContext(ctx, scope))
 		if diag.HasErrors() {
-			return nil, s.typeError(diag)
+			return nil, s.typeError(diag, scope)
 		}
+		at := &Attr{K: hclAttr.Name}
 		switch t := value.Type(); {
 		case isRef(value):
 			at.V = cty.CapsuleVal(ctyRefType, &Ref{V: value.GetAttr("__ref").AsString()})
@@ -284,7 +305,8 @@ func (s *State) toAttrs(ctx *hcl.EvalContext, hclAttrs hclsyntax.Attributes, sco
 }
 
 // typeError improves diagnostic reporting in case of parse error.
-func (s *State) typeError(diag hcl.Diagnostics) error {
+func (s *State) typeError(diag hcl.Diagnostics, scope []string) error {
+	path := strings.Join(scope, ".")
 	for _, d := range diag {
 		switch e := d.Expression.(type) {
 		case *hclsyntax.FunctionCallExpr:
@@ -300,6 +322,9 @@ func (s *State) typeError(diag hcl.Diagnostics) error {
 			}
 			if t, ok := s.findTypeSpec(e.Traversal.RootName()); ok && len(t.Attributes) > 0 {
 				d.Detail = fmt.Sprintf("Type %q requires at least 1 argument", t.Name)
+			} else if n := len(scope); n > 1 && (s.config.pathVars[path] != nil || s.config.pathFuncs[path] != nil) {
+				d.Summary = strings.Replace(d.Summary, "variable", fmt.Sprintf("%s.%s", scope[n-2], scope[n-1]), 1)
+				d.Detail = strings.Replace(d.Detail, "variable", scope[n-1], 1)
 			}
 		}
 	}
@@ -307,7 +332,20 @@ func (s *State) typeError(diag hcl.Diagnostics) error {
 }
 
 func isRef(v cty.Value) bool {
-	return v.Type().IsObjectType() && v.Type().HasAttribute("__ref")
+	t := v.Type()
+	if !t.IsObjectType() {
+		return false
+	}
+	if t.HasAttribute("__ref") {
+		return true
+	}
+	it := v.ElementIterator()
+	for it.Next() {
+		if _, v := it.Element(); isRef(v) {
+			return true
+		}
+	}
+	return false
 }
 
 func (s *State) toResource(ctx *hcl.EvalContext, block *hclsyntax.Block, scope []string) (*Resource, error) {
@@ -324,7 +362,7 @@ func (s *State) toResource(ctx *hcl.EvalContext, block *hclsyntax.Block, scope [
 	default:
 		return nil, fmt.Errorf("too many labels for block: %s", block.Labels)
 	}
-	ctx = s.mayExtendVars(ctx, scope)
+	ctx = s.mayScopeContext(ctx, scope)
 	attrs, err := s.toAttrs(ctx, block.Body.Attributes, scope)
 	if err != nil {
 		return nil, err
diff --git a/schemahcl/opts.go b/schemahcl/opts.go
index 63fae765..762097b5 100644
--- a/schemahcl/opts.go
+++ b/schemahcl/opts.go
@@ -18,12 +18,12 @@ import (
 type (
 	// Config configures an unmarshaling.
 	Config struct {
-		types    []*TypeSpec
-		newCtx   func() *hcl.EvalContext
-		pathVars map[string]map[string]cty.Value
-		datasrc  map[string]func(*hcl.EvalContext, *hclsyntax.Block) (cty.Value, error)
+		types     []*TypeSpec
+		newCtx    func() *hcl.EvalContext
+		pathVars  map[string]map[string]cty.Value
+		pathFuncs map[string]map[string]function.Function
+		datasrc   map[string]func(*hcl.EvalContext, *hclsyntax.Block) (cty.Value, error)
 	}
-
 	// Option configures a Config.
 	Option func(*Config)
 )
@@ -31,7 +31,8 @@ type (
 // New returns a State configured with options.
 func New(opts ...Option) *State {
 	cfg := &Config{
-		pathVars: make(map[string]map[string]cty.Value),
+		pathVars:  make(map[string]map[string]cty.Value),
+		pathFuncs: make(map[string]map[string]function.Function),
 		newCtx: func() *hcl.EvalContext {
 			return stdTypes(&hcl.EvalContext{
 				Functions: stdFuncs(),
@@ -97,31 +98,27 @@ func WithDataSource(name string, h func(*hcl.EvalContext, *hclsyntax.Block) (cty
 	}
 }
 
-// WithTypes configures the list of given types as identifiers in the unmarshaling context.
-func WithTypes(typeSpecs []*TypeSpec) Option {
-	newCtx := func() *hcl.EvalContext {
-		ctx := stdTypes(&hcl.EvalContext{
-			Functions: stdFuncs(),
-			Variables: make(map[string]cty.Value),
-		})
-		for _, ts := range typeSpecs {
-			typeSpec := ts
-			// If no required args exist, register the type as a variable in the HCL context.
-			if len(typeFuncReqArgs(typeSpec)) == 0 {
-				typ := &Type{T: typeSpec.T}
-				ctx.Variables[typeSpec.Name] = cty.CapsuleVal(ctyTypeSpec, typ)
-			}
-			// If func args exist, register the type as a function in HCL.
-			if len(typeFuncArgs(typeSpec)) > 0 {
-				ctx.Functions[typeSpec.Name] = typeFuncSpec(typeSpec)
-			}
+// WithTypes configures the given types as identifiers in the unmarshal
+// context. The path controls where the usage of this type is allowed.
+func WithTypes(path string, typeSpecs []*TypeSpec) Option {
+	vars := make(map[string]cty.Value)
+	funcs := make(map[string]function.Function)
+	for _, ts := range typeSpecs {
+		typeSpec := ts
+		// If no required args exist, register the type as a variable in the HCL context.
+		if len(typeFuncReqArgs(typeSpec)) == 0 {
+			typ := &Type{T: typeSpec.T}
+			vars[typeSpec.Name] = cty.CapsuleVal(ctyTypeSpec, typ)
+		}
+		// If func args exist, register the type as a function in HCL.
+		if len(typeFuncArgs(typeSpec)) > 0 {
+			funcs[typeSpec.Name] = typeFuncSpec(typeSpec)
 		}
-		ctx.Functions["sql"] = rawExprImpl()
-		return ctx
 	}
 	return func(c *Config) {
-		c.newCtx = newCtx
 		c.types = append(c.types, typeSpecs...)
+		c.pathVars[path] = vars
+		c.pathFuncs[path] = funcs
 	}
 }
 
diff --git a/schemahcl/stdlib.go b/schemahcl/stdlib.go
index fdf990b5..a3234c3f 100644
--- a/schemahcl/stdlib.go
+++ b/schemahcl/stdlib.go
@@ -161,6 +161,9 @@ func stdFuncs() map[string]function.Function {
 		"urlsetpath":      urlSetPathFunc,
 		"values":          stdlib.ValuesFunc,
 		"zipmap":          stdlib.ZipmapFunc,
+		// A patch from the past. Should be moved
+		// to specific scopes in the future.
+		"sql": rawExprImpl(),
 	}
 }
 
diff --git a/sql/mysql/sqlspec.go b/sql/mysql/sqlspec.go
index 37cb6365..e36cf6ac 100644
--- a/sql/mysql/sqlspec.go
+++ b/sql/mysql/sqlspec.go
@@ -78,7 +78,7 @@ func MarshalSpec(v any, marshaler schemahcl.Marshaler) ([]byte, error) {
 
 var (
 	hclState = schemahcl.New(
-		schemahcl.WithTypes(TypeRegistry.Specs()),
+		schemahcl.WithTypes("table.column.type", TypeRegistry.Specs()),
 		schemahcl.WithScopedEnums("table.index.type", IndexTypeBTree, IndexTypeHash, IndexTypeFullText, IndexTypeSpatial),
 		schemahcl.WithScopedEnums("table.primary_key.type", IndexTypeBTree, IndexTypeHash, IndexTypeFullText, IndexTypeSpatial),
 		schemahcl.WithScopedEnums("table.column.as.type", stored, persistent, virtual),
diff --git a/sql/postgres/sqlspec.go b/sql/postgres/sqlspec.go
index a7d3fb31..533d7063 100644
--- a/sql/postgres/sqlspec.go
+++ b/sql/postgres/sqlspec.go
@@ -117,7 +117,7 @@ func MarshalSpec(v any, marshaler schemahcl.Marshaler) ([]byte, error) {
 
 var (
 	hclState = schemahcl.New(
-		schemahcl.WithTypes(TypeRegistry.Specs()),
+		schemahcl.WithTypes("table.column.type", TypeRegistry.Specs()),
 		schemahcl.WithScopedEnums("table.index.type", IndexTypeBTree, IndexTypeBRIN, IndexTypeHash, IndexTypeGIN, IndexTypeGiST, "GiST", IndexTypeSPGiST, "SPGiST"),
 		schemahcl.WithScopedEnums("table.partition.type", PartitionTypeRange, PartitionTypeList, PartitionTypeHash),
 		schemahcl.WithScopedEnums("table.column.identity.generated", GeneratedTypeAlways, GeneratedTypeByDefault),
diff --git a/sql/sqlite/sqlspec.go b/sql/sqlite/sqlspec.go
index 3968f5c7..4658168a 100644
--- a/sql/sqlite/sqlspec.go
+++ b/sql/sqlite/sqlspec.go
@@ -201,7 +201,7 @@ var TypeRegistry = schemahcl.NewRegistry(
 
 var (
 	hclState = schemahcl.New(
-		schemahcl.WithTypes(TypeRegistry.Specs()),
+		schemahcl.WithTypes("table.column.type", TypeRegistry.Specs()),
 		schemahcl.WithScopedEnums("table.column.as.type", stored, virtual),
 		schemahcl.WithScopedEnums("table.foreign_key.on_update", specutil.ReferenceVars...),
 		schemahcl.WithScopedEnums("table.foreign_key.on_delete", specutil.ReferenceVars...),


Test patch:
diff --git a/internal/integration/hclsqlspec/hclsqlspec_test.go b/internal/integration/hclsqlspec/hclsqlspec_test.go
index 8a76605c..81a88570 100644
--- a/internal/integration/hclsqlspec/hclsqlspec_test.go
+++ b/internal/integration/hclsqlspec/hclsqlspec_test.go
@@ -347,7 +347,7 @@ table "accounts" {
 	require.NoError(t, err)
 }
 
-var hcl = schemahcl.New(schemahcl.WithTypes(postgres.TypeRegistry.Specs()))
+var hcl = schemahcl.New(schemahcl.WithTypes("table.column.type", postgres.TypeRegistry.Specs()))
 
 func TestMarshalTopLevel(t *testing.T) {
 	c := &sqlspec.Schema{
diff --git a/schemahcl/context_test.go b/schemahcl/context_test.go
index 1123dd97..6cf0df36 100644
--- a/schemahcl/context_test.go
+++ b/schemahcl/context_test.go
@@ -335,14 +335,19 @@ point {
 }
 
 func TestWithTypes(t *testing.T) {
-	f := `first    = int
-second   = bool
-third    = int(10)
-sized    = varchar(255)
-variadic = enum("a","b","c")
+	f := `parent "name" {
+  child "name" {
+    first    = int
+    second   = bool
+    third    = int(10)
+    sized    = varchar(255)
+    variadic = enum("a","b","c")
+  }
+}
 `
 	s := New(
 		WithTypes(
+			"parent.child",
 			[]*TypeSpec{
 				{Name: "bool", T: "bool"},
 				{
@@ -371,32 +376,38 @@ variadic = enum("a","b","c")
 		),
 	)
 	var test struct {
-		First    *Type `spec:"first"`
-		Second   *Type `spec:"second"`
-		Third    *Type `spec:"third"`
-		Varchar  *Type `spec:"sized"`
-		Variadic *Type `spec:"variadic"`
+		Parent struct {
+			Name  string `spec:",name"`
+			Child struct {
+				Name     string `spec:",name"`
+				First    *Type  `spec:"first"`
+				Second   *Type  `spec:"second"`
+				Third    *Type  `spec:"third"`
+				Varchar  *Type  `spec:"sized"`
+				Variadic *Type  `spec:"variadic"`
+			} `spec:"child"`
+		} `spec:"parent"`
 	}
 	err := s.EvalBytes([]byte(f), &test, nil)
 	require.NoError(t, err)
-	require.EqualValues(t, "int", test.First.T)
-	require.EqualValues(t, "bool", test.Second.T)
+	require.EqualValues(t, "int", test.Parent.Child.First.T)
+	require.EqualValues(t, "bool", test.Parent.Child.Second.T)
 
-	require.EqualValues(t, "varchar", test.Varchar.T)
-	require.Len(t, test.Varchar.Attrs, 1)
-	i, err := test.Varchar.Attrs[0].Int()
+	require.EqualValues(t, "varchar", test.Parent.Child.Varchar.T)
+	require.Len(t, test.Parent.Child.Varchar.Attrs, 1)
+	i, err := test.Parent.Child.Varchar.Attrs[0].Int()
 	require.NoError(t, err)
 	require.EqualValues(t, 255, i)
 
-	require.EqualValues(t, "enum", test.Variadic.T)
-	require.Len(t, test.Variadic.Attrs, 1)
-	vs, err := test.Variadic.Attrs[0].Strings()
+	require.EqualValues(t, "enum", test.Parent.Child.Variadic.T)
+	require.Len(t, test.Parent.Child.Variadic.Attrs, 1)
+	vs, err := test.Parent.Child.Variadic.Attrs[0].Strings()
 	require.NoError(t, err)
 	require.EqualValues(t, []string{"a", "b", "c"}, vs)
 
-	require.EqualValues(t, "int", test.Third.T)
-	require.Len(t, test.Third.Attrs, 1)
-	i, err = test.Third.Attrs[0].Int()
+	require.EqualValues(t, "int", test.Parent.Child.Third.T)
+	require.Len(t, test.Parent.Child.Third.Attrs, 1)
+	i, err = test.Parent.Child.Third.Attrs[0].Int()
 	require.NoError(t, err)
 	require.EqualValues(t, 10, i)
 
@@ -406,7 +417,7 @@ variadic = enum("a","b","c")
 }
 
 func TestEmptyStrSQL(t *testing.T) {
-	s := New(WithTypes(nil))
+	s := New(WithTypes("", nil))
 	h := `x = sql("")`
 	err := s.EvalBytes([]byte(h), &struct{}{}, nil)
 	require.ErrorContains(t, err, "empty expression")
@@ -414,7 +425,7 @@ func TestEmptyStrSQL(t *testing.T) {
 
 func TestOptionalArgs(t *testing.T) {
 	s := New(
-		WithTypes([]*TypeSpec{
+		WithTypes("block", []*TypeSpec{
 			{
 				T:    "float",
 				Name: "float",
@@ -425,32 +436,38 @@ func TestOptionalArgs(t *testing.T) {
 			},
 		}),
 	)
-	f := `arg_0 = float
-arg_1 = float(10)
-arg_2 = float(10,2)
+	f := `
+block "name" {
+  arg_0 = float
+  arg_1 = float(10)
+  arg_2 = float(10,2)
+}
 `
 	var test struct {
-		Arg0 *Type `spec:"arg_0"`
-		Arg1 *Type `spec:"arg_1"`
-		Arg2 *Type `spec:"arg_2"`
+		Block struct {
+			Name string `spec:",name"`
+			Arg0 *Type  `spec:"arg_0"`
+			Arg1 *Type  `spec:"arg_1"`
+			Arg2 *Type  `spec:"arg_2"`
+		} `spec:"block"`
 	}
 	err := s.EvalBytes([]byte(f), &test, nil)
 	require.NoError(t, err)
-	require.Nil(t, test.Arg0.Attrs)
+	require.Nil(t, test.Block.Arg0.Attrs)
 
-	require.Len(t, test.Arg1.Attrs, 1)
-	require.Equal(t, "precision", test.Arg1.Attrs[0].K)
-	i, err := test.Arg1.Attrs[0].Int()
+	require.Len(t, test.Block.Arg1.Attrs, 1)
+	require.Equal(t, "precision", test.Block.Arg1.Attrs[0].K)
+	i, err := test.Block.Arg1.Attrs[0].Int()
 	require.NoError(t, err)
 	require.EqualValues(t, 10, i)
 
-	require.Len(t, test.Arg2.Attrs, 2)
-	require.Equal(t, "precision", test.Arg2.Attrs[0].K)
-	i, err = test.Arg2.Attrs[0].Int()
+	require.Len(t, test.Block.Arg2.Attrs, 2)
+	require.Equal(t, "precision", test.Block.Arg2.Attrs[0].K)
+	i, err = test.Block.Arg2.Attrs[0].Int()
 	require.NoError(t, err)
 	require.EqualValues(t, 10, i)
-	require.Equal(t, "scale", test.Arg2.Attrs[1].K)
-	i, err = test.Arg2.Attrs[1].Int()
+	require.Equal(t, "scale", test.Block.Arg2.Attrs[1].K)
+	i, err = test.Block.Arg2.Attrs[1].Int()
 	require.NoError(t, err)
 	require.EqualValues(t, 2, i)
 }
diff --git a/sql/postgres/sqlspec_test.go b/sql/postgres/sqlspec_test.go
index 21d82d26..d1204cfc 100644
--- a/sql/postgres/sqlspec_test.go
+++ b/sql/postgres/sqlspec_test.go
@@ -12,6 +12,7 @@ import (
 	"ariga.io/atlas/sql/internal/spectest"
 	"ariga.io/atlas/sql/internal/sqlx"
 	"ariga.io/atlas/sql/schema"
+	"github.com/hashicorp/hcl/v2/hclparse"
 	"github.com/stretchr/testify/require"
 )
 
@@ -1820,3 +1821,52 @@ schema "s2" {
 `,
 		string(got))
 }
+
+func TestEvalHCLError(t *testing.T) {
+	for _, tt := range []struct {
+		text, err string
+	}{
+		{
+			text: `
+			table "test" {
+			  column "id" {
+			    type = string
+			  }
+			}`,
+			err: `schema.hcl:4,15-21: Unknown column.type; There is no type named "string".`,
+		},
+		{
+			text: `
+			table "test" {
+			  index "i" {
+			    type = unknown
+			  }
+			}`,
+			err: `schema.hcl:4,15-22: Unknown index.type; There is no type named "unknown".`,
+		},
+		{
+			text: `
+			table "test" {
+			  foreign_key "f" {
+			    on_delete = unknown
+			  }
+			}`,
+			err: `schema.hcl:4,20-27: Unknown foreign_key.on_delete; There is no on_delete named "unknown".`,
+		},
+		{
+			text: `
+			table "test" {
+			  partition {
+			    type = UNKNOWN
+			  }
+			}`,
+			err: `schema.hcl:4,15-22: Unknown partition.type; There is no type named "UNKNOWN".`,
+		},
+	} {
+		p := hclparse.NewParser()
+		_, diags := p.ParseHCL([]byte(tt.text), "schema.hcl")
+		require.False(t, diags.HasErrors())
+		err := EvalHCL.Eval(p, &schema.Schema{}, nil)
+		require.EqualError(t, err, tt.err)
+	}
+}