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.

Sign up for GitHub

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

Jump to bottom

Fix issue chaining multiple requests #3385

Merged
merged 4 commits into from
May 27, 2021
Merged

Fix issue chaining multiple requests #3385

merged 4 commits into from
May 27, 2021

Conversation

vincendep
Copy link
Contributor

Closes #3382

dimitropoulos
dimitropoulos suggested changes May 13, 2021
View reviewed changes
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the quick PR!

Other than two very minor things, we should be ready to go with this. I can confirm that this fixes the issue.

Here is how I tested this:
Run in Insomnia}

For this to work you'd need to run npm run serve from packages/insomnia-smoke-test.

@dimitropoulos
Copy link
Contributor

@vincendep sorry, I didn't catch it sooner. any idea what the changes to the package-lock are all about? I'm done for the day today but I'll take a look tomorrow and maybe we can get this merged tomorrow one way or the other.

@vincendep
Copy link
Contributor Author

vincendep commented May 14, 2021 •
edited
Loading

@vincendep sorry, I didn't catch it sooner. any idea what the changes to the package-lock are all about? I'm done for the day today but I'll take a look tomorrow and maybe we can get this merged tomorrow one way or the other.

mmm no idea what those changes are. I ve only run npm run bootstrap && npm run app-start from the root to test the changes I ve made. (But first time I was using a wrong version of node so I had to run a clean)

@dimitropoulos
Copy link
Contributor

haha, welp. I also nave no idea where those lockfile changes came from. I removed them and ran a bootstrap and they didn't return. /shrug.

All the same, I re-tested just to make sure and this seems to be working as expected. Thanks again for contributing!

@vincendep
Copy link
Contributor Author

thank you too...happy to contribute!

@dimitropoulos
Copy link
Contributor

ok, sorry to renege, but I would like @develohpanda to take a look at this. There was a test that needed to be updated. I studied it for some time, indeed, and concluded that the change we made to the behavior (to fix the bug) needed to be made to the original, but now I'm worried that there's a potential for a different use-case for recursion that there wasn't before. The test was written in such a way that looks like it is expecting an infinite loop, but now there isn't one. Since I'm not sure and out of an abundance of caution, let's @develohpanda a chance to take a look.

@develohpanda develohpanda self-requested a review May 17, 2021 05:12
@vercel vercel bot temporarily deployed to Preview May 25, 2021 04:20 Inactive
develohpanda
develohpanda approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@develohpanda develohpanda left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me - I tested some of the recursive scenarios I could think of both on Live and in this PR and they seem to be working as intended. I like the requestChain approach over the boolean flag.

I think it would be good to add a safety net into the chain, however - so it doesn't grow to an unreasonable amount (maybe hard-coded at 5 or 10?) I am mostly imagining malicious use of it; if someone was to export a workspace online with many (tens, hundreds) of requests chained together, if someone was to import and run a request that would be problematic.

develohpanda
develohpanda suggested changes May 25, 2021
View reviewed changes
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Comment on the test which is worth addressing

plugins/insomnia-plugin-response/__tests__/index.test.js Outdated Show resolved Hide resolved
Vincenzo De Petris and others added 4 commits May 28, 2021 08:07
@develohpanda develohpanda merged commit 6a60e2b into Kong:develop May 27, 2021
develohpanda added a commit that referenced this pull request Jun 22, 2021
@vincendep @dimitropoulos @develohpanda
Fix issue chaining multiple requests (#3385)
f6504aa 
* Fix issue chaining multiple requests

* refactor: use switch to prepare for leveraging exhaustiveness checking

since this will be in TypeScript soon, and this was as topical a time as any to make this change.

* fixes failing test with new behavior (and removes unused `fromResponseTag`)

* fix tests

Co-authored-by: Vincenzo De Petris <[email protected]>
Co-authored-by: Dimitri Mitropoulos <[email protected]>
Co-authored-by: Opender Singh <[email protected]>
@develohpanda develohpanda mentioned this pull request Aug 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@develohpanda develohpanda develohpanda approved these changes

@dimitropoulos dimitropoulos dimitropoulos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to chain multiple requests
3 participants
@vincendep @dimitropoulos @develohpanda