Allow nested aliases #118
Allow nested aliases #118
Conversation
There was a problem hiding this comment.
Thank you for breaking this into a separate PR! I originally misunderstood your initial PR description, but #117 cleared that up 😆.
I left some questions and general feedback!
General Feedback
- For documentation, I don't think you need to do anything in this PR.... We currently have usage docs that have examples of aliasing. IMO, another simple example of aliasing into a nested attribute would work, however those docs aren't in this repo. Whenever this gets merged, I will open a PR adding an example to that documentation site.
- We will need a changelog with this change, similar guidance from the other PR, here is some documentation on how to create one: https://github.com/hashicorp/terraform-plugin-framework-validators/blob/main/.github/CONTRIBUTING.md#changelog. I'd also consider this PR an enhancement, maybe with a message of
all: Added support for aliasing parameters to nested attributes - We will definitely need some tests for these changes. I'd prefer simple unit tests on all the
attrmapper/*_nested.gofiles and then maybe some more "integration" tests onresource_mapper.goanddatasource_mapper.go. Similar to the other PR, you'll want to runmake testdatato update any golden file tests.
| // No more path to traverse, apply merge, bidirectional | ||
| overriddenTarget, err := attribute.Merge(target) | ||
| errResult = errors.Join(errResult, err) | ||
| overriddenTarget, err = overriddenTarget.Merge(attribute) | ||
| errResult = errors.Join(errResult, err) |
There was a problem hiding this comment.
This is interesting, can you describe why this merge logic needs to happen bidirectionally? Would love to see this codified into a unit test with maybe a comment if this bidirectional merge is required.
| func (targetSlice DataSourceAttributes) MergeAttribute(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttributes, error) { | ||
| var errResult error | ||
| if len(path) == 0 { | ||
| return targetSlice, errResult | ||
| } | ||
| for i, target := range targetSlice { | ||
| if target.GetName() == path[0] { | ||
|
|
||
| if len(path) > 1 { | ||
| nestedTarget, ok := target.(DataSourceNestedAttribute) | ||
| if !ok { | ||
| // TODO: error? there is a nested override for an attribute that is not a nested type | ||
| break | ||
| } | ||
|
|
||
| // The attribute we need to override is deeper nested, move up | ||
| nextPath := path[1:] | ||
|
|
||
| overriddenTarget, err := nestedTarget.NestedMerge(nextPath, attribute, intermediateComputability) | ||
| errResult = errors.Join(errResult, err) | ||
|
|
||
| targetSlice[i] = overriddenTarget | ||
|
|
||
| } else { | ||
| // No more path to traverse, apply merge, bidirectional | ||
| overriddenTarget, err := attribute.Merge(target) | ||
| errResult = errors.Join(errResult, err) | ||
| overriddenTarget, err = overriddenTarget.Merge(attribute) | ||
| errResult = errors.Join(errResult, err) | ||
|
|
||
| targetSlice[i] = overriddenTarget | ||
| } | ||
|
|
||
| break | ||
| } | ||
| } | ||
|
|
||
| return targetSlice, errResult | ||
| } |
There was a problem hiding this comment.
This logic will also need to take into account the scenario of a new parameter being merged (not aliased), similar to the original Merge function.
Example:
isNewAttribute := true
for i, target := range targetSlice {
if target.GetName() == path[0] {
if len(path) > 1 {
// Recurse logic
} else {
// Merge logic
}
isNewAttribute = false
break
}
}
if isNewAttribute {
// Add this back to the original slice to avoid adding duplicate attributes from different mergeSlices
targetSlice = append(targetSlice, attribute)
}| @@ -58,6 +60,46 @@ func (targetSlice DataSourceAttributes) Merge(mergeSlices ...DataSourceAttribute | |||
| return targetSlice, errResult | |||
| } | |||
|
|
|||
| func (targetSlice DataSourceAttributes) MergeAttribute(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttributes, error) { | |||
There was a problem hiding this comment.
Perhaps we can give this a more explicit name like MergeAttributeAtPath
| @@ -20,6 +21,7 @@ type DataSourceAttribute interface { | |||
|
|
|||
| type DataSourceNestedAttribute interface { | |||
| ApplyNestedOverride([]string, explorer.Override) (DataSourceAttribute, error) | |||
| NestedMerge([]string, DataSourceAttribute, schema.ComputedOptionalRequired) (DataSourceAttribute, error) | |||
There was a problem hiding this comment.
Maybe a more explicit name like NestedMergeAtPath? (I'm not particularly fond of how our original code is named 😆)
| if err == nil { | ||
| a.ComputedOptionalRequired = intermediateComputability | ||
| } |
There was a problem hiding this comment.
I'm assuming the intention here is to bubble up the computability? So if a required parameter is aliased into a nested list that is computed, the nested list is marked as required?
Would like to have a unit test that codifies this behavior
| // TODO: currently, no errors can be returned from merging, but in the future we should consider raising errors/warnings for unexpected scenarios, like type mismatches between attribute schemas | ||
| dataSourceAttributes, _ := readParameterAttributes.Merge(readResponseAttributes) | ||
| dataSourceAttributes := readResponseAttributes |
There was a problem hiding this comment.
I think we can remove this renaming of the variable and maybe just change readResponseAttributes -> dataSourceAttributes declared on line 85
| } | ||
| } | ||
|
|
||
| // TODO: currently, no errors can be returned from merging, but in the future we should consider raising errors/warnings for unexpected scenarios, like type mismatches between attribute schemas | ||
| resourceAttributes, _ := createRequestAttributes.Merge(createResponseAttributes, readResponseAttributes, readParameterAttributes) | ||
| resourceAttributes, _ := createRequestAttributes.Merge(createResponseAttributes, readResponseAttributes) |
There was a problem hiding this comment.
With the introduction of "common" parameters in #114, it's possible a parameter could be intended for a nested attribute from the create response/request as well.
I think to avoid problems with that, we can move this final merge to happen before the parameters are merged, i.e. before line 143. And then update line 183 to merge with resourceAttributes.
So the order would be:
- Merge
createRequestAttributes->createResponseAttributes->readResponseAttributes. Resulting inresourceAttributes - Then merge each individual parameter attribute into
resourceAttributes
There was a problem hiding this comment.
I only left comments on this file, but the same feedback applies to internal/mapper/attrmapper/resource_attributes.go
randomswdev
force-pushed
the
override-nested-parameters
branch
from
f1d72c3 to
7ffa6d4
Compare
Addresses issue #117