Skip to content

Adds -g flag in curl#258

Merged
umeshp7 merged 6 commits intopostmanlabs:developfrom
someshkoli:fix/multibraces
Oct 29, 2020
Merged

Adds -g flag in curl#258
umeshp7 merged 6 commits intopostmanlabs:developfrom
someshkoli:fix/multibraces

Conversation

@someshkoli
Copy link
Contributor

@someshkoli someshkoli commented May 27, 2020

fixes #253
Since the curl snippets fail if the url parameter has nested braces {}[], we need an additional flag to process the curl command.
Solutions: Add -g flag in curl when request url has nested braces in it.

This PR adds the following changes:

  • checks if the url parameter has nested {}[]
  • Adds regex for checking the same pattern
  • Adds -g flag when nested {}[] is present
  • Accounts the following cases
    • {{}}
    • [[]]
    • {[]}
    • [{}]
  • Adds unit test for this condition

 - checks if the url parameter has nested {}[]
 - Adds regex for checking the same pattern
 - Adds -g flag when nested {}[] is present
 - Accounts the following cases
   - {{}}
   - [[]]
   - {[]}
   - [{}]
if (error) {
expect.fail(null, null, error);
}
expect(snippet).to.include("-g"); // eslint-disable-line quotes
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use single quotes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, picked up test snippet from existing one and forgot to check.

}
});
options = {
longFormat: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need longFormat as false?

'raw': ''
},
'url': {
'raw': 'https://google.com?a={{x:[[{[',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split these into different cases for each pattern you are checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

snippet += ` ${form('-m', format)} ${timeout}`;
}
// eslint-disable-next-line no-useless-escape
if ((url.match(/[\{\[]/g) || []).length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unnecessary escape characters. (/[{[]/g) <- this should work the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No actually they are not part of a string. Those escape characters are for the regex

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are redundant escape characters, not required for the regex. Removing them the regex will work the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

// eslint-disable-next-line no-useless-escape
Avoid disabling any lint rules as far as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are redundant escape characters, not required for the regex. Removing them the regex will work the same way.

yeah they work without escape character too. Thanks

 - Removes redundant backslash
 - Removes additional option in unit test
 - Adds unit test for all four different conditions
@webholik
Copy link
Contributor

curl gives special meaning to { and [ in urls. So even if the request does not contain nested braces,curl will change the meaning of the request. For example:

curl 'http://example.com/?q={foo:bar}' 

will be sent as:

 `http://example.com/?q=foo:bar`.

And all requests with unmatched { and [ will fail. I would recommend adding the -g options by default, since Postman requests can contain { and [ without any special meaning attatched.

@webholik webholik requested a review from umeshp7 September 28, 2020 03:58
@umeshp7
Copy link
Contributor

umeshp7 commented Oct 9, 2020

@webholik The changes look okay.
Could there be a use case where the user would want the braces to be treated differently and have the cURL request fail?

@webholik
Copy link
Contributor

webholik commented Oct 9, 2020

Could there be a use case where the user would want the braces to be treated differently and have the cURL request fail?

The requests are coming from Postman and the only use of braces there is to fill in variables. The variables will be substituted before the request going to codegen. And since we are using the -g flag, the presence or absence of braces will not affect cURL.

@umeshp7 umeshp7 merged commit 2ff4e2b into postmanlabs:develop Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cURL Breaking without -g

3 participants