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

cty: Fix panics with gob encoding of values containing numbers #32

Merged
merged 1 commit into from
Nov 26, 2019

Conversation

apparentlymart
Copy link
Collaborator

We previously had a more localized workaround for this that worked only for numbers directly, but it failed to deal with the same problem in situations where the number is embedded inside some other compound type.

Now we'll apply the fix recursively over the whole raw decoded data structure from gob, finding and fixing numbers inside all of the structural and collection types too.

Although this fixup is mostly contained within the gob-handling functions, it does bleed slightly into the set internals because otherwise we get a chicken/egg problem where gob decoding of a set of numbers fails before we get a chance to run the fixup.

The JSON and msgpack encodings of cty are the primary supported serialization formats for the standard cty type system and I'd still recommend using those instead of gob whenever possible, though as far as I know gob encoding should now work as well as it can.

We previously had a more localized workaround for this that worked only
for numbers directly, but it failed to deal with the same problem in
situations where the number is embedded inside some other compound type.

Now we'll apply the fix recursively over the whole raw decoded data
structure from gob, finding and fixing numbers inside all of the
structural and collection types too.

Although this fixup is mostly contained within the gob-handling functions,
it does bleed slightly into the set internals because otherwise we get
a chicken/egg problem where gob decoding of a set of numbers fails before
we get a chance to run the fixup.

The JSON and msgpack encodings of cty are the primary supported
serialization formats for the standard cty type system and I'd still
recommend using those instead of gob whenever possible, though as far as
I know gob encoding should now work as well as it can.
@apparentlymart apparentlymart added bug main The main "cty" package crash labels Nov 26, 2019
@apparentlymart apparentlymart merged commit c389e93 into master Nov 26, 2019
@apparentlymart apparentlymart deleted the b-number-panic-gob branch November 26, 2019 23:09
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #32 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #32     +/-   ##
=========================================
+ Coverage   75.92%   76.13%   +0.2%     
=========================================
  Files          74       74             
  Lines        5758     5796     +38     
=========================================
+ Hits         4372     4413     +41     
+ Misses        996      993      -3     
  Partials      390      390
Impacted Files Coverage Δ
cty/set_internals.go 99.14% <100%> (+0.02%) ⬆️
cty/gob.go 81.17% <100%> (+13.17%) ⬆️
cty/convert/compare_types.go 100% <0%> (ø) ⬆️
cty/tuple_type.go 88.37% <0%> (+6.97%) ⬆️

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 f21e35a...63b0988. Read the comment docs.

@azr
Copy link
Contributor

azr commented Nov 27, 2019

Thanks !

@azr
Copy link
Contributor

azr commented Nov 27, 2019

Ah; I tried this and had the same issue: panic: interface conversion: interface {} is big.Float, not *big.Float.


I hadn't read the docs carefully enough (json serialisation of cty values) then got scared because I dont have a cty.Type for my complex value so I was trying to Marshal/Unmarshal with the standard json or gob libraries. gob got me the closest to a working scenario ! Woops; sorry !

So I re-read the docs and now it seems to work fine with a "github.com/zclconf/go-cty/json".Marshal(v, cty.DynamicPseudoType) 👍

Sorry !!! 🤦‍♂

@apparentlymart
Copy link
Collaborator Author

Note that JSON encoding as cty.DynamicPseudoType produces a JSON payload that includes both the type and the value, so that if you later Unmarshal also with cty.DynamicPseudoType then you can recover exactly the same type without needing to provide it.

However, if your caller already knows what type it's providing/expecting then there is a different way that avoids encoding the type as part of the result:

json.Marshall(v, v.Type())
json.Unmarshal(v, whateverTypeYouAreExpecting)

It sounds like you have a situation where you don't have any sort of schema that you could derive an expected type from, so perhaps encoding as cty.DynamicPseudoType is the best answer to ensure you can round-trip properly, but I just wanted to point that out in case encoding the value and the type together isn't what you were intending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug crash main The main "cty" package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants