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

net/url: PathEscape and PathUnescape functions #13737

Closed
nigeltao opened this issue Dec 27, 2015 · 9 comments
Closed

net/url: PathEscape and PathUnescape functions #13737

nigeltao opened this issue Dec 27, 2015 · 9 comments
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nigeltao
Copy link
Contributor

Should we add PathEscape and PathUnescape functions, similar to QueryEscape and QueryUnescape? The functionality is already present (see func (*URL) EscapedPath), but it is awkward to use.

See https://groups.google.com/forum/#!topic/golang-dev/UDUqvuKrq14 for golang-dev discussion

@nigeltao nigeltao added this to the Go1.7 milestone Dec 27, 2015
@rsc
Copy link
Contributor

rsc commented Jan 6, 2016

I mailed with r.w.johnstone off list about this, and for his application he only needs to escape path elements to use when building URLs. For that, QueryEscape works fine, and is what other existing code uses as far as I know. Maybe instead of new API we should document that fact.

@jmhodges
Copy link
Contributor

There are a few places where QueryEscape doesn't quite work. Notably, spaces are to be encoded as "%20", instead of "+", when used in paths.

@bradfitz
Copy link
Contributor

bradfitz commented Apr 2, 2016

@jmhodges, fair enough. Want to send a change?

@jmhodges
Copy link
Contributor

jmhodges commented Apr 2, 2016

Yeah, was trying to pawn it off on a friend but he didn't bite.

@jmhodges
Copy link
Contributor

jmhodges commented Apr 2, 2016

So, I'm digging in here and trying to figure out how far to take this. I've come up with 4 options I could implement but none of them are super satisfying.

We have the concept of encoding in the code base. The obvious but non-working change would be to reuse the current escape func for PathSegmentEscape with the encodePath encoding. Doing so, however, would cause all of the / in the user's path segment to remain as / instead of being turned into %2F. This is because the shouldEscape func called in escape knows about encodePath and expects a full path to be given to it. We also can't just pass the encodeQueryComponent mode to escape because that's been special cased to return + when it sees a space.

The first option is to create a special mode called encodePathSegment to be used by escape and shouldEscape and only use it from PathSegmentEscape. That new code would never be seen by the other path escaping routines, however. This means we might introduce some inaccuracies about how we treat full paths in EscapedPath and validEncodedPath versus path segments with PathSegmentPath.

A second option is to teach the places calling escape(..., encodePath) and shouldEscape(..., encodePath) directly that they need to skip over / themselves and make encodePath really be about path segments and not the full path. We'd also, of course, have to teach shouldEscape(..., encodePath) that / should be encoded and the places calling shouldEscape would, too. The places that need to be taught are validEncodedPath, and URL.EscapedPath.

A third option is for escape itself to do the / skipping in encodePath mode while shouldEscape learns to always escape / when it sees it with encodePath. This would involve escape checking for '/' in both of its for-loops before calling shouldEscape(..., encodePath) and the other places that call shouldEscape(..., encodePath) (just validEncodedPath, currently) would have to do the same. It would probably be wise, in this case, to turn the encodePath references in shouldEscape to encodePathSegment and have escape only ever call shouldEscape with encodePathSegment instead of encodePath.

The last option I came up with is for PathSegmentEscape to call shouldEscape directly and not use escape at all. This would let us special case / but at the cost of duplicating the escape function's logic for quick returns and hex string length.

I'm pretty torn on these 4 options. Does anyone have a preference or alternative idea?

@jmhodges
Copy link
Contributor

jmhodges commented Apr 2, 2016

I'm leaning toward the third, but having a encoding that was just for escape and unescape, but not allowed in shouldEncode seemed like maybe a place for misuse.

@bradfitz
Copy link
Contributor

bradfitz commented Apr 2, 2016

I'm also fine with you creating an entirely new set of functions not reusing anything that's currently existing. That might even be safest for now. Write sufficient tests and we can refactor later.

I have a plan to create a new shared package golang.org/x/net/lex for all lexical matters & tables of the various RFCs about URLs and HTTP so http2 and http1 and net/url can share them, so I can do the deduplication later when lex exists.

@jmhodges
Copy link
Contributor

jmhodges commented Apr 4, 2016

Yeah, I'm on it. Option 3 for now because I'm not feeling sassy enough.

Oh man, a net/lex would be handy.

@bradfitz bradfitz modified the milestones: Go1.8, Go1.7 May 10, 2016
@bradfitz bradfitz removed their assignment May 10, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@gopherbot
Copy link

CL https://golang.org/cl/31322 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants