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

URL-encode POST parameters in yesod-test #1617

Merged
merged 5 commits into from Aug 20, 2019

Conversation

@league
Copy link
Contributor

commented Aug 13, 2019

Addresses #1616. Includes tests that fail without the corresponding change to the code. Potentially this change could cause somebody's tests to fail, if they were already working around the issue (because URL-encoding is not idempotent).

Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)
@league

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Wait – sorry, I got something wrong in the way it works for multipart/form-data. (I only ran into the bug with x-www-form-urlencoded.) Will push something new when I'm more confident!

@league

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Okay,* I think this is right (08a9632). multipart/form-data didn't need the fix, only x-www-form-urlencoded. (I fooled myself into thinking it did because of a confounding difference in one test returning text/plain and the other text/html.) Please squash when merging!

@snoyberg
Copy link
Member

left a comment

Please add a ChangeLog comment. Also, it doesn't look like the modified test case is testing both url-encoded and form data request types, am I correct?

yesod-test/Yesod/Test.hs Show resolved Hide resolved
@league

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Please add a ChangeLog comment. Also, it doesn't look like the modified test case is testing both url-encoded and form data request types, am I correct?

I believe it does test both types. The byLabel on line 160 tests passing through special characters, and that example also uses fileByLabel, which necessitates encoding using multipart/form-data.

yit "labels" $ do
get ("/form" :: Text)
statusIs 200

request $ do
setMethod "POST"
setUrl ("/form" :: Text)
byLabel "Some Label" "12345"
byLabel "Some Label" "foo+bar%41<&baz"
fileByLabel "Some File" "test/main.hs" "text/plain"

This comment has been minimized.

Copy link
@league

league Aug 19, 2019

Author Contributor

Here's the test case that passes through special characters with multipart/form-data. (This test did not fail previously, so the bug I'm fixing here is only with form-url-encoded. I thought it failed at one point, because the endpoint this test is accessing returns HTML instead fo text, so & comes through as &amp; etc.

This comment has been minimized.

Copy link
@snoyberg

snoyberg Aug 20, 2019

Member

Got it, thanks for clarifying.

@snoyberg snoyberg merged commit d7a2997 into yesodweb:master Aug 20, 2019

23 checks passed

yesodweb.yesod Build #20190819.9 succeeded
Details
yesodweb.yesod (Linux cabal-8.0.2) Linux cabal-8.0.2 succeeded
Details
yesodweb.yesod (Linux cabal-8.2.2) Linux cabal-8.2.2 succeeded
Details
yesodweb.yesod (Linux cabal-8.4.4) Linux cabal-8.4.4 succeeded
Details
yesodweb.yesod (Linux cabal-8.6.5) Linux cabal-8.6.5 succeeded
Details
yesodweb.yesod (Linux nightly) Linux nightly succeeded
Details
yesodweb.yesod (Linux pedantic) Linux pedantic succeeded
Details
yesodweb.yesod (Linux stack-def) Linux stack-def succeeded
Details
yesodweb.yesod (Linux stack-lts-11) Linux stack-lts-11 succeeded
Details
yesodweb.yesod (Linux stack-lts-12) Linux stack-lts-12 succeeded
Details
yesodweb.yesod (Linux stack-lts-13) Linux stack-lts-13 succeeded
Details
yesodweb.yesod (Linux stack-lts-9) Linux stack-lts-9 succeeded
Details
yesodweb.yesod (Linux stack-persistent-2-10) Linux stack-persistent-2-10 succeeded
Details
yesodweb.yesod (Linux stack-persistent-2-9) Linux stack-persistent-2-9 succeeded
Details
yesodweb.yesod (Windows stack-lts-11) Windows stack-lts-11 succeeded
Details
yesodweb.yesod (Windows stack-lts-12) Windows stack-lts-12 succeeded
Details
yesodweb.yesod (Windows stack-lts-13) Windows stack-lts-13 succeeded
Details
yesodweb.yesod (macOS stack-lts-11) macOS stack-lts-11 succeeded
Details
yesodweb.yesod (macOS stack-lts-12) macOS stack-lts-12 succeeded
Details
yesodweb.yesod (macOS stack-lts-13) macOS stack-lts-13 succeeded
Details
yesodweb.yesod (macOS stack-lts-9) macOS stack-lts-9 succeeded
Details
yesodweb.yesod (macOS stack-persistent-2-10) macOS stack-persistent-2-10 succeeded
Details
yesodweb.yesod (macOS stack-persistent-2-9) macOS stack-persistent-2-9 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.