helper/schema: Make nested Set(s) in List(s) work

[radeksimko]

Related to #7253 & #7254
Supersedes/fixes #7268

---

Issue explained below in (hopefully) simple ASCII art.

resource schema

.
├── TypeInt
└── TypeList
    └── TypeSet
    └── TypeString
    └── TypeInt
    └── TypeBool

resource CRUD

resourceXyzCreate() {
  d.Get("TypeInt") # OK
  d.Get("TypeList") # OK
}
resourceXyzRead() {
  d.Get("TypeInt") # OK
  d.Get("TypeList") # OK
}
resourceXyzUpdate() {
  d.Get("TypeInt") # OK
  d.Get("TypeList") # returns *Set(TypeSet=NIL, TypeString, TypeInt, TypeBool)
}

The nested TypeSet inside TypeList was set to nil and any data further nested in that Set were ignored inside Update function of a resource unless that TypeSet field was actually contained in the diff.

---

Summary (couple of whys)

• Previously there was a common exported ReadList function shared between all field readers (Diff, Map, Config), this was replaced with unexported implementations per each reader
  • it turns out each reader requires different approach, especially DiffFieldReader
  • this is a first step towards getting rid of the magical nestedConfigFieldReader
  • reader-specific function can call readPrimitive and doesn't have to perform the data-type detection anymore - theoretically should save us some CPU cycles and prevent the infinite loops (one of the reasons why nestedConfigFieldReader was there)
• readSet implementation of the Diff reader was only reading the diff and worked fine for creation/deletion of whole sets, but not so well for cases where only part of the set was being updated (contained in the diff).
  • readSet now fills in the gaps by getting the missing data from previous reader

Tests, yey

About 84% LOC of this PR are just tests covering the broken (now fixed) functionality - all contained in a separate commit.

---

Another PR will follow to fix the original CloudFront bug with data types.

[phinze]

+1 for panic here - as soon as I saw the "This should never happen" in the old shared impl i was like 💭 "we should panic then!" 😀

[radeksimko]

Right, that's more or less inherited from the original generic ReadList function, but it's still valid here too. 😄

[phinze]

Okay! This looks great @radeksimko, especially the extensive testing work.

Merge at will. 👍 🚀

[catsby]

Hello –

We've found some regressions in our tests and suspect this PR. I'm going to issue a revert (#7436), but first, here are some examples:

--- FAIL: TestAccAWSSecurityGroup_Change (25.93s)
    testing.go:255: Step 1 error: Error applying: 1 error(s) occurred:

        * aws_security_group.web: Error authorizing security group ingress rules: InvalidParameterValue: Invalid value '' for IP protocol. No protocol value given.

--- FAIL: TestAccFastlyServiceV1_gzips_basic (15.47s)
    testing.go:255: Step 1 error: Error applying: 1 error(s) occurred:

        * fastly_service_v1.foo: 400 - Bad Request
        Message: Name can't be blank

From https://travis-ci.org/hashicorp/terraform/builds/141247532 and https://travis-ci.org/hashicorp/terraform/builds/141254784, respectively.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.