helper/validation: Prevented panics with ToDiagFunc() function when used inside Schema type Elem field
#915
Conversation
… used inside `Schema` type `Elem` field Reference: #734
bflad
added
bug
| ws, es := validator(i, attr.Name) | ||
| // A practitioner-friendly key for any SchemaValidateFunc output. | ||
| // Generally this should be the last attribute name on the path. | ||
| // If not found for some unexpected reason, an empty string is fine |
There was a problem hiding this comment.
Can you provide a bit of details about why it wouldn't not be, in some cases, that the last element in the path is not the one that implements GetAttrStep?
And if it's not, then what are those? IndexStep?
There was a problem hiding this comment.
FYI I dug a bit into go-cty to try to understand this a bit: I do get that those *Step structs are representation of operations that can be done on a Path, but I don't fully follow how they get assembled and why we would hit a scenario like the one described in the comment you added.
There was a problem hiding this comment.
why it wouldn't not be, in some cases, that the last element in the path is not the one that implements GetAttrStep?
An IndexStep in cty represents a path inside a list, map, or set for one of the elements. For example, the path to the first element in a list is 0.
I don't fully follow how they get assembled
Paths are assembled as part of decoding a value. The core folks would be best to ask how this works in terms of how a configuration/schema gets decoded to certain types and paths.
why we would hit a scenario like the one described in the comment you added
If a new path type is added to cty. Just coding for future safety because the runtime code for providers should never introduce an unnecessary panic.
There was a problem hiding this comment.
I tried out the forcetypeassert linter in golangci-lint and it catches some other potentially problematic code, but unfortunately doesn't catch this particular code pattern of a type assertion on a slice element. Not sure its worth the effort of enabling it in this project, but maybe some of the others.
There was a problem hiding this comment.
Big 👍 from me on adding this linter everywhere.
|
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 #734