rails / journey

A router for rails
221 stars 57 forks source link

Journey accepts empty strings for required parts #36

Closed pixeltrix closed 7 years ago

pixeltrix commented 12 years ago

Although 70d101b3386ba26ff801cec496da9c23389434aa fixes the generation of URLs when passed nil required params it still works with empty strings, e.g:

get '/foo/:bar', :to => 'foo#bar', :as => :foobar
>> app.foobar_path("")
=> "/foo/"

Rack::Mount raises an error in the same situation, so is a regression from Rails 3.1 to Rails 3.2. This can be fixed by changing verified_required_parts! to:

def verify_required_parts! route, parts
  tests = route.path.requirements
  route.required_parts.all? { |key|
    if tests.key? key
      /\A#{tests[key]}\Z/ === parts[key]
    elsif parts[key].to_s.empty?
      false
    else
      true
    end
  }
end

If this is acceptable I can push the fix and tests (I'll also add an integration test in Rails). I have write access to the repository but I thought I'd run it past you first out of courtesy. :smile:

schneems commented 12 years ago

Looks like I accidentally fixed this issue in PR #44. Would you mind looking over that one and giving me some feedback?

I can add a test for this regression as well if you would like:

      route_name = "foobar"
      add_routes @router, [
        Router::Strexp.new("/foo/:id", { :id => /\d+/ }, ['/', '.', '?'], false)
      ], :name => route_name

      assert_raises(Journey::Router::RoutingError) do
        @formatter.generate(:path_info, route_name, {}, {})
      end
schneems commented 12 years ago

Were you able to take a look at #44? Did you have any problems, questions or issues with the proposed code change?

If #44 cannot be implemented for a technical or scope reason, this change looks good to me. Otherwise, we should close this issue out.

pixeltrix commented 7 years ago

Closing because it's fixed 😄