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

[FIX] cURL code snippet contains encoded value for special char + instead of char itself #735

Conversation

aman-v-singh
Copy link
Contributor

@aman-v-singh aman-v-singh commented Apr 30, 2024

Overview

This PR fixes issue where cURL snipet generated did not follow same behaviour as Postman Send request flow. i.e. + character in query were being encoded, but ideally they should not be as it has a special meaning in query params. With this change, we'll make it so + is not encoded anymore.

Copy link
Member

@VShingala VShingala left a comment

Choose a reason for hiding this comment

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

Let's add unit tests for the change before requesting review.

@@ -226,7 +226,8 @@ var self = module.exports = {
.replace(/%5D/g, ']')
.replace(/%7D/g, '}')
.replace(/%25/g, '%')
.replace(/'/g, '%27');
.replace(/'/g, '%27')
.replace(/%2B/g, '+');
Copy link
Member

Choose a reason for hiding this comment

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

@aman-v-singh Let's keep this at line no. 228. i.e. before we replace % chars.

@aman-v-singh
Copy link
Contributor Author

I included this change in the test and it is passing

Screenshot 2024-04-30 at 4 21 10 PM

Copy link
Member

@VShingala VShingala left a comment

Choose a reason for hiding this comment

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

@aman-v-singh I think changes looks good. Let's make sure we test this behaviour in app as well before making a release.

@aman-v-singh
Copy link
Contributor Author

@VShingala I added the fix for all the codegens along with tests.
I wish first to get the fix for unit-test PR to get merged and then once this PR passes the fixed tests I will create a track with this branch.

PS: here is the track with only curl changes.

…ors into IMPORT-1337-c-url-code-snippet-contains-encoded-value-for-special-char-instead-of-char-itself
@aman-v-singh aman-v-singh merged commit 7dcc69b into develop May 2, 2024
1 check passed
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.

None yet

2 participants