From 7d249365077d94197c28c39f914d978ed13c7445 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 2 Aug 2018 08:34:19 -0400
Subject: [PATCH] updates to teh StateUpgraders

Fix documentation.
Require StateUpgraders to be add in order, and test in validation.
---
 helper/schema/resource.go      | 28 +++++++++++++---------------
 helper/schema/resource_test.go |  8 ++++++--
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/helper/schema/resource.go b/helper/schema/resource.go
index b8e012d44..01ecd4cc3 100644
--- a/helper/schema/resource.go
+++ b/helper/schema/resource.go
@@ -4,7 +4,6 @@ import (
 	"errors"
 	"fmt"
 	"log"
-	"sort"
 	"strconv"
 
 	"github.com/hashicorp/terraform/config"
@@ -49,8 +48,8 @@ type Resource struct {
 	// MigrateState is deprecated and any new changes to a resource's schema
 	// 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.
+	// still be called if the stored SchemaVersion is less than the
+	// first version of the StateUpgraders.
 	//
 	// MigrateState is responsible for updating an InstanceState with an old
 	// version to the format expected by the current version of the Schema.
@@ -69,10 +68,11 @@ type Resource struct {
 	// called specifically by Terraform when the stored schema version is less
 	// than the current SchemaVersion of the Resource.
 	//
-	// StateUpgraders map specific schema versions to an StateUpgrader
-	// function. The registered versions are expected to be consecutive values.
-	// The initial value may be greater than 0 to account for legacy schemas
-	// that weren't recorded and can be handled by MigrateState.
+	// StateUpgraders map specific schema versions to a StateUpgrader
+	// function. The registered versions are expected to be ordered,
+	// consecutive values. The initial value may be greater than 0 to account
+	// for legacy schemas that weren't recorded and can be handled by
+	// MigrateState.
 	StateUpgraders []StateUpgrader
 
 	// The functions below are the CRUD operations for this resource.
@@ -185,12 +185,15 @@ type StateUpgrader struct {
 	Type cty.Type
 
 	// Upgrade takes the JSON encoded state and the provider meta value, and
-	// upgrades the state one single schema version.
+	// upgrades the state one single schema version. The provided state is
+	// deocded into the default json types using a map[string]interface{}. It
+	// is up to the StateUpgradeFunc to ensure that the returned value can be
+	// encoded using the new schema.
 	Upgrade StateUpgradeFunc
 }
 
-// See Resource documentation.
-type StateUpgradeFunc func(map[string]interface{}, interface{}) (map[string]interface{}, error)
+// See StateUpgrader
+type StateUpgradeFunc func(rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error)
 
 // See Resource documentation.
 type CustomizeDiffFunc func(*ResourceDiff, interface{}) error
@@ -474,11 +477,6 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error
 		}
 	}
 
-	// verify state upgraders are consecutive and have registered schema types
-	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 {
diff --git a/helper/schema/resource_test.go b/helper/schema/resource_test.go
index 94a1840db..d9f8820df 100644
--- a/helper/schema/resource_test.go
+++ b/helper/schema/resource_test.go
@@ -1507,8 +1507,7 @@ func TestResource_ValidateUpgradeState(t *testing.T) {
 		t.Fatal("StateUpgraders cannot skip versions")
 	}
 
-	// add the missing version
-	// out of order upgraders should be OK
+	// add the missing version, but fail because it's still out of order
 	r.StateUpgraders = append(r.StateUpgraders, StateUpgrader{
 		Version: 1,
 		Type: cty.Object(map[string]cty.Type{
@@ -1518,6 +1517,11 @@ func TestResource_ValidateUpgradeState(t *testing.T) {
 			return m, nil
 		},
 	})
+	if err := r.InternalValidate(nil, true); err == nil {
+		t.Fatal("upgraders must be defined in order")
+	}
+
+	r.StateUpgraders[1], r.StateUpgraders[2] = r.StateUpgraders[2], r.StateUpgraders[1]
 	if err := r.InternalValidate(nil, true); err != nil {
 		t.Fatal(err)
 	}