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

schema/ExactlyOneOf: Fix handling of unknowns in complex types #287

Merged
merged 3 commits into from
Jan 8, 2020

Conversation

radeksimko
Copy link
Member

Fixes #280


I also added some more tests to check that the same bug doesn't affect ConflictsWith or AtLeastOneOf - it doesn't, but it's IMO useful to have the test there to ensure it stays that way.


The reason ConflictsWith isn't affected is because it runs after we check for unknowns:

// If the value is unknown then we can't validate it yet.
// In particular, this avoids spurious type errors where downstream
// validation code sees UnknownVariableValue as being just a string.
// The SDK has to allow the unknown value through initially, so that
// Required fields set via an interpolated value are accepted.
if !isWhollyKnown(raw) {
if schema.Deprecated != "" {
return []string{fmt.Sprintf("%q: [DEPRECATED] %s", k, schema.Deprecated)}, nil
}
return nil, nil
}
err = validateConflictingAttributes(k, schema, c)
if err != nil {
return nil, []error{err}
}

I decided not to move things around in the main validate() as there is a risk of introducing some more unknown edge cases when folks combine ConflictsWith, AtLeastOneOf and ExactlyOneOf.

Perhaps that code deserves some cleanup, but I think it's safer to leave it for later, bigger refactoring and/or for when we introduce cty here.


I guess the reason IsComputed() wasn't used in the initial implementation is for its confusing name.

It does not to have anything to do with Computed in the schema, but "computed" here just means "unknown". Cty already uses that clearer naming convention of "(un)knowns", so we can adopt it here too when we switch to it from the old type system.

IsComputed() seems to be more or less equivalent of cty's IsWhollyKnown() as it leverages reflectwalk's ability to walk through the complex types and find nested unknowns there:

// Test if the value contains an unknown value
var w unknownCheckWalker
if err := reflectwalk.Walk(v, &w); err != nil {
panic(err)
}

@radeksimko radeksimko added the bug Something isn't working label Jan 7, 2020
@radeksimko radeksimko requested a review from a team January 7, 2020 15:38
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Great Work Radek!

@radeksimko radeksimko merged commit 0a644c9 into master Jan 8, 2020
@radeksimko radeksimko deleted the b-exactly-one-of-unknowns branch January 8, 2020 09:18
@ghost
Copy link

ghost commented Feb 8, 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 Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unset dynamic blocks appearing in validations
2 participants