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

cURL: Support all --data flags and fix parsing of key=value pairs #5818

Merged
merged 11 commits into from
Mar 31, 2023

Conversation

NickHackman
Copy link
Contributor

@NickHackman NickHackman commented Mar 3, 2023

changelog(Improvements): Improved parsing for cURL imports, supporting different data types.

Closes #5696
Closes #5400

Changes

  • Supports key=value in --data-urlencode
  • Supports all data types and handling their specific cases
  • Supports files and their patterns
  • Improved testing suite to make it clear what is supported and enabling easy extension in the future
  • Documented added code with related documentation links

Implementation follows https://curl.se/docs/manpage.html

Updates tests to support key=value pairs in urlencode

Issue: Kong#5696
Splits out the logic to handle the cases for the --data-urlencode flag from cURL.

docs: https://curl.se/docs/manpage.html#--data-urlencode
Issue: Kong#5696
This refactors the current implementation for handling --data[suffix] flags from cURL supporting all possible formats outside of reading files into Insomnia.

docs: https://curl.se/docs/manpage.html

Issues: Kong#5696, Kong#5400
Combines a lot of tests and covers all cases provided in the cURL man pages.
Copy link
Contributor Author

@NickHackman NickHackman left a comment

Choose a reason for hiding this comment

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

See comments for implementation details and specific notes 😄

const header = `-H 'Content-Type: ${mimeType}'`;

describe('encoding', () => {
describe('cURL --data flags', () => {
Copy link
Contributor Author

@NickHackman NickHackman Mar 3, 2023

Choose a reason for hiding this comment

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

Revamped the tests adding explicit checking for different command flags and arguments.

-d, --data, --data-raw, --data-ascii, --data-binary, and --data-urlencode

Comment on lines +237 to +266
/**
* cURL supported -d, and --date[suffix] flags.
*/
const dataFlags = [
/**
* https://curl.se/docs/manpage.html#-d
*/
'd',
'data',

/**
* https://curl.se/docs/manpage.html#--data-raw
*/
'data-raw',

/**
* https://curl.se/docs/manpage.html#--data-urlencode
*/
'data-urlencode',

/**
* https://curl.se/docs/manpage.html#--data-binary
*/
'data-binary',

/**
* https://curl.se/docs/manpage.html#--data-ascii
*/
'data-ascii',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Document with links to their sections in the man page

*
* @param keyedPairs pairs with cURL flags as keys.
*/
const pairsToDataParameters = (keyedPairs: PairsByName): Parameter[] => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted out textBodyParams section, Parameter[] are just generated in this method.

Comment on lines +327 to +330
if (pair.includes('@') && allowFiles) {
const [name, fileName] = pair.split('@');
return { name, fileName, type: 'file' };
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the man pages from cURL, it's only ever documented in --data-urlencode that you can specify files via

name@filename syntax, this has the consequence of all --data flags supporting that format for code simplicity.

Comment on lines +284 to +289
case 'd':
case 'data':
case 'data-ascii':
case 'data-binary':
dataParameters = dataParameters.concat(pairs.flatMap(pair => pairToParameters(pair, true)));
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these behave the same in this context, as they all support files (technically in different ways)

return pair.split('&').map(pair => {
if (pair.includes('@') && allowFiles) {
const [name, fileName] = pair.split('@');
return { name, fileName, type: 'file' };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the type file was never specified for --data flags.

Comment on lines +290 to +292
case 'data-raw':
dataParameters = dataParameters.concat(pairs.flatMap(pair => pairToParameters(pair)));
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

data-raw is just data, but doesn't parse the @ specially. So it just doesn't support files at all.

Comment on lines +306 to +308
default:
throw new Error(`unhandled data flag ${flagName}`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should never happen unless a new flag was added to dataFlags, but never handled here.

@NickHackman NickHackman marked this pull request as ready for review March 3, 2023 21:02
@NickHackman
Copy link
Contributor Author

Hello @filfreire (tagging you because you've been active on tagged issues), this PR is open to review whenever is convenient for you and your team 🚀

Please let me know if there's commits I need to rebase or required changes (also if I missed anything in the contributing guidelines). Happy to make any changes here, just trying to help resolve some open issues and give back to the project 😃

@filfreire filfreire requested a review from a team March 15, 2023 11:19
@filfreire
Copy link
Member

Thanks for contributing @NickHackman, we'll look into this asap. cc @Kong/team-insomnia

@NickHackman
Copy link
Contributor Author

"Test/OS (windows-latest)" failed due to a connection error

npm ERR! network Invalid response body while trying to fetch https://registry.npmjs.org/check-engine: read ECONNRESET

Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

wow, awesome work thanks @NickHackman

@filfreire filfreire added this pull request to the merge queue Mar 31, 2023
@filfreire filfreire removed this pull request from the merge queue due to a manual request Mar 31, 2023
@filfreire filfreire added this pull request to the merge queue Mar 31, 2023
Merged via the queue into Kong:develop with commit bed21be Mar 31, 2023
@filfreire
Copy link
Member

@NickHackman you can claim a free tshirt for this PR contribution - find more at https://konghq.com/community/open-source-contribution

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.

Small Bug when pasting cURL into the URL field Insomnia does not parse curl request correctly
3 participants