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

YAML Mapper not quoting # in cases where it should #492

Closed
MelleD opened this issue Aug 20, 2024 · 16 comments
Closed

YAML Mapper not quoting # in cases where it should #492

MelleD opened this issue Aug 20, 2024 · 16 comments
Labels
yaml Issue related to YAML format backend

Comments

@MelleD
Copy link

MelleD commented Aug 20, 2024

If I put a path in front of a hashtag, the Quotes is deleted every time. If there is nothing in front of the hashtag, the quotes are retained

This happens only with the feature YAMLGenerator.Feature.MINIMIZE_QUOTES

Example

String yaml = new YAMLMapper().enable( YAMLGenerator.Feature.MINIMIZE_QUOTES ).writeValueAsString( json );

Path is "#/components/responses/ClientError"

Out come is

      responses:
        "400":
          $ref: "#/components/responses/ClientError"

If I put something in front of the hashtag the " are deleted
Path is "core-api.yaml#/components/responses/ClientError"

Out come is which is invalid in open api. I would also expect the "

      responses:
        "400":
          $ref: core-api.yaml#/components/responses/ClientError

I this a bug or currently intended with the YAMLGenerator.Feature.MINIMIZE_QUOTES ?

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 20, 2024

Which version?

Also: are you having a problem here? My understanding of the behavior here is that quoting if the line starts with # is required to avoid value as becoming comment (instead of key), but that # is otherwise ok to have.

@cowtowncoder cowtowncoder added the yaml Issue related to YAML format backend label Aug 20, 2024
@MelleD
Copy link
Author

MelleD commented Aug 21, 2024

The problem is that I write an openapi file and there should be quotation in the ref.

References are referenced using the hashtag
https://swagger.io/docs/specification/using-ref/

With local references everything works, but with remote references that are in another file, the " are then deleted and the reference is therefore not valid.

Version is 2.17.2

EDIT: Maybe this something special for open api, right? I tried to remove the feature, but then I have a lot of unnecessary quotes. So in general I like this feature

Or here is the reason StringQuotingChecker#valueHasQuotableChar

            case '#':
                // [dataformats-text#201]: limit quoting with MINIMIZE_QUOTES
                // (but not recognized as comment unless starts line or preceded by whitespace)
                if (precededOnlyByBlank(inputStr, i)) {
                    return true;
                }
                break;

EDIT EDIT: I solved it like this

   private static class OpenApiStringQuotingChecker extends StringQuotingChecker.Default {

      @Override
      protected boolean valueHasQuotableChar( final String inputStr ) {
         if ( inputStr.contains( "#" ) ) {
            return true;
         }
         return super.valueHasQuotableChar( inputStr );
      }
   }

@cowtowncoder
Copy link
Member

@MelleD would you able to provide test code to show the issue? It sounds like # can be interpreted as comment even in cases where non-space characters precede it; if so, I'd like to address the problem.

@MelleD
Copy link
Author

MelleD commented Aug 22, 2024

Yes sure:

Here is an small open api example in JSON

{
  "openapi": "3.0.3",
  "info": {
    "title": "Test",
    "version": "v1.0.0"
  },
  "servers": [
    {
      "url": "https://test.example.com/api/v1.0.0",
      "variables": {
        "api-version": {
          "default": "v1.0.0"
        }
      }
    }
  ],
  "paths": {
    "/my-test/{test-Id}": {
      "get": {
        "parameters": [
          {
            "name": "test-Id",
            "in": "path",
            "description": "The ID of sth.",
            "required": true,
            "schema": {
              "type": "string"
            }
          }
        ],
        "responses": {
          "200": {
            "$ref": "#/components/responses/Test"
          },
          "400": {
            "$ref": "core-api.yaml#/components/responses/ClientError"
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "Test": {
        "description": "This is a test.",
        "type": "object",
        "properties": {}
      }
    },
    "responses": {
      "Test": {
        "content": {
          "application/json": {
            "schema": {
              "$ref": "#/components/schemas/Test"
            }
          }
        },
        "description": "The request was successful."
      }
    }
  }
}

You see the critical point is the 400 status code with the reference. I read the json and would like to convert it to YAML

Test case:

   @Test
   void testYamlGenerator() throws IOException {
      final String json = "{\n"
            + "  \"openapi\": \"3.0.3\",\n"
            + "  \"info\": {\n"
            + "    \"title\": \"Test\",\n"
            + "    \"version\": \"v1.0.0\"\n"
            + "  },\n"
            + "  \"servers\": [\n"
            + "    {\n"
            + "      \"url\": \"https://test.example.com/api/v1.0.0\",\n"
            + "      \"variables\": {\n"
            + "        \"api-version\": {\n"
            + "          \"default\": \"v1.0.0\"\n"
            + "        }\n"
            + "      }\n"
            + "    }\n"
            + "  ],\n"
            + "  \"paths\": {\n"
            + "    \"/my-test/{test-Id}\": {\n"
            + "      \"get\": {\n"
            + "        \"parameters\": [\n"
            + "          {\n"
            + "            \"name\": \"test-Id\",\n"
            + "            \"in\": \"path\",\n"
            + "            \"description\": \"The ID of sth.\",\n"
            + "            \"required\": true,\n"
            + "            \"schema\": {\n"
            + "              \"type\": \"string\"\n"
            + "            }\n"
            + "          }\n"
            + "        ],\n"
            + "        \"responses\": {\n"
            + "          \"200\": {\n"
            + "            \"$ref\": \"#/components/responses/Test\"\n"
            + "          },\n"
            + "          \"400\": {\n"
            + "            \"$ref\": \"core-api.yaml#/components/responses/ClientError\"\n"
            + "          }\n"
            + "        }\n"
            + "      }\n"
            + "    }\n"
            + "  },\n"
            + "  \"components\": {\n"
            + "    \"schemas\": {\n"
            + "      \"Test\": {\n"
            + "        \"description\": \"This is a test.\",\n"
            + "        \"type\": \"object\",\n"
            + "        \"properties\": {}\n"
            + "      }\n"
            + "    },\n"
            + "    \"responses\": {\n"
            + "      \"Test\": {\n"
            + "        \"content\": {\n"
            + "          \"application/json\": {\n"
            + "            \"schema\": {\n"
            + "              \"$ref\": \"#/components/schemas/Test\"\n"
            + "            }\n"
            + "          }\n"
            + "        },\n"
            + "        \"description\": \"The request was successful.\"\n"
            + "      }\n"
            + "    }\n"
            + "  }\n"
            + "}\n";
      final YAMLMapper yamlMapper = new YAMLMapper( ).enable( YAMLGenerator.Feature.MINIMIZE_QUOTES );

      System.out.println( yamlMapper.writeValueAsString( objectMapper.readTree(json) ) );
   }

Outcome is:

openapi: 3.0.3
info:
  title: Test
  version: v1.0.0
servers:
- url: https://test.example.com/api/v1.0.0
  variables:
    api-version:
      default: v1.0.0
paths:
  /my-test/{test-Id}:
    get:
      parameters:
      - name: test-Id
        in: path
        description: The ID of sth.
        required: true
        schema:
          type: string
      responses:
        "200":
          $ref: "#/components/responses/Test"
        "400":
          $ref: core-api.yaml#/components/responses/ClientError
components:
  schemas:
    Test:
      description: This is a test.
      type: object
      properties: {}
  responses:
    Test:
      content:
        application/json:
          schema:
            $ref: "#/components/schemas/Test"
      description: The request was successful.

You see on 400 there are the missing quotes

With the YAMLFactory and this class is working, but not sure if this a general rule or is this related to the open api spec with the hashtag reference see: https://swagger.io/docs/specification/using-ref/

  private static class OpenApiStringQuotingChecker extends StringQuotingChecker.Default {

      @Override
      protected boolean valueHasQuotableChar( final String inputStr ) {
         if ( inputStr.contains( "#" ) ) {
            return true;
         }
         return super.valueHasQuotableChar( inputStr );
      }
   }

Maybe a solution is to create a new feature flag for OPEN_API

openapi: 3.0.3
info:
  title: Test
  version: v1.0.0
servers:
- url: https://test.example.com/api/v1.0.0
  variables:
    api-version:
      default: v1.0.0
paths:
  /my-test/{test-Id}:
    get:
      parameters:
      - name: test-Id
        in: path
        description: The ID of sth.
        required: true
        schema:
          type: string
      responses:
        "200":
          $ref: "#/components/responses/Test"
        "400":
          $ref: "core-api.yaml#/components/responses/ClientError"
components:
  schemas:
    Test:
      description: This is a test.
      type: object
      properties: {}
  responses:
    Test:
      content:
        application/json:
          schema:
            $ref: "#/components/schemas/Test"
      description: The request was successful.

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 22, 2024

I am sorry -- I meant a smaller self-contained test case. Not an in-detail manual explanation.

EDIT: I missed the reproduction that is in there. Although it does not use assertions (but "look, see what happens") so need clarification still.

@cowtowncoder cowtowncoder changed the title YAML Mapper remove Quotes YAML Mapper not quoting # in cases where it should Aug 22, 2024
@cowtowncoder
Copy link
Member

(re-worded title based on my high-level understanding)

@MelleD
Copy link
Author

MelleD commented Aug 22, 2024

He? Just execute the unit test and you see the issue? What is smaller than a unit test?

@cowtowncoder
Copy link
Member

Uh oh. Sorry. I missed the obvious one -- you did include it. N/m. :)

@MelleD
Copy link
Author

MelleD commented Aug 22, 2024

@cowtowncoder ok :) . I tried to use a real and valid open api example.

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 22, 2024

@MelleD much appreciated: and just to make sure, I don't mind explanations that complement reproduction. But occasionally I get only explanations so I ... jumped to conclusions here.

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 22, 2024

@MelleD One more thing tho: I can see missing quotes (as per functionality) -- but is the actual problem that trying to re-read generated YAML will be missing content after #?
I assume so based on the initial description... but reproduction only writes out YAML and does not try re-read YAML to show that.

I mean, not having quotes is intentional if BUT ONLY IF it does not result in problems parsing resulting YAML.

On feature flag for Open API: no, I don't think that should be needed -- either output with URL anchors works in general or it doesn't.

cowtowncoder added a commit that referenced this issue Aug 23, 2024
@cowtowncoder
Copy link
Member

Ok. After adding a simple test for round-tripping, it looks like there IS NOT actual loss of content -- String value is retained and # is not taken to mean comment.
So this is only about cosmetic part of not retaining double-quotes; and as per #465 MINIMIZE_QUOTES will try to minimize amount of quotes.
Because of this, I think things are actually working as expected.
(took me a while to get there :) )

Now: if you do want to force quoting in this case, the use of custom StringQuotingChecker seems like the right way to go: that way you have full control over quoting details.

Closing; may be re-opened if I misunderstood something, or new problems surface.

@MelleD
Copy link
Author

MelleD commented Aug 23, 2024

How I said before for open api it’s not cosmetic. It is an semantic error see description.

Bildschirmfoto 2024-08-23 um 09 30 53
Bildschirmfoto 2024-08-23 um 09 30 58

@cowtowncoder
Copy link
Member

cowtowncoder commented Aug 23, 2024

@MelleD That is semantically equivalent: quoting is optional unless it affects decoding.

If not, please show how is it different wrt deserialized content differing. Usage via Open API in itself is irrelevant, what matters is logical content, not physical encoding.

Screenshot above does not show such problem.

@MelleD
Copy link
Author

MelleD commented Aug 23, 2024

Also possible that the Editor have a bug, without quotes currently you cannot jump to the reference with quotes it’s working

@cowtowncoder
Copy link
Member

@MelleD yes, that sounds plausible. I would suggest you use the custom StringQuotingChecker to enforce quoting -- if it's needed, it's needed. For this specific case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yaml Issue related to YAML format backend
Projects
None yet
Development

No branches or pull requests

2 participants