|
|
Created:
11 years, 3 months ago by bradfitz Modified:
11 years, 2 months ago Reviewers:
CC:
adg, rsc, campoy, golang-dev Visibility:
Public. |
Descriptionnet/http, net/url: deal with URL.Opaque beginning with //
Update issue 4860
Patch Set 1 #Patch Set 2 : diff -r bfb45be43e2b https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 3 : diff -r dbf506e7a2fe https://go.googlecode.com/hg/ #
MessagesTotal messages: 15
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit... File src/pkg/net/http/requestwrite_test.go (right): https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit... src/pkg/net/http/requestwrite_test.go:390: WantWrite: "GET http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" + wow, this is correct?
Sign in to reply to this message.
On Wed, Feb 20, 2013 at 8:33 PM, <adg@golang.org> wrote: > > http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" + > wow, this is correct? > That's up for debate. Arguably, it's what the user told us they wanted (they gave us conflicting information), and it's easier to do what they asked for, rather than try to invent heuristics and precedence rules. I don't really want to be parsing their Opaque value and trying to match it against Host, and I don't want the HTTP layer to use req.URL.Host sometimes but use parseHostOutOf(req.URL.Opaque) in other cases. Seems best to do what we're told if they're getting this low-level. The only thing this attempts to do is follow the docs on (*net.URL).RequestURI, which says it returns what you need to write to do an HTTP request. Given that you can't do an HTTP request with "//foo" (see RFC 2616 5.1.2 Request-URI), it seems best to prepend the scheme to make it an absoluteURI.
Sign in to reply to this message.
LGTM Shrug. On 21 February 2013 15:42, Brad Fitzpatrick <bradfitz@golang.org> wrote: > > > On Wed, Feb 20, 2013 at 8:33 PM, <adg@golang.org> wrote: > >> >> http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" + >> wow, this is correct? >> > > That's up for debate. > > Arguably, it's what the user told us they wanted (they gave us conflicting > information), and it's easier to do what they asked for, rather than try to > invent heuristics and precedence rules. I don't really want to be parsing > their Opaque value and trying to match it against Host, and I don't want > the HTTP layer to use req.URL.Host sometimes but use > parseHostOutOf(req.URL.Opaque) in other cases. Seems best to do what we're > told if they're getting this low-level. > > The only thing this attempts to do is follow the docs on > (*net.URL).RequestURI, which says it returns what you need to write to do > an HTTP request. > > Given that you can't do an HTTP request with "//foo" (see RFC 2616 5.1.2 > Request-URI), it seems best to prepend the scheme to make it an > absoluteURI. > >
Sign in to reply to this message.
I can wait for another shrug. :) On Feb 20, 2013 8:44 PM, "Andrew Gerrand" <adg@golang.org> wrote: > LGTM > > Shrug. > > > On 21 February 2013 15:42, Brad Fitzpatrick <bradfitz@golang.org> wrote: > >> >> >> On Wed, Feb 20, 2013 at 8:33 PM, <adg@golang.org> wrote: >> >>> >>> http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" + >>> wow, this is correct? >>> >> >> That's up for debate. >> >> Arguably, it's what the user told us they wanted (they gave us >> conflicting information), and it's easier to do what they asked for, rather >> than try to invent heuristics and precedence rules. I don't really want to >> be parsing their Opaque value and trying to match it against Host, and I >> don't want the HTTP layer to use req.URL.Host sometimes but use >> parseHostOutOf(req.URL.Opaque) in other cases. Seems best to do what we're >> told if they're getting this low-level. >> >> The only thing this attempts to do is follow the docs on >> (*net.URL).RequestURI, which says it returns what you need to write to do >> an HTTP request. >> >> Given that you can't do an HTTP request with "//foo" (see RFC 2616 5.1.2 >> Request-URI), it seems best to prepend the scheme to make it an >> absoluteURI. >> >> >
Sign in to reply to this message.
what's the motivation for case #2? https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit... File src/pkg/net/http/requestwrite_test.go (right): https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit... src/pkg/net/http/requestwrite_test.go:383: Opaque: "//y.google.com/%2F/%2F/", Not at all sure about this. I think Opaque should be used as is. Otherwise it's not "opaque".
Sign in to reply to this message.
Still not sure about the motivation, even after rereading the thread and the issue, but I suggest that instead people can just set Opaque = " http://y.google.com/asdf" if that's what they want the GET to say.
Sign in to reply to this message.
On Thu, Feb 21, 2013 at 10:46 AM, Russ Cox <rsc@golang.org> wrote: > Still not sure about the motivation, even after rereading the thread and > the issue, but I suggest that instead people can just set Opaque = " > http://y.google.com/asdf" if that's what they want the GET to say. > Oh, without a Scheme. Hm, that could work. Just &net.URL{Opaque: " http://foo.com/"}. The original motivation was that we thought there existed no *net.URL using Opaque (to contain %2F) which would work with both App Engine (which uses (*URL).String) and net/http.Transport, which uses (*net.URL).RequestURI(). So I made RequestURI return something valid for that case. I still think this CL makes sense, since RequestURI is documented to return something for an HTTP Request-URI line, and // isn't valid. But if a just-Opaque URL works, I care less. Francesc, want to try?
Sign in to reply to this message.
I'd rather document that RequestURI returns Opaque unmodified if set. That gives people complete control and keeps us out of the business of looking at ostensibly opaque fields.
Sign in to reply to this message.
On Thu, Feb 21, 2013 at 10:54 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > On Thu, Feb 21, 2013 at 10:46 AM, Russ Cox <rsc@golang.org> wrote: > >> Still not sure about the motivation, even after rereading the thread and >> the issue, but I suggest that instead people can just set Opaque = " >> http://y.google.com/asdf" if that's what they want the GET to say. >> > > Oh, without a Scheme. Hm, that could work. Just &net.URL{Opaque: " > http://foo.com/"}. > Actually that doesn't work. The *http.Transport refuses it: ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { log.Printf("Got request for %q", r.RequestURI) })) defer ts.Close() req, _ := http.NewRequest("GET", ts.URL, nil) *req.URL = url.URL{Opaque: ts.URL + "/why-hello-there/%2F/%2F/%2F/"} _, err = http.DefaultClient.Do(req) .... Do: Get http://127.0.0.1:42887/why-hello-there/%2F/%2F/%2F/: unsupported protocol scheme ""
Sign in to reply to this message.
Hi team, I'm not sure I fully understand the subtle differences between RequestURI and String. My only concern here is to be able to make APIs needing "%2F" in the URLs they request work both on and off App Engine. The fix I added on the App Engine side solves the problem in a pretty straight forward way. I'm not sure changing the behavior of the url.URL methods is a good idea. Wouldn't that be a non backward compatible change? It sounds dangerous to me. Francesc Campoy Flores | Go Developer Relations | campoy@google.com | 415-990-4126 On Thu, Feb 21, 2013 at 10:55 AM, Russ Cox <rsc@golang.org> wrote: > I'd rather document that RequestURI returns Opaque unmodified if set. That > gives people complete control and keeps us out of the business of looking > at ostensibly opaque fields. > >
Sign in to reply to this message.
Why not &net.URL{Scheme:"http", Host:"foo.com", Opaque:"http://fo.com/"} ? The scheme and host are needed for Get to know who to dial and how. The opaque is the GET line. Russ
Sign in to reply to this message.
On Thu, Feb 21, 2013 at 11:29 AM, Russ Cox <rsc@golang.org> wrote: > Why not &net.URL{Scheme:"http", Host:"foo.com", Opaque:"http://fo.com/"} ? > > The scheme and host are needed for Get to know who to dial and how. The > opaque is the GET line. > Because: http://play.golang.org/p/IYexHouPVH u := &url.URL{Scheme:"http", Host:"foo.com", Opaque:"http://foo.com/%2F/"} println(u.String()) "http:http://foo.com/%2F/" That's why it's failing on App Engine.
Sign in to reply to this message.
LGTM Sorry, I thought the general form was {scheme}://{host}{opaque}, but I reread the docs and see that it is {scheme}:{opaque}. This CL seems fine now. Weird, but fine.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=67ff484a1801 *** net/http, net/url: deal with URL.Opaque beginning with // Update issue 4860 R=adg, rsc, campoy CC=golang-dev https://codereview.appspot.com/7369045
Sign in to reply to this message.
|