Refactor op/script/component loading and spec validation

Signed-off-by: Solomon Hykes <sh.github.6811@hykes.org>

dagger/client.go

@@ -114,7 +114,7 @@ func (cfg *ClientConfig) Finalize(ctx context.Context) (map[string]string, error
 		return nil, errors.Wrap(err, "invalid client config")
 	}
 	// Finalize boot script
-	boot, err := v.Get("boot").Script()
+	boot, err := NewScript(v.Get("boot"))
 	if err != nil {
 		return nil, errors.Wrap(err, "invalid env boot script")
 	}

dagger/compiler.go

@@ -35,7 +35,7 @@ func (cc *Compiler) Spec() *Spec {
 	if err != nil {
 		panic(err)
 	}
-	cc.spec, err = v.Spec()
+	cc.spec, err = newSpec(v)
 	if err != nil {
 		panic(err)
 	}
@@ -56,12 +56,15 @@ func (cc *Compiler) Compile(name string, src interface{}) (*Value, error) {
 	return cc.Wrap(inst.Value(), inst), nil
 }
 
+// Compile a cue configuration, and load it as a script.
+// If the cue configuration is invalid, or does not match the script spec,
+//  return an error.
 func (cc *Compiler) CompileScript(name string, src interface{}) (*Script, error) {
 	v, err := cc.Compile(name, src)
 	if err != nil {
 		return nil, err
 	}
-	return v.Script()
+	return NewScript(v)
 }
 
 // Build a cue configuration tree from the files in fs.

dagger/component.go

@@ -3,21 +3,44 @@ package dagger
 import (
 	"context"
 	"os"
+
+	"github.com/pkg/errors"
 )
 
 type Component struct {
+	// Source value for the component, without spec merged
+	// eg. `{ string, #dagger: compute: [{do:"fetch-container", ...}]}`
 	v *Value
+
+	// Annotation value for the component , with spec merged.
+	// -> the contents of #dagger.compute
+	// eg. `compute: [{do:"fetch-container", ...}]`
+	//
+	// The spec is merged at this level because the Cue API
+	//  does not support merging embedded scalar with nested definition.
+	config *Value
+}
+
+func NewComponent(v *Value) (*Component, error) {
+	config := v.Get("#dagger")
+	if !config.Exists() {
+		return nil, os.ErrNotExist
+	}
+	spec := v.cc.Spec()
+	config, err := spec.Get("#ComponentConfig").Merge(v.Get("#dagger"))
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid component config")
+	}
+	return &Component{
+		v:      v,
+		config: config,
+	}, nil
 }
 
 func (c *Component) Value() *Value {
 	return c.v
 }
 
-func (c *Component) Exists() bool {
-	// Does #dagger exist?
-	return c.Config().Exists()
-}
-
 // Return the contents of the "#dagger" annotation.
 func (c *Component) Config() *Value {
 	return c.Value().Get("#dagger")
@@ -38,11 +61,7 @@ func (c *Component) Validate() error {
 
 // Return this component's compute script.
 func (c *Component) ComputeScript() (*Script, error) {
-	v := c.Value().Get("#dagger.compute")
+	return newScript(c.Config().Get("compute"))
-	if !v.Exists() {
-		return nil, os.ErrNotExist
-	}
-	return v.Script()
 }
 
 // Compute the configuration for this component.

dagger/component_test.go

@@ -5,6 +5,38 @@ import (
 	"testing"
 )
 
+func TestComponentNotExist(t *testing.T) {
+	cc := &Compiler{}
+	root, err := cc.Compile("root.cue", `
+foo: hello: "world"
+`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = NewComponent(root.Get("bar")) // non-existent key
+	if err != ErrNotExist {
+		t.Fatal(err)
+	}
+	_, err = NewComponent(root.Get("foo")) // non-existent #dagger
+	if err != ErrNotExist {
+		t.Fatal(err)
+	}
+}
+
+func TestLoadEmptyComponent(t *testing.T) {
+	cc := &Compiler{}
+	root, err := cc.Compile("root.cue", `
+foo: #dagger: {}
+`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = NewComponent(root.Get("foo"))
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 // Test that default values in spec are applied at the component level
 // See issue #19
 func TestComponentDefaults(t *testing.T) {
@@ -28,7 +60,7 @@ func TestComponentDefaults(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	c, err := v.Component()
+	c, err := NewComponent(v)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -53,7 +85,7 @@ func TestValidateEmptyComponent(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	_, err = v.Component()
+	_, err = NewComponent(v)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -65,7 +97,7 @@ func TestValidateSimpleComponent(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	c, err := v.Component()
+	c, err := NewComponent(v)
 	if err != nil {
 		t.Fatal(err)
 	}

dagger/env.go

@@ -187,8 +187,9 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) {
 		ctx := lg.WithContext(ctx)
 
 		lg.Debug().Msg("Env.Walk: processing")
+		// FIXME: get directly from state Value ? Locking issue?
 		val := env.cc.Wrap(v, flowInst)
-		c, err := val.Component()
+		c, err := NewComponent(val)
 		if os.IsNotExist(err) {
 			// Not a component: skip
 			return nil, nil
@@ -207,3 +208,9 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) {
 	}
 	return out, nil
 }
+
+// Return the component at the specified path in the config, eg. `www`
+// If the component does not exist, os.ErrNotExist is returned.
+func (env *Env) Component(target string) (*Component, error) {
+	return NewComponent(env.state.Get(target))
+}

dagger/gen.go

@@ -117,9 +117,4 @@ package dagger
 	src:  string | *"/"
 	dest: string | *"/"
 }
-
-#TestScript: #Script & [
-		{do: "fetch-container", ref: "alpine:latest"},
-		{do: "exec", args: ["echo", "hello", "world"]},
-]
 `

dagger/mount.go

@@ -13,6 +13,16 @@ type Mount struct {
 	v    *Value
 }
 
+func newMount(v *Value, dest string) (*Mount, error) {
+	if !v.Exists() {
+		return nil, ErrNotExist
+	}
+	return &Mount{
+		v:    v,
+		dest: dest,
+	}, nil
+}
+
 func (mnt *Mount) Validate(defs ...string) error {
 	return mnt.v.Validate(append(defs, "#Mount")...)
 }
@@ -26,7 +36,7 @@ func (mnt *Mount) LLB(ctx context.Context, s Solver) (llb.RunOption, error) {
 		return nil, fmt.Errorf("FIXME: cache mount not yet implemented")
 	}
 	// Compute source component or script, discarding fs writes & output value
-	from, err := mnt.v.Lookup("from").Executable()
+	from, err := newExecutable(mnt.v.Lookup("from"))
 	if err != nil {
 		return nil, errors.Wrap(err, "from")
 	}

dagger/op.go

@@ -14,6 +14,25 @@ type Op struct {
 	v *Value
 }
 
+func NewOp(v *Value) (*Op, error) {
+	spec := v.cc.Spec().Get("#Op")
+	final, err := spec.Merge(v)
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid op")
+	}
+	return newOp(final)
+}
+
+// Same as newOp, but without spec merge + validation.
+func newOp(v *Value) (*Op, error) {
+	if !v.Exists() {
+		return nil, ErrNotExist
+	}
+	return &Op{
+		v: v,
+	}, nil
+}
+
 func (op *Op) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error) {
 	action, err := op.Action()
 	if err != nil {
@@ -29,7 +48,7 @@ func (op *Op) Walk(ctx context.Context, fn func(*Op) error) error {
 	isCopy := (op.Validate("#Copy") == nil)
 	isLoad := (op.Validate("#Load") == nil)
 	if isCopy || isLoad {
-		if from, err := op.Get("from").Executable(); err == nil {
+		if from, err := newExecutable(op.Get("from")); err == nil {
 			if err := from.Walk(ctx, fn); err != nil {
 				return err
 			}
@@ -38,7 +57,7 @@ func (op *Op) Walk(ctx context.Context, fn func(*Op) error) error {
 	}
 	if err := op.Validate("#Exec"); err == nil {
 		return op.Get("mount").RangeStruct(func(k string, v *Value) error {
-			if from, err := op.Get("from").Executable(); err == nil {
+			if from, err := newExecutable(op.Get("from")); err == nil {
 				if err := from.Walk(ctx, fn); err != nil {
 					return err
 				}
@@ -98,7 +117,7 @@ func (op *Op) Copy(ctx context.Context, fs FS, out *Fillable) (FS, error) {
 	if err != nil {
 		return fs, err
 	}
-	from, err := op.Get("from").Executable()
+	from, err := newExecutable(op.Get("from"))
 	if err != nil {
 		return fs, errors.Wrap(err, "from")
 	}
@@ -167,7 +186,7 @@ func (op *Op) Exec(ctx context.Context, fs FS, out *Fillable) (FS, error) {
 	// mounts
 	if mounts := op.v.Lookup("mount"); mounts.Exists() {
 		if err := mounts.RangeStruct(func(k string, v *Value) error {
-			mnt, err := v.Mount(k)
+			mnt, err := newMount(v, k)
 			if err != nil {
 				return err
 			}
@@ -233,7 +252,7 @@ func (op *Op) Export(ctx context.Context, fs FS, out *Fillable) (FS, error) {
 }
 
 func (op *Op) Load(ctx context.Context, fs FS, out *Fillable) (FS, error) {
-	from, err := op.Get("from").Executable()
+	from, err := newExecutable(op.Get("from"))
 	if err != nil {
 		return fs, errors.Wrap(err, "load")
 	}

dagger/op_test.go

@@ -14,7 +14,7 @@ func TestLocalMatch(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	op, err := v.Op()
+	op, err := newOp(v)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -40,7 +40,7 @@ func TestCopyMatch(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	op, err := v.Op()
+	op, err := newOp(v)
 	if err != nil {
 		t.Fatal(err)
 	}

dagger/script.go

@@ -3,6 +3,7 @@ package dagger
 import (
 	"context"
 
+	"cuelang.org/go/cue"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
 )
@@ -15,15 +16,46 @@ type Script struct {
 	v *Value
 }
 
-func (s *Script) Validate() error {
+func NewScript(v *Value) (*Script, error) {
-	// FIXME this crashes when a script is incomplete or empty
+	spec := v.cc.Spec().Get("#Script")
-	return s.Value().Validate("#Script")
+	final, err := spec.Merge(v)
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid script")
+	}
+	return newScript(final)
+}
+
+// Same as newScript, but without spec merge + validation.
+func newScript(v *Value) (*Script, error) {
+	if !v.Exists() {
+		return nil, ErrNotExist
+	}
+	// Assume script is valid.
+	// Spec validation is already done at component creation.
+	return &Script{
+		v: v,
+	}, nil
 }
 
 func (s *Script) Value() *Value {
 	return s.v
 }
 
+// Return the operation at index idx
+func (s *Script) Op(idx int) (*Op, error) {
+	v := s.v.LookupPath(cue.MakePath(cue.Index(idx)))
+	if !v.Exists() {
+		return nil, ErrNotExist
+	}
+	return newOp(v)
+}
+
+// Return the number of operations in the script
+func (s *Script) Len() uint64 {
+	l, _ := s.v.Len().Uint64()
+	return l
+}
+
 // Run a dagger script
 func (s *Script) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error) {
 	err := s.v.RangeList(func(idx int, v *Value) error {
@@ -38,7 +70,7 @@ func (s *Script) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error)
 				Msg("script is unspecified, aborting execution")
 			return ErrAbortExecution
 		}
-		op, err := v.Op()
+		op, err := newOp(v)
 		if err != nil {
 			return errors.Wrapf(err, "validate op %d/%d", idx+1, s.v.Len())
 		}
@@ -58,7 +90,7 @@ func (s *Script) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error)
 
 func (s *Script) Walk(ctx context.Context, fn func(op *Op) error) error {
 	return s.v.RangeList(func(idx int, v *Value) error {
-		op, err := v.Op()
+		op, err := newOp(v)
 		if err != nil {
 			return errors.Wrapf(err, "validate op %d/%d", idx+1, s.v.Len())
 		}

dagger/script_test.go

@@ -6,10 +6,47 @@ import (
 	"testing"
 )
 
+// Test a script which loads a nested script
+func TestScriptLoadScript(t *testing.T) {
+	mkScript(t, 2, `
+	[
+		{
+			do: "load"
+			from: [
+				{
+					do: "fetch-container"
+					ref: "alpine:latest"
+				}
+			]
+		}
+	]
+	`)
+}
+
+// Test a script which loads a nested component
+func TestScriptLoadComponent(t *testing.T) {
+	mkScript(t, 2, `
+[
+	{
+		do: "load"
+		from: {
+			#dagger: compute: [
+				{
+					do: "fetch-container"
+					ref: "alpine:latest"
+				}
+			]
+		}
+	}
+]
+`)
+}
+
 // Test that default values in spec are applied
 func TestScriptDefaults(t *testing.T) {
 	cc := &Compiler{}
 	v, err := cc.Compile("", `
+	[
     {
         do: "exec"
         args: ["sh", "-c", """
@@ -17,15 +54,17 @@ func TestScriptDefaults(t *testing.T) {
         """]
 //      dir: "/"
     }
+	]
 `)
 	if err != nil {
 		t.Fatal(err)
 	}
-	op, err := v.Op()
+	script, err := NewScript(v)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := op.Validate(); err != nil {
+	op, err := script.Op(0)
+	if err != nil {
 		t.Fatal(err)
 	}
 	dir, err := op.Get("dir").String()
@@ -64,7 +103,7 @@ func TestLocalScript(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	s, err := v.Script()
+	s, err := NewScript(v)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -84,12 +123,14 @@ func TestLocalScript(t *testing.T) {
 func TestWalkBootScript(t *testing.T) {
 	ctx := context.TODO()
 
-	cc := &Compiler{}
+	cfg := &ClientConfig{}
-	cfg, err := cc.Compile("clientconfig.cue", baseClientConfig)
+	_, err := cfg.Finalize(context.TODO())
 	if err != nil {
 		t.Fatal(err)
 	}
-	script, err := cfg.Get("boot").Script()
+
+	cc := &Compiler{}
+	script, err := cc.CompileScript("boot.cue", cfg.Boot)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -160,3 +201,28 @@ func TestWalkBiggerScript(t *testing.T) {
 		t.Fatal(got)
 	}
 }
+
+// UTILITIES
+
+// Compile a script and check that it has the correct
+// number of operations.
+func mkScript(t *testing.T, nOps int, src string) *Script {
+	cc := &Compiler{}
+	s, err := cc.CompileScript("test.cue", src)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Count ops (including nested `from`)
+	n := 0
+	err = s.Walk(context.TODO(), func(op *Op) error {
+		n++
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n != nOps {
+		t.Fatal(n)
+	}
+	return s
+}

dagger/spec.cue

@@ -112,8 +112,3 @@ package dagger
 	src:  string | *"/"
 	dest: string | *"/"
 }
-
-#TestScript: #Script & [
-		{do: "fetch-container", ref: "alpine:latest"},
-		{do: "exec", args: ["echo", "hello", "world"]},
-]

dagger/spec.go

@@ -10,6 +10,16 @@ type Spec struct {
 	root *Value
 }
 
+func newSpec(v *Value) (*Spec, error) {
+	// Spec contents must be a struct
+	if _, err := v.Struct(); err != nil {
+		return nil, err
+	}
+	return &Spec{
+		root: v,
+	}, nil
+}
+
 // eg. Validate(op, "#Op")
 func (s Spec) Validate(v *Value, defpath string) error {
 	// Lookup def by name, eg. "#Script" or "#Copy"

dagger/types.go

@@ -2,16 +2,35 @@ package dagger
 
 import (
 	"context"
+	"fmt"
+	"os"
 
 	cueflow "cuelang.org/go/tools/flow"
 )
 
+var ErrNotExist = os.ErrNotExist
+
 // Implemented by Component, Script, Op
 type Executable interface {
 	Execute(context.Context, FS, *Fillable) (FS, error)
 	Walk(context.Context, func(*Op) error) error
 }
 
+func newExecutable(v *Value) (Executable, error) {
+	// NOTE: here we need full spec validation,
+	//   so we call NewScript, NewComponent, NewOp.
+	if script, err := NewScript(v); err == nil {
+		return script, nil
+	}
+	if component, err := NewComponent(v); err == nil {
+		return component, nil
+	}
+	if op, err := NewOp(v); err == nil {
+		return op, nil
+	}
+	return nil, fmt.Errorf("value is not executable")
+}
+
 // Something which can be filled in-place with a cue value
 type Fillable struct {
 	t *cueflow.Task

dagger/value.go

@@ -2,7 +2,6 @@ package dagger
 
 import (
 	"fmt"
-	"os"
 
 	"cuelang.org/go/cue"
 	cueformat "cuelang.org/go/cue/format"
@@ -210,6 +209,8 @@ func (v *Value) Save(fs FS, filename string) (FS, error) {
 	}), nil
 }
 
+// Check that a value is valid. Optionally check that it matches
+// all the specified spec definitions..
 func (v *Value) Validate(defs ...string) error {
 	if err := v.val.Validate(); err != nil {
 		return err
@@ -238,90 +239,3 @@ func (v *Value) IsEmptyStruct() bool {
 	}
 	return false
 }
-
-// Component returns the component value if v is a valid dagger component or an error otherwise.
-// If no '#dagger' annotation is present, os.ErrNotExist
-// is returned.
-func (v *Value) Component() (*Component, error) {
-	c := &Component{
-		v: v,
-	}
-	if !c.Exists() {
-		return c, os.ErrNotExist
-	}
-	if err := c.Validate(); err != nil {
-		return c, err
-	}
-	return c, nil
-}
-
-func (v *Value) Script() (*Script, error) {
-	s := &Script{
-		v: v,
-	}
-	if err := s.Validate(); err != nil {
-		return s, err
-	}
-	return s, nil
-}
-
-func (v *Value) Executable() (Executable, error) {
-	if script, err := v.Script(); err == nil {
-		return script, nil
-	}
-	if component, err := v.Component(); err == nil {
-		return component, nil
-	}
-	if op, err := v.Op(); err == nil {
-		return op, nil
-	}
-	return nil, fmt.Errorf("value is not executable")
-}
-
-// ScriptOrComponent returns one of:
-//  (1) the component value if v is a valid component (type *Component)
-//  (2) the script value if v is a valid script (type *Script)
-//  (3) an error otherwise
-func (v *Value) ScriptOrComponent() (interface{}, error) {
-	s, err := v.Script()
-	if err == nil {
-		return s, nil
-	}
-	c, err := v.Component()
-	if err == nil {
-		return c, nil
-	}
-	return nil, fmt.Errorf("not a script or component")
-}
-
-func (v *Value) Op() (*Op, error) {
-	// Merge #Op definition from spec to get default values
-	spec := v.cc.Spec()
-	v, err := spec.Get("#Op").Merge(v)
-	if err != nil {
-		return nil, err
-	}
-	op := &Op{
-		v: v,
-	}
-	return op, nil
-}
-
-func (v *Value) Mount(dest string) (*Mount, error) {
-	mnt := &Mount{
-		v:    v,
-		dest: dest,
-	}
-	return mnt, mnt.Validate()
-}
-
-// Interpret this value as a spec
-func (v *Value) Spec() (*Spec, error) {
-	// Spec must be a struct
-	if _, err := v.Struct(); err != nil {
-		return nil, err
-	}
-	return &Spec{
-		root: v,
-	}, nil
-}

dagger/value_test.go

@@ -29,28 +29,10 @@ func TestJSON(t *testing.T) {
 	}
 }
 
-func TestCompileBootScript(t *testing.T) {
-	cc := &Compiler{}
-	cfg, err := cc.Compile("boot.cue", baseClientConfig)
-	if err != nil {
-		t.Fatal(err)
-	}
-	s, err := cfg.Get("bootscript").Script()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if err := s.Validate(); err != nil {
-		t.Fatal(err)
-	}
-}
-
 func TestCompileSimpleScript(t *testing.T) {
 	cc := &Compiler{}
-	s, err := cc.CompileScript("simple.cue", `[{do: "local", dir: "."}]`)
+	_, err := cc.CompileScript("simple.cue", `[{do: "local", dir: "."}]`)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := s.Validate(); err != nil {
-		t.Fatal(err)
-	}
 }

examples/tests/compute/noop/main.cue

@@ -9,11 +9,3 @@ realempty: {
 empty: {
 	#dagger: compute: []
 }
-
-// additional prop, should not error
-withprops: {
-	#dagger: {
-		compute: []
-		foo: bar: "foo"
-	}
-}

examples/tests/compute/undefined_prop/main.cue

@@ -4,8 +4,7 @@ hello: "world"
 
 bar: string
 
-#dagger: {
+#dagger: compute: [
-	compute: [
 	{
 		do:  "fetch-container"
 		ref: "alpine"
@@ -13,8 +12,6 @@ bar: string
 	{
 		do:  "exec"
 		dir: "/"
-			args: ["sh", "-c", "echo \(foo.bar)"]
+		args: ["sh", "-c", "echo \(bar)"]
 	},
 ]
-	foo: bar: bar
-}

examples/tests/test.sh

@@ -25,11 +25,11 @@ test::compute(){
       "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/invalid/int
   test::one "Compute: invalid struct should fail" --exit=1 --stdout= \
       "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/invalid/struct
-  test::one "Compute: noop should succeed" --exit=0 --stdout='{"empty":{},"realempty":{},"withprops":{}}'  \
+  test::one "Compute: noop should succeed" --exit=0 --stdout='{"empty":{},"realempty":{}}'  \
       "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/noop
   test::one "Compute: simple should succeed" --exit=0 --stdout="{}" \
       "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/simple
-  test::one "Compute: unresolved should be ignored" --exit=0 --stdout='{"hello":"world"}'  \
+  test::one "Compute: script with undefined values should not fail" --exit=0 --stdout='{"hello":"world"}'  \
       "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/undefined_prop
 }