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

Use resource types for parameter validation #9229

Closed
jeskew opened this issue Dec 7, 2022 · 6 comments · Fixed by #13047
Closed

Use resource types for parameter validation #9229

jeskew opened this issue Dec 7, 2022 · 6 comments · Fixed by #13047
Assignees
Labels
enhancement New feature or request type system
Milestone

Comments

@jeskew
Copy link
Contributor

jeskew commented Dec 7, 2022

Is your feature request related to a problem? Please describe.
With the user-defined types preview enabled, users can declare custom types and validate that parameters match those type declarations. If a parameter value is passed directly to a resource as a property, however, users need to re-declare the existing type definition as type symbols within their template (or forgo validation).

Describe the solution you'd like
It should be possible to use an existing resource type (or portion thereof) from a built-in or imported namespace. This could be accomplished via typeof function or keyword, such as in typeof('Microsoft.KeyVault/vaults@2022-07-01#properties.accessPolicies[*]').

If we want to leave room for a similar function that gets the assigned type of a variable or parameter, we might need two functions or to require the resource keyword:

param foo { bar: string }
param bar typeof(foo).bar
param accessPolicy typeof(resource 'Microsoft.KeyVault/vaults@2022-07-01').properties.accessPolicies[*]
@jeskew
Copy link
Contributor Author

jeskew commented Jan 20, 2023

One thing to keep in mind with provider-supplied types is that the compiler doesn't trust that they are completely accurate (or that they will remain completely accurate during the period between when the template is compiled and when it is deployed). Type checking based on resource types will generate warnings accompanied by links to report type inaccuracies, while type checking based on something defined within the template itself will generate errors.

Consequently, typeof(resource ...) should not have the same representation in the compiled template that custom types explicitly defined by the template author do. Explicitly defined types are compiled into ARM constraints, violations of which cause the deployment to fail. One potential solution here would be to compile typeof(resource ...) derived types into a metadata entry. For example,

param accessPolicy = typeof(resource 'Microsoft.KeyVault/vaults@2022-07-01#/properties/accessPolicies/*')

might compile to:

{
  "parameters": {
    "accessPolicy": {
      "type": "object",
      "metadata": {
        "__bicep_resource_type": "Microsoft.KeyVault/vaults@2022-07-01#/properties/accessPolicies/*" 
      }
    }
  }
}

Some open questions with this approach:

  1. What happens if a compiled template being consumed as a module refers to a type that is unknown to the parent template? For example, if the module is using an earlier version of Bicep.
  2. It is possible for resource type definitions to diverge over time. Is it OK if a parent template and a module, compiled using different versions of Bicep, report incompatible types under the same identifier?

@jeskew jeskew added the discussion This is a discussion issue and not a change proposal. label Jan 20, 2023
@jeskew jeskew added this to the Committed Backlog milestone Feb 6, 2023
@richardissimo
Copy link

On first reading, this looks like it is simply a way of allowing people to declare a variable's type based on an existing type. For example, following the example in there, I can see that if it was in place, then rather than writing...

param serviceBusNamespaceName string

...then I could declare...

param serviceBusNamespaceName typeof(resource 'Microsoft.ServiceBus/namespaces@2022-01-01-preview').properties.name

So we've replaced the direct reference to a string type with an indirect reference to the same type.

However, I've been referred here from #9797 by @jeskew, with the implication that this has an additional behaviour, which I didn't "get" this from reading the proposal, so I wanted to check... @jeskew are you proposing that:

When declaring a parameter by using a reference to a property, the declaration is saying that not only the type of that property would come across to this declaration; but any validation decorators applied to that property would come too. So for example, if the Service Bus name property had 3 validation decorators (e.g. a regex validation, minimum and maximum string length), those would be applied to this parameter.

@alex-frankel
Copy link
Collaborator

Yes that's right. Can you clarify why you would not want that? IOW, what are you looking to enable with this typeof() parameter type if not to restrict the input to what that property allows?

@richardissimo
Copy link

richardissimo commented Feb 14, 2023

Can you clarify why you would not want that?

I didn't say that I didn't want it, I was just surprised :) ... I've used plenty of languages over the years, and I don't recall seeing something like this before (a type declaration which includes specific validation logic); but that doesn't mean it isn't a good idea.

I'm guessing that it probably infers all of the decorators, because there is probably no way to distinguish the validator decorators from the others; and that's probably fine, too.

The derived declaration may want to override some of them, like @description: for example, there might be two parameters both of the same type, and you might want to distinguish between them.

There might need to be a way to prevent something coming across, although I'm not experienced enough with Bicep to give an example... perhaps the @metadata might apply to the original declaration, but not the derived declaration?

@iivmok
Copy link

iivmok commented Oct 15, 2023

To me and our large insurance company's usage even just a typehint to the language server would already help immensely. Currently all var objects are typeless basically, and telling the lang server something simple, that this is indeed a Microsoft.Web/sites@2022-09-01.properties (or partial of it) and not just a random object would be really helpful.

@jeskew
Copy link
Contributor Author

jeskew commented Nov 1, 2023

Based on a team discussion, the initial implementation will use a slightly different syntax from the proposal described above. Instead of a typeof(resource ...) function, there will instead be a resource<...> utility type (syntax and name cribbed from TypeScript). E.g., instead of typeof(resource 'Microsoft.KeyVault/vaults@2022-07-01').properties.accessPolicies[*], the syntax would be resource<'Microsoft.KeyVault/vaults@2022-07-01'>.properties.accessPolicies[*].

@jeskew jeskew self-assigned this Nov 1, 2023
@jeskew jeskew removed the discussion This is a discussion issue and not a change proposal. label Nov 1, 2023
@jeskew jeskew modified the milestones: Committed Backlog, v0.24 Nov 10, 2023
jeskew added a commit that referenced this issue Jan 5, 2024
Partially address #9229 

This PR adds a new ambient type symbol name `resource` (in the `sys`
namespace) and kind of syntax for using types that accept parameters
(`resource<'type@version'>`). This new kind of syntax is called a
`ParameterizedTypeInstantiation` node because, following C#'s example,
each appearance of the syntax is expected to produce a type symbol and
an IR expression that can be used in place of a concrete type clause.

This PR does **not** implement the logic to use the type of a specific
property within a resource body, as that is turning out to be fairly
complex, and this PR is already large and pretty dense.

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/12591)

---------

Co-authored-by: Anthony Martin <38542602+anthony-c-martin@users.noreply.github.com>
@stephaniezyen stephaniezyen modified the milestones: v0.24, v0.25 Jan 16, 2024
@jeskew jeskew closed this as completed in 7f8aac1 Jan 26, 2024
asilverman pushed a commit that referenced this issue Jan 26, 2024
Resolves #12920
Resolves #9229

This PR allows elements or properties of types to be used as standalone
type expressions. This feature is compatible with both resource-derived
types and compile-time imports, as shown in the following example:

types.bicep
```bicep
@export()
type myObject = {
  quux: int
  saSku: resource<'Microsoft.Storage/storageAccounts@2022-09-01'>.sku
}
```

main.bicep
```bicep
import * as types from 'types.bicep'

type test = {
  baz: types.myObject
}

type test2 = {
  foo: {
    bar: test
  }
}

type test3 = test2.foo.bar.baz.quux
// ^^ compiles to {"$ref": "#/definitions/_1.myObject/properties/quux"}

type test4 = test2.foo.bar.baz.saSku.name
// ^^ compiles to:
// {
//   "type": "string",
//   "metadata": {
//     "__bicep_resource_derived_type!": "Microsoft.Storage/storageAccounts@2022-09-01#properties/sku/properties/name"
//   }
// }
```

Accessing the following element kinds is supported:
* Properties may be dereferenced via dot notation
* Tuple items may be dereferenced via array index
* An object's additional properties type declaration may be dereferenced
via a special `.*` syntax
* A typed array's element type declaration may be dereferenced via a
special `[*]` syntax

For example:

```bicep
type anObject = {
  property: int
  *: string
}

type strings = string[]

type tuple = [int, string]

param propertyDeref anObject.property = 10 // type compiles to {"$ref": "#/definitions/anObject/properties/property"}
param additionalPropertiesDeref anObject.* = 'foo' // type compiles to {"$ref": "#/definitions/anObject/additionalProperties"}
param elementDeref strings[*] = 'bar' // type compiles to {"$ref": "#/definitions/strings/items"}
param itemDeref tuple[1] = 'baz' // type compiles to {"$ref": "#/definitions/tuple/prefixItems/1"}
```
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/13047)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request type system
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants