RequiredWith schema check and changes in validation for optional schema fields #342
Conversation
|
|
|
I've already signed the Hashicorp CLA. As best practice I didn't push my commits with my official email address but with |
helper/schema/schema.go
Outdated
| @@ -773,6 +777,16 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro | |||
| } | |||
| } | |||
|
|
|||
| if len(v.AllOf) > 0 { | |||
| if len(v.ExactlyOneOf) > 0 || len(v.AtLeastOneOf) > 0 { | |||
There was a problem hiding this comment.
I don't believe the AtLeastOneOf check should be done because you could use a different set of keys in AtLeastOneOf in addition to the keys you're checking in AllOf. It's pretty wonky but let's take a keys example where you have to put up AtLeastOne key but one of those keys has two parts so you'd throw AtLeastOneOf on all the key related attributes and then throw an extra AllOf block on the two keys that must be specified together.
The check for ExactlyOneOf is spot on though.
There was a problem hiding this comment.
From my point of view it makes no sense to combine AllOf with any of the other mentioned "operators", because AllOf shall guarantee that all listed attributes are specified, either explicitly or by a default value. I don't see any reason why this should be combined with AtLeastOne or ExactlyOneOf. Thus the if statement in line 781.
There was a problem hiding this comment.
To further expand on my example, over in AzureLand we deprecated connection_string in favor of account_name and account_key. I had to do manual checks to get this to work but with this new validation we could instead do
"connection_string": {
AtLeastOneOf: []string{"connection_string", "account_name", "account_key"},
},
"account_name": {
AtLeastOneOf: []string{"connection_string", "account_name", "account_key"},
AllOf: []string{"account_key", "account_name"},
},
"account_key": {
AtLeastOneOf: []string{"connection_string", "account_key", "account_name"},
AllOf: []string{"account_name", "account_key"},
},
I'm omitting certain extra validation steps just to illustrate a use case where you must specify one of these three keys but two of them are paired together
There was a problem hiding this comment.
Thanks for the example. I see the issue here. Because you want to deprecate the connection_string you made it optional as well as the other two new properties. So the user might specify none of the above and Terraform wouldn't notice in the validation phase.
But, from my point of view, this type of check you shouldn't do on properties because this would mix the semantics between a validation for a specific property and the validation for a complete schema of a resource (data source).
My understanding (until now) for all those "operators" like AtLeastOneOf or ConflictsWith is that they are scoped to the specific property where they are defined and with the line AtLeastOneOf: []string{"connection_string", "account_key", "account_name"}, at the property account_key you would violate this semantic, because you might require the connection_string to be set although it (strictly saying) collides with the account_key attribute.
There was a problem hiding this comment.
The nice thing about all these validations is that we can daisy chain them together. I specifically omitted ConflictsWith but we could add that here for sure to get a more complete picture. I think the thing to note is that we would have to write this logic no matter what to get the desired effect and it's be best to get that logic written during plan -time rather than erroring out mid Terraform run.
"connection_string": {
AtLeastOneOf: []string{"connection_string", "account_name", "account_key"},
ConflictsWith: []string{"account_name", "account_key"},
},
"account_name": {
AtLeastOneOf: []string{"connection_string", "account_name", "account_key"},
AllOf: []string{"account_key", "account_name"},
ConflictsWith: []string{"connection_string"},
},
"account_key": {
AtLeastOneOf: []string{"connection_string", "account_key", "account_name"},
AllOf: []string{"account_name", "account_key"},
ConflictsWith: []string{"connection_string"},
},
There was a problem hiding this comment.
One additional word: because introducing a schema validation would impose a major change of the SDK, I could definitely live with the solution proposed by you above. But I would suggest that someone else from the SDK Team at Hashicorp (perhaps @paultyng) would give his or hers opinion about this.
There was a problem hiding this comment.
Hello @tmeckel thank you for the contribution! The SDK as its written today unfortunately doesn't naturally support whole resource validation (unless dealing with CustomizeDiff.... let's not go there), these special behaviors are our current best effort way of creating groups or sets of keys that enable plan time validation that would otherwise be done manually and during apply time.
@mbfrahry is right that these behaviors are meant to all work together. For example it would be great to be able to enforce ExactlyOneOf and then for a particular attribute of that grouping also enforce AllOf. I would like to suggest another name though, perhaps RequiredWith, that might align closer with the goal here.
If you were looking to enforce attributes being set across a whole resource, you would just mark them as required. As I understand it, this will give us the ability to enforce requirement only when one of the grouping is set (then the rest must also be set), otherwise they are marked Optional: true.
There was a problem hiding this comment.
@appilon Thanks for expressing you opinion here and giving some more explanation on how the various attributes are used or shall be used together.
As I wrote before, I am aware that introducing a way to validating the schema as a whole would mean a massive change of the SDK, and also from the fact that I don't slavishly cling to the if statement, I would suggest that I rename the property AllOf to RequiredWith and remove the if statement.
If you guys fine with this, I can provide a changed PR today.
There was a problem hiding this comment.
👍 thanks @tmeckel this would be ideal, once those changes are made we will prioritize getting this PR merged.
|
@tmeckel thanks for the PR, i know this is frequently requested. My initial take though is that the |
@paultyng sure we can rename the field to a different name, I do not cling to the name |
|
@tmeckel, I mistakenly wrote this functionality out before it was pointed out that you had already done the work! I used |
|
@mbfrahry yes your |
…ons after checking of an optional element has a value
… helper\schema\schema_test.go
… property constraints; updated tests accordingly
tmeckel
changed the title
There was a problem hiding this comment.
Hey @tmeckel, went through another time and did find a bug from some of the code that was shifted around. We should also update your main PR description to RequiredWith in case someone peeks back through the PR at some point.
I think once we get those changes in we should be good to go.
helper/schema/schema.go
Outdated
| return nil, []error{err} | ||
| } | ||
|
|
||
| err = validateExactlyOneAttribute(k, schema, c) |
There was a problem hiding this comment.
The validateExactlyOneAttribute and validateAtLeastOneAttribute do want to be where they were before because they are "Required" in the sense that one of whatever is in that list must be set. But that validation error will never occur since we get out of the function if the key hasn't been set and it's not Required. You can see how that happens from the AtLeastOneOf test that was updated down below.
The RequiredWith validation does make sense here since the keys in that list are only Required when one of keys in that list is set.
There was a problem hiding this comment.
Actually that's why I initially filed the PR: before the discussion here, it made no sense to me checking anything on an optional property which has no value. That's why my explanation in the chapter Semantic changes in validation for optional schema fields above.
After yours and @appilon's explanation how you guys want to use the various validation "operators" I fully understand why you come up with the request to change the implementation.
But the question remains: how can I define validation inside the schema that a property needs other properties but only if it has a value? If you always validate the property regardless it it has a value or not, how do I implement my scenario?
There was a problem hiding this comment.
I noted in the very last sentence of my last comment that you're right that RequiredWith needs to happen right where it is. This is especially true because of the reason you just stated
Does RequiredWith hit all the checks you need now or are we still missing some functionality that warrants another validator?
There was a problem hiding this comment.
I was aware what you said about the position of RequiredWith but my question included also the two other validation functions (validateExactlyOneAttribute and validateAtLeastOneAttribute).
If you leave them at the old position, form my understanding, there's no possibility to model the requirement, that a validation is only performed on an optional attribute when it has a value (or a default value).
I know that this collides with your statements about how you wanna use the AtLeastOne attribute to ensure that at least one of the a set of properties has been set for a set of optional attributes.
So, my question, how to model the requirement, that a validation is only performed when a property has a value? Is this even possible in the current configuration? Do miss something? Especially when I take into account the other options in the schema, like Optional, Required etc.
There was a problem hiding this comment.
I need a little more confirmation on what exactly you are looking for. Are you looking for ExactlyOneOf/AtLeastOneOf validation to be only be run when a value has been set? Or are where these checks preventing any form of validation from being run?
If you're looking for ExactlyOneOf functionality for only when an attribute has been set than you should just use ConflictsWith. That ensures that only one value in that set of keys can be set at a time and only is checked when the key in question is set.
AtLeastOneOf validation is a little trickier since its goal is to have at least one attribute in a list of keys to be set. If you only want that validation for when a key is set then the whole point is moot because the key is just Optional at that point.
Do you have a real world example you can share that would better help me see what you're trying to achieve?
Regardless, making changes to ExactlyOneOf/AtLeastOneOf is outside the scope of adding RequiredWith. We should move them back and open an issue on missing functionality so we can track that in a single issue and move forward on getting this piece in since many developers (myself included) could make good use of RequiredWith today.
There was a problem hiding this comment.
So my proposal would be to add validation functions like AtLeastOneOf or ExactlyOneOf (or both, or with other names) to the Resource and move the tests for AtLeastOneOf or ExactlyOneOf for schema properties after the test if a value is specified.
Moving all the ConflictsWith/ExactlyOneOf/AtLeastOneOf to the Resource level has been on my radar for quite some time actually but time has not been on my side and from the brief time I looked into it, it's non-trivial.
There was a problem hiding this comment.
I apologize if I'm not seeing how all these pieces fit together but I have not looked into the azure devops api and am doing a best effort solution on the information you're providing me.
If I sounded harsh I apologize. It wasn't definitely my intention to disqualify your efforts to bring up alternate solutions. On the contrary, they gave me a deeper insight into how the SDK works. Thanks for the extensive discussion! Highly appreciated.
There was a problem hiding this comment.
Unfortunately, much of provider development is still tribal knowledge and learning by doing. I wish this wasn't the case but we do have plans in the future to ease the learning curve.
Happy to discuss further and you can @ me on a PR if you'd like me to review what solution you ultimately end up going with.
There was a problem hiding this comment.
Well I guess I'll use the RequiredWith constrained for the current implementation of the DevOps provider. Cheapest way to go and, according to HCL, the easiest to understand for the user of the provider. When do you think the PR will be merged?
Aside I'm happy to know, that I can @ you on the DevOps PR or some other people here when I might file another PR for the SDK or if I've an issue with it. 👍 👍 Always hard to figure out who's in charge for a project on GitHub.
There was a problem hiding this comment.
I agree that that is the way you should go with RequiredWith!
It's on the sdk team now to get it merged and released but they know it's ready to go!
I updated the PR description as requested |
…k to their old positions as requested by review in helper/schema/schema.go
RequiredWith schema check and changes in validation for optional schema fields
RequiredWith schema check and changes in validation for optional schema fields (cherry picked from commit e1ddbee)
|
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
locked and limited conversation to collaborators
This PR contains
the new
RequiredWithschema validation check (including unit tests), andRequiredWith validation check
The new
RequiredWithschema validation check, complements the existingExactlyOneOfandAtLeastOneOfin the way that when the schema field has an assigned value, all fields listed must have assigned values as well.The new
RequiredWithvalidation check allows building (mutual) dependencies over multiple schema fields, whereas the existingExactlyOneOfandAtLeastOneOfchecks only ensure fulfilled dependencies with only one other schema field.