Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

feat: oas dereferencing #1097

Closed
wants to merge 13 commits into from
Closed

feat: oas dereferencing #1097

wants to merge 13 commits into from

Conversation

erunion
Copy link
Member

@erunion erunion commented Dec 10, 2020

🧰 What's being changed?

  • Bubbles up the OAS dereferencing we're doing for response examples up into the root Doc component so OAS definitions are dereferenced before the operation loads.
  • ResponseExample and ResponseTabs components are no longer async as they're now supplied a fully dereferenced OAS.
  • Minor performance improvements to the ResponseExample component for APIs from our manual editor. Previously we were upgrading their responses into a shape that we could handle within the render method, causing that work to happen anytime the component changed. Since this work should be static it's now happening within the constructor of the Response component.

🧪 Testing

The following areas need some attention:

  • Definitions with $ref can still be loaded handled within the form.
  • Response examples still work. We have some operations in the petstore that have these.
  • Request examples rendered in the JSON editor still work.
  • Definitions that don't pass validation get errors surfaced. In order to test this you'll need to comment out these lines because we do additional validation within the local dev server.

🐳 Notes

Why do this?

A couple reasons!

  1. I am currently working on exposing request examples in the form by way of input placeholders and in order to do that I need to merge our getParametersAsJsonSchema library with sampleFromSchema. Problem is that the latter expects a dereferenced definition and the former does $ref lookups.
  2. Circular schemas. With the json-schema-ref-parser library that we're using, and have been using for the past month to generate response examples, we now have slightly better handling for circular schemas in that we don't fully crap out the explorer when we encounter them with a maximum call stack error. json-schema-ref-parser has some options to ignore circular schemas and leave the $ref intact within the OAS so this allows us to quickly detect a ciruclar schema so we can ignore it without having to write our own recursive traversal mapping logic.
  3. Performance reasons. Since we currently don't memoize $ref lookups, we end up doing a lot of the same work multiple times depending on how often a $ref is used within an OAS. By dereferencing the OAS up front we now no longer have $ref components anywhere within the spec and don't need to fetch schemas on demand.
    • There's still a lot of work to be done here with regard to memoizing as large APIs are still extremely slow to render due to the response example generation. I'm working on more improvements for this in separate PRs.

Why dereference here?

Instead of doing this dereference work within the explorer and creating these async components, why not send the explorer a fully dereferenced OAS instead? Good question! The problem with doing that is right now for our legacy Swagger definitions we're storing them in the DB with circular-json in order for us to be stringify circular schemas. We currently do not do this for OAS definitions.

It's currently an open question whether we still need to do this, or if we could stringify a dereferenced (while ignoring circular schemas as we do in oas, see this code), but until we solve that this is the way to go.

Testing issues

The testing changes here are extremely shitty and I am not happy with them at all. I tried moving these Doc component tests over to using @testing-library/react but with the current HTML that the explorer has makes it extremely difficult to create tests off roles without adding data-testid literally everywhere.

Another thing is that the OAS dereferencing error states are really difficult to test because React error boundaries don't support catching exceptions thrown from componentDidMount so we have to hack it into catching exceptions by throwing them within a setState call (facebook/react#11334 (comment)). Problem with this is that this method only makes the errors able to get caught in the error boundary when running in a browser, Enzyme just throws it. There's got to be a better way to handle these.

But yeah, those error boundary tests are being skipped right now. 😔

@erunion erunion added exploration type:refactor Issues about tackling technical debt type:perf Issues with performance labels Dec 10, 2020
@erunion erunion marked this pull request as ready for review December 12, 2020 06:53
@erunion
Copy link
Member Author

erunion commented Dec 14, 2020

Talked offline with @domharrington and I think I can clean this up a bit by wrapping the Doc component in a new component that does OAS dereferencing. This way the Doc only ever accepts a fully dereferenced OAS and doesn't need to be async because the new component that'll wrap it is async.

@erunion erunion marked this pull request as draft December 14, 2020 21:34
@erunion
Copy link
Member Author

erunion commented Dec 15, 2020

In fixing this mess I'm ending up rewriting most of this branch so I'm closing this out and will open up a new draft when that's close.

@erunion erunion closed this Dec 15, 2020
@erunion erunion deleted the feat/dereferencing branch December 15, 2020 17:02
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
exploration type:perf Issues with performance type:refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant