Repository navigation
fix(openapi3): warn when a server variable with enum values has no default - #12302
Yashvardhan Thanvi (Thanatos9404) wants to merge 3 commits into
Conversation
A server variable typed as an enum, a union of string literals or a string literal was emitted with `default: ""` next to its `enum` when the TypeSpec property had no default. OpenAPI 3.1 and later require the default to be one of the enum values (3.0 recommends it), so the emitted document was invalid. When there is no explicit default and an enum was computed, use the first enum value. A variable typed as the service's versions enum uses the version of the document being emitted instead, so each versioned document defaults to its own version. Explicit defaults and plain string variables are unchanged. The emitter shares this code for every OpenAPI version, so 3.0, 3.1 and 3.2 all get the same output.
microsoft-github-policy-service
Bot
added
the
emitter:openapi3
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
commit: |
|
All changed packages have been documented.
Show changes
|
…first enum value When a server variable can take more than one value and has no default, the emitter still uses the first value so the document stays valid, but now reports a missing-server-variable-default warning so the author can set the default they want. The versions enum case is not a guess and does not warn, and a variable with a single possible value does not warn either. The warning is reported once per parameter, not once per emitted version.
Mark Cowlishaw (markcowl)
left a comment
Mark Cowlishaw (markcowl)
left a comment
There was a problem hiding this comment.
I really like this contribution. I think we should limit this to a warning diagnostic and not choose a default, because invalid is marginally better than valid and incorrect, IMO. I think the diagnostic should apply only for 3.1+, but it might be fine for 3.0 as well, since this is a should there.
| ### Example | ||
|
|
||
| ```typespec | ||
| @server("https://{region}.example.com", "Regional endpoint", { |
There was a problem hiding this comment.
I like the diagnostic, I don't like choosing a default for the user. If we were to do so, the first variant makes sense, but the possibility of changing an invalid spec into a valid but incorrect spec isn't appealing.
There was a problem hiding this comment.
Makes sense, thanks. I dropped the default selection, so a variable without a default keeps default: "" as before, and missing-server-variable-default is now reported only when emitting OpenAPI 3.1.0 or later, where the default must be one of the enum values (d20dd31). That also removes the special case for the versions enum. Happy to turn the warning on for 3.0 as well if you would rather have it there.
…f picking one Keep the empty default when a server variable with enum values has no default, and report missing-server-variable-default only for OpenAPI 3.1.0 and later, which require the default to be one of the enum values.
Yashvardhan Thanvi (Thanatos9404)
changed the title
Fixes #6024
A server variable typed as an enum, a union of string literals or a single string literal gets an
enumin the emitted document, but without a TypeSpec default itsdefaultis"". OpenAPI 3.1 and 3.2 saydefaultMUST be one of theenumvalues (3.0 says SHOULD), so those documents are invalid.Picking a value for the author could turn an invalid document into a valid but wrong one, so the emitter keeps
""and reports a newmissing-server-variable-defaultwarning when emitting OpenAPI 3.1.0 or later, asking for a default on the parameter. In a versioned service the warning is reported once rather than once per version.Testing:
servers.test.tscovers enums, unions, a single literal, explicit defaults and a versioned service for each OpenAPI version. Without the change the 10 warning cases for 3.1.0 and 3.2.0 fail; with it all 54 tests pass.