feat(oas-normalize): reworking how .validate() works, new .convert() method
#920
Conversation
erunion
added
enhancement
erunion
changed the title
.validate() works, new .convert() method.validate() works, new .convert() method
| - name: Run benchmarks | ||
| run: npm run bench |
There was a problem hiding this comment.
It would be rad if there were a github action to post in a PR comment if the benchmark results show any impacts, positive or neg, to performance.
There was a problem hiding this comment.
Yeahh 100%. Like i said in slack, i think you'd have to store the benchmark results somewhere and compare the current run to a known baseline or something.
| @@ -1,7 +1,6 @@ | |||
| import type { ComponentsObject, HttpMethods, OASDocument, TagObject } from '../types.js'; | |||
|
|
|||
| import jsonPointer from 'jsonpointer'; | |||
| import { getAPIDefinitionType } from 'oas-normalize/lib/utils'; | |||
There was a problem hiding this comment.
I'm decoupling oas-normalize from being a dependency in oas because the only thing it's using is this method which checks if a openapi property is present in an object. I don't really want us straying down an is-odd path of loading in a whole ass library for an easy one-liner.
| }); | ||
| ``` | ||
|
|
||
| ### `#bundle()` | ||
| ### `.load()` |
There was a problem hiding this comment.
.load() has long been silently marked as private with a @private JSDoc but we actually use it in a number of apps of ours so might as well make it actually public and documented.
| * @private | ||
| */ | ||
| static async convertPostmanToOpenAPI(schema: any) { | ||
| private static async convertPostmanToOpenAPI(schema: any) { |
There was a problem hiding this comment.
Making this one actually private because it's only used internally.
erunion
requested review from
kanadgupta,
domharrington,
julshotal and
a team
| @@ -1,7 +1,6 @@ | |||
| import type { ComponentsObject, HttpMethods, OASDocument, TagObject } from '../types.js'; | |||
|
|
|||
| import jsonPointer from 'jsonpointer'; | |||
| import { getAPIDefinitionType } from 'oas-normalize/lib/utils'; | |||
Co-authored-by: Kanad Gupta <git@kanad.dev>
🐳 Context
Within our new Git backend we've run into a fun problem where with a refactor I'm working on to stop caching upconverted and dereferenced OpenAPI definitions in a database cache, instead opting to pull them straight from our Git filesystem, I have unfortunately did a number on an internal endpoint of ours' performance by reducing it from 300+ ops/sec to ~30.
The reason this happened is because in order for this endpoint to continue to return upconverted OpenAPI definitions (dereferencing is no longer wanted there) I have to run the file we have in Git, which may be an OpenAPI, Swagger, Postman, or YAML file, through the
.validate()method inoas-normalizein order to do this conversion.Because I already know that these API definitions are valid I just want to convert, however unfortunately
oas-normalizedoesn't have any available methods for this.🧰 Changes
Important
These are breaking changes to
oas-normalize.In order to resolve the above conundrum I am heavily reworking how the existing
.validate()method works to now only validate a given API definition. It will continue to convert Postman collections to OpenAPI in order to validate them as OpenAPI definitons (because we don't have a Postman validator), but will now just return a boolean in the event of the definition being valid, or throwing an error if it isn't.For the conversion side of this that
.validate()no longer handles I am introducing a new.convert()method that will do as it says: convert a given API definition to an OpenAPI JSON object. If it's already OpenAPI, and JSON, it'll return what's already there, but if it's Swagger or Postman it'll do the conversion to OpenAPI there and return the result of that -- the same behavior that.validate()previously contained.Additionally with this, the
.validate()method had aconvertToLatestflag that allowed you, if you so wished, to not convert a Swagger definition to OpenAPI after validating it. I am removing this flag because as far as I could tell we were always supplyingconvertToLatest(it was the default behavior). Instead in its place, if you just want to validate a Swagger definition now, and not convert it to OpenAPI, don't feed it through.convert()after running.validate().To be honest I always really disliked that
.validate()would validate and convert; seemed like a weird dark pattern and potential footgun.⏱️ Benchmarks
Because the work that spawned this was discovered through the process of writing a benchmark I'm also creating some new benchmarks here for the
.validate()and.convert()methods. Diffing the current.validate()benchmark versus the new.convert()one you can see pretty clearly that this new.convert()method will greatly improve our performance over the current implementation and usage of.validate()..validate()currently inmain.validate()now.convert()