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

Addresses problem with URL encodings #1432

Merged
merged 2 commits into from
Oct 24, 2021
Merged

Addresses problem with URL encodings #1432

merged 2 commits into from
Oct 24, 2021

Conversation

GambolingPangolin
Copy link
Contributor

@GambolingPangolin GambolingPangolin commented Jun 27, 2021

This changes the way URL encoding for query parameters is handled in servant-client, making it possible to correctly encode arbitrary binary data into query parameter values. There is no comparable change for servant-client. Indeed, the query parameter decoding function in the FromHttpApiData typeclass expects to recieve data in Text form for decoding, whereas arbitrary URL-decoded data can only be correctly be represented as a ByteString.

Closes #1418

@alpmestan
Copy link
Contributor

alpmestan commented Aug 19, 2021

@GambolingPangolin Hello, sorry for the delay, I haven't had much time this summer to carefully follow up on servant stuffs. Would you mind rebasing so that we can see if this passes CI?

There's a bit of unnecessary churn in your patch (imports etc), but it otherwise looks good to me! Thanks for the new test.

@GambolingPangolin
Copy link
Contributor Author

@alpmestan I shrank the patch and rebased. It would be a nice quality of life improvement to impose formatting rules on the sources so that I could defer code layout more to an automated tool. I totally get the objection to churn, but want to share my 2c.

@alpmestan
Copy link
Contributor

@GambolingPangolin I understand the motivation, having had these discussions countless times, but it's going to be tough to agree on a particular style with all the folks involved in the project =) CI should be running, let's see how this goes.

@akhesaCaro
Copy link
Contributor

akhesaCaro commented Sep 7, 2021

To support older version of GHC you need to explicitly import the <> operator.

import Data.Semigroup (<>)

You can also test it using the nix-shell (nix/nix-shell) and provide the version of GHC you want to use. See https://github.com/haskell-servant/servant/tree/master/nix

@GambolingPangolin
Copy link
Contributor Author

@akhesaCaro Thanks for the tip about testing with nix. I'm confused about the CI failure, though. It looks like the error message concerns a module that I did not touch with my change. Should I change the imports there, or is this a sign of some other problem?

This changes the way URL encoding for query parameters is handled,
making it possible to correctly encode arbitrary binary data into query
parameter values.

Closes #1418
@GambolingPangolin
Copy link
Contributor Author

@alpmestan @akhesaCaro did you see my previous comment?

@akhesaCaro
Copy link
Contributor

akhesaCaro commented Oct 3, 2021

I stopped the support of GHC < 8.6.5 since yesterday. You just have to rebase and it will pass.

@GambolingPangolin
Copy link
Contributor Author

@akhesaCaro I rebased this patch and it's ready for another try 😁

@GambolingPangolin
Copy link
Contributor Author

@akhesaCaro bump. Lemme know if you want me to do anything else.

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.

Double encoding of percent signs (servant-client)
3 participants