Allow validated variables (e.g. :application) to accept proc values (#1711)

* Refactor validation into separate decorator class
* Add deferred validation of proc values

This fixes a bug where the `:application` validation rule would fail if
given a proc value.

The fix involves changing how validation rules are applied to procs.
For a proc, instead of validating immediately, we defer validation until
when the proc is fetched and called.

Since this causes the validation logic to be non-trivial, I factored it
out into a separate class using the decorator pattern. Now `Variables`
handles the core set/fetch logic, and `ValidatedVariables` decorates it
to layer on validation.

CHANGELOG.md

@@ -15,6 +15,9 @@ https://github.com/capistrano/capistrano/compare/v3.5.0...HEAD
   * `doctor` no longer erroneously warns that `:git_strategy` and other SCM options are "unrecognized" (@shanesaww)
   * Add `net-ssh` gem version to `doctor:gems` output (@lebedev-yury)
   * Deprecate `remote_file` feature (will be removed in Capistrano 3.7.0) (@lebedev-yury)
+  * Fix `NoMethodError: undefined method gsub` when setting `:application` to a
+    Proc. [#1681](https://github.com/capistrano/capistrano/issues/1681)
+    (@mattbrictson)
 
 ## `3.5.0`
 

lib/capistrano/configuration.rb

@@ -3,6 +3,7 @@ require_relative "configuration/question"
 require_relative "configuration/plugin_installer"
 require_relative "configuration/server"
 require_relative "configuration/servers"
+require_relative "configuration/validated_variables"
 require_relative "configuration/variables"
 
 module Capistrano
@@ -23,7 +24,7 @@ module Capistrano
                    :set, :fetch, :fetch_for, :delete, :keys, :validate
 
     def initialize(values={})
-      @variables = Variables.new(values)
+      @variables = ValidatedVariables.new(Variables.new(values))
     end
 
     def ask(key, default=nil, options={})

lib/capistrano/configuration/validated_variables.rb

@@ -0,0 +1,75 @@
+require "capistrano/proc_helpers"
+require "delegate"
+
+module Capistrano
+  class Configuration
+    # Decorates a Variables object to additionally perform an optional set of
+    # user-supplied validation rules. Each rule for a given key is invoked
+    # immediately whenever `set` is called with a value for that key.
+    #
+    # If `set` is called with a block, validation is not performed immediately.
+    # Instead, the validation rules are invoked the first time `fetch` is used
+    # to access the value.
+    #
+    # A rule is simply a block that accepts two arguments: key and value. It is
+    # up to the rule to raise an exception when it deems the value is invalid
+    # (or just print a warning).
+    #
+    # Rules can be registered using the DSL like this:
+    #
+    #   validate(:my_key) do |key, value|
+    #     # rule goes here
+    #   end
+    #
+    class ValidatedVariables < SimpleDelegator
+      include Capistrano::ProcHelpers
+
+      def initialize(variables)
+        super(variables)
+        @validators = {}
+      end
+
+      # Decorate Variables#set to add validation behavior.
+      def set(key, value=nil, &block)
+        if value.nil? && callable_without_parameters?(block)
+          super(key, nil, &assert_valid_later(key, &block))
+        else
+          assert_valid_now(key, block || value)
+          super
+        end
+      end
+
+      # Register a validation rule for the given key.
+      def validate(key, &validator)
+        vs = (validators[key] || [])
+        vs << validator
+        validators[key] = vs
+      end
+
+      private
+
+      attr_reader :validators
+
+      # Wrap a block with a proc that validates the value of the block. This
+      # allows us to defer validation until the time the value is requested.
+      def assert_valid_later(key)
+        lambda do
+          value = yield
+          assert_valid_now(key, value)
+          value
+        end
+      end
+
+      # Runs all validation rules registered for the given key against the
+      # user-supplied value for that variable. If no validator raises an
+      # exception, the value is assumed to be valid.
+      def assert_valid_now(key, value)
+        return unless validators.key?(key)
+
+        validators[key].each do |validator|
+          validator.call(key, value)
+        end
+      end
+    end
+  end
+end

lib/capistrano/configuration/variables.rb

@@ -1,9 +1,11 @@
+require "capistrano/proc_helpers"
+
 module Capistrano
   class Configuration
     # Holds the variables assigned at Capistrano runtime via `set` and retrieved
     # with `fetch`. Does internal bookkeeping to help identify user mistakes
     # like spelling errors or unused variables that may lead to unexpected
-    # behavior. Also allows validation rules to be registered with `validate`.
+    # behavior.
     class Variables
       CAPISTRANO_LOCATION = File.expand_path("../..", __FILE__).freeze
       IGNORED_LOCATIONS = [
@@ -15,6 +17,8 @@ module Capistrano
       ].freeze
       private_constant :CAPISTRANO_LOCATION, :IGNORED_LOCATIONS
 
+      include Capistrano::ProcHelpers
+
       def initialize(values={})
         @trusted_keys = []
         @fetched_keys = []
@@ -31,7 +35,7 @@ module Capistrano
       end
 
       def set(key, value=nil, &block)
-        invoke_validations(key, value, &block)
+        assert_value_or_block_not_both(value, block)
         @trusted_keys << key if trusted?
         remember_location(key)
         values[key] = block || value
@@ -61,12 +65,6 @@ module Capistrano
         values.delete(key)
       end
 
-      def validate(key, &validator)
-        vs = (validators[key] || [])
-        vs << validator
-        validators[key] = vs
-      end
-
       def trusted_keys
         @trusted_keys.dup
       end
@@ -106,25 +104,11 @@ module Capistrano
         (locations[key] ||= []) << location
       end
 
-      def callable_without_parameters?(x)
-        x.respond_to?(:call) && (!x.respond_to?(:arity) || x.arity == 0)
-      end
-
-      def validators
-        @validators ||= {}
-      end
-
-      def invoke_validations(key, value, &block)
+      def assert_value_or_block_not_both(value, block)
         unless value.nil? || block.nil?
           raise Capistrano::ValidationError,
                 "Value and block both passed to Configuration#set"
         end
-
-        return unless validators.key? key
-
-        validators[key].each do |validator|
-          validator.call(key, block || value)
-        end
       end
 
       def trace_set(key)

lib/capistrano/proc_helpers.rb

@@ -0,0 +1,13 @@
+module Capistrano
+  module ProcHelpers
+    module_function
+
+    # Tests whether the given object appears to respond to `call` with
+    # zero parameters. In Capistrano, such a proc is used to represent a
+    # "deferred value". That is, a value that is resolved by invoking `call` at
+    # the time it is first needed.
+    def callable_without_parameters?(x)
+      x.respond_to?(:call) && (!x.respond_to?(:arity) || x.arity == 0)
+    end
+  end
+end

spec/lib/capistrano/configuration_spec.rb

@@ -150,13 +150,23 @@ module Capistrano
           end
         end
 
-        it "validates without error" do
+        it "validates string without error" do
           config.set(:key, "longer_value")
         end
 
-        it "raises an exception" do
+        it "validates proc without error" do
+          config.set(:key) { "longer_value" }
+          expect(config.fetch(:key)).to eq "longer_value"
+        end
+
+        it "raises an exception on invalid string" do
           expect { config.set(:key, "sho") }.to raise_error(Capistrano::ValidationError)
         end
+
+        it "raises an exception on invalid string provided by proc" do
+          config.set(:key) { "sho" }
+          expect { config.fetch(:key) }.to raise_error(Capistrano::ValidationError)
+        end
       end
 
       context "appending" do