Azure / azure-openapi-validator

Azure Open API Validator
MIT License
47 stars 46 forks source link

[ProvisioningStateMustBeReadOnly] Rule ignores siblings of $ref #637

Open cataggar opened 9 months ago

cataggar commented 9 months ago

I am working on https://github.com/Azure/azure-rest-api-specs-pr/pull/15631 and there are many instances of ProvisioningStateMustBeReadOnly errors that are false positives. https://github.com/Azure/azure-rest-api-specs-pr/pull/15631/checks?check_run_id=19650955197

All of the provisioningStates are marked as readOnly, for example:

        "provisioningState": {
          "$ref": "#/definitions/AddonProvisioningState",
          "description": "The state of the addon provisioning",
          "readOnly": true
        }

I see this test was considered flaky before in #561. cc @AkhilaIlla

cataggar commented 9 months ago

I saw this discussed in https://github.com/Azure/azure-sdk-tools/issues/6191. The solution was to add use-read-only-status-schema: true to the tspconfig.yml:

options:
  "@azure-tools/typespec-autorest":
    azure-resource-provider-folder: "resource-manager"
    emitter-output-dir: "{project-root}/.."
    examples-directory: examples
    output-file: "{azure-resource-provider-folder}/{service-name}/{version-status}/{version}/vmware.json"
    omit-unreachable-types: true
    use-read-only-status-schema: true
mikeharder commented 3 months ago

This should be fixed in coordination wtih https://github.com/Azure/oav/issues/848.

Overview

The rule is a "false positive" from the perspective of autorest, but it's correct from the perspective of OpenAPI 2.0. Siblings of $ref are allowed in the former, but ignored in the latter (https://github.com/Azure/azure-openapi-validator/discussions/706).

Spectral

ProvisioningStateMustBeReadOnly is a "spectral" rule:

https://github.com/Azure/azure-openapi-validator/blob/d37228967e2d5333a2a6196702d78ce3e4f1e222/packages/rulesets/src/spectral/az-arm.ts#L208-L221

It's probably ignoring the readonly property, because the OpenAPI 2.0 spec says it should be ignored. It might be possible to configure spectral allow siblings of $ref, but I'm not sure if or how yet, other than converting the autorest to OpenAPI 3.1 which allows siblings of $ref.

Relevant unit test from spectral:

https://github.com/stoplightio/spectral/blob/9e906eaa0e792d229e1fd553f018ab8e6afef12e/packages/rulesets/src/oas/__tests__/no-%24ref-siblings.test.ts

Convert to Native

Otherwise, this rule could be converted from spectral to native, which should be able to allow siblings of $ref.

Documentation

Until fixed, we should improve the documentation:

  1. Add a bullet to https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/ci-fix.md#swagger-lintdiff-for-typespec-troubleshooting-guides
  2. Add a suggestion to add use-read-only-status-schema: true to the rule error message.
mikeharder commented 3 months ago

@markcowl: How would you characterize setting use-read-only-status-schema: true in tspconfig.yaml? Is this a "workaround" for the bugs in azure-openapi-validator and oav? Or should specs configure this for other reasons, and the errors are just a good reminder?

Put another way, if the bugs in azure-openapi-validator and oav are fixed, is TypeSpec setting use-read-only-status-schema still useful? Ignoring our tooling, how does this setting impact the spec, the SDKs, or customers?