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

User-defined types - Mandatory property based on value from another property #9641

Open
slavizh opened this issue Jan 25, 2023 · 15 comments
Open
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered type system

Comments

@slavizh
Copy link
Contributor

slavizh commented Jan 25, 2023

Is your feature request related to a problem? Please describe.
Hi, is there a way to define property to be mandatory only if another property is set to certain value?
Also let's say I have property property1 that is not mandatory and has default value of foo1. The allowed values for property1 are foo1 and foo2. If the value is foo1 than property2 is mandatory if value is foo2, property 3 is mandatory. Also is there a way that if I do not define the value of property1 because it has default value to signal in intelliisense that property2 is mandatory?

Describe the solution you'd like
A clear and concise description of what you want to happen.

@slavizh slavizh added the enhancement New feature or request label Jan 25, 2023
@ghost ghost added the Needs: Triage 🔍 label Jan 25, 2023
@jeskew
Copy link
Contributor

jeskew commented Jan 25, 2023

This might be solved to some degree with tagged unions (tracked in #9230), but that wouldn't cover all cases in which a property is conditionally required.

@slavizh
Copy link
Contributor Author

slavizh commented Jan 26, 2023

if I am not mistaken this is possible with JSON schemas so it should be possible Bicep to come up with syntax that will transform to that.

@alex-frankel
Copy link
Collaborator

@jeskew is a tagged union equivalent to a discriminated union? What @slavizh is describing sounds like the same semantic as a discriminated union (i.e. the kind property on many azure resource types)

@jeskew
Copy link
Contributor

jeskew commented Jan 27, 2023

@alex-frankel Yes, "tagged union" is a synonym for discriminated union. I think what's being asked for here is a little different (@slavizh please correct me if I'm wrong) in that any property (not just the discriminator) could trigger additional constraints. The JSON schema equivalents would be dependentSchemas, dependentRequired, and if/then/else.

I'm not personally in favor of implementing if/then/else in the ARM type system, if only because Azure resource provider schemas use discriminated unions instead of the more open-ended JSON schema features (which are flexible enough to make client generation really challenging).

@slavizh
Copy link
Contributor Author

slavizh commented Jan 31, 2023

@jeskew jeskew added the Needs: Upvote This issue requires more votes to be considered label Feb 6, 2023
@slavizh
Copy link
Contributor Author

slavizh commented Sep 11, 2023

@jeskew looking at the latest release and discriminator is there a way to define union of array of objects likes:

type typeA = {
  type: 'a'
  value: string
}

type typeB = {
  type: 'b'
  value: int
}

@discriminator('type')
type unionAB = typeA[]? | typeB[]?

Or something like that. Of course the syntax above gives error but it is for illustration purpose.

@jeskew
Copy link
Contributor

jeskew commented Sep 11, 2023

@slavizh You can define an array whose elements are a tagged union:

type typeA = {
  type: 'a'
  value: string
}

type typeB = {
  type: 'b'
  value: int
}

@discriminator('type')
type unionAB = typeA | typeB

type unionAbArray = unionAB[]

If the array needs to be all strings or all ints, you could define that as a slightly different tagged union:

type typeA = {
  type: 'a'
  value: string[]
}

type typeB = {
  type: 'b'
  value: int[]
}

@discriminator('type')
type unionAB = typeA | typeB

@slavizh
Copy link
Contributor Author

slavizh commented Sep 11, 2023

@jeskew Great! May be one last question. I would guess you will always need some property called type or some other name in order to distinguish which object you are defining, correct?

@jeskew
Copy link
Contributor

jeskew commented Sep 11, 2023

Right, the outer wrapper needs to be an object with a discriminator property. That property doesn't need to be named type, but it would need to be defined in each variant of the tagged union with a string literal type.

@slavizh
Copy link
Contributor Author

slavizh commented Sep 12, 2023

@jeskew Thanks! we will soon start building user defined templates for our modules so if there is some feedback I will report it here on GitHub bicep repo.

@slavizh
Copy link
Contributor Author

slavizh commented Sep 27, 2023

@jeskew I have managed to play more with the discriminator feature but it seems it is limited to certain scenarios. For example let's say that you have propertyB that is mandatory if propertyA is set as value A, but you also have propertyC that is mandatory if propertyB is set to value B. With that kind of logic the best you can do is to introduce propertyD that serves as type that covers all the different scenarios depending on the values from propertyA, propertyB and propertyC. Now imagine that you have a dozen of other properties that you need to define in every type and even if you have more dependencies. With the latter you really have to become very creating on the values available for propertyD as they will not have to be very long but at the same time descriptive.
This is of course one illustrative example and there are more that we have in real world. Also this means that we have to start to make breaking changes in our code in order to fit to the model available. On top of this one of the reasons we provide our templates to users is to simplify some of the issues with ARM RPs in terms of how the APIs configures certain features, By using only this methods to have some conditions on properties I think our input (parameters) will become more complex to enter.

@jeskew
Copy link
Contributor

jeskew commented Sep 27, 2023

@slavizh Tagged union polymorphism looks a bit different from the kind of dependent relationships between properties that you're describing, and tagged unions are usually impossible to retrofit onto existing data structures in a backwards compatible way. In your scenario, you could introduce a new discriminator property (this is the "propertyD" you're talking about), but it has to be a required property on all variants, so that's a breaking change right off the bat.

If you do have room to make breaking changes, one of the strategies we suggest in API reviews is to group properties that must be specified together as nested types. For example, if you have two properties, propertyE and propertyF, where users must specify either both or neither, that can instead be modeled as an optional property whose value is an object with two required properties. Nested values can themselves be tagged unions for more complex scenarios.

One option you could try is to just capture the permitted permutations in documentation. This works best if various properties are conditionally required, as you can just mark the property as optional in the type definition and then let an access fail at runtime if the submitted value violates the documented contract. For example,

param foo {
  @description('Always required')
  propA: string

  @description('Required if propA == \'bar\'')
  propB: bool?

  @description('Required if propB == true')
  propC: string?
}

var c = foo.?propB == true ? foo.propC : null

Consumers of your module still get IntelliSense, but they must rely on documentation to avoid deployment failures, so it's not ideal.

I'm still really wary of implementing something like JSON schema's if/then/else in Bicep because I think the syntax (and the resultant types, TBH) will be really confusing. There's not much prior art to lean on because this feature (as far as I can tell) doesn't really exist outside of JSON schema, since type systems attached to a programming language expect users to enforce this kind of logic with imperative statements. One possible Bicep syntax might look something like @scope(<matcher>) { ... } blocks in CSS, but it would make types much more difficult to understand at a glance.

@slavizh
Copy link
Contributor Author

slavizh commented Sep 28, 2023

ok

@slavizh
Copy link
Contributor Author

slavizh commented Oct 20, 2023

@jeskew FYI if you use sealed with discriminator it does not work correctly in VSC intellisense. You basically get empty object as suggestion and none of the two types. Code:

@sealed()
type typeA = {
  type: 'a'
  value: string
}

@sealed()
type typeB = {
  type: 'b'
  value: int
}

@sealed()
@discriminator('type')
type unionAB = typeA | typeB

type unionAbArray = unionAB[]

param foo unionAbArray

Not sure if this is a bug or expected behavior, but it took me some time to find the reason as code was without errors.

@slavizh
Copy link
Contributor Author

slavizh commented Oct 20, 2023

@jeskew actually I think I have found a bug that is not the sealed() one described above. If in your bicepparam file you point to the above intellisense works correclty. But if you compile the bicep file to json and point the bicepparam file to the json intellisense does not work correctly.
This basically breaks any modules uploaded to Container registry as they are stored in json.

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered type system
Projects
None yet
Development

No branches or pull requests

4 participants