Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lang: Fix non-string key panics in map function #24277

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Mar 4, 2020

The map function assumed that the key arguments were strings, and would panic if they were not.

After this commit, calling map(1, 2) will result in a map {"1" = 2}, and calling map(null, 1) will result in a syntax error.

$ terraform console
> map(1, 2)
{
  "1" = 2
}
> map(null, 1)

>
Error: Error in function call

  on <console-input> line 1:
  (source code not available)

Call to function "map" failed: argument 1 is a null key.

Fixes #23346, fixes #23043

The map function assumed that the key arguments were strings, and would
panic if they were not.

After this commit, calling `map(1, 2)` will result in a map `{"1" = 1}`,
and calling `map(null, 1)` will result in a syntax error.

Fixes #23346, fixes #23043
@alisdair alisdair requested a review from a team March 4, 2020 15:56
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Mar 4, 2020
@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

Merging #24277 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24277      +/-   ##
==========================================
- Coverage   56.96%   56.95%   -0.02%     
==========================================
  Files         656      656              
  Lines       59223    59225       +2     
==========================================
- Hits        33737    33730       -7     
- Misses      22471    22483      +12     
+ Partials     3015     3012       -3
Impacted Files Coverage Δ
lang/funcs/collection.go 77.43% <100%> (+0.17%) ⬆️
checkpoint.go 4.76% <0%> (-28.58%) ⬇️
states/statemgr/filesystem.go 55.64% <0%> (-0.84%) ⬇️
backend/remote/backend_common.go 55.63% <0%> (+0.7%) ⬆️
dag/marshal.go 53.4% <0%> (+1.13%) ⬆️
communicator/communicator.go 83.33% <0%> (+3.7%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bd40fd...37006c5. Read the comment docs.

@alisdair alisdair merged commit 29c0cb7 into master Mar 4, 2020
@alisdair alisdair deleted the alisdair/fix-non-string-map-key-panics branch March 4, 2020 19:30
if err != nil {
return cty.NilVal, err
}
if keyVal.IsNull() {
return cty.NilVal, fmt.Errorf("argument %d is a null key", i+1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super important here, but just wanted to point out that you can return the special error type ArgError from a function (e.g. using NewArgErrorf instead of Errorf here) to indicate that a specific argument is incorrect.

When using functions with HCL, HCL recognizes that error type and indicates the specific single argument as the problem, rather than the entire call.

@ghost
Copy link

ghost commented Apr 4, 2020

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 ghost locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
3 participants