StateUpgrade redux

It turns out that state upgrades need to be handled differently since
providers are going to be backwards compatible. This means that new
state upgrades may still be stored in the flatmap format when used wih
terraform 0.11. Because we can't account for the specific version which
could produce a legacy state, all future state upgrades need to record
the schema types for decoding.

Rather than defining a single Upgrade function for states, we now have a
list of functions, each of which handle upgrading a specific version to
the next. In practice this isn't much different from the way many
resources implement upgrades themselves, with a separate function for
each version dispatched from the MigrateState function. The only added
burden is the recording of the schema type, and we intend to supply
tools and helper function to prevent the need to copy the entire
existing schema in all cases.

helper/schema/resource.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"sort"
 	"strconv"
 
 	"github.com/hashicorp/terraform/config"
@@ -45,17 +46,8 @@ type Resource struct {
 	// their Versioning at any integer >= 1
 	SchemaVersion int
 
-	// LegacySchema is a record of the last schema version and type that
-	// existed before the addition of an UpgradeState function.
-	//
-	// This allows the resource schema to continue to evolve, while providing a
-	// record of how to decode a legacy state to be upgraded.
-	//
-	// LegacySchema is required when implementing UpgradeState.
-	LegacySchema LegacySchemaVersion
-
 	// MigrateState is deprecated and any new changes to a resource's schema
-	// should be handled by UpgradeState. Existing MigrateState implementations
+	// should be handled by StateUpgraders. Existing MigrateState implementations
 	// should remain for compatibility with existing state. MigrateState will
 	// still be called if the stored SchemaVersion is lower than the
 	// LegacySchema.Version value.
@@ -72,16 +64,16 @@ type Resource struct {
 	// needs to make any remote API calls.
 	MigrateState StateMigrateFunc
 
-	// UpgradeState is responsible for upgrading an existing state with an old
+	// StateUpgraders contains the functions responsible for upgrading an
-	// schema version to the current schema. It is called specifically by
+	// existing state with an old schema version to a newer schema. It is
-	// Terraform when the stored schema version is less than the current
+	// called specifically by Terraform when the stored schema version is less
-	// SchemaVersion of the Resource.
+	// than the current SchemaVersion of the Resource.
 	//
-	// StateUpgradeFunc takes the schema version, the state decoded using the
+	// StateUpgraders map specific schema versions to an StateUpgrader
-	// default json types in a map[string]interface{}, and the provider meta
+	// function. The registered versions are expected to be consecutive values.
-	// value. The returned map value should encode into the proper format json
+	// The initial value may be greater than 0 to account for legacy schemas
-	// to match the current provider schema.
+	// that weren't recorded and can be handled by MigrateState.
-	UpgradeState StateUpgradeFunc
+	StateUpgraders []StateUpgrader
 
 	// The functions below are the CRUD operations for this resource.
 	//
@@ -163,11 +155,6 @@ type Resource struct {
 	Timeouts *ResourceTimeout
 }
 
-type LegacySchemaVersion struct {
-	Version int
-	Type    cty.Type
-}
-
 // See Resource documentation.
 type CreateFunc func(*ResourceData, interface{}) error
 
@@ -187,8 +174,23 @@ type ExistsFunc func(*ResourceData, interface{}) (bool, error)
 type StateMigrateFunc func(
 	int, *terraform.InstanceState, interface{}) (*terraform.InstanceState, error)
 
+type StateUpgrader struct {
+	// Version is the version schema that this Upgrader will handle, converting
+	// it to Version+1.
+	Version int
+
+	// Type describes the schema that this function can upgrade. Type is
+	// required to decode the schema if the state was stored in a legacy
+	// flatmap format.
+	Type cty.Type
+
+	// Upgrade takes the JSON encoded state and the provider meta value, and
+	// upgrades the state one single schema version.
+	Upgrade StateUpgradeFunc
+}
+
 // See Resource documentation.
-type StateUpgradeFunc func(int, map[string]interface{}, interface{}) (map[string]interface{}, error)
+type StateUpgradeFunc func(map[string]interface{}, interface{}) (map[string]interface{}, error)
 
 // See Resource documentation.
 type CustomizeDiffFunc func(*ResourceDiff, interface{}) error
@@ -472,12 +474,34 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error
 		}
 	}
 
-	if r.LegacySchema.Version >= r.SchemaVersion {
+	// verify state upgraders are consecutive and have registered schema types
-		return errors.New("LegacySchema.Version cannot be >= SchemaVersion")
+	sort.Slice(r.StateUpgraders, func(i, j int) bool {
+		return r.StateUpgraders[i].Version < r.StateUpgraders[j].Version
+	})
+
+	lastVersion := -1
+	for _, u := range r.StateUpgraders {
+		if lastVersion >= 0 && u.Version-lastVersion > 1 {
+			return fmt.Errorf("missing schema version between %d and %d", lastVersion, u.Version)
+		}
+
+		if u.Version >= r.SchemaVersion {
+			return fmt.Errorf("StateUpgrader version %d is >= current version %d", u.Version, r.SchemaVersion)
+		}
+
+		if !u.Type.IsObjectType() {
+			return fmt.Errorf("StateUpgrader %d type is not cty.Object", u.Version)
+		}
+
+		if u.Upgrade == nil {
+			return fmt.Errorf("StateUpgrader %d missing StateUpgradeFunc", u.Version)
+		}
+
+		lastVersion = u.Version
 	}
 
-	if r.UpgradeState != nil && !r.LegacySchema.Type.IsObjectType() {
+	if lastVersion >= 0 && lastVersion != r.SchemaVersion-1 {
-		return fmt.Errorf("LegacySchema.Type requires a cty.Object, got: %#v", r.LegacySchema.Type)
+		return fmt.Errorf("missing StateUpgrader between %d and %d", lastVersion, r.SchemaVersion)
 	}
 
 	// Data source

helper/schema/resource_test.go

@@ -1375,7 +1375,9 @@ func TestResourceData_timeouts(t *testing.T) {
 }
 
 func TestResource_UpgradeState(t *testing.T) {
-	// Schema v2 it deals only in newfoo, which tracks foo as an int
+	// While this really only calls itself and therefore doesn't test any of
+	// the Resource code directly, it still serves as an example of registering
+	// a StateUpgrader.
 	r := &Resource{
 		SchemaVersion: 2,
 		Schema: map[string]*Schema{
@@ -1386,25 +1388,25 @@ func TestResource_UpgradeState(t *testing.T) {
 		},
 	}
 
-	r.LegacySchema.Version = 1
+	r.StateUpgraders = []StateUpgrader{
-	r.LegacySchema.Type = cty.Object(map[string]cty.Type{
+		{
-		"id":     cty.String,
+			Version: 1,
-		"oldfoo": cty.Number,
+			Type: cty.Object(map[string]cty.Type{
-	})
+				"id":     cty.String,
+				"oldfoo": cty.Number,
+			}),
+			Upgrade: func(m map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
 
-	r.UpgradeState = func(
+				oldfoo, ok := m["oldfoo"].(float64)
-		v int,
+				if !ok {
-		m map[string]interface{},
+					t.Fatalf("expected 1.2, got %#v", m["oldfoo"])
-		meta interface{}) (map[string]interface{}, error) {
+				}
+				m["newfoo"] = int(oldfoo * 10)
+				delete(m, "oldfoo")
 
-		oldfoo, ok := m["oldfoo"].(float64)
+				return m, nil
-		if !ok {
+			},
-			t.Fatalf("expected 1.2, got %#v", m["oldfoo"])
+		},
-		}
-		m["newfoo"] = int(oldfoo * 10)
-		delete(m, "oldfoo")
-
-		return m, nil
 	}
 
 	oldStateAttrs := map[string]string{
@@ -1413,11 +1415,12 @@ func TestResource_UpgradeState(t *testing.T) {
 	}
 
 	// convert the legacy flatmap state to the json equivalent
-	val, err := hcl2shim.HCL2ValueFromFlatmap(oldStateAttrs, r.LegacySchema.Type)
+	ty := r.StateUpgraders[0].Type
+	val, err := hcl2shim.HCL2ValueFromFlatmap(oldStateAttrs, ty)
 	if err != nil {
 		t.Fatal(err)
 	}
-	js, err := ctyjson.Marshal(val, r.LegacySchema.Type)
+	js, err := ctyjson.Marshal(val, ty)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1428,7 +1431,7 @@ func TestResource_UpgradeState(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	actual, err := r.UpgradeState(2, m, nil)
+	actual, err := r.StateUpgraders[0].Upgrade(m, nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -1445,7 +1448,7 @@ func TestResource_UpgradeState(t *testing.T) {
 
 func TestResource_ValidateUpgradeState(t *testing.T) {
 	r := &Resource{
-		SchemaVersion: 2,
+		SchemaVersion: 3,
 		Schema: map[string]*Schema{
 			"newfoo": &Schema{
 				Type:     TypeInt,
@@ -1458,26 +1461,78 @@ func TestResource_ValidateUpgradeState(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	r.LegacySchema.Version = 2
+	r.StateUpgraders = append(r.StateUpgraders, StateUpgrader{
-	if err := r.InternalValidate(nil, true); err == nil {
+		Version: 2,
-		t.Fatal("LegacySchema.Version cannot be >= SchemaVersion")
+		Type: cty.Object(map[string]cty.Type{
-	}
+			"id": cty.String,
-
+		}),
-	r.LegacySchema.Version = 1
+		Upgrade: func(m map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
-
+			return m, nil
-	r.UpgradeState = func(v int, m map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+		},
-		return m, nil
-	}
-
-	if err := r.InternalValidate(nil, true); err == nil {
-		t.Fatal("UpgradeState requires LegacySchema.Type")
-	}
-
-	r.LegacySchema.Type = cty.Object(map[string]cty.Type{
-		"id": cty.String,
 	})
-
 	if err := r.InternalValidate(nil, true); err != nil {
 		t.Fatal(err)
 	}
+
+	// check for missing type
+	r.StateUpgraders[0].Type = cty.Type{}
+	if err := r.InternalValidate(nil, true); err == nil {
+		t.Fatal("StateUpgrader must have type")
+	}
+	r.StateUpgraders[0].Type = cty.Object(map[string]cty.Type{
+		"id": cty.String,
+	})
+
+	// check for missing Upgrade func
+	r.StateUpgraders[0].Upgrade = nil
+	if err := r.InternalValidate(nil, true); err == nil {
+		t.Fatal("StateUpgrader must have an Upgrade func")
+	}
+	r.StateUpgraders[0].Upgrade = func(m map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+		return m, nil
+	}
+
+	// check for skipped version
+	r.StateUpgraders[0].Version = 0
+	r.StateUpgraders = append(r.StateUpgraders, StateUpgrader{
+		Version: 2,
+		Type: cty.Object(map[string]cty.Type{
+			"id": cty.String,
+		}),
+		Upgrade: func(m map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+			return m, nil
+		},
+	})
+	if err := r.InternalValidate(nil, true); err == nil {
+		t.Fatal("StateUpgraders cannot skip versions")
+	}
+
+	// add the missing version
+	// out of order upgraders should be OK
+	r.StateUpgraders = append(r.StateUpgraders, StateUpgrader{
+		Version: 1,
+		Type: cty.Object(map[string]cty.Type{
+			"id": cty.String,
+		}),
+		Upgrade: func(m map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+			return m, nil
+		},
+	})
+	if err := r.InternalValidate(nil, true); err != nil {
+		t.Fatal(err)
+	}
+
+	// can't add an upgrader for a schema >= the current version
+	r.StateUpgraders = append(r.StateUpgraders, StateUpgrader{
+		Version: 3,
+		Type: cty.Object(map[string]cty.Type{
+			"id": cty.String,
+		}),
+		Upgrade: func(m map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+			return m, nil
+		},
+	})
+	if err := r.InternalValidate(nil, true); err == nil {
+		t.Fatal("StateUpgraders cannot have a version >= current SchemaVersion")
+	}
 }