Skip to content

Commit

Permalink
Move Path::Pattern factories into test helper
Browse files Browse the repository at this point in the history
`Pattern#from_sting` was introduced in bb207ea to support creating
patterns from strings in tests (removing a conditional in the initialize
method that had been there only for the sake of those tests).

`Pattern#build` was introduced in 947ebe9, and was similarly used only
in tests.

My understanding is that [`Mapping#path`][path] is the only place where
we initialize a `Path::Pattern` in library code.

Since these two methods appear to be used only in tests, this
commit moves them out of the class and into a test helper.

My reasoning for doing this is that I am doing some performance work
that may involve a modification to how `Path::Pattern`s get initialized,
and I will have more confidence in that work if I know for sure that
these methods are test helpers that can be modified freely.

[path]: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/mapper.rb#L192-L194
  • Loading branch information
composerinteralia committed Jul 27, 2020
1 parent a9336a6 commit 8eec6ee
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 46 deletions.
10 changes: 0 additions & 10 deletions actionpack/lib/action_dispatch/journey/path/pattern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,6 @@ module Path # :nodoc:
class Pattern # :nodoc:
attr_reader :spec, :requirements, :anchored

def self.from_string(string)
build(string, {}, "/.?", true)
end

def self.build(path, requirements, separators, anchored)
parser = Journey::Parser.new
ast = parser.parse path
new ast, requirements, separators, anchored
end

def initialize(ast, requirements, separators, anchored)
@spec = ast
@requirements = requirements
Expand Down
51 changes: 27 additions & 24 deletions actionpack/test/journey/path/pattern_test.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
# frozen_string_literal: true

require "abstract_unit"
require "support/path_helper"

module ActionDispatch
module Journey
module Path
class TestPattern < ActiveSupport::TestCase
include PathHelper

SEPARATORS = ["/", ".", "?"].join

x = /.+/
Expand All @@ -23,7 +26,7 @@ class TestPattern < ActiveSupport::TestCase
"/:foo|*bar" => %r{\A/(?:([^/.?]+)|(.+))\Z},
}.each do |path, expected|
define_method(:"test_to_regexp_#{Regexp.escape(path)}") do
path = Pattern.build(
path = build_path(
path,
{ controller: /.+/ },
SEPARATORS,
Expand All @@ -47,7 +50,7 @@ class TestPattern < ActiveSupport::TestCase
"/:foo|*bar" => %r{\A/(?:([^/.?]+)|(.+))(?:\b|\Z|/)},
}.each do |path, expected|
define_method(:"test_to_non_anchored_regexp_#{Regexp.escape(path)}") do
path = Pattern.build(
path = build_path(
path,
{ controller: /.+/ },
SEPARATORS,
Expand All @@ -70,7 +73,7 @@ class TestPattern < ActiveSupport::TestCase
"/:controller/*foo/bar" => %w{ controller foo },
}.each do |path, expected|
define_method(:"test_names_#{Regexp.escape(path)}") do
path = Pattern.build(
path = build_path(
path,
{ controller: /.+/ },
SEPARATORS,
Expand All @@ -81,7 +84,7 @@ class TestPattern < ActiveSupport::TestCase
end

def test_to_regexp_with_extended_group
path = Pattern.build(
path = build_path(
"/page/:name",
{ name: /
#ROFL
Expand All @@ -102,13 +105,13 @@ def test_optional_names
["/:foo(/:bar)", %w{ bar }],
["/:foo(/:bar)/:lol(/:baz)", %w{ bar baz }],
].each do |pattern, list|
path = Pattern.from_string pattern
path = path_from_string pattern
assert_equal list.sort, path.optional_names.sort
end
end

def test_to_regexp_match_non_optional
path = Pattern.build(
path = build_path(
"/:name",
{ name: /\d+/ },
SEPARATORS,
Expand All @@ -119,7 +122,7 @@ def test_to_regexp_match_non_optional
end

def test_to_regexp_with_group
path = Pattern.build(
path = build_path(
"/page/:name",
{ name: /(tender|love)/ },
SEPARATORS,
Expand All @@ -132,7 +135,7 @@ def test_to_regexp_with_group

def test_ast_sets_regular_expressions
requirements = { name: /(tender|love)/, value: /./ }
path = Pattern.build(
path = build_path(
"/page/:name/:value",
requirements,
SEPARATORS,
Expand All @@ -147,7 +150,7 @@ def test_ast_sets_regular_expressions
end

def test_match_data_with_group
path = Pattern.build(
path = build_path(
"/page/:name",
{ name: /(tender|love)/ },
SEPARATORS,
Expand All @@ -159,7 +162,7 @@ def test_match_data_with_group
end

def test_match_data_with_multi_group
path = Pattern.build(
path = build_path(
"/page/:name/:id",
{ name: /t(((ender|love)))()/ },
SEPARATORS,
Expand All @@ -174,7 +177,7 @@ def test_match_data_with_multi_group

def test_star_with_custom_re
z = /\d+/
path = Pattern.build(
path = build_path(
"/page/*foo",
{ foo: z },
SEPARATORS,
Expand All @@ -184,7 +187,7 @@ def test_star_with_custom_re
end

def test_insensitive_regexp_with_group
path = Pattern.build(
path = build_path(
"/page/:name/aaron",
{ name: /(tender|love)/i },
SEPARATORS,
Expand All @@ -196,27 +199,27 @@ def test_insensitive_regexp_with_group
end

def test_to_regexp_with_strexp
path = Pattern.build("/:controller", {}, SEPARATORS, true)
path = build_path("/:controller", {}, SEPARATORS, true)
x = %r{\A/([^/.?]+)\Z}

assert_equal(x.source, path.source)
end

def test_to_regexp_defaults
path = Pattern.from_string "/:controller(/:action(/:id))"
path = path_from_string "/:controller(/:action(/:id))"
expected = %r{\A/([^/.?]+)(?:/([^/.?]+)(?:/([^/.?]+))?)?\Z}
assert_equal expected, path.to_regexp
end

def test_failed_match
path = Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
uri = "content"

assert_not path =~ uri
end

def test_match_controller
path = Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
uri = "/content"

match = path =~ uri
Expand All @@ -228,7 +231,7 @@ def test_match_controller
end

def test_match_controller_action
path = Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
uri = "/content/list"

match = path =~ uri
Expand All @@ -240,7 +243,7 @@ def test_match_controller_action
end

def test_match_controller_action_id
path = Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
uri = "/content/list/10"

match = path =~ uri
Expand All @@ -252,7 +255,7 @@ def test_match_controller_action_id
end

def test_match_literal
path = Path::Pattern.from_string "/books(/:action(.:format))"
path = path_from_string "/books(/:action(.:format))"

uri = "/books"
match = path =~ uri
Expand All @@ -262,7 +265,7 @@ def test_match_literal
end

def test_match_literal_with_action
path = Path::Pattern.from_string "/books(/:action(.:format))"
path = path_from_string "/books(/:action(.:format))"

uri = "/books/list"
match = path =~ uri
Expand All @@ -272,7 +275,7 @@ def test_match_literal_with_action
end

def test_match_literal_with_action_and_format
path = Path::Pattern.from_string "/books(/:action(.:format))"
path = path_from_string "/books(/:action(.:format))"

uri = "/books/list.rss"
match = path =~ uri
Expand All @@ -282,7 +285,7 @@ def test_match_literal_with_action_and_format
end

def test_named_captures
path = Path::Pattern.from_string "/books(/:action(.:format))"
path = path_from_string "/books(/:action(.:format))"

uri = "/books/list.rss"
match = path =~ uri
Expand All @@ -293,7 +296,7 @@ def test_named_captures
def test_requirements_for_missing_keys_check
name_regex = /test/

path = Pattern.build(
path = build_path(
"/page/:name",
{ name: name_regex },
SEPARATORS,
Expand All @@ -308,7 +311,7 @@ def test_requirements_for_missing_keys_check
def test_requirements_for_missing_keys_check_memoization
name_regex = /test/

path = Pattern.build(
path = build_path(
"/page/:name",
{ name: name_regex },
SEPARATORS,
Expand Down
27 changes: 15 additions & 12 deletions actionpack/test/journey/route_test.rb
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
# frozen_string_literal: true

require "abstract_unit"
require "support/path_helper"

module ActionDispatch
module Journey
class TestRoute < ActiveSupport::TestCase
include PathHelper

def test_initialize
app = Object.new
path = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
defaults = {}
route = Route.new(name: "name", app: app, path: path, defaults: defaults)

Expand All @@ -18,7 +21,7 @@ def test_initialize

def test_route_adds_itself_as_memo
app = Object.new
path = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
route = Route.new(name: "name", app: app, path: path)

route.ast.grep(Nodes::Terminal).each do |node|
Expand All @@ -27,28 +30,28 @@ def test_route_adds_itself_as_memo
end

def test_path_requirements_override_defaults
path = Path::Pattern.build(":name", { name: /love/ }, "/", true)
path = build_path(":name", { name: /love/ }, "/", true)
defaults = { name: "tender" }
route = Route.new(name: "name", path: path, defaults: defaults)
assert_equal(/love/, route.requirements[:name])
end

def test_ip_address
path = Path::Pattern.from_string "/messages/:id(.:format)"
path = path_from_string "/messages/:id(.:format)"
route = Route.new(name: "name", path: path, constraints: { ip: "192.168.1.1" },
defaults: { controller: "foo", action: "bar" })
assert_equal "192.168.1.1", route.ip
end

def test_default_ip
path = Path::Pattern.from_string "/messages/:id(.:format)"
path = path_from_string "/messages/:id(.:format)"
route = Route.new(name: "name", path: path,
defaults: { controller: "foo", action: "bar" })
assert_equal(//, route.ip)
end

def test_format_with_star
path = Path::Pattern.from_string "/:controller/*extra"
path = path_from_string "/:controller/*extra"
route = Route.new(name: "name", path: path,
defaults: { controller: "foo", action: "bar" })
assert_equal "/foo/himom", route.format(
Expand All @@ -57,7 +60,7 @@ def test_format_with_star
end

def test_connects_all_match
path = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
path = path_from_string "/:controller(/:action(/:id(.:format)))"
route = Route.new(name: "name", path: path, constraints: { action: "bar" },
defaults: { controller: "foo" })

Expand All @@ -68,21 +71,21 @@ def test_connects_all_match
end

def test_extras_are_not_included_if_optional
path = Path::Pattern.from_string "/page/:id(/:action)"
path = path_from_string "/page/:id(/:action)"
route = Route.new(name: "name", path: path, defaults: { action: "show" })

assert_equal "/page/10", route.format(id: 10)
end

def test_extras_are_not_included_if_optional_with_parameter
path = Path::Pattern.from_string "(/sections/:section)/pages/:id"
path = path_from_string "(/sections/:section)/pages/:id"
route = Route.new(name: "name", path: path, defaults: { action: "show" })

assert_equal "/pages/10", route.format(id: 10)
end

def test_extras_are_not_included_if_optional_parameter_is_nil
path = Path::Pattern.from_string "(/sections/:section)/pages/:id"
path = path_from_string "(/sections/:section)/pages/:id"
route = Route.new(name: "name", path: path, defaults: { action: "show" })

assert_equal "/pages/10", route.format(id: 10, section: nil)
Expand All @@ -91,10 +94,10 @@ def test_extras_are_not_included_if_optional_parameter_is_nil
def test_score
defaults = { controller: "pages", action: "show" }

path = Path::Pattern.from_string "/page/:id(/:action)(.:format)"
path = path_from_string "/page/:id(/:action)(.:format)"
specific = Route.new name: "name", path: path, required_defaults: [:controller, :action], defaults: defaults

path = Path::Pattern.from_string "/:controller(/:action(/:id))(.:format)"
path = path_from_string "/:controller(/:action(/:id))(.:format)"
generic = Route.new name: "name", path: path

knowledge = { "id" => true, "controller" => true, "action" => true }
Expand Down
22 changes: 22 additions & 0 deletions actionpack/test/support/path_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# frozen_string_literal: true

module ActionDispatch
module Journey
module PathHelper
def path_from_string(string)
build_path(string, {}, "/.?", true)
end

def build_path(path, requirements, separators, anchored)
parser = ActionDispatch::Journey::Parser.new
ast = parser.parse path
ActionDispatch::Journey::Path::Pattern.new(
ast,
requirements,
separators,
anchored
)
end
end
end
end

0 comments on commit 8eec6ee

Please sign in to comment.