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

dynblock,hcldec: Decode unknown dynamic block bodies as entirely unknown values #461

Merged
merged 2 commits into from
Apr 16, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Apr 13, 2021

The dynblock extension paired with hcldec previously used a single object with unknown attributes as a sentinel to indicate that a decoded dynamic block was unknown. This however poses multiple problems with consumers of the data which are not aware of the dynamic construct, since they may misinterpret the number of values, and can be confused when known values are mixed with the sentinel value.

Rather than using a sentinel value within the container, here we propose to ensure a decoded block container which contains any dynamic block with an unknown for_each argument will result in an entirely unknown value. This better serves the purpose of the original sentinel value, as there is no other way to assign an unknown value to a block container, and allows consumers to understand that the final container value itself is not entirely known.

We accomplish this by adding an UnknownBody interface to the hcldec package, which can be used to signal when a block should be decoded as unknown. Any body that satisfies this interface and returns true will cause the entire block container to be decoded as an unknown value, regardless of other known bodies being decoded for that particular block.

@jbardin jbardin force-pushed the jbardin/hcldec-unknown-body branch from cb21ec4 to db8d67d Compare April 13, 2021 21:41
@jbardin jbardin changed the title [DRAFT] Unknown dynamic block bodies dynblock,hcldec: Decode unknown dynamic block bodies as entirely unknown values Apr 14, 2021
@jbardin jbardin marked this pull request as ready for review April 14, 2021 17:06
Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

We've been chatting directly about whether we need this separate ExpandWithUnknown entry point or if it's better to change the (arguably incorrect) behavior of the existing entry point, so we'll continue researching that, but I had a few little bits I noticed while I was reading through before we had that discussion that I just wanted to save here so I don't forget about them!

ext/dynblock/expand_body_test.go Outdated Show resolved Hide resolved
hcldec/spec.go Outdated Show resolved Hide resolved
@jbardin jbardin force-pushed the jbardin/hcldec-unknown-body branch 2 times, most recently from f33cc90 to fbbccfe Compare April 15, 2021 17:08
@jbardin
Copy link
Member Author

jbardin commented Apr 15, 2021

Rebased with some refactoring to allow dynamic to no longer interfere with the MinItems and MaxItems validations. Removed the new entrypoint for now, with the aim of making this the default behavior. I can push that change back in if it turns out another project will be adversely affected by the change.

Allow unknown block bodies during decoding
Allow dynamic blocks to indicate when the entire block value is unknown
@jbardin jbardin force-pushed the jbardin/hcldec-unknown-body branch from fbbccfe to adfdd23 Compare April 15, 2021 17:18
Copy link
Member

@apparentlymart apparentlymart 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 good to me!

The old result here was confusing and inconsistent, so I think it's justified to change it to behave more in line with the design intent, rather than staying bug-compatible. The old behavior remains for applications that aren't using unknowns, or that are using the other API entry points (the main hcl package API or gohcl), so this should only affect applications that are intentionally working with unknown values and thus should be ready to deal with unknown values in results, per our usual assumptions.

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