helper/schema: Add Resource type flags for disabling legacy type system protocol flags

.changes/unreleased/ENHANCEMENTS-20230808-111940.yaml

@@ -0,0 +1,8 @@
+kind: ENHANCEMENTS
+body: 'helper/schema: Added `Resource` type `EnableLegacyTypeSystemApplyErrors`
+  field, which will prevent Terraform from demoting data consistency errors
+  to warning logs during `ApplyResourceChange` (`Create`, `Update`, and `Delete`)
+  operations with the resource'
+time: 2023-08-08T11:19:40.137598-04:00
+custom:
+  Issue: "1227"

.changes/unreleased/ENHANCEMENTS-20230808-111941.yaml

@@ -0,0 +1,7 @@
+kind: ENHANCEMENTS
+body: 'helper/schema: Added `Resource` type `EnableLegacyTypeSystemPlanErrors`
+  field, which can be used to prevent Terraform from demoting data consistency
+  errors to warning logs during `PlanResourceChange` operations with the resource'
+time: 2023-08-08T11:19:41.137598-04:00
+custom:
+  Issue: "1227"

.changes/unreleased/NOTES-20230808-103724.yaml

@@ -0,0 +1,12 @@
+kind: NOTES
+body: 'helper/schema: The `Resource` type `EnableApplyLegacyTypeSystemErrors`
+  and `EnablePlanLegacyTypeSystemErrors` fields can be enabled to more easily
+  discover resource data consistency errors which Terraform would normally
+  demote to warning logs. Before enabling the flag in a production release for
+  a resource, the resource should be exhaustively acceptance tested as there may
+  be unrecoverable error situations for practitioners. It is recommended to
+  first enable and test in environments where it is easy to clean up resources,
+  potentially outside of Terraform.'
+time: 2023-08-08T10:37:24.017324-04:00
+custom:
+  Issue: "1227"

helper/schema/grpc_provider.go

@@ -662,20 +662,23 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot
 	ctx = logging.InitContext(ctx)
 	resp := &tfprotov5.PlanResourceChangeResponse{}
 
+	res, ok := s.provider.ResourcesMap[req.TypeName]
+	if !ok {
+		resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("unknown resource type: %s", req.TypeName))
+		return resp, nil
+	}
+	schemaBlock := s.getResourceSchemaBlock(req.TypeName)
+
 	// This is a signal to Terraform Core that we're doing the best we can to
 	// shim the legacy type system of the SDK onto the Terraform type system
 	// but we need it to cut us some slack. This setting should not be taken
 	// forward to any new SDK implementations, since setting it prevents us
 	// from catching certain classes of provider bug that can lead to
 	// confusing downstream errors.
-	resp.UnsafeToUseLegacyTypeSystem = true //nolint:staticcheck
-
-	res, ok := s.provider.ResourcesMap[req.TypeName]
-	if !ok {
-		resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("unknown resource type: %s", req.TypeName))
-		return resp, nil
+	if !res.EnableLegacyTypeSystemPlanErrors {
+		//nolint:staticcheck // explicitly for this SDK
+		resp.UnsafeToUseLegacyTypeSystem = true
 	}
-	schemaBlock := s.getResourceSchemaBlock(req.TypeName)
 
 	priorStateVal, err := msgpack.Unmarshal(req.PriorState.MsgPack, schemaBlock.ImpliedType())
 	if err != nil {
@@ -1075,7 +1078,10 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro
 	// forward to any new SDK implementations, since setting it prevents us
 	// from catching certain classes of provider bug that can lead to
 	// confusing downstream errors.
-	resp.UnsafeToUseLegacyTypeSystem = true //nolint:staticcheck
+	if !res.EnableLegacyTypeSystemApplyErrors {
+		//nolint:staticcheck // explicitly for this SDK
+		resp.UnsafeToUseLegacyTypeSystem = true
+	}
 
 	return resp, nil
 }

helper/schema/grpc_provider_test.go

@@ -575,77 +575,113 @@ func TestUpgradeState_flatmapStateMissingMigrateState(t *testing.T) {
 }
 
 func TestPlanResourceChange(t *testing.T) {
-	r := &Resource{
-		SchemaVersion: 4,
-		Schema: map[string]*Schema{
-			"foo": {
-				Type:     TypeInt,
-				Optional: true,
+	t.Parallel()
+
+	testCases := map[string]struct {
+		TestResource                   *Resource
+		ExpectedUnsafeLegacyTypeSystem bool
+	}{
+		"basic": {
+			TestResource: &Resource{
+				SchemaVersion: 4,
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeInt,
+						Optional: true,
+					},
+				},
+			},
+			ExpectedUnsafeLegacyTypeSystem: true,
+		},
+		"EnableLegacyTypeSystemPlanErrors": {
+			TestResource: &Resource{
+				EnableLegacyTypeSystemPlanErrors: true,
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeInt,
+						Optional: true,
+					},
+				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: false,
 		},
 	}
 
-	server := NewGRPCProviderServer(&Provider{
-		ResourcesMap: map[string]*Resource{
-			"test": r,
-		},
-	})
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
 
-	schema := r.CoreConfigSchema()
-	priorState, err := msgpack.Marshal(cty.NullVal(schema.ImpliedType()), schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
 
-	// A propsed state with only the ID unknown will produce a nil diff, and
-	// should return the propsed state value.
-	proposedVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
-		"id": cty.UnknownVal(cty.String),
-	}))
-	if err != nil {
-		t.Fatal(err)
-	}
-	proposedState, err := msgpack.Marshal(proposedVal, schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+			server := NewGRPCProviderServer(&Provider{
+				ResourcesMap: map[string]*Resource{
+					"test": testCase.TestResource,
+				},
+			})
 
-	config, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
-		"id": cty.NullVal(cty.String),
-	}))
-	if err != nil {
-		t.Fatal(err)
-	}
-	configBytes, err := msgpack.Marshal(config, schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+			schema := testCase.TestResource.CoreConfigSchema()
+			priorState, err := msgpack.Marshal(cty.NullVal(schema.ImpliedType()), schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	testReq := &tfprotov5.PlanResourceChangeRequest{
-		TypeName: "test",
-		PriorState: &tfprotov5.DynamicValue{
-			MsgPack: priorState,
-		},
-		ProposedNewState: &tfprotov5.DynamicValue{
-			MsgPack: proposedState,
-		},
-		Config: &tfprotov5.DynamicValue{
-			MsgPack: configBytes,
-		},
-	}
+			// A propsed state with only the ID unknown will produce a nil diff, and
+			// should return the propsed state value.
+			proposedVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
+				"id": cty.UnknownVal(cty.String),
+			}))
+			if err != nil {
+				t.Fatal(err)
+			}
+			proposedState, err := msgpack.Marshal(proposedVal, schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	resp, err := server.PlanResourceChange(context.Background(), testReq)
-	if err != nil {
-		t.Fatal(err)
-	}
+			config, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
+				"id": cty.NullVal(cty.String),
+			}))
+			if err != nil {
+				t.Fatal(err)
+			}
+			configBytes, err := msgpack.Marshal(config, schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	plannedStateVal, err := msgpack.Unmarshal(resp.PlannedState.MsgPack, schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+			testReq := &tfprotov5.PlanResourceChangeRequest{
+				TypeName: "test",
+				PriorState: &tfprotov5.DynamicValue{
+					MsgPack: priorState,
+				},
+				ProposedNewState: &tfprotov5.DynamicValue{
+					MsgPack: proposedState,
+				},
+				Config: &tfprotov5.DynamicValue{
+					MsgPack: configBytes,
+				},
+			}
 
-	if !cmp.Equal(proposedVal, plannedStateVal, valueComparer) {
-		t.Fatal(cmp.Diff(proposedVal, plannedStateVal, valueComparer))
+			resp, err := server.PlanResourceChange(context.Background(), testReq)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			plannedStateVal, err := msgpack.Unmarshal(resp.PlannedState.MsgPack, schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if !cmp.Equal(proposedVal, plannedStateVal, valueComparer) {
+				t.Fatal(cmp.Diff(proposedVal, plannedStateVal, valueComparer))
+			}
+
+			//nolint:staticcheck // explicitly for this SDK
+			if testCase.ExpectedUnsafeLegacyTypeSystem != resp.UnsafeToUseLegacyTypeSystem {
+				//nolint:staticcheck // explicitly for this SDK
+				t.Fatalf("expected UnsafeLegacyTypeSystem %t, got: %t", testCase.ExpectedUnsafeLegacyTypeSystem, resp.UnsafeToUseLegacyTypeSystem)
+			}
+		})
 	}
 }
 
@@ -730,12 +766,13 @@ func TestPlanResourceChange_bigint(t *testing.T) {
 }
 
 func TestApplyResourceChange(t *testing.T) {
-	testCases := []struct {
-		Description  string
-		TestResource *Resource
+	t.Parallel()
+
+	testCases := map[string]struct {
+		TestResource                   *Resource
+		ExpectedUnsafeLegacyTypeSystem bool
 	}{
-		{
-			Description: "Create",
+		"Create": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -749,9 +786,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "CreateContext",
+		"CreateContext": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -765,9 +802,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "CreateWithoutTimeout",
+		"CreateWithoutTimeout": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -781,9 +818,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "Create_cty",
+		"Create_cty": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -806,9 +843,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "CreateContext_SchemaFunc",
+		"CreateContext_SchemaFunc": {
 			TestResource: &Resource{
 				SchemaFunc: func() map[string]*Schema {
 					return map[string]*Schema{
@@ -823,12 +860,32 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
+		},
+		"EnableLegacyTypeSystemApplyErrors": {
+			TestResource: &Resource{
+				EnableLegacyTypeSystemApplyErrors: true,
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeInt,
+						Optional: true,
+					},
+				},
+				CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics {
+					rd.SetId("bar")
+					return nil
+				},
+			},
+			ExpectedUnsafeLegacyTypeSystem: false,
 		},
 	}
 
-	for _, testCase := range testCases {
-		testCase := testCase
-		t.Run(testCase.Description, func(t *testing.T) {
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
 			server := NewGRPCProviderServer(&Provider{
 				ResourcesMap: map[string]*Resource{
 					"test": testCase.TestResource,
@@ -892,6 +949,12 @@ func TestApplyResourceChange(t *testing.T) {
 			if id != "bar" {
 				t.Fatalf("incorrect final state: %#v\n", newStateVal)
 			}
+
+			//nolint:staticcheck // explicitly for this SDK
+			if testCase.ExpectedUnsafeLegacyTypeSystem != resp.UnsafeToUseLegacyTypeSystem {
+				//nolint:staticcheck // explicitly for this SDK
+				t.Fatalf("expected UnsafeLegacyTypeSystem %t, got: %t", testCase.ExpectedUnsafeLegacyTypeSystem, resp.UnsafeToUseLegacyTypeSystem)
+			}
 		})
 	}
 }