Fix SingleNestedAttribute validation failure
#118
Conversation
There was a problem hiding this comment.
Could we also add a .changelog entry? https://github.com/hashicorp/engineering-docs/blob/master/terraform/sdk/changelog.md
(I know that link is to a private repo, I'll open an issue to get it added to our contributing guide)
tfsdk/attribute.go
Outdated
|
|
||
| resp.Diagnostics = nestedAttrResp.Diagnostics | ||
| if len(o.Attrs) > 0 { |
There was a problem hiding this comment.
Is there a reason this became necessary?
The for loop should cover this case, by just not entering the for loop, which it looks like is what this does?
Unless this is to cover the case where the schema has attributes defined, but Terraform sends a value omitting those attributes, which as far as I know, should never happen?
There was a problem hiding this comment.
I think, as @kmoe mentions in #112 (comment), it's the call to tftypes.WalkAttributePath in Config.GetAttribute called from Attribute.validate on the nested Null Attribute value that fails.
There was a problem hiding this comment.
Could we use o.Null then, instead of checking the number of attributes?
There was a problem hiding this comment.
Let me take a look...
There was a problem hiding this comment.
Yes !o.Null works just as well.
Commit pushed.
There was a problem hiding this comment.
Just tested this and hit a failure with unknown attributes; it looks like you'll need to add a check for !o.Unknown too.
tfsdk/attribute.go
Outdated
| nestedAttr.validate(ctx, nestedAttrReq, nestedAttrResp) | ||
| if !ok { | ||
| err := fmt.Errorf("unknown attribute value type (%T) for nesting mode (%T) at path: %s", req.AttributeConfig, nm, req.AttributePath) | ||
| resp.Diagnostics = append(resp.Diagnostics, &tfprotov6.Diagnostic{ |
There was a problem hiding this comment.
Really sorry to do this to you, @ewbankkit, but could you update to use the helpers added by #110? Just want to keep our consistency @bflad worked so hard to introduce. 😦
Sorry, ping me in chat when you do that and we'll get it merged straightaway before something else comes up. Thanks so much for the patience on this.
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes #112.
Don't attempt validation if the attribute is missing from configuration.