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

[Bug-fix] Ignoring circular references to prevent stack-overflow while importing an open-api specs #4015

Merged
merged 6 commits into from Mar 15, 2022

Conversation

MrSnix
Copy link
Contributor

@MrSnix MrSnix commented Sep 8, 2021

The issue:
insomnia-importers uses a recursive approach to generate sample parameters for requests. SwaggerParser does not ignore the circular references and includes them in the final object, which is then processed by the generateParameterExample() function causing the stack-overflow.

File used to test:

  1. https://nv-grt.myjetbrains.com/youtrack/api/openapi.json
  2. https://github.com/Kong/insomnia/files/6641041/openapi.txt
  3. https://github.com/Kong/insomnia/files/6590757/swagger.json.txt

Before:
image

After:
image
image
image

How did you fix it?

  try {
    apiDocument = await SwaggerParser.validate(apiDocument, {
      dereference: {
        circular: 'ignore', // Here the new option
      },
    }) as OpenAPIV3.Document;
  } catch (err) {
    console.log('[openapi-3] Import file validation failed', err);
  }

Please, check the SwaggerParser documentation about this specific option: Click here.

Thoughts:

  • I think some people with really huge (i mean, really really huge) file imports may still experience this stack size error but those with a performant system should overcome it.

Other options:

  1. Set dereference.circular option to false. Then, if a circular reference is found, a ReferenceError will be thrown.
    (But I don't think this is what Insomnia should be doing)
  2. Set dereference.circular option to ignore. In this case, all non-circular references will still be dereferenced as normal, but any circular references will remain in the schema. (I think this is the right path)
  3. Use the bundle method rather than the dereference method. Bundling does not result in circular references, because it simply converts external $ref pointers to internal ones. (I don't think we should modify the spec file at all...)

As always opinions, feedback and observations are welcome.

Closes #1367

changelog(Fixes): Fixed importing OpenAPI specs with circular references

@MrSnix
Copy link
Contributor Author

MrSnix commented Sep 8, 2021

As a side note, I found another bug that read "description" as a custom HTTP method. I fixed that too but it is not committed on this PR. Should I open another pull request?

@reynolek
Copy link
Contributor

reynolek commented Sep 8, 2021

@MrSnix Yeah please open another PR for that to keep the changes in context. Appreciate the work! Will get to reviewing shortly.

@MrSnix MrSnix changed the title Ignoring circular references to prevent stack-overflow while importing an open-api specs [Bug-fix] Ignoring circular references to prevent stack-overflow while importing an open-api specs Sep 17, 2021
@timmywheels
Copy link

Thanks @MrSnix -- this fix worked for me. +1 on dereference.circular: ignore as setting it to false would presumably leave a number of users in the same predicament.

@vercel vercel bot temporarily deployed to Preview November 4, 2021 10:08 Inactive
@MarkusRohlof
Copy link

Any movement on this? Maybe any way I can configure it in my client? Without this fix we are forced to use REST clients other than insomnia because it breaks a major functionality...

@vercel vercel bot temporarily deployed to Preview January 12, 2022 13:58 Inactive
Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing this @MrSnix 👍

Tested this out locally, and seems the fix has done the trick ✅

  • On 2021.7.2 I can reproduce the RangeError: Maximum call stack size exceeded
  • On this branch, I can successfully import these collections.

Some side-notes for future reference for the team, not related to this PR:

  1. There's a weird bug (already existed before this PR) in import:
  • If folks import an openapi doc that has some errors picked up by insomnia, they will still be able to see requests on Debug tab
  • If on the other hand folks create an empty design document and copy paste the same json contents of the faulty openapi doc into the Design tab, in that case we don’t create any requests at all when they switch to Debug
  1. This has been mentioned in other issues, but I've noticed again that the user experience is poor with large openapi files. From the instant a user pastes a large json into the design tab, insomnia will completely freeze for 10 to 15 seconds.Error details seem to only show up after 1-2 minutes. There’s some beefy JSONPath calls from spectral that seem to cause the freeze

@filfreire filfreire added T-healthy Change was tested and seems ok T-tested-by-author Change has been tested by Insomnia Team / Kong Test Engineers labels Jan 13, 2022
@vercel vercel bot temporarily deployed to Preview January 15, 2022 20:32 Inactive
@vercel vercel bot temporarily deployed to Preview January 24, 2022 21:01 Inactive
@vercel vercel bot temporarily deployed to Preview February 2, 2022 15:17 Inactive
@filfreire filfreire added the insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest) label Feb 8, 2022
@dimitropoulos
Copy link
Contributor

@MrSnix please take another look if you can: we tried to reproduce this just now and were not able to reproduce it.

@SferaDev
Copy link

SferaDev commented Mar 9, 2022

@dimitropoulos Last week I faced this same issue on: Insomnia 2022.1.0 Release date: 01/03/2022. I tried locally this PR (fast-forwarding to main), and it was fixing the issue.

For reference, I was importing an open-api 3 spec that had a ref Column.columns: Column[].

@dimitropoulos
Copy link
Contributor

@SferaDev that's very helpful. do you think you could provide a minimal openapi spec that triggers this code path?

@MrSnix oh yeah, I forgot to mention but we looked at this on the stream if you wanna see what we did to test it https://youtu.be/GuTf_yOvcUg?t=567

@MrSnix
Copy link
Contributor Author

MrSnix commented Mar 9, 2022

@MrSnix please take another look if you can: we tried to reproduce this just now and were not able to reproduce it.

Hi, @dimitropoulos I think I know what happened and why you weren't able to reproduce it 🥳 .

The issue:
I watched the stream and it looks like the insomnia-importer module wasn't re-built.

Why this happen:
This is some kind of undocumented behaviour 🧐 but calling npm run app-start (by npm run hard-reset) it's not enough because you rebuild only the insomnia-app package and you don't rebuild the insomnia-importers package (where the fix is).

npm run app-start builds only the core package, not its own dependencies.

You need to always call npm run bootstrap in order to allow lerna to rebuild the deps packages 👍🏻 .

You may see that source code is there ... you are right ... when you switch the branch you are basically updating the source code but it's not in the dist folder simply because webpack doesn't rebuild it because it's not watching it, it doesn't detect that the source code on the deps modules has been updated. I have learnt this while working on other PRs and it drove me crazy until I found this tricky behaviour. (While streaming and testing this, you switched branches without calling lerna, you basically had the dist folder containing the old code but you had the source code with the new one)

If you want to be 💯 sure, just run "npm run hard-reset" (but npm run bootstrap should work just fine too) before and after this commit.

I tested again this fix locally and it works 🎂 , just remember to execute npm run hard-reset or npm run bootstrap when you are changing code inside insomnia-importer or it won't be rebuilt.

Sorry for the long answer.

@dimitropoulos
Copy link
Contributor

oh dang. wow. great point! I'll give this another look, then, as soon as possible. thank you so much!

@dimitropoulos dimitropoulos self-assigned this Mar 10, 2022
@filfreire
Copy link
Member

@dimitropoulos @MrSnix - Retesting both the fix and latest beta side by side, and indeed the issue seems to be fixed with this PR:

Kapture.2022-03-15.at.10.36.19.mp4

Sidenote: @Kong/team-insomnia we should notice that for swagger files like the ones posted as examples in the PR 4015 - even though with this fix it is now possible to import them - we still have to look into performance degradation when handling any kind of large design docs/ request collections:

Kapture.2022-03-15.at.10.36.19.mp4

@dimitropoulos dimitropoulos enabled auto-merge (squash) March 15, 2022 15:13
@dimitropoulos dimitropoulos merged commit 28f6c2b into Kong:develop Mar 15, 2022
@MrSnix MrSnix deleted the bug/call-stack-size branch March 15, 2022 16:54
@lastnamehurt
Copy link

Thank you so much! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest) T-healthy Change was tested and seems ok T-tested-by-author Change has been tested by Insomnia Team / Kong Test Engineers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Maximum call stack size exceeded
9 participants