Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Correct handling of "/:name.?:format?" and "/:user@?:host?" #492

Merged
merged 16 commits into from Jul 12, 2012

Conversation

Projects
None yet
4 participants
Contributor

floere commented Mar 23, 2012

This pull request adds the correct handling of the following URL path patterns to Sinatra. These are the patterns which this is about:

 Pattern             | Old Regexp                                               | Example                          | Should Be
 "/:name.?:format?"  | /^\/([^\/?#]+)(?:\.|%2E)?([^\/?#]+)?$/                   | "/foo.bar"                       | ["foo", "bar"]
 "/:name.?:format?"  | /^\/([^\/?#]+)(?:\.|%2E)?([^\/?#]+)?$/                   | "/foo%2Ebar"                     | ["foo", "bar"]                  
 "/:user@?:host?"    | /^\/([^\/?#]+)(?:@|%40)?([^\/?#]+)?$/                    | "/foo@bar"                       | ["foo", "bar"]                    
 "/:user@?:host?"    | /^\/([^\/?#]+)(?:@|%40)?([^\/?#]+)?$/                    | "/foo.foo@bar"                   | ["foo.foo", "bar"]                
 "/:user@?:host?"    | /^\/([^\/?#]+)(?:@|%40)?([^\/?#]+)?$/                    | "/foo@bar.bar"                   | ["foo", "bar.bar"]

Why? / History

This is the result of a long discussion starting here:
https://twitter.com/#!/konstantinhaase/status/182480862326165504

The discussion being here:
https://gist.github.com/2154980

What? / Code changes

I mainly touched the base.rb file, specifically the Base#compile method. My plan was to generalize some of the other cases to keep the needed changes to the code low. This plan resulted in 2 other patterns -> regexps being changed (and imho improved).

I added a quite comprehensive spec in the file compile_test.rb, which can be run with ruby test/compile_test.rb.

Possible discussion worthy points

Using /^...$/ instead of /\A...\z/ regexps

One bigger change is found in switching from /^...$/ to /\A...\z/, i.e. switching from line-based matching to global matching with anchoring at the beginning of the string. Two reasons:

  1. I believe it to be "more correct".
  2. It is thought to be more performant (for some cases, see http://www.ruby-doc.org/core-1.9.3/Regexp.html).

If this is a problem, please discuss.

Pattern "/:name.?:format?" question

I was wondering about one pattern (see header) – on an example of "/.bar" is supposed to result in [".bar", nil]. This seems strange to me – it seems to me as if it should result in a non-match, i.e. nil.
I changed the tests to reflect my line of thought. If this is untrue, please tell me why and I'll change the tests and add the commit to this pull request.

Thanks for reading! If there are any cleanups to be done, please let me know.

@rkh rkh commented on the diff Mar 23, 2012

lib/sinatra/base.rb
@@ -1302,17 +1302,21 @@ def compile!(verb, path, block, options = {})
def compile(path)
keys = []
if path.respond_to? :to_str
- pattern = path.to_str.gsub(/[^\?\%\\\/\:\*\w]/) { |c| encoded(c) }
+ ignore = ""
+ pattern = path.to_str.gsub(/[^\?\%\\\/\:\*\w]/) do |c|
+ ignore << escaped(c).join if c.match(/[\.@]/)
@rkh

rkh Mar 23, 2012

Owner

I think we might also want this for other symbols, like :, so maybe remove the if or something?

@floere

floere Mar 23, 2012

Contributor

Sure, I can do that. Do you have more example patterns -> regexps?

Owner

rkh commented Mar 23, 2012

With this patch, does "/:foo" still parse "/foo.bar"?

Contributor

floere commented Mar 23, 2012

(That is my answer :) )

Contributor

floere commented Mar 23, 2012

As a note, the tests basically contain the list here: https://gist.github.com/2154980#gistcomment-168954 (with some additions)

The case "/:foo" parses "/foo.bar" wasn't in there which is why I added it.

Contributor

floere commented Mar 27, 2012

How are we going to proceed with this? Are there any stopping points from merging? If yes, what are they?

Owner

rkh commented Mar 27, 2012

Sorry, it's mainly a time management thing, I'd like to go over the code, again, refactor the tests, play around with it, run it in some real apps, and such. I want this to be part of the 1.4.0 release, as it is a behavioral change, but I'm also planning to have a 1.3.3 release first.

ests commented Apr 10, 2012

+1, this is the functionality I would love to see merged too. Consuming API webservice I've written in Sinatra, by using AcitveResource was a bit of pain, because by default AR generates request URIs like "/user/1.json" and ect.

Owner

rkh commented Apr 10, 2012

This will be merged in for 1.4.0. I will take care of this during or after RailsConf.

Contributor

floere commented Apr 11, 2012

@rkh Thanks for the info. If you need changes (i.e. how do the test need to be refactored?), don't hesitate to tell me.

Owner

rkh commented Apr 11, 2012

I don't like the giant_array.each { ... } approach and would rather have some minimal DSL there.

Contributor

floere commented Apr 11, 2012

I'm surprised. What's the reasoning? Just personal style?

Owner

rkh commented Apr 11, 2012

Readability, easier to grab what's going on for someone new joining the project. You actually like those arrays nested in arrays nested in arrays with a bunch of code right below it?

Contributor

floere commented Apr 11, 2012

Let's move this in a constructive direction.

DSL example:

pattern_converts_into "/:name.?:format?", /^\/([^\/?#]+)(?:\.|%2E)?([^\/?#]+)?$/
pattern_resolves "/:name.?:format?", "/foo.bar", ["foo", "bar"]

Let's go from here.

Owner

rkh commented Apr 11, 2012

That's better, but I was thinking of maybe actually generating routes and testing if a requests goes through and what params looks like. That's the only way to actually make sure it works.

Something like this:

parses_pattern("/:name.?:format?", "/foo.bar", "name" => "foo", "format" => "bar")
does_not_parse("/:name.:format", "/")

Not sure about the method names.

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

Contributor

floere commented Apr 19, 2012

Undecided? ;)

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

@rkh rkh closed this Apr 19, 2012

@rkh rkh reopened this Apr 19, 2012

Owner

rkh commented Apr 19, 2012

Ah, sorry, this was not related to the Pull Request. We tried to figure out why github is not sending out pubsub notifications for some repos/pull requests.

Contributor

floere commented Apr 19, 2012

No worries.

This pull request fails (merged dfa8409 into b882ab3).

Contributor

floere commented Apr 26, 2012

Ok. This pull request seems to be 1.9.2 only. Shall I have a look?

Owner

rkh commented Apr 26, 2012

That'd be great. Ignore the Puma failure (fixed in master).

This pull request fails (merged 540b171 into b882ab3).

Contributor

floere commented Jun 17, 2012

@travisbot Not sure who fails here ;)

Error: #<NativeException: org.virtualbox_4_1.VBoxException:
The function "powerDown" returned an error condition:
"The virtual machine is being powered down"  (0x80bb0002)>

This pull request fails (merged be59b2b into b882ab3).

This pull request passes (merged 9f08499 into b882ab3).

This pull request fails (merged 98ef21b into b882ab3).

This pull request fails (merged 57fc08d into b882ab3).

Contributor

floere commented Jun 18, 2012

@travisbot Well, it "fails": SIGSEGV (0xb) at pc=0xb19c633b, pid=2081, tid=3078609776

Anyway, @rkh, I believe we're good to go afaics.

This pull request passes (merged 57babc5 into b882ab3).

This pull request fails (merged 6462a00 into b882ab3).

ests commented Jun 19, 2012

Having notifications turned on is no longer nice, got spammed by Mr. Travisbot ;)

This pull request fails (merged de21260 into b882ab3).

Contributor

floere commented Jun 20, 2012

@ests Heh, I agree. See below "Disable notifications for this Pull Request".

Contributor

floere commented Jun 20, 2012

Also, the order of comments is out of sync. The one that passes (3rd last) is the latest one.

Owner

rkh commented Jun 20, 2012

Sorry, we are looking into optimizing @travisbot comments.

Contributor

floere commented Jun 20, 2012

Eh, no worries. I wonder though why they can be that much out of order. Isn't some sort of queue used?

Owner

rkh commented Jun 20, 2012

Not sure. We have them in RabbitMQ. We had some issues with Heroku and other services being down a lot the last few days, maybe something went wrong there.

Contributor

floere commented Jun 20, 2012

Even the it's strange it's out of order, don't you think? OTOH, it's not that important (to me).

Contributor

floere commented Jun 25, 2012

Anything stopping this one from being pulled?

Owner

rkh commented Jun 25, 2012

Me spending too much time at conferences. I'll look into this this week, potentially today.

Contributor

floere commented Jun 25, 2012

Hehe, thanks for the quick response. I was mainly wondering whether I can remove this from my TODO list.

Contributor

floere commented Jul 12, 2012

Still spending too much time at conferences?

The changes (https://github.com/sinatra/sinatra/pull/492/files) are pretty small – and beautify the tests quite a bit. If I can help with the review, let me know.

This pull request fails (merged e59b62e into b882ab3).

Contributor

floere commented Jul 12, 2012

Again, it doesn't really fail – it segfaults (ie. we don't know whether it would fail).

@rkh rkh added a commit that referenced this pull request Jul 12, 2012

@rkh rkh Merge pull request #492 from floere/master
Correct handling of "/:name.?:format?" and "/:user@?:host?"
762967f

@rkh rkh merged commit 762967f into sinatra:master Jul 12, 2012

Owner

rkh commented Jul 12, 2012

Thanks for all the effort. :)

Contributor

floere commented Jul 12, 2012

My pleasure. Note that since the pattern behaviour changes, some apps might break who have misused the patterns that now work correctly. (I did not know where to describe that though, and forgot to ask, I'm afraid)

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