search

snoyberg / http-enumerator

HTTP client package with enumerator interface and HTTPS support.
27 stars 9 forks source link

simpleHttp mis-encodes url ? #13

Closed simonmichael closed 13 years ago

simonmichael commented 13 years ago

Using the latest http-enumerator from github, this fails for me:

simpleHttp "http://joyful.com/darcsweb/darcsweb.cgi?r=rss2irc;a=summary"

apache log shows:

"GET /darcsweb/darcsweb.cgi?r=hledger%3Ba%3Drss HTTP/1.1" 200 1299 "-" "-"

while for curl the log shows:

"GET /darcsweb/darcsweb.cgi?r=hledger;a=rss HTTP/1.1"

Am I doing it wrong ?

simonmichael commented 13 years ago

A http-types issue: https://github.com/aristidb/http-types/issues/9

snoyberg commented 13 years ago

Good catch, thank you for writing the patch. Am I correct that patching http-types fixes this issue as well?

aristidb commented 13 years ago

Should simpleHTTP actually parse the whole URL? If it parsed only as much as necessary, it would be more resilient against exotic / broken URLs.

snoyberg commented 13 years ago

There is definitely logic to that approach, but it's also very convenient to get the completely parsed version, so that you can make minor modifications to the Request value like adding extra parameters. I'm fairly ambivalent on this issue.

aristidb commented 13 years ago

If you use simpleHTTP, you cannot modify the Request value. How about this? Have two versions of parseUrl (maybe just a Bool parameter), one of which parses the Query string, and the other stuffing everything into path.

aristidb commented 13 years ago

Ah, you're fast. Already implemented in 36cefb018c5c30fdfbfb

simonmichael commented 13 years ago

Yes, the patch to http-types solved this simpleHttp issue at least for my needs. I've added related links on the other issue page.

simonmichael commented 13 years ago

Once this is released on hackage, I'll be able to depend on it for the next rss2irc release.

simonmichael commented 13 years ago

Let me clarify that: I'm hoping that I'll soon be able to depend on a release of http-enumerators that depends on http-types >= 0.6.4, since the alternative (depending on http-types > 0.6.3 and allowing current http-enumerator) leads to breakage among installed packages and much rebuilding of http-enumerator by cabal-install.

snoyberg commented 13 years ago

I don't think that this situation will be addressed by adding a stricter lower bound in http-enumerator instead. This is just one of those cases which cabal (unfortunately) doesn't handle well. Though as usual, I could be mistaken.

simonmichael commented 13 years ago

Well, let me try to convince you a little more: it did solve my practical problems with dependency thrashing, since it allowed me to stick to a sensible-sounding rule of thumb: depend on the higher-level packages where possible, not their lower dependencies, and definitely not with tighter bounds than those higher-level packages use.

More importantly, isn't it necessary to release a new h-e with this higher h-t bound, to fix this simpleHttp bug ? That seems the only way to guarantee that http-enumerator can operate correctly with this kind of url, which occurs widely in haskell-land due to darcsweb.

aristidb commented 13 years ago

Given that Michael wrote code that should fix this even without going through http-types...

simpleHttp does not use parseQuery anymore.

snoyberg commented 13 years ago

@Aristid: That's true, that might fix Simon's problem.

@Simon: Please try out the new package. If you still have problems with it, let me know and I'll add a stricter lower bound on http-types.

simonmichael commented 13 years ago

Thanks for the new release(s) which I'm sure will help - will try when I return from vacation next week.

On May 1, 2011, at 3:26 AM, snoyberg reply@reply.github.com wrote:

@Aristid: That's true, that might fix Simon's problem.

@Simon: Please try out the new package. If you still have problems with it, let me know and I'll add a stricter lower bound on http-types.

Reply to this email directly or view it on GitHub: https://github.com/snoyberg/http-enumerator/issues/13#comment_1083215

simonmichael commented 13 years ago

This was indeed resolved, rss2irc now depends on http-enumerator >= 0.6.3. Thank you both.