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

postman import auth (from headers and/or auth object) #4375

Merged
merged 8 commits into from
May 12, 2022

Conversation

sergiyladnych
Copy link
Contributor

@sergiyladnych sergiyladnych commented Jan 16, 2022

Closes #4214:

-Body type set to "Other" (instead of extracting from the content-type header, for instance)
-Auth not set (instead of extracting from the Authorization header, for instance)
-URLs are empty
-Query parameters don't import

One test input-output is added. It checks all implemented scenario.

changelog(Improvements): Adds ability to import postman authorization (from headers as well)

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.

@sergiyladnych thank you for this PR! 🎉

Tested out locally, seems to be working ok ✅

-Body type was set to JSON in all cases
-Auth was set for the examples of Bearer, AWS, Basic, Digest and Oauth 1 instead of showing up empty
-URLs are no long empty, and properly filled in
-And query parameters seemed to import ok 👍

Example screenshot, left includes this PR changes, right is latest stable version:
Screenshot 2022-01-18 at 18 23 00

There's some things we might need to look out for, but I would wait for review from one of my dev colleagues:

  • @sergiyladnych I would suggest you clean up a bit the fixture json files, for example, removing the requests that not used:

Screenshot 2022-01-18 at 18 32 23

  • I noticed that in some cases there are missing fields. We are in the end limited by these kinds of edge case postman exports where postman does not include the $.item[].request.auth field.

  • @wdawson at one point it might be worth to deep-dive in one of our upcoming cycles in reviewing all sorts of imports/exports of postman and others deeply, I'm looking through some and find that many miss certain (optional) fields that might be of interest for us to check, example for oauth v1:
    ouath1_example

@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 18, 2022
@dimitropoulos dimitropoulos self-requested a review January 24, 2022 20:36
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

howdy @sergiyladnych! @gatzjames and @filfreire and I looked at this today and decided we want to start moving forward with writing unit tests for these cases. @gatzjames also had an idea for how we can sideline having to maintain the regexes at all - hopefully you'll see more on that soon. I rebased and pushed (hopefully that's ok, if not let me know).

We're going to be looking into this soon, apologies on blocking the merge right away, but rest assured this is high on our radar.

@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
@sergiyladnych
Copy link
Contributor Author

@dimitropoulos Hello,
I'm not sure that fully understood your latest comment. Is there some expectations from me to change smth? If yes could you elaborate on it.

@dimitropoulos
Copy link
Contributor

hey @sergiyladnych. so basically if you want to write more tests of the sort that were added in the last commit please feel free (it'll really speed things up), but if not, then members from the insomnia team are going to write them before we merge this PR. from a functionality standpoint, too, @gatzjames had some ideas, so we may use the tests as a way to verify that switching from the current approach with regexes is compatible with his ideas for a different solution.

@dimitropoulos dimitropoulos removed the insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest) label May 11, 2022
@dimitropoulos dimitropoulos changed the title postman import auth from headers and url from parts postman import auth (from headers and/or auth object) May 11, 2022
@dimitropoulos
Copy link
Contributor

after quite a lot of work on this PR, it's time to draw a line somewhere. what's here now provides significant value compared to what we had before and is now very thoroughly tested.

The only bit that remains is the url parsing of parameters cd5ae6c which we will need to handle separately. We did try and dig into it but it's a bit of a rabbithole of complexity, as it turns out, whereas the auth translation stuff is more straightforward. The reasons mostly have to do with parsing the URL vs the provided parameters and how to parse the URL to remove or add the parameters (since Insomnia and Postman have different behaviors in this respect).

@filfreire filfreire requested review from filfreire and removed request for develohpanda May 12, 2022 08:56
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.

LGTM

@dimitropoulos dimitropoulos self-requested a review May 12, 2022 11:50
@dimitropoulos dimitropoulos merged commit dd501ee into Kong:develop May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Importing a Postman collection loses almost all information
3 participants