[Swagger] Specs are valid, when only warnings are shown #4294

Bl4d3s · 2019-11-02T18:33:51Z

If the official example from here is used against the swagger Validator it generates a valid badge, but does also generate some warning level errors. (See valid badge and debug messages)

I adjusted the parsing of the endjust endpoint so warnings don't generate an invalid swagger Spec.

I also added a test for this scenario.

shields-ci · 2019-11-02T18:36:33Z

	Messages
📖	✨ Thanks for your contribution to Shields, @Bl4d3s!

Generated by 🚫 dangerJS against 3678db4

Bl4d3s · 2019-11-02T18:44:40Z

Unfortunatley I don't know why some tests are now failing, if someone can point me in the right direction, I'll gladly fix it. (with npm test locally all went well)

Edit: Now they have passed ?

services/swagger/swagger.service.js

calebcartwright · 2019-11-02T19:39:01Z

Unfortunatley I don't know why some tests are now failing, if someone can point me in the right direction, I'll gladly fix it. (with npm test locally all went well)

Edit: Now they have passed ?

Sorry I forgot to mention it earlier, but the original problem was the title of the PR (which started with [Service]). After I changed it to put the brackets around Swagger the tests passed. The way the CI process works in Shields for PRs is that the service tests for services listed inisde [] are executed. Since you had [Service] in the PR it was trying to run the tests for a service named Service which does not exist

services/swagger/swagger.service.js

calebcartwright · 2019-11-03T20:02:26Z

If I'm following correctly, the objective was to make the resulting badge consistent with the Swagger validator by handling warning-only level "'errors".

As noted in the PR description, using the sample https://petstore3.swagger.io/api/v3/openapi.json returns valid with the Swagger validator, but invalid for Shields

http://validator.swagger.io/validator?url=https://petstore3.swagger.io/api/v3/openapi.json

Prod Shields
https://img.shields.io/swagger/valid/2.0/https/petstore3.swagger.io/api/v3/openapi

However, AFACIT the staging app for this PR is still saying the sample is invalid
https://shields-staging-pr-4294.herokuapp.com/swagger/valid/2.0/https/petstore3.swagger.io/api/v3/openapi.json

Did I get any of that wrong? If not, does that mean the original problem still exists?

Bl4d3s · 2019-11-03T20:29:12Z

If I'm following correctly, the objective was to make the resulting badge consistent with the Swagger validator by handling warning-only level "'errors".

As noted in the PR description, using the sample https://petstore3.swagger.io/api/v3/openapi.json returns valid with the Swagger validator, but invalid for Shields

http://validator.swagger.io/validator?url=https://petstore3.swagger.io/api/v3/openapi.json

Prod Shields
https://img.shields.io/swagger/valid/2.0/https/petstore3.swagger.io/api/v3/openapi

However, AFACIT the staging app for this PR is still saying the sample is invalid
https://shields-staging-pr-4294.herokuapp.com/swagger/valid/2.0/https/petstore3.swagger.io/api/v3/openapi.json

Did I get any of that wrong? If not, does that mean the original problem still exists?

You are right, I tested with my own specification, located here, where it works. Ill look into it and update the PR

calebcartwright · 2019-11-03T20:45:31Z

I wonder if it's an issue with the file extension (when it is a json based spec) getting sucked in and dropped from being passed along to the validator service.

TBH we should probably move the spec location param from a route param to a query param, refs #3714 and #4178

Bl4d3s · 2019-11-03T20:47:02Z

I wonder if it's an issue with the file extension (when it is a json based spec) getting sucked in and dropped from being passed along to the validator service.

TBH we should probably move the spec location param from a route param to a query param, refs #3714 and #4178

It most definitly is.
See attached log of badge debug output

npm run badge -- swagger/valid/2.0/https/petstore3.swagger.io/api/v3/openapi.json

[email protected] badge /home/michael/GitRepos/shields
cross-env NODE_CONFIG_ENV=test TRACE_SERVICES=true node scripts/badge-cli.js >"swagger/valid/2.0/https/petstore3.swagger.io/api/v3/openapi.json"

1103204445 FsTokenPersistence configured with /home/michael/GitRepos/shields/private/github->user-tokens.json
1103204445 Server is starting up: http://localhost:1111/
Service class 👩‍🍳
SwaggerValidatorService
Named params 🎫
{ scheme: 'https', url: 'petstore3.swagger.io/api/v3/openapi' }
Query params 🖍
{
color: undefined,
colorA: undefined,
colorB: undefined,
label: undefined,
labelColor: undefined,
link: undefined,
logo: undefined,
logoColor: undefined,
logoPosition: undefined,
logoWidth: undefined,
style: undefined
}
Request 🏹
http://validator.swagger.io/validator/debug
Response status code 🎯
200
Response JSON (before validation) 🎯
{
schemaValidationMessages: [
{
level: 'error',
message: "Can't read from file https://petstore3.swagger.io/api/v3/openapi"
}
]
}
Response after validation 🛁
{
schemaValidationMessages: [
{
level: 'error',
message: "Can't read from file https://petstore3.swagger.io/api/v3/openapi"
}
]
}
Service data 🛡
{ message: 'invalid', color: 'red' }
Rendered badge 🛡
{
label: 'swagger',
message: 'invalid',
color: 'red',
link: [],
name: 'swagger',
value: 'invalid'
}

Bl4d3s · 2019-11-03T21:13:30Z

I'm not entierly sure if this is the correct approach, as it will be a breaking change. Is this how you intended it or how should I do it? Locally now both the json and yaml spec show a valid spec, the tests Im currently fixing

calebcartwright · 2019-11-03T21:14:52Z

services/swagger/swagger.service.js

@@ -22,20 +22,21 @@ module.exports = class SwaggerValidatorService extends BaseJsonService {
  static get route() {
    return {
      base: 'swagger/valid/2.0',
-      pattern: ':scheme(http|https)?/:url*',
+      pattern: ':scheme(http|https)?/:fileExtension(json|yaml)?/:url*',


We should move the spec url path from route parameters to a single query param, with a redirector to maintain backwards compatibility for existing badges.

The Website service is probably a decent reference, https://github.com/badges/shields/pull/4028/files

shields/services/website/website.service.js

Lines 41 to 47 in 7f54e09

static get route() {

return {

base: '',

pattern: 'website',

queryParamSchema: queryParamSchema.concat(urlQueryParamSchema),

}

}

https://github.com/badges/shields/blob/master/services/website/website-redirect.service.js

Actually the TwitterUrl badge may be a simpler/better reference, as neither it nor this Swagger service had the same complexity the Website badge did

https://github.com/badges/shields/blob/master/services/twitter/twitter.service.js
https://github.com/badges/shields/blob/master/services/twitter/twitter-redirect.service.js

I managed to move it to a query param, will look into the redirector in the following week

So I wrote the redirector, and theoretically its working, but I've got one problem
This
http://localhost:8080/swagger/valid/2.0/https/raw.githubusercontent.com%2FOAI%2FOpenAPI-Specification%2Fmaster%2Fexamples%2Fv2.0%2Fjson%2Fp.etstore-expanded.json
will get
http://localhost:8080/swagger/valid/3.0/spec.json?url=https%3A%2F%2Fraw.githubusercontent.com%2FOAI%2FOpenAPI-Specification%2Fmaster%2Fexamples%2Fv2.0%2Fjson%2Fp.etstore-expanded

So the file extension does get appended on my path instead of begin part of the pattern. Any idea why?

Here the code:

module.exports = [ redirector({ category: 'other', name: 'SwaggerUrlRedirect', route: { base: 'swagger/valid/2.0', pattern: ':scheme(http|https)?/:url*', }, transformPath: () => `/swagger/valid/3.0/spec`, transformQueryParams: ({ scheme, url }) => ({ url: `${scheme}://${url}` }), dateAdded: new Date('2019-11-03'), }), ]

For the route pattern, try this instead:

pattern: ':scheme(http|https)?/:url+',

Also, I don't think the scheme portion should really be optional (I know it was already, just think we should also change that too given what we are issuing to the upstream API)

shields/services/swagger/swagger.service.js

Lines 52 to 63 in 7f54e09

async fetch({ scheme, urlF }) {

const url = 'http://validator.swagger.io/validator/debug'

return this._requestJson({

url,

schema: validatorSchema,

options: {

qs: {

url: `${scheme}://${urlF}`,

},

},

})

}

Did you also remove the ? from the scheme?

Tried with removing and without yes

Ah okay, would you mind trying :url(.+) as well? Sorry I know I've seen this before recently just don't remember off hand what the fix was

No change also tried with * and (:url.+) and :url(.+)+

Could this be some sort of issue with the badge formats? Supported is svg and json ... and the .json is interpreted as the format of the badge when I see correctly

calebcartwright · 2019-11-03T22:53:33Z

It might also be helpful to add a NotFound check here, since presumably, everyone that's tried to get a badge for a json based spec would've received invalid regardless of whether the spec was valid. It looks like the Validator API will return an HTTP 200 response with a single error like the one below, and it would be great if we could detect that and convey that in the badge (for example ) instead of invalid

{ 
  "schemaValidationMessages":[ 
    { 
      "level":"error",
      "message":"Can't read from file {{spec url}}"
    }
  ]
}

We could do that by detecting that condition as part of the transform function (single error level error with that message) and then throwing a Shields error:

  throw new NotFound({ prettyMessage: 'spec not found' })

…orted swagger, WIP redirector

Bl4d3s · 2019-11-03T23:21:04Z

I did implement the not found feature, and pushed the (currently not working) redirect, maybe you can see the issue better this way

calebcartwright · 2019-11-03T23:32:43Z

I did implement the not found feature, and pushed the (currently not working) redirect, maybe you can see the issue better this way

Thanks that should help! My understanding is that a swagger spec will always have a .json or a .yml/.yaml extension, do you know if that is correct?

Bl4d3s · 2019-11-03T23:34:50Z

I did implement the not found feature, and pushed the (currently not working) redirect, maybe you can see the issue better this way

Thanks that should help! My understanding is that a swagger spec will always have a .json or a .yml/.yaml extension, do you know if that is correct?

Yes, quote from the website

API specifications can be written in YAML or JSON.

I did some further digging. It really has to do sth with the badge format .json at the end is always interpreted as the badge format not as part of the url

calebcartwright · 2019-11-04T02:04:46Z

Okay I managed to get this working, though with one caveat that IMO we'll need to accept. I'd started making some local changes so the service class diverged just a bit from what you have here. Feel free to incorporate or toss, but something like this should get us to a point of having this solved.

swagger.service.js

'use strict'

const Joi = require('@hapi/joi')
const { optionalUrl } = require('../validators')
const { BaseJsonService, NotFound } = require('..')

const schema = Joi.object({
  schemaValidationMessages: Joi.array().items(
    Joi.object({
      level: Joi.string().required(),
      message: Joi.string().required(),
    }).required()
  ),
}).required()

const queryParamSchema = Joi.object({
  specUrl: optionalUrl.required(),
}).required()

module.exports = class SwaggerValidatorService extends BaseJsonService {
  static get category() {
    return 'other'
  }

  static get route() {
    return {
      base: 'swagger/valid',
      pattern: '2.0',
      queryParamSchema,
    }
  }

  static get examples() {
    return [
      {
        title: 'Swagger Validator',
        staticPreview: this.render({ status: 'valid' }),
        namedParams: {},
        queryParams: {
          specUrl:
            'https://raw.githubusercontent.com/OAI/OpenAPI-Specification/master/examples/v2.0/json/petstore-expanded.json',
        },
      },
    ]
  }

  static get defaultBadgeData() {
    return { label: 'swagger' }
  }

  static render({ status }) {
    if (status === 'valid') {
      return { message: status, color: 'brightgreen' }
    } else {
      return { message: status, color: 'red' }
    }
  }

  async fetch({ specUrl }) {
    return this._requestJson({
      url: 'http://validator.swagger.io/validator/debug',
      schema,
      options: { qs: { url: specUrl } },
    })
  }

  transform({ json, specUrl }) {
    const valMessages = json.schemaValidationMessages

    if (!valMessages || valMessages.length === 0) {
      return { status: 'valid' }
    }

    if (valMessages.length === 1) {
      const { message, level } = valMessages[0]
      if (level === 'error' && message === `Can't read from file ${specUrl}`) {
        throw new NotFound({ prettyMessage: 'spec not found or unreadable' })
      }
    }

    if (valMessages.every(msg => msg.level === 'warning')) {
      return { status: 'valid' }
    }

    return { status: 'invalid' }
  }

  async handle(_routeParams, { specUrl }) {
    const json = await this.fetch({ specUrl })
    const { status } = this.transform({ json, specUrl })
    return this.constructor.render({ status })
  }
}

swagger-redirect.service.js

'use strict'

const { redirector } = require('..')

module.exports = [
  redirector({
    category: 'other',
    name: 'SwaggerRedirect',
    route: {
      base: 'swagger/valid/2.0',
      pattern: ':scheme(https|http)/:url*',
    },
    transformPath: () => '/swagger/valid/2.0',
    transformQueryParams: ({ scheme, url }) => {
      const suffix = /(yaml|yml|json)$/.test(url) ? '' : '.json'
      return { specUrl: `${scheme}://${url}${suffix}` }
    },
    dateAdded: new Date('2019-11-03'),
  }),
]

Currently, no one is able to use the Swagger badge with the default/extensionless mode when using a json-based spec.

So this doesn't actually work, as the filename is misinterpreted as the requested .json format from Shields
https://img.shields.io/swagger/valid/2.0/https/raw.githubusercontent.com/OAI/OpenAPI-Specification/master/examples/v2.0/json/petstore-expanded.json
Badge doesn't work because it returns json --> , but the json response is also misleading (it's really a case of the spec not being found)

{"label":"swagger","message":"invalid","color":"red","link":[],"name":"swagger","value":"invalid"}

Shields users would have to explicitly include the .svg extension to get a working badge
https://img.shields.io/swagger/valid/2.0/https/raw.githubusercontent.com/OAI/OpenAPI-Specification/master/examples/v2.0/json/petstore-expanded.json.svg

Using the approach I've shared above, this edge case (json-based spec without explicit badge extension) is still providing a json response, but at least now it is the correct json response.

{"label":"swagger","message":"valid","color":"brightgreen","link":[],"name":"swagger","value":"valid"}

I do not see a way to magically correct that original edge case as part of the redirector, so I believe this the best we can do. We knew that there would be a handful of edge cases that would be problematic when we updated the badge formats to .svg by default (and dropped the need to always explicitly include .svg).

Everything works fine when explicitly including the extension with the legacy routes, and everything will work fine going forward (since the url of the spec has been moved to the query param)

calebcartwright · 2019-11-04T02:08:40Z

Also, I'd love to actually get some live service tests for this. If there's no publicly accessible specs that are relatively consistent (ones that will be valid or invalid for an extended duration) then we can have the tests point to a live spec with an assertion that uses Joi.allow('valid', 'invalid') for the message.

We can also add some tests for the not found scenario now that we're properly detecting them.

Bl4d3s · 2019-11-04T14:19:15Z

Now json should be usable with json format by default, svg needs to be specified in addition just as you said.

I also added live tests and tests for the redirector.
One live test I'm not sure though: The invalid spec I just used the schema for a spec, but its not a spec as such. (but valid json)

calebcartwright · 2019-11-04T21:56:45Z

services/swagger/swagger.tester.js

@@ -21,6 +24,27 @@ t.create('Valid')
    color: 'brightgreen',
  })



Let's remove any of the mocked tests that are duplicative of the real/live tests. You acn also remove the Live prefix from the test names, as our test runner automatically denotes whether a test is mocked or live in the output

I removed all mocked ones besides the invalid one

calebcartwright · 2019-11-04T21:57:29Z

services/swagger/swagger.tester.js

+  })
+
+// Isn't a spec, but valid json
+t.create('Live invalid')


One live test I'm not sure though: The invalid spec I just used the schema for a spec, but its not a spec as such. (but valid json)

I'm guessing this is the test you were referring to?

Yes exactly

calebcartwright · 2019-11-06T01:38:13Z

services/swagger/swagger.tester.js

+    color: 'brightgreen',
+  })
+
+// Isn't a spec, but valid json


I think this is fine, as it still allows us to validate our integration with/handling of the response from the validator API 👍

calebcartwright

Thanks so much for this! There were a lot of unexpected additional items that were needed to fix this, so really appreciate you working through them all!

shields-deployment · 2019-11-06T02:19:13Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

when the service responds with only warnings the spec is valid

f93ea99

calebcartwright changed the title ~~[Service] Swagger Specs are valid, when only warnings are shown~~ [Swagger] Specs are valid, when only warnings are shown Nov 2, 2019

calebcartwright added the service-badge New or updated service badge label Nov 2, 2019

calebcartwright reviewed Nov 2, 2019

View reviewed changes

services/swagger/swagger.service.js Show resolved Hide resolved

Michael Schmitz added 2 commits November 2, 2019 20:23

add transform method, move color selection to render

019d492

replace forEach with every

c302cc1

calebcartwright reviewed Nov 2, 2019

View reviewed changes

services/swagger/swagger.service.js Outdated Show resolved Hide resolved

shields-cd temporarily deployed to shields-staging-pr-4294 November 3, 2019 00:24 Inactive

move file extension to query param

6801d7c

shields-cd temporarily deployed to shields-staging-pr-4294 November 3, 2019 21:10 Inactive

calebcartwright reviewed Nov 3, 2019

View reviewed changes

add spec not found, moved url to query param, updated version of supp…

055815b

…orted swagger, WIP redirector

shields-cd temporarily deployed to shields-staging-pr-4294 November 3, 2019 23:20 Inactive

fix typo

e642c69

shields-cd temporarily deployed to shields-staging-pr-4294 November 3, 2019 23:52 Inactive

fix json redirect, add live tests, add redirecter tests

731f91d

shields-cd temporarily deployed to shields-staging-pr-4294 November 4, 2019 14:17 Inactive

calebcartwright reviewed Nov 4, 2019

View reviewed changes

fix naming, remove mocked test where live one exist

5257cbb

shields-cd temporarily deployed to shields-staging-pr-4294 November 5, 2019 09:12 Inactive

calebcartwright reviewed Nov 6, 2019

View reviewed changes

calebcartwright approved these changes Nov 6, 2019

View reviewed changes

Merge branch 'master' into swaggerSpec_warnings_are_valid_spec

3678db4

shields-cd deployed to shields-staging-pr-4294 November 6, 2019 01:40 View deployment

calebcartwright merged commit 74a5353 into badges:master Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Swagger] Specs are valid, when only warnings are shown #4294

[Swagger] Specs are valid, when only warnings are shown #4294

Bl4d3s commented Nov 2, 2019

shields-ci commented Nov 2, 2019 •

edited

Loading

Bl4d3s commented Nov 2, 2019 •

edited

Loading

calebcartwright commented Nov 2, 2019 •

edited

Loading

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

calebcartwright Nov 3, 2019

calebcartwright Nov 3, 2019

Bl4d3s Nov 3, 2019

Bl4d3s Nov 3, 2019 •

edited

Loading

calebcartwright Nov 3, 2019

calebcartwright Nov 3, 2019

Bl4d3s Nov 3, 2019

calebcartwright Nov 3, 2019

Bl4d3s Nov 3, 2019

Bl4d3s Nov 3, 2019

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019 •

edited

Loading

calebcartwright commented Nov 4, 2019

calebcartwright commented Nov 4, 2019

Bl4d3s commented Nov 4, 2019

calebcartwright Nov 4, 2019

Bl4d3s Nov 5, 2019

calebcartwright Nov 4, 2019

Bl4d3s Nov 5, 2019

calebcartwright Nov 6, 2019

calebcartwright left a comment

shields-deployment bot commented Nov 6, 2019

	static get route() {
	return {
	base: '',
	pattern: 'website',
	queryParamSchema: queryParamSchema.concat(urlQueryParamSchema),
	}
	}

	async fetch({ scheme, urlF }) {
	const url = 'http://validator.swagger.io/validator/debug'
	return this._requestJson({
	url,
	schema: validatorSchema,
	options: {
	qs: {
	url: `${scheme}://${urlF}`,
	},
	},
	})
	}

[Swagger] Specs are valid, when only warnings are shown #4294

[Swagger] Specs are valid, when only warnings are shown #4294

Conversation

Bl4d3s commented Nov 2, 2019

shields-ci commented Nov 2, 2019 • edited Loading

Bl4d3s commented Nov 2, 2019 • edited Loading

calebcartwright commented Nov 2, 2019 • edited Loading

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bl4d3s Nov 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019

calebcartwright commented Nov 3, 2019

Bl4d3s commented Nov 3, 2019 • edited Loading

calebcartwright commented Nov 4, 2019

calebcartwright commented Nov 4, 2019

Bl4d3s commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

shields-deployment bot commented Nov 6, 2019

shields-ci commented Nov 2, 2019 •

edited

Loading

Bl4d3s commented Nov 2, 2019 •

edited

Loading

calebcartwright commented Nov 2, 2019 •

edited

Loading

Bl4d3s Nov 3, 2019 •

edited

Loading

Bl4d3s commented Nov 3, 2019 •

edited

Loading