jhthorsen / mojolicious-plugin-openapi

OpenAPI / Swagger plugin for Mojolicious
54 stars 44 forks source link

Rendered spec is not valid under certain conditions #62

Closed le-garff-yoann closed 6 years ago

le-garff-yoann commented 6 years ago

With the last version of this plugin / JSON::Validator.

For exemple, this spec (https://github.com/le-garff-yoann/mojolicious-plugin-openapi-issue-62/blob/master/share/t-app.json) is rendered on the basePath as:

{
  "basePath": "/api",
  "consumes": [
    "application/json"
  ],
  "definitions": {
    "__tmp_t-app_share_t-app_json-_responses_error": {
      "description": "Self sufficient",
      "schema": {
        "additionalProperties": false,
        "properties": {
          "error": {
            "type": "string"
          }
        },
        "required": [
          "error"
        ],
        "type": "object"
      }
    }
  },
  "host": "localhost:3000",
  "info": {
    "license": {
      "name": "Apache License, Version 2.0",
      "url": "http://www.apache.org/licenses/LICENSE-2.0.html"
    },
    "title": "t-app",
    "version": "0.1.0"
  },
  "paths": {
    "/t": {
      "get": {
        "operationId": "listT",
        "responses": {
          "200": {
            "description": "Self sufficient",
            "schema": {
              "items": {
                "type": "string"
              },
              "type": "array"
            }
          },
          "default": {
            "$ref": "#/definitions/__tmp_t-app_share_t-app_json-_responses_error"
          }
        },
        "tags": [
          "t"
        ],
        "x-mojo-to": "Controller::OpenAPI::T#list"
      }
    }
  },
  "produces": [
    "application/json"
  ],
  "responses": {
    "error": {
      "description": "Self sufficient",
      "schema": {
        "additionalProperties": false,
        "properties": {
          "error": {
            "type": "string"
          }
        },
        "required": [
          "error"
        ],
        "type": "object"
      }
    }
  },
  "swagger": "2.0"
}

This rendered spec isn't valid if you try to load it with OpenAPI::Client - http://editor.swagger.io -http://bigstickcarpet.com/swagger-parser/www/index.html (while the original one is).

N.B:

I cannot tell if:

jhthorsen commented 6 years ago

Is this still an issue? I can't seem to reproduce it myself with...

le-garff-yoann commented 6 years ago

Unfortunately, it's still the case even with the latest version of JSON::Validator.

The whole thing run on the official Vagrant image of Debian 8.1 with perlbrew perl 5.26.1. The directory of my testing app (https://github.com/le-garff-yoann/mojolicious-plugin-openapi-issue-62) is located on /tmp. I start it as root.

I could test it with a different Vagrant image but because i use perlbrew i doubt that it will change anything.

jhthorsen commented 6 years ago

How does the $app->plugin(OpenAPI => ...) line look like?

le-garff-yoann commented 6 years ago

It's the same with another image (centos 7).

I use this app (https://github.com/le-garff-yoann/mojolicious-plugin-openapi-issue-62) for the testing relative to this issue. So the line for $app->plugin(OpenAPI => ...) is https://github.com/le-garff-yoann/mojolicious-plugin-openapi-issue-62/blob/master/lib/TApp.pm#L12.

jhthorsen commented 6 years ago

This is now fixed in JSON::Validator 2.03.

https://github.com/jhthorsen/json-validator/commit/990bce645604a2ea9f4a43f3eb419bcdbbd58b3f

The commit message is pretty bad, since the commit also contains moving bundled refs into the "x-bundled" key, instead of using "definitions".

mohawk2 commented 6 years ago

I gather from @jhthorsen that the problem described here is connected to https://github.com/jhthorsen/json-validator/issues/98, in that the solution in the above-mentioned commit also solves this. I put this here for a link since it took me 15 minutes to find this again!

Also, I do not see in that commit any test that reproduces the problem reported here, which is that bundle can return a spec that fails to validate. I will add such a test since that seems important.

I think the reason the commit mentioned above "worked" is because the following is what was happening:

The test that links to this issue I first checked failed before the above commit, and should avoid any recurrence going forwards.