legacy provider needs to handle StateUpgraders

In order to not require state migrations to be supported in both
MigrateState and StateUpgraders, the legacy provider codepath needs to
handle the StateUpgraders transparently during Refresh.

helper/schema/core_schema_test.go

@@ -4,17 +4,11 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/configs/configschema"
 )
 
-var (
-	equateEmpty  = cmpopts.EquateEmpty()
-	typeComparer = cmp.Comparer(cty.Type.Equals)
-)
-
 // add the implicit "id" attribute for test resources
 func testResource(block *configschema.Block) *configschema.Block {
 	if block.Attributes == nil {

helper/schema/resource.go

@@ -340,6 +340,59 @@ func (r *Resource) ReadDataApply(
 	return r.recordCurrentSchemaVersion(state), err
 }
 
+// RefreshWithoutUpgrade reads the instance state, but does not call
+// MigrateState or the StateUpgraders, since those are now invoked in a
+// separate API call.
+// RefreshWithoutUpgrade is part of the new plugin shims.
+func (r *Resource) RefreshWithoutUpgrade(
+	s *terraform.InstanceState,
+	meta interface{}) (*terraform.InstanceState, error) {
+	// If the ID is already somehow blank, it doesn't exist
+	if s.ID == "" {
+		return nil, nil
+	}
+
+	rt := ResourceTimeout{}
+	if _, ok := s.Meta[TimeoutKey]; ok {
+		if err := rt.StateDecode(s); err != nil {
+			log.Printf("[ERR] Error decoding ResourceTimeout: %s", err)
+		}
+	}
+
+	if r.Exists != nil {
+		// Make a copy of data so that if it is modified it doesn't
+		// affect our Read later.
+		data, err := schemaMap(r.Schema).Data(s, nil)
+		data.timeouts = &rt
+
+		if err != nil {
+			return s, err
+		}
+
+		exists, err := r.Exists(data, meta)
+		if err != nil {
+			return s, err
+		}
+		if !exists {
+			return nil, nil
+		}
+	}
+
+	data, err := schemaMap(r.Schema).Data(s, nil)
+	data.timeouts = &rt
+	if err != nil {
+		return s, err
+	}
+
+	err = r.Read(data, meta)
+	state := data.State()
+	if state != nil && state.ID == "" {
+		state = nil
+	}
+
+	return r.recordCurrentSchemaVersion(state), err
+}
+
 // Refresh refreshes the state of the resource.
 func (r *Resource) Refresh(
 	s *terraform.InstanceState,
@@ -375,12 +428,10 @@ func (r *Resource) Refresh(
 		}
 	}
 
-	needsMigration, stateSchemaVersion := r.checkSchemaVersion(s)
-	if needsMigration && r.MigrateState != nil {
-		s, err := r.MigrateState(stateSchemaVersion, s, meta)
-		if err != nil {
-			return s, err
-		}
+	// there may be new StateUpgraders that need to be run
+	s, err := r.upgradeState(s, meta)
+	if err != nil {
+		return s, err
 	}
 
 	data, err := schemaMap(r.Schema).Data(s, nil)
@@ -398,6 +449,72 @@ func (r *Resource) Refresh(
 	return r.recordCurrentSchemaVersion(state), err
 }
 
+func (r *Resource) upgradeState(s *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
+	var err error
+
+	needsMigration, stateSchemaVersion := r.checkSchemaVersion(s)
+	migrate := needsMigration && r.MigrateState != nil
+
+	if migrate {
+		s, err = r.MigrateState(stateSchemaVersion, s, meta)
+		if err != nil {
+			return s, err
+		}
+	}
+
+	if len(r.StateUpgraders) == 0 {
+		return s, nil
+	}
+
+	// If we ran MigrateState, then the stateSchemaVersion value is no longer
+	// correct. We can expect the first upgrade function to be the correct
+	// schema type version.
+	if migrate {
+		stateSchemaVersion = r.StateUpgraders[0].Version
+	}
+
+	schemaType := r.CoreConfigSchema().ImpliedType()
+	// find the expected type to convert the state
+	for _, upgrader := range r.StateUpgraders {
+		if stateSchemaVersion == upgrader.Version {
+			schemaType = upgrader.Type
+		}
+	}
+
+	// StateUpgraders only operate on the new JSON format state, so the state
+	// need to be converted.
+	stateVal, err := StateValueFromInstanceState(s, schemaType)
+	if err != nil {
+		return nil, err
+	}
+
+	jsonState, err := StateValueToJSONMap(stateVal, schemaType)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, upgrader := range r.StateUpgraders {
+		if stateSchemaVersion != upgrader.Version {
+			continue
+		}
+
+		jsonState, err = upgrader.Upgrade(jsonState, meta)
+		if err != nil {
+			return nil, err
+		}
+		stateSchemaVersion++
+	}
+
+	// now we need to re-flatmap the new state
+	stateVal, err = JSONMapToStateValue(jsonState, r.CoreConfigSchema())
+	if err != nil {
+		return nil, err
+	}
+
+	s = InstanceStateFromStateValue(stateVal, r.SchemaVersion)
+	return s, nil
+}
+
 // InternalValidate should be called to validate the structure
 // of the resource.
 //
@@ -603,7 +720,15 @@ func (r *Resource) checkSchemaVersion(is *terraform.InstanceState) (bool, int) {
 	}
 
 	stateSchemaVersion, _ := strconv.Atoi(rawString)
-	return stateSchemaVersion < r.SchemaVersion, stateSchemaVersion
+
+	// Don't run MigrateState if the version is handled by a StateUpgrader,
+	// since StateMigrateFuncs are not required to handle unknown versions
+	maxVersion := r.SchemaVersion
+	if len(r.StateUpgraders) > 0 {
+		maxVersion = r.StateUpgraders[0].Version
+	}
+
+	return stateSchemaVersion < maxVersion, stateSchemaVersion
 }
 
 func (r *Resource) recordCurrentSchemaVersion(

helper/schema/resource_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/hcl2shim"
 	"github.com/hashicorp/terraform/terraform"
@@ -1540,3 +1541,156 @@ func TestResource_ValidateUpgradeState(t *testing.T) {
 		t.Fatal("StateUpgraders cannot have a version >= current SchemaVersion")
 	}
 }
+
+// The legacy provider will need to be able to handle both types of schema
+// transformations, which has been retrofitted into the Refresh method.
+func TestResource_migrateAndUpgrade(t *testing.T) {
+	r := &Resource{
+		SchemaVersion: 4,
+		Schema: map[string]*Schema{
+			"four": {
+				Type:     TypeInt,
+				Required: true,
+			},
+		},
+		// this MigrateState will take the state to version 2
+		MigrateState: func(v int, is *terraform.InstanceState, _ interface{}) (*terraform.InstanceState, error) {
+			switch v {
+			case 0:
+				_, ok := is.Attributes["zero"]
+				if !ok {
+					return nil, fmt.Errorf("zero not found in %#v", is.Attributes)
+				}
+				is.Attributes["one"] = "1"
+				delete(is.Attributes, "zero")
+				fallthrough
+			case 1:
+				_, ok := is.Attributes["one"]
+				if !ok {
+					return nil, fmt.Errorf("one not found in %#v", is.Attributes)
+				}
+				is.Attributes["two"] = "2"
+				delete(is.Attributes, "one")
+			default:
+				return nil, fmt.Errorf("invalid schema version %d", v)
+			}
+			return is, nil
+		},
+	}
+
+	r.Read = func(d *ResourceData, m interface{}) error {
+		return d.Set("four", 4)
+	}
+
+	r.StateUpgraders = []StateUpgrader{
+		{
+			Version: 2,
+			Type: cty.Object(map[string]cty.Type{
+				"id":  cty.String,
+				"two": cty.Number,
+			}),
+			Upgrade: func(m map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
+				_, ok := m["two"].(float64)
+				if !ok {
+					return nil, fmt.Errorf("two not found in %#v", m)
+				}
+				m["three"] = float64(3)
+				delete(m, "two")
+				return m, nil
+			},
+		},
+		{
+			Version: 3,
+			Type: cty.Object(map[string]cty.Type{
+				"id":    cty.String,
+				"three": cty.Number,
+			}),
+			Upgrade: func(m map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
+				_, ok := m["three"].(float64)
+				if !ok {
+					return nil, fmt.Errorf("three not found in %#v", m)
+				}
+				m["four"] = float64(4)
+				delete(m, "three")
+				return m, nil
+			},
+		},
+	}
+
+	testStates := []*terraform.InstanceState{
+		{
+			ID: "bar",
+			Attributes: map[string]string{
+				"id":   "bar",
+				"zero": "0",
+			},
+			Meta: map[string]interface{}{
+				"schema_version": "0",
+			},
+		},
+		{
+			ID: "bar",
+			Attributes: map[string]string{
+				"id":  "bar",
+				"one": "1",
+			},
+			Meta: map[string]interface{}{
+				"schema_version": "1",
+			},
+		},
+		{
+			ID: "bar",
+			Attributes: map[string]string{
+				"id":  "bar",
+				"two": "2",
+			},
+			Meta: map[string]interface{}{
+				"schema_version": "2",
+			},
+		},
+		{
+			ID: "bar",
+			Attributes: map[string]string{
+				"id":    "bar",
+				"three": "3",
+			},
+			Meta: map[string]interface{}{
+				"schema_version": "3",
+			},
+		},
+		{
+			ID: "bar",
+			Attributes: map[string]string{
+				"id":   "bar",
+				"four": "4",
+			},
+			Meta: map[string]interface{}{
+				"schema_version": "4",
+			},
+		},
+	}
+
+	for i, s := range testStates {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			newState, err := r.Refresh(s, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			expected := &terraform.InstanceState{
+				ID: "bar",
+				Attributes: map[string]string{
+					"id":   "bar",
+					"four": "4",
+				},
+				Meta: map[string]interface{}{
+					"schema_version": "4",
+				},
+			}
+
+			if !cmp.Equal(expected, newState, equateEmpty) {
+				t.Fatal(cmp.Diff(expected, newState, equateEmpty))
+			}
+		})
+	}
+}