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

Fix OpenAPI YAML converts strings to boolean #5456

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wanlwanl
Copy link
Member

@wanlwanl wanlwanl commented Dec 27, 2024

Problem: yaml 1.1 treats some string as other types of value. Check out: https://perlpunk.github.io/yaml-test-schema/schemas.html
Fix: #5377
Solution: make serialization compatible with YAML 1.1
Todo: test other types (in next PR).

@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:openapi3 Issues for @typespec/openapi3 emitter label Dec 27, 2024
@wanlwanl wanlwanl enabled auto-merge December 27, 2024 05:39
@wanlwanl wanlwanl requested a review from chrisradek December 27, 2024 05:39
@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 27, 2024

All changed packages have been documented.

  • @typespec/openapi3
Show changes

@typespec/openapi3 - fix ✏️

Fix: OpenAPI YAML converts strings to boolean

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs 🛝 VSCode Extension

@wanlwanl wanlwanl changed the title fix Fix Problem of YAML 1.1 Treats Some String as Other Types of Values Dec 27, 2024
@wanlwanl wanlwanl changed the title Fix Problem of YAML 1.1 Treats Some String as Other Types of Values Fix Problem of YAML 1.1 Treats Some Strings as Other Types of Values Dec 27, 2024
@wanlwanl wanlwanl changed the title Fix Problem of YAML 1.1 Treats Some Strings as Other Types of Values Fix OpenAPI YAML converts strings to boolean Dec 27, 2024
Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we should do that, there was already a discussion about this. Openapi recommend using yaml 1.2 we shouldn’t try to use 1.1 at least by default.

In order to preserve the ability to round-trip between YAML and JSON formats, YAML version 1.2 is RECOMMENDED along with some additional constraints

@wanlwanl
Copy link
Member Author

wanlwanl commented Jan 6, 2025

I don’t think we should do that, there was already a discussion about this. Openapi recommend using yaml 1.2 we shouldn’t try to use 1.1 at least by default.

In order to preserve the ability to round-trip between YAML and JSON formats, YAML version 1.2 is RECOMMENDED along with some additional constrain
@timotheeguerin
How about add a new OAS3 emitter option like this:

    "yaml-version": {
      type: "string",
      enum: ["1.1", "1.2"],
      nullable: true,
      default: "1.2",
      description:
        "Specifies the YAML version to use when processing or generating YAML files. The default value is 1.2. Supported values are:\n- 1.1: For compatibility with older YAML specifications.\n1.2: For compliance with the latest YAML standard.",
    },

@chrisradek
Copy link
Member

I see 2 options using the yaml library:

  1. Set compat: "yaml-1.1" as one of the stringify options. This will attempt to emit yaml that is compatible with yaml 1.2 and 1.1 (and does what we want for these true/false string values).
  2. Set the toStringDefaults.defaultStringType to Scalar.QUOTE_SINGLE or Scalar.QUOTE_DOUBLE. This will cause all strings to be wrapped in quotes though, including key names.

I think option 1 provides a nicer looking output, but I don't know what edge cases we might run into with the compatibility mode set - I don't think we're doing anything too fancy though where this should be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emitter:openapi3 Issues for @typespec/openapi3 emitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: OpenAPI YAML converts strings to boolean
4 participants