lang/funcs: fix error when matchkeys encountered a variable#21576
lang/funcs: fix error when matchkeys encountered a variable#21576mildwonkey merged 3 commits intomasterfrom
matchkeys encountered a variable#21576Conversation
`matchkeys` was returning a (false) error if the searchset was a variable, since then the type of the keylist and searchset parameters would not match. This does slightly change the behavior: previously matchkeys would produce an error if the parameters were not of the same type, for e.g. if searchset was a list of strings and keylist was a list of integers. This no longer produces an error.
apparentlymart
left a comment
apparentlymart
left a comment
There was a problem hiding this comment.
I'm still trying to puzzle out what the goals of this function are, since it seems to be aimed at a very specific use-case that isn't documented anywhere. I left some feedback inline based on what I inferred from reviewing the existing implementation and the discussion around it in #12389. I'm still not totally sure! 😬
| argTypes[i] = args[i+1].Type() | ||
| } | ||
|
|
||
| ty, _ := convert.UnifyUnsafe(argTypes) |
There was a problem hiding this comment.
I think we may need to repeat this step in Impl below and then convert args[1] and args[2] to the unified type, because the main loop in Impl is calling stdlib.Equal for each pair of elements and that will return false if the given element types are not the same.
This wouldn't come up in the situation that was causing the confusing error here, because the Impl function doesn't run at all if any of the arguments are unknown, but I think it would arise in the following situation:
matchkeys(["a", "b"], [1, 2], [1, "2"])In the above case, argument 1 would become a list of number while argument 2 would become a list of string. convert.UnifyUnsafe here would then unify that to list of string, but if we don't repeat the unify in Impl below then we'll end up comparing numbers to strings using Equal, and so that'll always return false.
The above example seems contrived because it's built out of constant values, but I expect stuff like that will crop up in real-world configurations where these lists are being constructed dynamically from values coming from variables and resource attributes.
There was a problem hiding this comment.
Oh odd, I thought I was doing that already - I've gone back and forth a few times so I must've reverted that when I wasn't looking 😄
There was a problem hiding this comment.
Excellent, I was also able to add a test for this scenario - I was briefly confused that I couldn't put a tuple in the test but coffee kicked in and I remembered this situation is exactly why we now have functions_test.go
Added higher-level test for matchkeys to exercise mixing types in searchset. This had to be in the functions tests so the HCL auto conversion from tuple to list would occur.
mildwonkey
force-pushed
the
mildwonkey/b-matchkeys
branch
from
86fb853 to
30a924e
Compare
|
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. |
ghost
locked and limited conversation to collaborators
2 participants
matchkeyswas returning a (false) error if the searchset was avariable, since then the type of the keylist and searchset parameters
would not match.
This does slightly change the behavior: previously matchkeys would
produce an error if the parameters were not of the same type, for e.g.
if searchset was a list of strings and keylist was a list of integers.
This no longer produces an error.