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

Fix CoalesceList panic when passed null arguments #81

Merged
merged 1 commit into from Dec 16, 2020

Conversation

alisdair
Copy link
Contributor

The coalescelist function was moved here from hashicorp/terraform in #37. While the Terraform function documentation doesn't specify whether or not null arguments are permitted, the implementation does allow them:

var CoalesceListFunc = function.New(&function.Spec{
Params: []function.Parameter{},
VarParam: &function.Parameter{
Name: "vals",
Type: cty.DynamicPseudoType,
AllowUnknown: true,
AllowDynamicType: true,
AllowNull: true,
},

However as reported in hashicorp/terraform#26988, passing a null argument causes a panic. This commit fixes the crash and adds test coverage for the function.

@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #81 (c4f4382) into main (4a53886) will increase coverage by 0.32%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   70.36%   70.69%   +0.32%     
==========================================
  Files          79       79              
  Lines        6550     6552       +2     
==========================================
+ Hits         4609     4632      +23     
+ Misses       1497     1476      -21     
  Partials      444      444              
Impacted Files Coverage Δ
cty/function/stdlib/collection.go 26.02% <100.00%> (+3.86%) ⬆️

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 4a53886...c4f4382. Read the comment docs.

@apparentlymart
Copy link
Collaborator

Thanks for this!

These functions that originated in Terraform do unfortunately seem to have inherited some quirks from their origins as HIL functions 😖 but I agree that skipping over the nulls as if they were empty lists seems like a reasonable behavior given the mission of this function, and is consistent with how coalesce handles null and empty strings.

@apparentlymart apparentlymart merged commit 4085b5a into zclconf:main Dec 16, 2020
@alisdair alisdair deleted the coalescelist-null-arguments branch December 16, 2020 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants