Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route constraints with an array do not allow building a path only parsing a path. #47726

Open
urkle opened this issue Mar 21, 2023 · 6 comments

Comments

@urkle
Copy link
Contributor

urkle commented Mar 21, 2023

Steps to reproduce

# Given a route like this.
MyApplication::Application.routes.draw do
    get '/download/:platform' => 'download#platform',
        constraints: {
          platform: %w[windows linux macos],
        },
        as: :download_platform
end

Expected behavior

Routing test (rspec)

expect(get("/download/windows")).to route_to('download#platform',
                                                         platform: platform)

url building test (rspec)

def some_function(platform)
    download_platform_path(platform: platform)
end

expect(some_function('windows')).to eq(download_platform_path('windows'))

Actual behavior

The routing spec passes, but the url building test fails with a No route matches ... possible unmatched constraints: [:platform] error.

However,

If I change the constraint to this

MyApplication::Application.routes.draw do
    get '/download/:platform' => 'download#platform',
        constraints: {
          platform: /(windows|linux|macos)/,
        },
        as: :download_platform
end

both pass correctly.

System configuration

Rails version: 2.7.3

Ruby version: 6.0.5

@dorianmariecom
Copy link
Contributor

I can reproduce with:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "action_controller/railtie"

class DownloadController < ActionController::Base
  def platform
  end
end

class TestApp < Rails::Application
  routes.draw do
    get '/download/:platform' => 'download#platform',
      constraints: {
        platform: %w[windows linux macos],
      },
      as: :download_platform
  end
end

require "minitest/autorun"

class BugTest < Minitest::Test
  include Rails.application.routes.url_helpers

  def test_returns_success
    assert_equal "/download/windows", download_platform_path("windows")
  end
end

@dorianmariecom
Copy link
Contributor

The constraints triggers the bug

@dorianmariecom
Copy link
Contributor

Here is a patch with the a failing test on rails/rails:

diff --git a/actionpack/test/dispatch/routing/custom_url_helpers_test.rb b/actionpack/test/dispatch/routing/custom_url_helpers_test.rb
index a1a1e79884..56cb2ec2ab 100644
--- a/actionpack/test/dispatch/routing/custom_url_helpers_test.rb
+++ b/actionpack/test/dispatch/routing/custom_url_helpers_test.rb
@@ -85,6 +85,11 @@ class ProductPage < Page; end
     get "/profile", to: "users#profile", as: :profile
     get "/media/:id", to: "media#show", as: :media
     get "/pages/:id", to: "pages#show", as: :page
+    get(
+      "/download/:platform" => "download#platform",
+      constraints: { platform: %w[windows linux macos] },
+      as: :download
+    )
 
     resources :categories, :collections, :products, :manufacturers
 
@@ -145,6 +150,10 @@ def params
     ActionController::Parameters.new(page: 2, size: 25)
   end
 
+  def test_constraints_path
+    assert_equal "/download/macos", download_path(platform: :macos)
+  end
+
   def test_direct_paths
     assert_equal "/", website_path
     assert_equal "/", Routes.url_helpers.website_path

@dorianmariecom
Copy link
Contributor

It looks like array constraints aren't supported and that your array is being converted to a regexp

route.path.requirements_for_missing_keys_check
# => {:platform=>/\A["windows", "linux", "macos"]\Z/}

@dorianmariecom
Copy link
Contributor

dorianmariecom commented Mar 21, 2023

Ok, got a patch, gonna submit a pull request:

diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb
index 61d6d5c184..cf2ca9d7d3 100644
--- a/actionpack/lib/action_dispatch/journey/formatter.rb
+++ b/actionpack/lib/action_dispatch/journey/formatter.rb
@@ -175,6 +175,11 @@ def missing_keys(route, parts)
                 missing_keys ||= []
                 missing_keys << key
               end
+            when Array
+              unless tests[key].any? { |value| value.match?(parts[key]) }
+                missing_keys ||= []
+                missing_keys << key
+              end
             else
               unless tests[key].match?(parts[key])
                 missing_keys ||= []
diff --git a/actionpack/lib/action_dispatch/journey/path/pattern.rb b/actionpack/lib/action_dispatch/journey/path/pattern.rb
index d156adc108..29253e4211 100644
--- a/actionpack/lib/action_dispatch/journey/path/pattern.rb
+++ b/actionpack/lib/action_dispatch/journey/path/pattern.rb
@@ -172,8 +172,14 @@ def to_regexp
         end
 
         def requirements_for_missing_keys_check
-          @requirements_for_missing_keys_check ||= requirements.transform_values do |regex|
-            /\A#{regex}\Z/
+          @requirements_for_missing_keys_check ||= requirements.transform_values do |object|
+            if object.is_a?(Array)
+              object.map do |element|
+                /\A#{element}\Z/
+              end
+            else
+              /\A#{object}\Z/
+            end
           end
         end

@javierav
Copy link
Contributor

It looks like array constraints aren't supported and that your array is being converted to a regexp

route.path.requirements_for_missing_keys_check
# => {:platform=>/\A["windows", "linux", "macos"]\Z/}

I also have the same issue. The documentation is clear, but not works:

https://api.rubyonrails.org/classes/ActionDispatch/Routing/Mapper/Base.html#method-i-match

:constraints
Constrains parameters with a hash of regular expressions or an object that responds to matches?. In addition, constraints other than path can also be specified with any object that responds to === (e.g. String, Array, Range, etc.).

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

Successfully merging a pull request may close this issue.

4 participants