Routes + scope + multiple params with similar matching = problems on replacing params values #13349

Closed
fguillen opened this Issue Dec 17, 2013 · 0 comments

Comments

Projects
None yet
2 participants

I'm in Rails 4.0.2

If I define this route:

scope "/:family/:family_kind" do
  resources :users, :only => [:show], :as => :user_by_family
end

I get this route built:

user_by_family GET    /:family/:family_kind/users/:id(.:format) users#show

Which is right.

But if I try to build a path using this:

user_by_family_path("FAMILY_ID", "FAMILY_KIND", @user)

I get:

/FAMILY_ID/FAMILY_ID_kind/users/1

Be aware how the first value has been used to replace both params in the scope, ignoring the _kind part of the second scope param.

It works properly if I use the explicit version:

user_by_family_path(:family => "FAMILY_ID", :family_kind => "FAMILY_KIND", :id => @user)

pixeltrix was assigned Jan 4, 2014

@pixeltrix pixeltrix added a commit that referenced this issue Jan 5, 2014

@pixeltrix pixeltrix Use a custom route vistor for optimized route generation
Using a Regexp to replace dynamic segments in a path string is fraught
with difficulty and can lead to odd edge cases like #13349. Since we
already have a parsed representation of the path it makes sense to use
that to generate an array of segments that can be used to build an
optimized route's path quickly.

Tests on a simple route (e.g. /posts/:id) show a speedup of 35%:
https://gist.github.com/pixeltrix/8261932

Calculating -------------------------------------
    Current Helper:       5274 i/100ms
    New Helper:           8050 i/100ms
-------------------------------------------------
    Current Helper:     79263.6 (±3.7%) i/s -     395550 in   4.997252s
    New Helper:        153464.5 (±4.9%) i/s -     772800 in   5.047834s

Tests on a more complex route show even an greater performance boost:
https://gist.github.com/pixeltrix/8261957

Calculating -------------------------------------
    Current Helper:       2367 i/100ms
    New Helper:           5382 i/100ms
-------------------------------------------------
    Current Helper:     29506.0 (±3.2%) i/s -     149121 in   5.059294s
    New Helper:         78815.5 (±4.1%) i/s -     398268 in   5.062161s

It also has the added benefit of fixing the edge cases described above.

Fixes #13349
d017e92

pixeltrix closed this in d017e92 Jan 5, 2014

@oliveiraethales oliveiraethales added a commit to oliveiraethales/rails that referenced this issue Jan 5, 2014

@pixeltrix @oliveiraethales pixeltrix + oliveiraethales # This is a combination of 2 commits.
# The first commit's message is:

Use a custom route vistor for optimized route generation

Using a Regexp to replace dynamic segments in a path string is fraught
with difficulty and can lead to odd edge cases like #13349. Since we
already have a parsed representation of the path it makes sense to use
that to generate an array of segments that can be used to build an
optimized route's path quickly.

Tests on a simple route (e.g. /posts/:id) show a speedup of 35%:
https://gist.github.com/pixeltrix/8261932

Calculating -------------------------------------
    Current Helper:       5274 i/100ms
    New Helper:           8050 i/100ms
-------------------------------------------------
    Current Helper:     79263.6 (±3.7%) i/s -     395550 in   4.997252s
    New Helper:        153464.5 (±4.9%) i/s -     772800 in   5.047834s

Tests on a more complex route show even an greater performance boost:
https://gist.github.com/pixeltrix/8261957

Calculating -------------------------------------
    Current Helper:       2367 i/100ms
    New Helper:           5382 i/100ms
-------------------------------------------------
    Current Helper:     29506.0 (±3.2%) i/s -     149121 in   5.059294s
    New Helper:         78815.5 (±4.1%) i/s -     398268 in   5.062161s

It also has the added benefit of fixing the edge cases described above.

Fixes #13349

# This is the 2nd commit message:

When deleting a previously deleted model, returns and do nothing

	modified:   CHANGELOG.md
	modified:   lib/rails/generators/migration.rb
	modified:   test/generators/model_generator_test.rb
c9bf1dd

@pixeltrix pixeltrix added a commit that referenced this issue Feb 11, 2014

@pixeltrix pixeltrix Use a custom route vistor for optimized route generation
Using a Regexp to replace dynamic segments in a path string is fraught
with difficulty and can lead to odd edge cases like #13349. Since we
already have a parsed representation of the path it makes sense to use
that to generate an array of segments that can be used to build an
optimized route's path quickly.

Tests on a simple route (e.g. /posts/:id) show a speedup of 35%:
https://gist.github.com/pixeltrix/8261932

Calculating -------------------------------------
    Current Helper:       5274 i/100ms
    New Helper:           8050 i/100ms
-------------------------------------------------
    Current Helper:     79263.6 (±3.7%) i/s -     395550 in   4.997252s
    New Helper:        153464.5 (±4.9%) i/s -     772800 in   5.047834s

Tests on a more complex route show even an greater performance boost:
https://gist.github.com/pixeltrix/8261957

Calculating -------------------------------------
    Current Helper:       2367 i/100ms
    New Helper:           5382 i/100ms
-------------------------------------------------
    Current Helper:     29506.0 (±3.2%) i/s -     149121 in   5.059294s
    New Helper:         78815.5 (±4.1%) i/s -     398268 in   5.062161s

It also has the added benefit of fixing the edge cases described above.

Fixes #13349

(cherry picked from commit d017e92)

Conflicts:
	actionpack/CHANGELOG.md
	actionpack/lib/action_dispatch/routing/route_set.rb
ccb0301
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment