RequiredWith schema check and changes in validation for optional schema fields

[tmeckel]

This PR contains

the new RequiredWith schema validation check (including unit tests), and

RequiredWith validation check

The new RequiredWith schema validation check, complements the existing ExactlyOneOfand AtLeastOneOf in the way that when the schema field has an assigned value, all fields listed must have assigned values as well.

The new RequiredWith validation check allows building (mutual) dependencies over multiple schema fields, whereas the existing ExactlyOneOf and AtLeastOneOf checks only ensure fulfilled dependencies with only one other schema field.

[hashicorp-cla]

All committers have signed the CLA.

[tmeckel]

I've already signed the Hashicorp CLA. As best practice I didn't push my commits with my official email address but with [email protected]. So perhaps this is the reason why the above check falsely states that signing the CLA is pending.

[tmeckel]

@paultyng, @appilon Hi guys! Any chance to get this PR reviewed and merged?

[mbfrahry]

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.

[tmeckel]

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.

[mbfrahry]

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

[tmeckel]

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.

[mbfrahry]

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"},
  },

[tmeckel]

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.

[appilon]

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.

[tmeckel]

@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.

[appilon]

👍 thanks @tmeckel this would be ideal, once those changes are made we will prioritize getting this PR merged.

[tmeckel]

@appilon @mbfrahry As planned I push the changes to the PR.

[paultyng]

@tmeckel thanks for the PR, i know this is frequently requested. My initial take though is that the AllOf name is unclear to me what it means. Could we change it to something like RequiredWith since its the opposite of ConflictsWith?

[tmeckel]

@tmeckel thanks for the PR, i know this is frequently requested. My initial take though is that the AllOf name is unclear to me what it means. Could we change it to something like RequiredWith since its the opposite of ConflictsWith?

@paultyng sure we can rename the field to a different name, I do not cling to the name AllOf. But from my point of view RequiredWith is somewhat misleading as well. The name should clearly show that the properties listed must all be specified when calling the resource (or data source). That's why I named it AllOf and because I see it in a "group" with AtLeastOne and ExactlyOneOf.

[mbfrahry]

@tmeckel, I mistakenly wrote this functionality out before it was pointed out that you had already done the work! I used PairedWith but also ContingentOn gets the point across as well. Lots of options but finding the right one is difficult. From your comment, GroupedWith could work too.

[tmeckel]

@mbfrahry yes your PairedWith and my implementation of AllOf definitely overlap or merely do the same thing. But I didn't find PairedWith, ContingentOn or GroupedWith somewhere in the code here. Is this something you're discussing at Hashicorp internally?

[mbfrahry]

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.

[mbfrahry]

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.

[tmeckel]

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?

[mbfrahry]

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?

[tmeckel]

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.

[mbfrahry]

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.

[mbfrahry]

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.

[tmeckel]

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.

[mbfrahry]

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.

[tmeckel]

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.

[mbfrahry]

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!

[tmeckel]

We should also update your main PR description to RequiredWith in case someone peeks back through the PR at some point.

I updated the PR description as requested

[mbfrahry]

LGTM!

[appilon]

LGTM! Thanks for the contribution

[ghost]

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.