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

Allow null values for jsonencode #54

Merged
merged 2 commits into from
May 28, 2020
Merged

Conversation

pselle
Copy link
Contributor

@pselle pselle commented May 27, 2020

This addresses hashicorp/terraform#23062, where jsonencode does not allow null values, but it is documented in Terraform that it does: https://www.terraform.io/docs/configuration/functions/jsonencode.html.

Terraform currently uses this zclconf edition of the go-cty library, but if this change is unwarranted here, perhaps it would be a better fit to migrate Terraform to https://github.com/hashicorp/go-cty and make changes there.

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #54 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #54   +/-   ##
=======================================
  Coverage   70.53%   70.54%           
=======================================
  Files          79       79           
  Lines        7151     7153    +2     
=======================================
+ Hits         5044     5046    +2     
  Misses       1664     1664           
  Partials      443      443           
Impacted Files Coverage Δ
cty/function/stdlib/json.go 91.66% <100.00%> (+0.75%) ⬆️

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 6e20bc2...0c4042c. Read the comment docs.

@apparentlymart
Copy link
Collaborator

Thanks for looking at this!

My intuition here was that jsonencode(null) would return the string "null" rather than actually returning a null value, because then the result can be used anywhere that JSON syntax is expected. I think if this returned a "real" null then it would end up just pushing the error further downstream, by assigning a null to an argument that isn't expecting it. 🤔 What do you think?

@pselle
Copy link
Contributor Author

pselle commented May 27, 2020

Null is an allowed value according to the JSON spec, and it's what the Terraform docs say is expected behavior (encoding null results in null).

To be honest, reading the issue I wondered if this was a case where "this is fixed by updating the docs" but it appears null is a valid case. If we look at other languages, json.dumps in Python translates Python's None (since there's no null in Python) to null in JSON.

So, I think this is the right move, and returning the string null would be less correct that returning null.

@alisdair
Copy link
Contributor

It doesn't seem right to me that we're returning a cty.NullVal here. My understanding is that any non-error return type of jsonencode should always be a cty.String, which is a JSON representation of the passed value.

In this case, I think that means cty.StringVal("null"), i.e. a JSON document which consists only of the JavaScript null value.

For example, for JavaScript:

> JSON.stringify(null)
'null'

Python:

>>> json.dumps(None)
'null'

Ruby:

> JSON.generate(nil)
=> "null"

All of these are strings.

@pselle
Copy link
Contributor Author

pselle commented May 27, 2020

@alisdair Thanks! Your comment helped me understand what I imagine was Martin's suggestion. I've committed an update.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

This looks right to me!

@apparentlymart
Copy link
Collaborator

Ahh yes, sorry... that was indeed what I was meaning to say. Sorry for being unclear! 😖

This looks great, and I'm going to merge it now.

@apparentlymart apparentlymart merged commit e76fe06 into zclconf:master May 28, 2020
@pselle pselle deleted the jsonencode branch May 29, 2020 11:14
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.

3 participants