upup: replace parse code with standard json unmarshaller

Instead of reimplementing the unmarshal code, we implement a trick: we
implement an alternative JSON representation of an object: a string.

We unmarshal as normal, and then we reconcile these pointer values to
the primary values, by walking the unmarshalled tree.

upup/pkg/fi/cloudup/loader.go

@@ -3,6 +3,7 @@ package cloudup
 import (
 	"bytes"
 	"encoding/base64"
+	"encoding/json"
 	"fmt"
 	"github.com/golang/glog"
 	"io"
@@ -17,18 +18,11 @@ import (
 	"text/template"
 )
 
-type deferredType int
-
 const (
 	KEY_NAME = "name"
 	KEY_TYPE = "_type"
 )
 
-const (
-	deferredUnit deferredType = iota
-	deferredResource
-)
-
 type Loader struct {
 	StateDir      string
 	OptionsLoader *loader.OptionsLoader
@@ -43,11 +37,9 @@ type Loader struct {
 	config    interface{}
 
 	Resources map[string]fi.Resource
-	deferred  []*deferredBinding
+	//deferred          []*deferredBinding
 
 	tasks map[string]fi.Task
-
-	unmarshaller utils.Unmarshaller
 }
 
 type templateResource struct {
@@ -77,17 +69,9 @@ func (a *templateResource) Curry(args []string) fi.TemplateResource {
 	return curried
 }
 
-type deferredBinding struct {
-	name         string
-	dest         utils.Settable
-	src          string
-	deferredType deferredType
-}
-
 func (l *Loader) Init() {
 	l.tasks = make(map[string]fi.Task)
 	l.typeMap = make(map[string]reflect.Type)
-	l.unmarshaller.SpecialCases = l.unmarshalSpecialCases
 	l.Resources = make(map[string]fi.Resource)
 	l.TemplateFunctions = make(template.FuncMap)
 }
@@ -123,6 +107,10 @@ func (l *Loader) executeTemplate(key string, d string, args []string) (string, e
 	funcMap["RenderResource"] = func(resourceName string, args []string) (string, error) {
 		return l.renderResource(resourceName, args)
 	}
+	funcMap["HasTag"] = func(tag string) bool {
+		_, found := l.Tags[tag]
+		return found
+	}
 	for k, fn := range l.TemplateFunctions {
 		funcMap[k] = fn
 	}
@@ -198,60 +186,75 @@ func (l *Loader) Build(baseDir string) (map[string]fi.Task, error) {
 }
 
 func (l *Loader) processDeferrals() error {
-	if len(l.deferred) != 0 {
+	for taskKey, task := range l.tasks {
-		unitMap := make(map[string]fi.Task)
+		taskValue := reflect.ValueOf(task)
 
-		for k, o := range l.tasks {
+		err := utils.ReflectRecursive(taskValue, func(path string, f *reflect.StructField, v reflect.Value) error {
-			if unit, ok := o.(fi.Task); ok {
+			if utils.IsPrimitiveValue(v) {
-				unitMap[k] = unit
+				return nil
-			}
 			}
 
-		for _, d := range l.deferred {
+			if path == "" {
-			src := d.src
+				// Don't process top-level value
+				return nil
+			}
 
-			switch d.deferredType {
+			switch v.Kind() {
-			case deferredUnit:
+			case reflect.Interface, reflect.Ptr:
-				unit, found := unitMap[src]
+				if v.CanInterface() && !v.IsNil() {
-				if !found {
+					// TODO: Can we / should we use a type-switch statement
-					glog.Infof("Known targets:")
+					intf := v.Interface()
-					for k := range unitMap {
+					if hn, ok := intf.(fi.HasName); ok {
+						name := hn.GetName()
+						if name != nil {
+							primary := l.tasks[*name]
+							if primary == nil {
+								glog.Infof("Known tasks:")
+								for k := range l.tasks {
 									glog.Infof("  %s", k)
 								}
-					return fmt.Errorf("cannot resolve link at %q to %q", d.name, d.src)
+								return fmt.Errorf("Unable to find task %q, referenced from %s:%s", *name, taskKey, path)
 							}
 
-				d.dest.Set(reflect.ValueOf(unit))
+							glog.Infof("Replacing task %q at %s:%s", *name, taskKey, path)
-
+							v.Set(reflect.ValueOf(primary))
-			case deferredResource:
+						}
-				// Resources can contain template 'arguments', separated by spaces
+						return utils.SkipReflection
+					} else if rh, ok := intf.(*fi.ResourceHolder); ok {
+						//Resources can contain template 'arguments', separated by spaces
 						// <resourcename> <arg1> <arg2>
-				tokens := strings.Split(src, " ")
+						tokens := strings.Split(rh.Name, " ")
 						match := tokens[0]
 						args := tokens[1:]
 
 						match = strings.TrimPrefix(match, "resources/")
-				found := l.Resources[match]
+						resource := l.Resources[match]
 
-				if found == nil {
+						if resource == nil {
 							glog.Infof("Known resources:")
 							for k := range l.Resources {
 								glog.Infof("  %s", k)
 							}
-					return fmt.Errorf("cannot resolve resource link %q (at %q)", d.src, d.name)
+							return fmt.Errorf("Unable to find resource %q, referenced from %s:%s", rh.Name, taskKey, path)
 						}
 
-				err := l.populateResource(d.name, d.dest, found, args)
+						err := l.populateResource(rh, resource, args)
 						if err != nil {
 							return fmt.Errorf("error setting resource value: %v", err)
 						}
-
+						return utils.SkipReflection
-			default:
-				panic("unhandled deferred type")
 					}
 				}
 			}
 
+			return nil
+		})
+
+		if err != nil {
+			return fmt.Errorf("unexpected error resolving task %q: %v", taskKey, err)
+		}
+	}
+
 	return nil
 }
 
@@ -366,19 +369,22 @@ func (l *Loader) loadObjectMap(key string, data map[string]interface{}) (map[str
 		}
 
 		o := reflect.New(t)
-		err := l.unmarshaller.UnmarshalStruct(key+":"+k, o, v)
+
+		glog.Warningf("replace with partial unmarshal...")
+		jsonValue, err := json.Marshal(v)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("error marshalling to json: %v", err)
 		}
-		//glog.Infof("Built %s:%s => %v", key, k, o.Interface())
+		err = json.Unmarshal(jsonValue, o.Interface())
+		if err != nil {
+			return nil, fmt.Errorf("error parsing %q: %v", key, err)
+		}
+		glog.Infof("Built %s:%s => %v", key, k, o.Interface())
 
 		if inferredName {
-			nameField := o.Elem().FieldByName("Name")
+			hn, ok := o.Interface().(fi.HasName)
-			if nameField.IsValid() {
+			if ok {
-				err := l.unmarshaller.UnmarshalSettable(k+":Name", utils.Settable{Value: nameField}, name)
+				hn.SetName(name)
-				if err != nil {
-					return nil, err
-				}
 			}
 		}
 		loaded[k] = o.Interface()
@@ -386,87 +392,11 @@ func (l *Loader) loadObjectMap(key string, data map[string]interface{}) (map[str
 	return loaded, nil
 }
 
-func (l *Loader) unmarshalSpecialCases(name string, dest utils.Settable, src interface{}) (bool, error) {
+func (l *Loader) populateResource(rh *fi.ResourceHolder, resource fi.Resource, args []string) error {
-	if dest.Type().Kind() == reflect.Slice {
+	if resource == nil {
-		switch src := src.(type) {
-		case []interface{}:
-			destValueArray := reflect.MakeSlice(dest.Type(), len(src), len(src))
-			for i, srcElem := range src {
-				done, err := l.unmarshalSpecialCases(fmt.Sprintf("%s[%d]", name, i),
-					utils.Settable{Value: destValueArray.Index(i)},
-					srcElem)
-				if err != nil {
-					return false, err
-				}
-				if !done {
-					return false, nil
-				}
-			}
-			dest.Set(destValueArray)
-			return true, nil
-		default:
-			return false, fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, dest.Value.Type().Name())
-		}
-	}
-
-	if dest.Type().Kind() == reflect.Ptr || dest.Type().Kind() == reflect.Interface {
-		resourceType := reflect.TypeOf((*fi.Resource)(nil)).Elem()
-		if dest.Value.Type().AssignableTo(resourceType) {
-			d := &deferredBinding{
-				name:         name,
-				dest:         dest,
-				deferredType: deferredResource,
-			}
-			switch src := src.(type) {
-			case string:
-				d.src = src
-			default:
-				return false, fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, dest.Value.Type().Name())
-			}
-			l.deferred = append(l.deferred, d)
-			return true, nil
-		}
-
-		taskType := reflect.TypeOf((*fi.Task)(nil)).Elem()
-		if dest.Value.Type().AssignableTo(taskType) {
-			d := &deferredBinding{
-				name:         name,
-				dest:         dest,
-				deferredType: deferredUnit,
-			}
-			switch src := src.(type) {
-			case string:
-				d.src = src
-			default:
-				return false, fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, dest.Value.Type().Name())
-			}
-			l.deferred = append(l.deferred, d)
-			return true, nil
-		}
-	}
-
-	return false, nil
-}
-
-func (l *Loader) populateResource(name string, dest utils.Settable, src interface{}, args []string) error {
-	if src == nil {
 		return nil
 	}
 
-	destTypeName := utils.BuildTypeName(dest.Type())
-
-	switch destTypeName {
-	case "Resource":
-		{
-			switch src := src.(type) {
-			case []byte:
-				if len(args) != 0 {
-					return fmt.Errorf("cannot have arguments with static resources")
-				}
-				dest.Set(reflect.ValueOf(fi.NewBytesResource(src)))
-
-			default:
-				if resource, ok := src.(fi.Resource); ok {
 	if len(args) != 0 {
 		templateResource, ok := resource.(fi.TemplateResource)
 		if !ok {
@@ -474,18 +404,9 @@ func (l *Loader) populateResource(name string, dest utils.Settable, src interfac
 		}
 		resource = templateResource.Curry(args)
 	}
-					dest.Set(reflect.ValueOf(resource))
+	rh.Resource = resource
-				} else {
+
-					return fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, destTypeName)
-				}
-			}
 	return nil
-		}
-
-	default:
-		return fmt.Errorf("unhandled destination type for %q: %s", name, destTypeName)
-	}
-
 }
 
 func (l *Loader) buildNodeConfig(target string, configResourceName string, args []string) (string, error) {

upup/pkg/fi/dryrun_target.go

@@ -205,6 +205,8 @@ func asString(value reflect.Value) string {
 			intf := v.Addr().Interface()
 			if _, ok := intf.(Resource); ok {
 				fmt.Fprintf(b, "<resource>")
+			} else if _, ok := intf.(*ResourceHolder); ok {
+				fmt.Fprintf(b, "<resource>")
 			} else if compareWithID, ok := intf.(CompareWithID); ok {
 				id := compareWithID.CompareWithID()
 				if id == nil {

upup/pkg/fi/named.go

@@ -0,0 +1,7 @@
+package fi
+
+// HasName indicates that the task has a Name
+type HasName interface {
+	GetName() *string
+	SetName(name string)
+}

upup/pkg/fi/nodeup/nodetasks/file.go

@@ -58,18 +58,22 @@ func NewFileTask(name string, src fi.Resource, destPath string, meta string) (*F
 	return f, nil
 }
 
-func (f *File) GetDependencies(tasks map[string]fi.Task) []string {
+func (f *File) GetDependencies(tasks map[string]fi.Task) []fi.Task {
-	var deps []string
+	var deps []fi.Task
 	if f.Owner != nil {
-		deps = append(deps, "user/"+*f.Owner)
+		ownerTask := tasks["user/"+*f.Owner]
+		if ownerTask == nil {
+			glog.Fatalf("Unable to find task %q", "user/"+*f.Owner)
+		}
+		deps = append(deps, ownerTask)
 	}
 
 	// Depend on disk mounts
 	// For simplicity, we just depend on _all_ disk mounts
 	// We could check the mountpath, but that feels excessive...
-	for k, v := range tasks {
+	for _, v := range tasks {
 		if _, ok := v.(*MountDiskTask); ok {
-			deps = append(deps, k)
+			deps = append(deps, v)
 		}
 	}
 

upup/pkg/fi/resources.go

@@ -3,6 +3,7 @@ package fi
 import (
 	"bytes"
 	"encoding/hex"
+	"encoding/json"
 	"fmt"
 	"hash"
 	"io"
@@ -193,3 +194,36 @@ func (r *FileResource) Open() (io.ReadSeeker, error) {
 	}
 	return in, err
 }
+
+type ResourceHolder struct {
+	Name     string
+	Resource Resource
+}
+
+func (o *ResourceHolder) UnmarshalJSON(data []byte) error {
+	var jsonName string
+	err := json.Unmarshal(data, &jsonName)
+	if err != nil {
+		return err
+	}
+	o.Name = jsonName
+	return nil
+}
+
+func (o *ResourceHolder) Unwrap() Resource {
+	return o.Resource
+}
+
+func (o *ResourceHolder) AsString() (string, error) {
+	return ResourceAsString(o.Unwrap())
+}
+
+func (o *ResourceHolder) AsBytes() ([]byte, error) {
+	return ResourceAsBytes(o.Unwrap())
+}
+
+func WrapResource(r Resource) *ResourceHolder {
+	return &ResourceHolder{
+		Resource: r,
+	}
+}

upup/pkg/fi/topological_sort.go

@@ -9,7 +9,7 @@ import (
 )
 
 type HasDependencies interface {
-	GetDependencies(tasks map[string]Task) []string
+	GetDependencies(tasks map[string]Task) []Task
 }
 
 // FindTaskDependencies returns a map from each task's key to the discovered list of dependencies
@@ -23,12 +23,21 @@ func FindTaskDependencies(tasks map[string]Task) map[string][]string {
 
 	for k, t := range tasks {
 		task := t.(Task)
-		var dependencyKeys []string
 
+		var dependencies []Task
 		if hd, ok := task.(HasDependencies); ok {
-			dependencyKeys = hd.GetDependencies(tasks)
+			dependencies = hd.GetDependencies(tasks)
 		} else {
-			dependencyKeys = reflectForDependencies(task, taskToId)
+			dependencies = reflectForDependencies(tasks, task)
+		}
+
+		var dependencyKeys []string
+		for _, dep := range dependencies {
+			dependencyKey, found := taskToId[dep]
+			if !found {
+				glog.Fatalf("dependency not found: %v", dep)
+			}
+			dependencyKeys = append(dependencyKeys, dependencyKey)
 		}
 
 		edges[k] = dependencyKeys
@@ -42,23 +51,12 @@ func FindTaskDependencies(tasks map[string]Task) map[string][]string {
 	return edges
 }
 
-func reflectForDependencies(task Task, taskToId map[interface{}]string) []string {
+func reflectForDependencies(tasks map[string]Task, task Task) []Task {
 	v := reflect.ValueOf(task).Elem()
-	dependencies := getDependencies(v)
+	return getDependencies(tasks, v)
-
-	var dependencyKeys []string
-	for _, dep := range dependencies {
-		dependencyKey, found := taskToId[dep]
-		if !found {
-			glog.Fatalf("dependency not found: %v", dep)
-		}
-		dependencyKeys = append(dependencyKeys, dependencyKey)
-	}
-
-	return dependencyKeys
 }
 
-func getDependencies(v reflect.Value) []Task {
+func getDependencies(tasks map[string]Task, v reflect.Value) []Task {
 	var dependencies []Task
 
 	err := utils.ReflectRecursive(v, func(path string, f *reflect.StructField, v reflect.Value) error {
@@ -82,10 +80,15 @@ func getDependencies(v reflect.Value) []Task {
 
 			// TODO: Can we / should we use a type-switch statement
 			intf := v.Addr().Interface()
-			if dep, ok := intf.(Task); ok {
+			if hd, ok := intf.(HasDependencies); ok {
+				deps := hd.GetDependencies(tasks)
+				dependencies = append(dependencies, deps...)
+			} else if dep, ok := intf.(Task); ok {
 				dependencies = append(dependencies, dep)
 			} else if _, ok := intf.(Resource); ok {
 				// Ignore: not a dependency (?)
+			} else if _, ok := intf.(*ResourceHolder); ok {
+				// Ignore: not a dependency (?)
 			} else if _, ok := intf.(*pkix.Name); ok {
 				// Ignore: not a dependency
 			} else {

upup/pkg/fi/utils/marshal.go

@@ -1,232 +0,0 @@
-package utils
-
-import (
-	"fmt"
-	"github.com/golang/glog"
-	"reflect"
-	"strings"
-)
-
-// This file is (probably) the most complex code we have
-// It populates an object's fields, from the values stored in a map
-// We typically get this map by unmarshalling yaml or json
-// So: why not just use go's built in unmarshalling?
-// The reason is that we want richer functionality for when we link objects to each other
-// By doing our own marshalling, we can then link objects just by specifying a string identifier.
-// Then, while we're here, we add nicer functionality like case-insensitivity and nicer handling for resources.
-
-// Unmarshaller implements our specialized marshalling from a map to an object
-type Unmarshaller struct {
-	SpecialCases UnmarshallerSpecialCaseHandler
-}
-
-// UnmarshallerSpecialCaseHandler is the function type that a handler for non-standard types must implement
-type UnmarshallerSpecialCaseHandler func(name string, dest Settable, src interface{}) (bool, error)
-
-// Settable is a workaround for the fact that map entries are not settable
-type Settable struct {
-	Value reflect.Value
-
-	MapValue reflect.Value
-	MapKey   reflect.Value
-}
-
-// Set sets the target value to the specified value
-func (s *Settable) Set(v reflect.Value) {
-	if s.MapValue.IsValid() {
-		s.MapValue.SetMapIndex(s.MapKey, v)
-	} else {
-		s.Value.Set(v)
-	}
-}
-
-func (s *Settable) Type() reflect.Type {
-	return s.Value.Type()
-}
-
-func (r *Unmarshaller) UnmarshalSettable(name string, dest Settable, src interface{}) error {
-	if src == nil {
-		return nil
-	}
-
-	// Divert special cases
-	switch dest.Type().Kind() {
-	case reflect.Map:
-		return r.unmarshalMap(name, dest, src)
-	}
-
-	destTypeName := BuildTypeName(dest.Type())
-
-	switch destTypeName {
-	case "*string":
-		{
-			switch src := src.(type) {
-			case string:
-				v := src
-				dest.Set(reflect.ValueOf(&v))
-				return nil
-			default:
-				return fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, destTypeName)
-			}
-		}
-
-	case "string":
-		{
-			switch src := src.(type) {
-			case string:
-				v := src
-				dest.Set(reflect.ValueOf(v))
-				return nil
-			default:
-				return fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, destTypeName)
-			}
-		}
-
-	case "[]string":
-		{
-			switch src := src.(type) {
-			case string:
-				// We allow a single string to populate an array
-				v := []string{src}
-				dest.Set(reflect.ValueOf(v))
-				return nil
-			case []interface{}:
-				v := []string{}
-				for _, i := range src {
-					v = append(v, fmt.Sprintf("%v", i))
-				}
-				dest.Set(reflect.ValueOf(v))
-				return nil
-			default:
-				return fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, destTypeName)
-			}
-		}
-
-	case "*int64":
-		{
-			switch src := src.(type) {
-			case int:
-				v := int64(src)
-				dest.Set(reflect.ValueOf(&v))
-				return nil
-			case float64:
-				v := int64(src)
-				dest.Set(reflect.ValueOf(&v))
-				return nil
-			default:
-				return fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, destTypeName)
-			}
-		}
-
-	case "*bool":
-		{
-			switch src := src.(type) {
-			case bool:
-				v := bool(src)
-				dest.Set(reflect.ValueOf(&v))
-				return nil
-			default:
-				return fmt.Errorf("unhandled conversion for %q: %T -> %s", name, src, destTypeName)
-			}
-		}
-
-	default:
-		if r.SpecialCases != nil {
-			handled, err := r.SpecialCases(name, dest, src)
-			if err != nil {
-				return err
-			}
-			if handled {
-				return nil
-			}
-		}
-		return fmt.Errorf("unhandled destination type for %q: %s", name, destTypeName)
-	}
-}
-
-func (r *Unmarshaller) unmarshalMap(name string, dest Settable, src interface{}) error {
-	if src == nil {
-		return nil
-	}
-
-	glog.V(4).Infof("populateMap on type %s", BuildTypeName(dest.Type()))
-
-	destType := dest.Type()
-
-	if destType.Kind() != reflect.Map {
-		glog.Errorf("expected map type, got %v", destType)
-	}
-
-	if dest.Value.IsNil() {
-		m := reflect.MakeMap(dest.Type())
-		dest.Set(m)
-		dest = Settable{Value: m}
-	}
-
-	srcMap, ok := src.(map[string]interface{})
-	if ok {
-		for k, v := range srcMap {
-			newValue := reflect.New(destType.Elem()).Elem()
-
-			settable := Settable{
-				Value:    newValue,
-				MapValue: dest.Value,
-				MapKey:   reflect.ValueOf(k),
-			}
-			settable.Set(newValue)
-			err := r.UnmarshalSettable(name+"."+k, settable, v)
-			if err != nil {
-				return err
-			}
-		}
-		return nil
-	}
-
-	return fmt.Errorf("unexpected source type for map %q: %T", name, src)
-}
-
-func (r *Unmarshaller) UnmarshalStruct(name string, dest reflect.Value, src interface{}) error {
-	m, ok := src.(map[string]interface{})
-	if !ok {
-		return fmt.Errorf("unexpected type of source data for %q: %T", name, src)
-	}
-
-	if dest.Kind() == reflect.Ptr && !dest.IsNil() {
-		dest = dest.Elem()
-	}
-
-	if dest.Kind() != reflect.Struct {
-		return fmt.Errorf("UnmarshalStruct called on non-struct: %v", dest.Kind())
-	}
-
-	// TODO: Pre-calculate / cache?
-	destType := dest.Type()
-	fieldMap := map[string]reflect.StructField{}
-	for i := 0; i < destType.NumField(); i++ {
-		f := destType.Field(i)
-		fieldName := f.Name
-		fieldName = strings.ToLower(fieldName)
-		_, exists := fieldMap[fieldName]
-		if exists {
-			glog.Fatalf("ambiguous field name in %q: %q", destType.Name(), fieldName)
-		}
-		fieldMap[fieldName] = f
-	}
-
-	//t := dest.Type()
-	for k, v := range m {
-		k = strings.ToLower(k)
-		fieldInfo, found := fieldMap[k]
-		if !found {
-			return fmt.Errorf("unknown field %q in %q", k, name)
-		}
-		field := dest.FieldByIndex(fieldInfo.Index)
-
-		err := r.UnmarshalSettable(name+"."+k, Settable{Value: field}, v)
-		if err != nil {
-			return err
-		}
-	}
-
-	return nil
-}