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

[Bug]: typespec-autorest - readonly is wrong added to some enum #2042

Open
4 tasks done
dolauli opened this issue Jan 7, 2025 · 4 comments
Open
4 tasks done

[Bug]: typespec-autorest - readonly is wrong added to some enum #2042

dolauli opened this issue Jan 7, 2025 · 4 comments
Labels
bug Something isn't working needs-area

Comments

@dolauli
Copy link

dolauli commented Jan 7, 2025

Describe the bug

When generating swagger from tsp, the property readonly is wrong added to some enums.

Following is an example.

For union RevocationStatus, we add the readonly property.

But for the unions in the same tsp files like SkuName, we do not add readonly.

Reproduction

An example has been provided above

Checklist

  • Follow our Code of Conduct
  • Check that this issue is about the Azure libraries for typespec. For bug in the typespec language or core libraries file it in the TypeSpec repo
  • Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
  • The provided reproduction is a minimal reproducible example of the bug.
@dolauli dolauli added the bug Something isn't working label Jan 7, 2025
@markcowl
Copy link
Member

markcowl commented Jan 7, 2025

This is due to the requirement for status enums to be read-only for RPaaS validation, behavior is controlled by this setting: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/codesigning/CodeSigning.Management/tspconfig.yaml#L8

Closing as by design

@markcowl markcowl closed this as completed Jan 7, 2025
@dolauli
Copy link
Author

dolauli commented Jan 8, 2025

Hi @markcowl, Are you suggesting that the status enums are not actually read-only and that we include them only to bypass RPaaS validation? Otherwise, we should mark them as read-only in the TypeSpec definition. Is that correct?

By the way, since this setting is only applied to the typespec-autorest emitter, it causes inconsistency between SDK/PowerShell modules generated from Swagger and those generated from typespec.

@markcowl markcowl reopened this Jan 8, 2025
@markcowl
Copy link
Member

markcowl commented Jan 8, 2025

Not exactly. In cases where this occurs, all references to the enum type are through readOnly properties. If autorest is used, properties are allowed to have readOnly:true as a sibling to the schema reference. Other validators (such as the validator used by RPaaS) do not recognize readOnly as a sibling to a schema reference in a property definition. Since it is specifically required and checked that provisioningState is treated as readOnly, we need to generate the swagger this way to allow server-side validation.

I am not sure I understand how this could impact PowerShell commands - if every property referencing the enum is readOnly, how would generation of an enumeration be changed?

@dolauli
Copy link
Author

dolauli commented Jan 9, 2025

@markcowl Here is how the inconsistency happens.

When using autorest.powershell to generate the module from swagger, status will be generated as the read-only property, since RevocationStatus is a read-only enum.

While using typespec-autorest to generate the module from typespec, we will generate status as read-write property, since neither the union RevocationStatus nor the property is marked as read-only in typespec.

So if I understand correctly, we should add @visibility("read") for the property status, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-area
Projects
None yet
Development

No branches or pull requests

2 participants