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

Lengthy resolution / validation on large / complex OAS 2.0 definitions #190

Open
MikeRalphson opened this issue Nov 13, 2018 · 7 comments
Open

Comments

@MikeRalphson
Copy link
Contributor

MikeRalphson commented Nov 13, 2018

As promised, when resolving / validating this definition https://raw.githubusercontent.com/Bungie-net/api/master/openapi-2.json

using Sway 2.0.5 from npm and this snippet

const SwaggerApi = require('sway');
const util = require('util');
const yaml = require('js-yaml');
const fs = require('fs');

const input = process.argv[2];

SwaggerApi.create({definition: input, jsonRefs: {resolveCirculars:true} })
  .then(function (api) {
     let drr = api.definitionRemotesResolved;
     fs.writeFileSync('./resolved.yaml',yaml.safeDump(drr),'utf8');
     //console.log(util.inspect(api));
     let val = api.validate();
     console.log(util.inspect(val.errors));
  }, function (err) {
     console.error(err.stack);
});

I get the following result

RangeError: Maximum call stack size exceeded
    at doWalk (/home/mike/nodejs/testsway/node_modules/sway/lib/helpers.js:599:19)
    at /home/mike/nodejs/testsway/node_modules/sway/lib/helpers.js:610:11
    at /home/mike/nodejs/testsway/node_modules/lodash/lodash.js:4911:15
    at baseForOwn (/home/mike/nodejs/testsway/node_modules/lodash/lodash.js:2996:24)
    at /home/mike/nodejs/testsway/node_modules/lodash/lodash.js:4880:18
    at Function.forEach (/home/mike/nodejs/testsway/node_modules/lodash/lodash.js:9344:14)
    at doWalk (/home/mike/nodejs/testsway/node_modules/sway/lib/helpers.js:609:11)
    at /home/mike/nodejs/testsway/node_modules/sway/lib/helpers.js:610:11
    at /home/mike/nodejs/testsway/node_modules/lodash/lodash.js:4911:15
    at baseForOwn (/home/mike/nodejs/testsway/node_modules/lodash/lodash.js:2996:24)

with the following timings

real	0m59.217s
user	1m3.064s
sys	0m3.795s

Impact: in APIs.guru's Travis CI setup, we're having to skip validation of this definition, as otherwise the job doesn't complete in time.

@MikeRalphson
Copy link
Contributor Author

Again, as requested, comparative time for oas-kit to fetch, resolve, convert and validate:

time node oas-validate.js --verbose --verbose --resolve https://raw.githubusercontent.com/Bungie-net/api/master/openapi-2.json
Gathering...
GET https://raw.githubusercontent.com/Bungie-net/api/master/openapi-2.json
Finished resolution! 
Finished resolution! 
https://raw.githubusercontent.com/Bungie-net/api/master/openapi-2.json
#/paths/~1Forum~1GetTopicsPaged~1{page}~1{pageSize}~1{group}~1{sort}~1{quickDate}~1{categoryFilter}~1/get/parameters/6/schema
Invalid type integer for format byte
  Bungie.Net API 2.3.2
  www.bungie.net 

Failures:
https://raw.githubusercontent.com/Bungie-net/api/master/openapi-2.json

Tests: 0 passing, 1 failing, 0 warnings
real	0m2.567s
user	0m2.899s
sys	0m0.143s

@whitlockjc
Copy link
Member

If you remove options.jsonRefs.resolveCircular, it looks a good bit more reasonable: 6.77s user 0.19s system 104% cpu 6.665 total options.jsonRefs.resolveCircular shouldn't be necessary, but it also shouldn't be breaking things either. Even once that is fixed, the performance difference is noticeable but I'm not 100% sure where the cost is coming. From a resolution perspective, we do resolve references twice, but at different levels, and that is a likely culprit. (The idea here was to have a way to resolve all remotes first to get a locals-only representation for things like swagger-ui or similar, and one fully-resolved.) From a validation perspective, #160 has been around for a while and that could play a role in the speed.

Thanks for the findings.

@MikeRalphson
Copy link
Contributor Author

Thanks. I think in apis.guru we use the same options for all definitions, and it's likely some other ones need resolveCircular. I'll look at making it optional. We also only need a "locals only" rendering, not every $ref resolved.

@whitlockjc
Copy link
Member

I wonder what the detail is in resolution detail/scope between json-refs (sway) and oas-kit. I hope we're comparing apples to apples.

@MikeRalphson
Copy link
Contributor Author

Yes oas-kit only resolves external references, leaving "locals only". It also bails on the first validation error, unlike Sway. But then, it is also doing the work of converting OAS 2.0 to 3.0.0. Definitely some fruit disparity here though 😄

@MikeRalphson
Copy link
Contributor Author

I'm going to try and get another of our dependencies, api-spec-converter to update to Sway ^2.0.5 as that seems to be the main culprit now.

@whitlockjc
Copy link
Member

sway isn't without it's warts and I'd love to fix them, like #160. Thanks again for taking the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants