Merge pull request #18939 from georgeclaghorn/variant-inquiry

Provide friendlier access to request variants

actionpack/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   Provide friendlier access to request variants.
+
+        request.variant = :phone
+        request.variant.phone?  # true
+        request.variant.tablet? # false
+
+        request.variant = [:phone, :tablet]
+        request.variant.phone?                  # true
+        request.variant.desktop?                # false
+        request.variant.any?(:phone, :desktop)  # true
+        request.variant.any?(:desktop, :watch)  # false
+
+    *George Claghorn*
+
 *   Fix regression where a gzip file response would have a Content-type,
     even when it was a 304 status code.
 
@@ -14,7 +28,7 @@
     *Adam Forsyth*
 
 *   Drop request class from RouteSet constructor.
- 
+
     If you would like to use a custom request class, please subclass and implement
     the `request_class` method.
 

actionpack/lib/action_controller/metal/mime_responds.rb

@@ -288,16 +288,17 @@ module ActionController #:nodoc:
         end
 
         def variant
-          if @variant.nil?
+          if @variant.empty?
             @variants[:none] || @variants[:any]
-          elsif (@variants.keys & @variant).any?
-            @variant.each do |v|
-              return @variants[v] if @variants.key?(v)
-            end
           else
-            @variants[:any]
+            @variants[variant_key]
           end
         end
+
+        private
+          def variant_key
+            @variant.find { |variant| @variants.key?(variant) } || :any
+          end
       end
     end
   end

actionpack/lib/action_dispatch/http/mime_negotiation.rb

@@ -10,8 +10,6 @@ module ActionDispatch
         self.ignore_accept_header = false
       end
 
-      attr_reader :variant
-
       # The MIME type of the HTTP request, such as Mime::XML.
       #
       # For backward compatibility, the post \format is extracted from the
@@ -75,18 +73,22 @@ module ActionDispatch
 
       # Sets the \variant for template.
       def variant=(variant)
-        if variant.is_a?(Symbol)
-          @variant = [variant]
-        elsif variant.nil? || variant.is_a?(Array) && variant.any? && variant.all?{ |v| v.is_a?(Symbol) }
-          @variant = variant
+        variant = Array(variant)
+
+        if variant.all? { |v| v.is_a?(Symbol) }
+          @variant = ActiveSupport::ArrayInquirer.new(variant)
         else
-          raise ArgumentError, "request.variant must be set to a Symbol or an Array of Symbols, not a #{variant.class}. " \
+          raise ArgumentError, "request.variant must be set to a Symbol or an Array of Symbols. " \
             "For security reasons, never directly set the variant to a user-provided value, " \
             "like params[:variant].to_sym. Check user-provided value against a whitelist first, " \
             "then set the variant: request.variant = :tablet if params[:variant] == 'tablet'"
         end
       end
 
+      def variant
+        @variant ||= ActiveSupport::ArrayInquirer.new
+      end
+
       # Sets the \format by string extension, which can be used to force custom formats
       # that are not controlled by the extension.
       #

actionpack/test/dispatch/request_test.rb

@@ -1128,35 +1128,46 @@ class RequestEtag < BaseRequestTest
 end
 
 class RequestVariant < BaseRequestTest
-  test "setting variant" do
-    request = stub_request
+  setup do
+    @request = stub_request
+  end
 
-    request.variant = :mobile
-    assert_equal [:mobile], request.variant
+  test 'setting variant to a symbol' do
+    @request.variant = :phone
 
-    request.variant = [:phone, :tablet]
-    assert_equal [:phone, :tablet], request.variant
+    assert @request.variant.phone?
+    assert_not @request.variant.tablet?
+    assert @request.variant.any?(:phone, :tablet)
+    assert_not @request.variant.any?(:tablet, :desktop)
+  end
 
+  test 'setting variant to an array of symbols' do
+    @request.variant = [:phone, :tablet]
+
+    assert @request.variant.phone?
+    assert @request.variant.tablet?
+    assert_not @request.variant.desktop?
+    assert @request.variant.any?(:tablet, :desktop)
+    assert_not @request.variant.any?(:desktop, :watch)
+  end
+
+  test 'clearing variant' do
+    @request.variant = nil
+
+    assert @request.variant.empty?
+    assert_not @request.variant.phone?
+    assert_not @request.variant.any?(:phone, :tablet)
+  end
+
+  test 'setting variant to a non-symbol value' do
     assert_raise ArgumentError do
-      request.variant = [:phone, "tablet"]
-    end
-
-    assert_raise ArgumentError do
-      request.variant = "yolo"
+      @request.variant = 'phone'
     end
   end
 
-  test "reset variant" do
-    request = stub_request
-
-    request.variant = nil
-    assert_equal nil, request.variant
-  end
-
-  test "setting variant with non symbol value" do
-    request = stub_request
+  test 'setting variant to an array containing a non-symbol value' do
     assert_raise ArgumentError do
-      request.variant = "mobile"
+      @request.variant = [:phone, 'tablet']
     end
   end
 end

activesupport/CHANGELOG.md

@@ -1,3 +1,23 @@
+*   Added `ActiveSupport::ArrayInquirer` and `Array#inquiry`.
+
+    Wrapping an array in an `ArrayInquirer` gives a friendlier way to check its
+    contents:
+
+        variants = ActiveSupport::ArrayInquirer.new([:phone, :tablet])
+
+        variants.phone?    # => true
+        variants.tablet?   # => true
+        variants.desktop?  # => false
+
+        variants.any?(:phone, :tablet)   # => true
+        variants.any?(:phone, :desktop)  # => true
+        variants.any?(:desktop, :watch)  # => false
+
+    `Array#inquiry` is a shortcut for wrapping the receiving array in an
+    `ArrayInquirer`.
+
+    *George Claghorn*
+
 *   Deprecate `alias_method_chain` in favour of `Module#prepend` introduced in Ruby 2.0
 
     *Kir Shatrov*

activesupport/lib/active_support.rb

@@ -59,6 +59,7 @@ module ActiveSupport
     autoload :StringInquirer
     autoload :TaggedLogging
     autoload :XmlMini
+    autoload :ArrayInquirer
   end
 
   autoload :Rescuable

activesupport/lib/active_support/array_inquirer.rb

@@ -0,0 +1,38 @@
+module ActiveSupport
+  # Wrapping an array in an +ArrayInquirer+ gives a friendlier way to check
+  # its string-like contents:
+  #
+  #   variants = ActiveSupport::ArrayInquirer.new([:phone, :tablet])
+  #
+  #   variants.phone?    # => true
+  #   variants.tablet?   # => true
+  #   variants.desktop?  # => false
+  #
+  #   variants.any?(:phone, :tablet)   # => true
+  #   variants.any?(:phone, :desktop)  # => true
+  #   variants.any?(:desktop, :watch)  # => false
+  class ArrayInquirer < Array
+    def any?(*candidates, &block)
+      if candidates.none?
+        super
+      else
+        candidates.any? do |candidate|
+          include?(candidate) || include?(candidate.to_sym)
+        end
+      end
+    end
+
+    private
+      def respond_to_missing?(name, include_private = false)
+        name[-1] == '?'
+      end
+
+      def method_missing(name, *args)
+        if name[-1] == '?'
+          any?(name[0..-2])
+        else
+          super
+        end
+      end
+  end
+end

activesupport/lib/active_support/core_ext/array.rb

@@ -4,3 +4,4 @@ require 'active_support/core_ext/array/conversions'
 require 'active_support/core_ext/array/extract_options'
 require 'active_support/core_ext/array/grouping'
 require 'active_support/core_ext/array/prepend_and_append'
+require 'active_support/core_ext/array/inquiry'

activesupport/lib/active_support/core_ext/array/inquiry.rb

@@ -0,0 +1,15 @@
+class Array
+  # Wraps the array in an +ArrayInquirer+ object, which gives a friendlier way
+  # to check its string-like contents.
+  #
+  #   pets = [:cat, :dog].inquiry
+  #
+  #   pets.cat?     # => true
+  #   pets.ferret?  # => false
+  #
+  #   pets.any?(:cat, :ferret)  # => true
+  #   pets.any?(:ferret, :alligator)  # => false
+  def inquiry
+    ActiveSupport::ArrayInquirer.new(self)
+  end
+end

activesupport/test/array_inquirer_test.rb

@@ -0,0 +1,35 @@
+require 'abstract_unit'
+
+class ArrayInquirerTest < ActiveSupport::TestCase
+  def setup
+    @array_inquirer = ActiveSupport::ArrayInquirer.new([:mobile, :tablet])
+  end
+
+  def test_individual
+    assert @array_inquirer.mobile?
+    assert @array_inquirer.tablet?
+    assert_not @array_inquirer.desktop?
+  end
+
+  def test_any
+    assert @array_inquirer.any?(:mobile, :desktop)
+    assert @array_inquirer.any?(:watch, :tablet)
+    assert_not @array_inquirer.any?(:desktop, :watch)
+  end
+
+  def test_any_with_block
+    assert @array_inquirer.any? { |v| v == :mobile }
+    assert_not @array_inquirer.any? { |v| v == :desktop }
+  end
+
+  def test_respond_to
+    assert_respond_to @array_inquirer, :development?
+  end
+
+  def test_inquiry
+    result = [:mobile, :tablet].inquiry
+
+    assert_instance_of ActiveSupport::ArrayInquirer, result
+    assert_equal @array_inquirer, result
+  end
+end