helper/schema: Use a more targeted shim for nested set diff applying

We previously attempted to make the special diff apply behavior for nested
sets of objects work with attribute mode by totally discarding attribute
mode for all shims.

In practice, that is too broad a solution: there are lots of other shimming
behaviors that we _don't_ want when attribute mode is enabled. In
particular, we need to make sure that the difference between null and
empty can be seen in configuration.

As a compromise then, we will give all of the shims access to the real
ConfigMode and then do a more specialized fixup within the diff-apply
logic: we'll construct a synthetic nested block schema and then use that
to run our existing logic to deal with nested sets of objects, while
using the previous behavior in all other cases.

In effect, this means that the special new behavior only applies when the
provider uses the opt-in ConfigMode setting on a particular attribute,
and thus this change has much less risk of causing broad, unintended
regressions elsewhere.
This commit is contained in:
Martin Atkins 2019-04-16 15:48:37 -07:00
parent 5f29339121
commit 861a2ebf26
5 changed files with 64 additions and 6 deletions

View File

@ -4,6 +4,7 @@ import (
"reflect" "reflect"
"testing" "testing"
"github.com/davecgh/go-spew/spew"
"github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/terraform" "github.com/hashicorp/terraform/terraform"
) )
@ -136,6 +137,6 @@ func TestDiffApply_set(t *testing.T) {
} }
if !reflect.DeepEqual(attrs, expected) { if !reflect.DeepEqual(attrs, expected) {
t.Fatalf("\nexpected: %#v\ngot: %#v\n", expected, attrs) t.Fatalf("wrong result\ngot: %s\nwant: %s\n", spew.Sdump(attrs), spew.Sdump(expected))
} }
} }

View File

@ -18,6 +18,7 @@ func testResourceConfigMode() *schema.Resource {
Type: schema.TypeList, Type: schema.TypeList,
ConfigMode: schema.SchemaConfigModeAttr, ConfigMode: schema.SchemaConfigModeAttr,
Optional: true, Optional: true,
Computed: true,
Elem: &schema.Resource{ Elem: &schema.Resource{
Schema: map[string]*schema.Schema{ Schema: map[string]*schema.Schema{
"foo": { "foo": {

View File

@ -99,6 +99,20 @@ resource "test_resource_config_mode" "foo" {
}, },
resource.TestStep{ resource.TestStep{
Config: strings.TrimSpace(` Config: strings.TrimSpace(`
resource "test_resource_config_mode" "foo" {
resource_as_attr_dynamic = [
{
},
]
}
`),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr.#", "1"),
resource.TestCheckResourceAttr("test_resource_config_mode.foo", "resource_as_attr_dynamic.#", "1"),
),
},
resource.TestStep{
Config: strings.TrimSpace(`
resource "test_resource_config_mode" "foo" { resource "test_resource_config_mode" "foo" {
resource_as_attr = [] resource_as_attr = []
resource_as_attr_dynamic = [] resource_as_attr_dynamic = []

View File

@ -133,9 +133,10 @@ func LegacyResourceSchema(r *Resource) *Resource {
return newResource return newResource
} }
// LegacySchema takes a *Schema and returns a deep copy with 0.12 specific // LegacySchema takes a *Schema and returns a deep copy with some 0.12-specific
// features removed. This is used by the shims to get a configschema that // features disabled. This is used by the shims to get a configschema that
// directly matches the structure of the schema.Resource. // better reflects the given schema.Resource, without any adjustments we
// make for when sending a schema to Terraform Core.
func LegacySchema(s *Schema) *Schema { func LegacySchema(s *Schema) *Schema {
if s == nil { if s == nil {
return nil return nil
@ -143,7 +144,6 @@ func LegacySchema(s *Schema) *Schema {
// start with a shallow copy // start with a shallow copy
newSchema := new(Schema) newSchema := new(Schema)
*newSchema = *s *newSchema = *s
newSchema.ConfigMode = SchemaConfigModeAuto
newSchema.SkipCoreTypeCheck = false newSchema.SkipCoreTypeCheck = false
switch e := newSchema.Elem.(type) { switch e := newSchema.Elem.(type) {

View File

@ -646,8 +646,10 @@ func (d *InstanceDiff) applyBlockDiff(path []string, attrs map[string]string, sc
func (d *InstanceDiff) applyAttrDiff(path []string, attrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) { func (d *InstanceDiff) applyAttrDiff(path []string, attrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) {
ty := attrSchema.Type ty := attrSchema.Type
switch { switch {
case ty.IsListType(), ty.IsTupleType(), ty.IsMapType(), ty.IsSetType(): case ty.IsListType(), ty.IsTupleType(), ty.IsMapType():
return d.applyCollectionDiff(path, attrs, attrSchema) return d.applyCollectionDiff(path, attrs, attrSchema)
case ty.IsSetType():
return d.applySetDiff(path, attrs, attrSchema)
default: default:
return d.applySingleAttrDiff(path, attrs, attrSchema) return d.applySingleAttrDiff(path, attrs, attrSchema)
} }
@ -873,6 +875,46 @@ func (d *InstanceDiff) applyCollectionDiff(path []string, attrs map[string]strin
return result, nil return result, nil
} }
func (d *InstanceDiff) applySetDiff(path []string, attrs map[string]string, attrSchema *configschema.Attribute) (map[string]string, error) {
// We only need this special behavior for sets of object.
if !attrSchema.Type.ElementType().IsObjectType() {
// The normal collection apply behavior will work okay for this one, then.
return d.applyCollectionDiff(path, attrs, attrSchema)
}
// When we're dealing with a set of an object type we actually want to
// use our normal _block type_ apply behaviors, so we'll construct ourselves
// a synthetic schema that treats the object type as a block type and
// then delegate to our block apply method.
synthSchema := &configschema.Block{
Attributes: make(map[string]*configschema.Attribute),
}
for name, ty := range attrSchema.Type.ElementType().AttributeTypes() {
// We can safely make everything into an attribute here because in the
// event that there are nested set attributes we'll end up back in
// here again recursively and can then deal with the next level of
// expansion.
synthSchema.Attributes[name] = &configschema.Attribute{
Type: ty,
Optional: true,
}
}
parentPath := path[:len(path)-1]
childName := path[len(path)-1]
containerSchema := &configschema.Block{
BlockTypes: map[string]*configschema.NestedBlock{
childName: {
Nesting: configschema.NestingSet,
Block: *synthSchema,
},
},
}
return d.applyBlockDiff(parentPath, attrs, containerSchema)
}
// countFlatmapContainerValues returns the number of values in the flatmapped container // countFlatmapContainerValues returns the number of values in the flatmapped container
// (set, map, list) indexed by key. The key argument is expected to include the // (set, map, list) indexed by key. The key argument is expected to include the
// trailing ".#", or ".%". // trailing ".#", or ".%".