From fd08a918beb76a6529149f4df19402006f3842ea Mon Sep 17 00:00:00 2001 From: Matt Brictson Date: Tue, 21 Jun 2016 09:36:56 -0700 Subject: [PATCH] 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 | 3 + lib/capistrano/configuration.rb | 3 +- .../configuration/validated_variables.rb | 75 +++++++++++++++++++ lib/capistrano/configuration/variables.rb | 30 ++------ lib/capistrano/proc_helpers.rb | 13 ++++ spec/lib/capistrano/configuration_spec.rb | 14 +++- 6 files changed, 112 insertions(+), 26 deletions(-) create mode 100644 lib/capistrano/configuration/validated_variables.rb create mode 100644 lib/capistrano/proc_helpers.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index d4e30700..939825ce 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/lib/capistrano/configuration.rb b/lib/capistrano/configuration.rb index b88ce537..28a07da4 100644 --- a/lib/capistrano/configuration.rb +++ b/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={}) diff --git a/lib/capistrano/configuration/validated_variables.rb b/lib/capistrano/configuration/validated_variables.rb new file mode 100644 index 00000000..a93f0678 --- /dev/null +++ b/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 diff --git a/lib/capistrano/configuration/variables.rb b/lib/capistrano/configuration/variables.rb index bbe4c2dc..a2801ec7 100644 --- a/lib/capistrano/configuration/variables.rb +++ b/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) diff --git a/lib/capistrano/proc_helpers.rb b/lib/capistrano/proc_helpers.rb new file mode 100644 index 00000000..68652814 --- /dev/null +++ b/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 diff --git a/spec/lib/capistrano/configuration_spec.rb b/spec/lib/capistrano/configuration_spec.rb index cfa1872b..d8cc0909 100644 --- a/spec/lib/capistrano/configuration_spec.rb +++ b/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