decode path info #177

Merged
merged 1 commit into from Jul 19, 2014

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Jul 15, 2014

before this change a path_info fragment would enter a resource
as an encoded value:

{
:fragment => 'hello%20world'
}

I would expect to receive the value differently:
{
:fragment => 'hello world'
}

@ghost

View changes

spec/spec_helper.rb
@@ -14,7 +14,7 @@
config.before(:suite) do
options = {
- :Logger => Logger.new("/dev/null"),
+ :Logger => (File.exist?('/dev/null') ? Logger.new('/dev/null') : nil),

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jul 15, 2014

this is from a different pull request. the tests don't run on Windows7 without it, so I couldn't make this change without this commit. I will rebase and remove it from this PR.

@ghost

ghost Jul 15, 2014

this is from a different pull request. the tests don't run on Windows7 without it, so I couldn't make this change without this commit. I will rebase and remove it from this PR.

Robert Gleeson
decode path_info fragment(s).
before this change a path_info fragment would enter a resource
as an encoded value:

{
  :fragment => 'hello%20world'
}

I would expect to receive the value differently:
{
  :fragment => 'hello world'
}

@ghost ghost referenced this pull request Jul 15, 2014

Closed

decode wildcard path. #178

@ghost ghost changed the title from Decode path info to decode path info Jul 15, 2014

@ghost ghost referenced this pull request Jul 16, 2014

Closed

decode wildcard path. #179

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jul 19, 2014

I think this change is reasonable, but small enough if you'd prefer to do it another way.

ghost commented Jul 19, 2014

I think this change is reasonable, but small enough if you'd prefer to do it another way.

@seancribbs

This comment has been minimized.

Show comment Hide comment
@seancribbs

seancribbs Jul 19, 2014

Member

Where did you commit/push this? It says "unknown repository" at the top of the pull request.

Member

seancribbs commented Jul 19, 2014

Where did you commit/push this? It says "unknown repository" at the top of the pull request.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jul 19, 2014

Oh right, I pushed to my fork which I deleted. Is it not merge able?

ghost commented Jul 19, 2014

Oh right, I pushed to my fork which I deleted. Is it not merge able?

@seancribbs

This comment has been minimized.

Show comment Hide comment
@seancribbs

seancribbs Jul 19, 2014

Member

I think the ref is still there, it just looks very odd.

Member

seancribbs commented Jul 19, 2014

I think the ref is still there, it just looks very odd.

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jul 19, 2014

yeah, I deleted the repo without thinking of these PRs

ghost commented Jul 19, 2014

yeah, I deleted the repo without thinking of these PRs

@seancribbs

This comment has been minimized.

Show comment Hide comment
@seancribbs

seancribbs Jul 19, 2014

Member

👍 merging

Member

seancribbs commented Jul 19, 2014

👍 merging

seancribbs added a commit that referenced this pull request Jul 19, 2014

@seancribbs seancribbs merged commit f3f876f into webmachine:master Jul 19, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jul 19, 2014

cool thanks

ghost commented Jul 19, 2014

cool thanks

@bethesque

This comment has been minimized.

Show comment Hide comment
@bethesque

bethesque Sep 19, 2014

Contributor

Thanks! I was about to work on this myself, but I've just found your commit. @seancribbs , do you have a release date planned?

Contributor

bethesque commented on f063e3b Sep 19, 2014

Thanks! I was about to work on this myself, but I've just found your commit. @seancribbs , do you have a release date planned?

This comment has been minimized.

Show comment Hide comment
@samwgoldman

samwgoldman Sep 20, 2014

Contributor

Is the CGI encoding used here the same as the percent encoding defined in the spec? I couldn't find anything conclusive, but I believe these are slightly different encodings. The addressable gem provides a decode_component method that purports to do the right thing.

Contributor

samwgoldman replied Sep 20, 2014

Is the CGI encoding used here the same as the percent encoding defined in the spec? I couldn't find anything conclusive, but I believe these are slightly different encodings. The addressable gem provides a decode_component method that purports to do the right thing.

This comment has been minimized.

Show comment Hide comment
@bethesque

bethesque Sep 20, 2014

Contributor

That's a good point @samwgoldman

URI.decode("%20+")
=> " +"

CGI::unescape("%20+")
=> "  "

URI.encode("+")
=> "+"

According to http://en.wikipedia.org/wiki/Percent-encoding, the "+" representation of a space is part of the application/x-www-form-urlencoded spec, and therefore is only relevant to the query string party of a URL.

As CGI.unescape decodes a + to a space, I would think that it should be used for query strings, not URL components. I suggest that URI.decode should be used instead of CGI.unescape for our purposes.

@samwgoldman, I couldn't find the decode_component method in the https://github.com/sporkmonger/addressable codebase, do you have a link somewhere?

Contributor

bethesque replied Sep 20, 2014

That's a good point @samwgoldman

URI.decode("%20+")
=> " +"

CGI::unescape("%20+")
=> "  "

URI.encode("+")
=> "+"

According to http://en.wikipedia.org/wiki/Percent-encoding, the "+" representation of a space is part of the application/x-www-form-urlencoded spec, and therefore is only relevant to the query string party of a URL.

As CGI.unescape decodes a + to a space, I would think that it should be used for query strings, not URL components. I suggest that URI.decode should be used instead of CGI.unescape for our purposes.

@samwgoldman, I couldn't find the decode_component method in the https://github.com/sporkmonger/addressable codebase, do you have a link somewhere?

This comment has been minimized.

Show comment Hide comment
@samwgoldman

samwgoldman Sep 20, 2014

Contributor
Contributor

samwgoldman replied Sep 20, 2014

This comment has been minimized.

Show comment Hide comment
@bethesque

bethesque Sep 20, 2014

Contributor

I've tried to work out what the exact difference is between Addressable::URI.unencode_component and URI.decode, but I can't find anything helpful so far. I had a bit of a play around encoding and decoding various random strings, but haven't found anything. This stackoverflow post was quite useful, but didn't give me the exact answer we're after http://stackoverflow.com/a/13059657

I guess the question is, is it worth including an extra dependency (or copying the unencode_component method into the Webmachine codebase), or would URI.decode do?

Contributor

bethesque replied Sep 20, 2014

I've tried to work out what the exact difference is between Addressable::URI.unencode_component and URI.decode, but I can't find anything helpful so far. I had a bit of a play around encoding and decoding various random strings, but haven't found anything. This stackoverflow post was quite useful, but didn't give me the exact answer we're after http://stackoverflow.com/a/13059657

I guess the question is, is it worth including an extra dependency (or copying the unencode_component method into the Webmachine codebase), or would URI.decode do?

This comment has been minimized.

Show comment Hide comment
@bethesque

bethesque Sep 23, 2014

Contributor

I've opened a PR to use URI.decode here seancribbs#191

If we discover that URI.decode has it's own issues, we can look at the Addressable implementation.

Contributor

bethesque replied Sep 23, 2014

I've opened a PR to use URI.decode here seancribbs#191

If we discover that URI.decode has it's own issues, we can look at the Addressable implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment