8000 Deserialize payload to str when content type is www-form-urlencoded by sravs-dev · Pull Request #5536 · StackStorm/st2 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Deserialize payload to str when content type is www-form-urlencoded #5536

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
Merged
merged 9 commits into from
Mar 3, 2022
Merged

Deserialize payload to str when content type is www-form-urlencoded #5536

merged 9 commits into from
Mar 3, 2022

Conversation

sravs-dev
Copy link
Contributor
@sravs-dev sravs-dev commented Jan 12, 2022 •
edited by arm4b
Loading

…oded

Opened in favor of #5513

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Jan 12, 2022
@sravs-dev sravs-dev changed the title Deserialize bytes payload to str when content type is www-form-urlenc… Deserialize payload to str when content type is www-form-urlencoded Jan 12, 2022
@sravs-dev sravs-dev mentioned this pull request Jan 12, 2022
Closed
@sravs-dev
Copy link
Contributor Author

@m4dcoder closed #5513 because of CLA issues and opened this one. Could you please review.
Thanks!

@arm4b arm4b added the bug fix label Jan 12, 2022
@arm4b arm4b added this to the 3.7.0 milestone Jan 12, 2022
arm4b
arm4b approved these changes Jan 12, 2022
View reviewed changes
Copy link
Member
@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for the contribution and recreating the clean PR 👍

@arm4b arm4b requested a review from m4dcoder January 12, 2022 14:19
@arm4b
Copy link
Member
arm4b commented Jan 12, 2022

@m4dcoder looks your previous review comments in #5513 were addressed, but please comment if you think we need something else here before merging it tomorrow.

@jamesdreid
Copy link

I have not dug too deeply into this PR, but at face value this may fix Issue #4853 as well.

m4dcoder
m4dcoder requested changes Jan 13, 2022
View reviewed changes
Copy link
Contributor
@m4dcoder m4dcoder left a comment

Choose a reason for hiding this comment

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

@sravs-dev Is there a unit test that shows an example for unsupported content type for x-www-form-urlencoded and how failure is handled?

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Feb 18, 2022
@sravs-dev
Copy link
Contributor Author

@sravs-dev Is there a unit test that shows an example for unsupported content type for x-www-form-urlencoded and how failure is handled?

@armab @m4dcoder When I try to give invalid content/body for x-www-form-urlencoded mimetype, the request building itself fails like this failure in unit test.
Also, I noticed there is an existing test case for un supported content type here . I believe the existing test case would suffice. Let me know your thoughts.

@sravs-dev
Copy link
Contributor Author

@m4dcoder Could you please take a look at the comment above?

@m4dcoder
Copy link
Contributor

@sravs-dev What I meant on unsupported content type for x-www-form-urlencoded is a use case where six.ensure_str returns an error and how that error is handled? Is this possible to test?

@sravs-dev
Copy link
Contributor Author

six.ensure_str

@m4dcoder Thanks for your reply. six.ensure_str takes bytes input, I dont think we can simulate this input as its sent by the API internally. We can only send the payload as key value pair for x-www-form-urlencoded content type.
If six. ensure_str throws error, I guess we would get 500 error similar to what I faced in this issue .
I will go ahead and revert the test case that's passing invalid data to the API.

@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Mar 2, 2022
@sravs-dev
Copy link
Contributor Author

@armab @m4dcoder On the latest commit, all CI Checks have passed except Python 3.8 chunk 1 which is failing in other PRs as well, so it's not related to my changes.

@arm4b arm4b merged commit 721aa69 into StackStorm:master Mar 3, 2022
@arm4b
Copy link
Member
arm4b commented Mar 3, 2022

Merged! Thanks everyone 👍

@sravs-dev sravs-dev deleted the sravanthi/fix_api_bug branch March 4, 2022 03:05
@arm4b arm4b mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arm4b arm4b arm4b approved these changes

@m4dcoder m4dcoder m4dcoder approved these changes

Assignees
No one assigned
Labels
bug fix size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sravs-dev @arm4b @jamesdreid @m4dcoder
0