diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bcf1a364..291947f2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * [#2663](https://github.com/ruby-grape/grape/pull/2663): Refactor `ParamsScope` and `Parameters` DSL to use named kwargs - [@ericproulx](https://github.com/ericproulx). * [#2664](https://github.com/ruby-grape/grape/pull/2664): Drop `test-prof` dependency - [@ericproulx](https://github.com/ericproulx). * [#2665](https://github.com/ruby-grape/grape/pull/2665): Pass `attrs` directly to `AttributesIterator` instead of `validator` - [@ericproulx](https://github.com/ericproulx). +* [#2657](https://github.com/ruby-grape/grape/pull/2657): Instantiate validators at definition time - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index 50d6584ba..baad800b0 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,30 @@ Upgrading Grape ### Upgrading to >= 3.2 +#### Custom validators must not mutate instance variables at request time + +Validator instances are now instantiated once at route definition time and frozen. Any custom validator that assigns or mutates instance variables inside `validate_param!` or `validate!` will raise `FrozenError` at request time. + +```ruby +# Before — broken in >= 3.2 +def validate_param!(attr_name, params) + @computed = expensive_computation(params[attr_name]) # FrozenError + raise Grape::Exceptions::Validation.new(...) unless @computed.valid? +end + +# After — compute in initialize, read at request time +def initialize(attrs, options, required, scope, opts) + super + @computed = expensive_computation(@option) +end + +def validate_param!(attr_name, params) + raise Grape::Exceptions::Validation.new(...) unless @computed.valid_for?(params[attr_name]) +end +``` + +See [#2657](https://github.com/ruby-grape/grape/pull/2657) for more information. + #### `with` now uses keyword arguments The `with` DSL method now uses `**opts` instead of a positional hash. Calls using bare keyword syntax are unaffected: diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index dd610cf44..e55859836 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -176,7 +176,7 @@ def run status 204 else run_filters before_validations, :before_validation - run_validators validations, request + run_validators request: request run_filters after_validations, :after_validation response_object = execute end @@ -205,11 +205,14 @@ def execute end end - def run_validators(validators, request) + def run_validators(request:) + validators = inheritable_setting.route[:saved_validations] + return if validators.empty? + validation_errors = [] Grape::Validations::ParamScopeTracker.track do - ActiveSupport::Notifications.instrument('endpoint_run_validators.grape', endpoint: self, validators: validators, request: request) do + ActiveSupport::Notifications.instrument('endpoint_run_validators.grape', endpoint: self, validators: validators, request:) do validators.each do |validator| validator.validate(request) rescue Grape::Exceptions::Validation => e @@ -222,7 +225,7 @@ def run_validators(validators, request) end end - validation_errors.any? && raise(Grape::Exceptions::ValidationErrors.new(errors: validation_errors, headers: header)) + raise(Grape::Exceptions::ValidationErrors.new(errors: validation_errors, headers: header)) if validation_errors.any? end def run_filters(filters, type = :other) @@ -239,16 +242,6 @@ def run_filters(filters, type = :other) end end - def validations - saved_validations = inheritable_setting.route[:saved_validations] - return if saved_validations.nil? - return enum_for(:validations) unless block_given? - - saved_validations.each do |saved_validation| - yield Grape::Validations::ValidatorFactory.create_validator(saved_validation) - end - end - def options? options[:options_route_enabled] && env[Rack::REQUEST_METHOD] == Rack::OPTIONS diff --git a/lib/grape/util/deep_freeze.rb b/lib/grape/util/deep_freeze.rb new file mode 100644 index 000000000..9abaf40f4 --- /dev/null +++ b/lib/grape/util/deep_freeze.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Grape + module Util + module DeepFreeze + module_function + + # Recursively freezes Hash (keys and values), Array (elements), and String + # objects. All other types are returned as-is. + # + # Already-frozen objects (including Symbols, Integers, true/false/nil, and + # any object that was previously frozen) are returned immediately via the + # +obj.frozen?+ guard. + # + # Intentionally left unfrozen: + # - Procs / lambdas — may be deferred DB-backed callables + # - Coercers (e.g. ArrayCoercer) — use lazy ivar memoization at request time + # - Classes / Modules — shared constants that must remain open + # - ParamsScope — self-freezes at the end of its own initialize + def deep_freeze(obj) + return obj if obj.frozen? + + case obj + when Hash + obj.each do |k, v| + deep_freeze(k) + deep_freeze(v) + end + obj.freeze + when Array + obj.each { |v| deep_freeze(v) } + obj.freeze + when String + obj.freeze + else + obj + end + end + end + end +end diff --git a/lib/grape/validations/contract_scope.rb b/lib/grape/validations/contract_scope.rb index b3ccd4ae9..07db7132d 100644 --- a/lib/grape/validations/contract_scope.rb +++ b/lib/grape/validations/contract_scope.rb @@ -21,13 +21,7 @@ def initialize(api, contract = nil, &block) end api.inheritable_setting.namespace_stackable[:contract_key_map] = key_map - - validator_options = { - validator_class: Grape::Validations.require_validator(:contract_scope), - opts: { schema: contract, fail_fast: false } - } - - api.inheritable_setting.namespace_stackable[:validations] = validator_options + api.inheritable_setting.namespace_stackable[:validations] = Validators::ContractScopeValidator.new(schema: contract) end end end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index ac09eb0b6..d50357d8a 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -3,8 +3,7 @@ module Grape module Validations class ParamsScope - attr_accessor :element, :parent - attr_reader :type, :nearest_array_ancestor + attr_reader :parent, :type, :nearest_array_ancestor, :full_path def qualifying_params ParamScopeTracker.current&.qualifying_params(self) @@ -21,7 +20,7 @@ def qualifying_params SPECIAL_JSON = [JSON, Array[JSON]].freeze class Attr - attr_accessor :key, :scope + attr_reader :key, :scope # Open up a new ParamsScope::Attr # @param key [Hash, Symbol] key of attr @@ -77,11 +76,13 @@ def initialize(api:, element: nil, element_renamed: nil, parent: nil, optional: # instance_eval, so local variables from initialize are unreachable. # configure_declared_params consumes it and clears @declared_params to nil. @declared_params = [] + @full_path = build_full_path instance_eval(&block) if block configure_declared_params @nearest_array_ancestor = find_nearest_array_ancestor + freeze end def configuration @@ -95,9 +96,9 @@ def should_validate?(parameters) return false if @optional && (scoped_params.blank? || all_element_blank?(scoped_params)) return false unless meets_dependency?(scoped_params, parameters) - return true if parent.nil? + return true if @parent.nil? - parent.should_validate?(parameters) + @parent.should_validate?(parameters) end def meets_dependency?(params, request_params) @@ -124,17 +125,14 @@ def meets_hash_dependency?(params) # params might be anything what looks like a hash, so it must implement a `key?` method return false unless params.respond_to?(:key?) - @dependent_on.each do |dependency| + @dependent_on.all? do |dependency| if dependency.is_a?(Hash) - dependency_key = dependency.keys[0] - proc = dependency.values[0] - return false unless proc.call(params[dependency_key]) - elsif params[dependency].blank? - return false + key, callable = dependency.first + callable.call(params[key]) + else + params[dependency].present? end end - - true end # @return [String] the proper attribute name, with nesting considered. @@ -194,12 +192,11 @@ def push_declared_params(attrs, **opts) @declared_params.concat(attrs.map { |attr| ::Grape::Validations::ParamsScope::Attr.new(attr, opts[:declared_params_scope]) }) end - # Get the full path of the parameter scope in the hierarchy. - # - # @return [Array] the nesting/path of the current parameter scope - def full_path + private + + def build_full_path if nested? - (@parent.full_path + [@element]) + @parent.full_path + [@element] elsif lateral? @parent.full_path else @@ -207,8 +204,6 @@ def full_path end end - private - # Add a new parameter which should be renamed when using the +#declared+ # method. # @@ -314,17 +309,19 @@ def new_group_scope(group, &) self.class.new(api: @api, parent: self, group: group, &) end - # Pushes declared params to parent or settings + # Pushes declared params to parent or settings, then clears @declared_params. + # Clearing here (rather than in initialize) keeps the lifecycle ownership in + # one place: this method both consumes and invalidates the ivar so that + # push_declared_params cannot be called on the frozen scope later. def configure_declared_params push_renamed_param(full_path, @element_renamed) if @element_renamed if nested? - @parent.push_declared_params [element => @declared_params] + @parent.push_declared_params [@element => @declared_params] else @api.inheritable_setting.namespace_stackable[:declared_params] = @declared_params end - - # params were stored in settings, it can be cleaned from the params scope + ensure @declared_params = nil end @@ -354,7 +351,7 @@ def validates(attrs, validations) document_params attrs, validations, coerce_type, values, except_values - opts = derive_validator_options(validations) + opts = derive_validator_options(validations).freeze # Validate for presence before any other validators validates_presence(validations, attrs, opts) @@ -362,7 +359,7 @@ def validates(attrs, validations) # Before we run the rest of the validators, let's handle # whatever coercion so that we are working with correctly # type casted values - coerce_type validations, attrs, required, opts + coerce_type validations.extract!(:coerce, :coerce_with, :coerce_message), attrs, required, opts validations.each do |type, options| # Don't try to look up validators for documentation params that don't have one. @@ -435,7 +432,12 @@ def check_coerce_with(validations) def coerce_type(validations, attrs, required, opts) check_coerce_with(validations) - return unless validations.key?(:coerce) + # Falsy check (not key?) is intentional: when a remountable API is first + # evaluated on its base instance (no configuration supplied yet), + # configuration[:some_type] evaluates to nil. Skipping instantiation + # here is correct — the real mounted instance will replay this step with + # the actual type value. + return unless validations[:coerce] coerce_options = { type: validations[:coerce], @@ -443,9 +445,6 @@ def coerce_type(validations, attrs, required, opts) message: validations[:coerce_message] } validate('coerce', coerce_options, attrs, required, opts) - validations.delete(:coerce_with) - validations.delete(:coerce) - validations.delete(:coerce_message) end def guess_coerce_type(coerce_type, *values_list) @@ -469,15 +468,15 @@ def check_incompatible_option_values(default, values, except_values) end def validate(type, options, attrs, required, opts) - validator_options = { - attributes: attrs, - options: options, - required: required, - params_scope: self, - opts: opts, - validator_class: Validations.require_validator(type) - } - @api.inheritable_setting.namespace_stackable[:validations] = validator_options + validator_class = Validations.require_validator(type) + validator_instance = validator_class.new( + attrs, + options, + required, + self, + opts + ) + @api.inheritable_setting.namespace_stackable[:validations] = validator_instance end def validate_value_coercion(coerce_type, *values_list) diff --git a/lib/grape/validations/validator_factory.rb b/lib/grape/validations/validator_factory.rb deleted file mode 100644 index 0e2022d3a..000000000 --- a/lib/grape/validations/validator_factory.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Grape - module Validations - class ValidatorFactory - def self.create_validator(options) - options[:validator_class].new(options[:attributes], - options[:options], - options[:required], - options[:params_scope], - options[:opts]) - end - end - end -end diff --git a/lib/grape/validations/validators/all_or_none_of_validator.rb b/lib/grape/validations/validators/all_or_none_of_validator.rb index 2fe553a15..59741a7cc 100644 --- a/lib/grape/validations/validators/all_or_none_of_validator.rb +++ b/lib/grape/validations/validators/all_or_none_of_validator.rb @@ -4,11 +4,16 @@ module Grape module Validations module Validators class AllOrNoneOfValidator < MultipleParamsBase + def initialize(attrs, options, required, scope, opts) + super + @exception_message = message(:all_or_none) + end + def validate_params!(params) keys = keys_in_common(params) - return if keys.empty? || keys.length == all_keys.length + return if keys.empty? || keys.length == @attrs.length - raise Grape::Exceptions::Validation.new(params: all_keys, message: message(:all_or_none)) + raise Grape::Exceptions::Validation.new(params: all_keys, message: @exception_message) end end end diff --git a/lib/grape/validations/validators/allow_blank_validator.rb b/lib/grape/validations/validators/allow_blank_validator.rb index e631fa2b0..bf9a4b13b 100644 --- a/lib/grape/validations/validators/allow_blank_validator.rb +++ b/lib/grape/validations/validators/allow_blank_validator.rb @@ -4,15 +4,20 @@ module Grape module Validations module Validators class AllowBlankValidator < Base - def validate_param!(attr_name, params) - return if (options_key?(:value) ? @option[:value] : @option) || !params.is_a?(Hash) + def initialize(attrs, options, required, scope, opts) + super + + @value = option_value + @exception_message = message(:blank) + end - value = params[attr_name] - value = value.scrub if value.respond_to?(:valid_encoding?) && !value.valid_encoding? + def validate_param!(attr_name, params) + return if @value || !hash_like?(params) + value = scrub(params[attr_name]) return if value == false || value.present? - raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: message(:blank)) + validation_error!(attr_name) end end end diff --git a/lib/grape/validations/validators/at_least_one_of_validator.rb b/lib/grape/validations/validators/at_least_one_of_validator.rb index 3467e4f1d..dda9c746a 100644 --- a/lib/grape/validations/validators/at_least_one_of_validator.rb +++ b/lib/grape/validations/validators/at_least_one_of_validator.rb @@ -4,10 +4,15 @@ module Grape module Validations module Validators class AtLeastOneOfValidator < MultipleParamsBase + def initialize(attrs, options, required, scope, opts) + super + @exception_message = message(:at_least_one) + end + def validate_params!(params) - return unless keys_in_common(params).empty? + return if keys_in_common(params).any? - raise Grape::Exceptions::Validation.new(params: all_keys, message: message(:at_least_one)) + raise Grape::Exceptions::Validation.new(params: all_keys, message: @exception_message) end end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 58e3b65bd..1ecb3bf38 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -3,6 +3,14 @@ module Grape module Validations module Validators + # Base class for all parameter validators. + # + # == Freeze contract + # Validator instances are shared across requests and are frozen after + # initialization (via +.new+). All inputs (+options+, +opts+, +attrs+) + # arrive pre-frozen from the DSL boundary, so subclass ivars derived + # from them are frozen by construction. Lazy ivar assignment + # (e.g. +memoize+, ||=) will raise +FrozenError+ at request time. class Base include Grape::Util::Translation @@ -12,17 +20,16 @@ class Base # by a +requires+ or +optional+ directive during # parameter definition. # @param attrs [Array] names of attributes to which the Validator applies - # @param options [Object] implementation-dependent Validator options + # @param options [Object] implementation-dependent Validator options; deep-frozen on assignment # @param required [Boolean] attribute(s) are required or optional # @param scope [ParamsScope] parent scope for this Validator # @param opts [Hash] additional validation options def initialize(attrs, options, required, scope, opts) - @attrs = Array(attrs) - @option = options + @attrs = Array(attrs).freeze + @option = Grape::Util::DeepFreeze.deep_freeze(options) @required = required @scope = scope - @fail_fast = opts[:fail_fast] - @allow_blank = opts[:allow_blank] + @fail_fast, @allow_blank = opts.values_at(:fail_fast, :allow_blank) end # Validates a given request. @@ -36,8 +43,22 @@ def validate(request) validate!(request.params) end + def self.new(...) + super.freeze + end + + def self.inherited(klass) + super + Validations.register(klass) + end + + def fail_fast? + @fail_fast + end + # Validates a given parameter hash. - # @note Override #validate if you need to access the entire request. + # @note Override #validate_param! for per-parameter validation, + # or #validate if you need access to the entire request. # @param params [Hash] parameters to validate # @raise [Grape::Exceptions::Validation] if validation failed # @return [void] @@ -51,7 +72,7 @@ def validate!(params) next if !@scope.required? && empty_val next unless @scope.meets_dependency?(val, params) - validate_param!(attr_name, val) if @required || (val.respond_to?(:key?) && val.key?(attr_name)) + validate_param!(attr_name, val) if @required || (hash_like?(val) && val.key?(attr_name)) rescue Grape::Exceptions::Validation => e array_errors << e end @@ -59,23 +80,54 @@ def validate!(params) raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors.any? end - def self.inherited(klass) - super - Validations.register(klass) + protected + + # Validates a single attribute. Override in subclasses. + # @param attr_name [Symbol, String] the attribute name + # @param params [Hash] the parameter hash containing the attribute + # @raise [Grape::Exceptions::Validation] if validation failed + # @return [void] + def validate_param!(attr_name, params) + raise NotImplementedError end - def message(default_key = nil) - options = instance_variable_get(:@option) - options_key?(:message) ? options[:message] : default_key + private + + def validation_error!(attr_name, message = @exception_message) + raise Grape::Exceptions::Validation.new(params: @scope.full_name(attr_name), message: message) + end + + def hash_like?(obj) + obj.respond_to?(:key?) end def options_key?(key, options = nil) - options = instance_variable_get(:@option) if options.nil? - options.respond_to?(:key?) && options.key?(key) && !options[key].nil? + current_options = options || @option + hash_like?(current_options) && current_options.key?(key) && !current_options[key].nil? end - def fail_fast? - @fail_fast + # Returns the effective message for a validation error. + # Prefers an explicit +:message+ option, then +default_key+. + # If both are nil, the block (if given) is called to compute a fallback — + # useful for validators that build a message Hash for deferred i18n interpolation. + # @example + # @exception_message = message(:presence) # symbol key or custom message + # @exception_message = message { build_hash_message } # computed fallback + def message(default_key = nil) + key = options_key?(:message) ? @option[:message] : default_key + return key unless key.nil? + + yield if block_given? + end + + def option_value + options_key?(:value) ? @option[:value] : @option + end + + def scrub(value) + return value unless value.respond_to?(:valid_encoding?) && !value.valid_encoding? + + value.scrub end end end diff --git a/lib/grape/validations/validators/coerce_validator.rb b/lib/grape/validations/validators/coerce_validator.rb index eaf7c4069..f4d2a7dd2 100644 --- a/lib/grape/validations/validators/coerce_validator.rb +++ b/lib/grape/validations/validators/coerce_validator.rb @@ -7,19 +7,23 @@ class CoerceValidator < Base def initialize(attrs, options, required, scope, opts) super - @converter = if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) - type - else - Types.build_coercer(type, method: @option[:method]) - end + raw_type = @option[:type] + type = hash_like?(raw_type) ? raw_type[:value] : raw_type + @converter = + if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) + type + else + Types.build_coercer(type, method: @option[:method]) + end + @exception_message = message(:coerce) end def validate_param!(attr_name, params) - raise validation_exception(attr_name) unless params.is_a? Hash + raise validation_exception(attr_name) unless hash_like?(params) new_value = coerce_value(params[attr_name]) - raise validation_exception(attr_name, new_value.message) unless valid_type?(new_value) + raise validation_exception(attr_name, new_value.message) if new_value.is_a?(Types::InvalidValue) # Don't assign a value if it is identical. It fixes a problem with Hashie::Mash # which looses wrappers for hashes and arrays after reassigning values @@ -37,36 +41,22 @@ def validate_param!(attr_name, params) private - # @!attribute [r] converter - # Object that will be used for parameter coercion and type checking. - # - # See {Types.build_coercer} - # - # @return [Object] - attr_reader :converter - - def valid_type?(val) - !val.is_a?(Types::InvalidValue) - end + # @converter is intentionally left mutable — coercers built by + # Types.build_coercer may lazily memoize internal state at call time. + # No freeze_state! override is needed: deep_freeze already skips + # non-Hash/Array/String objects, so @converter is left unfrozen automatically. def coerce_value(val) - converter.call(val) + @converter.call(val) # Some custom types might fail, so it should be treated as an invalid value rescue StandardError Types::InvalidValue.new end - # Type to which the parameter will be coerced. - # - # @return [Class] - def type - @option[:type].is_a?(Hash) ? @option[:type][:value] : @option[:type] - end - def validation_exception(attr_name, custom_msg = nil) Grape::Exceptions::Validation.new( - params: [@scope.full_name(attr_name)], - message: custom_msg || message(:coerce) + params: @scope.full_name(attr_name), + message: custom_msg || @exception_message ) end end diff --git a/lib/grape/validations/validators/contract_scope_validator.rb b/lib/grape/validations/validators/contract_scope_validator.rb index b8a3365c1..f92bcd2c8 100644 --- a/lib/grape/validations/validators/contract_scope_validator.rb +++ b/lib/grape/validations/validators/contract_scope_validator.rb @@ -3,12 +3,10 @@ module Grape module Validations module Validators - class ContractScopeValidator < Base - attr_reader :schema - - def initialize(_attrs, _options, _required, _scope, opts) - super - @schema = opts.fetch(:schema) + class ContractScopeValidator + def initialize(schema:) + @schema = schema + freeze end # Validates a given request. @@ -16,7 +14,7 @@ def initialize(_attrs, _options, _required, _scope, opts) # @raise [Grape::Exceptions::ValidationArrayErrors] if validation failed # @return [void] def validate(request) - res = schema.call(request.params) + res = @schema.call(request.params) if res.success? request.params.deep_merge!(res.to_h) @@ -26,13 +24,17 @@ def validate(request) raise Grape::Exceptions::ValidationArrayErrors.new(build_errors_from_messages(res.errors.messages)) end + def fail_fast? + false + end + private def build_errors_from_messages(messages) messages.map do |message| full_name = message.path.first.to_s full_name << "[#{message.path[1..].join('][')}]" if message.path.size > 1 - Grape::Exceptions::Validation.new(params: [full_name], message: message.text) + Grape::Exceptions::Validation.new(params: full_name, message: message.text) end end end diff --git a/lib/grape/validations/validators/default_validator.rb b/lib/grape/validations/validators/default_validator.rb index 157aa988c..cea268a18 100644 --- a/lib/grape/validations/validators/default_validator.rb +++ b/lib/grape/validations/validators/default_validator.rb @@ -4,23 +4,17 @@ module Grape module Validations module Validators class DefaultValidator < Base - def initialize(attrs, options, required, scope, opts = {}) - @default = options + def initialize(attrs, options, required, scope, opts) super - end - - def validate_param!(attr_name, params) - params[attr_name] = if @default.is_a? Proc - if @default.parameters.empty? - @default.call - else - @default.call(params) - end - elsif @default.frozen? || !@default.duplicable? - @default - else - @default.dup - end + # !important, lazy call at runtime + @default_call = + if @option.is_a?(Proc) + @option.arity.zero? ? ->(_p) { @option.call } : ->(p) { @option.call(p) } + elsif @option.duplicable? + ->(_p) { @option.dup } + else + ->(_p) { @option } + end end def validate!(params) @@ -28,7 +22,7 @@ def validate!(params) attrs.each do |resource_params, attr_name| next unless @scope.meets_dependency?(resource_params, params) - validate_param!(attr_name, resource_params) if resource_params.is_a?(Hash) && resource_params[attr_name].nil? + resource_params[attr_name] = @default_call.call(resource_params) if resource_params.is_a?(Hash) && resource_params[attr_name].nil? end end end diff --git a/lib/grape/validations/validators/exactly_one_of_validator.rb b/lib/grape/validations/validators/exactly_one_of_validator.rb index aa1c54711..22fe531a3 100644 --- a/lib/grape/validations/validators/exactly_one_of_validator.rb +++ b/lib/grape/validations/validators/exactly_one_of_validator.rb @@ -4,12 +4,18 @@ module Grape module Validations module Validators class ExactlyOneOfValidator < MultipleParamsBase + def initialize(attrs, options, required, scope, opts) + super + @exactly_one_exception_message = message(:exactly_one) + @mutual_exclusion_exception_message = message(:mutual_exclusion) + end + def validate_params!(params) keys = keys_in_common(params) return if keys.length == 1 - raise Grape::Exceptions::Validation.new(params: all_keys, message: message(:exactly_one)) if keys.empty? + raise Grape::Exceptions::Validation.new(params: all_keys, message: @exactly_one_exception_message) if keys.empty? - raise Grape::Exceptions::Validation.new(params: keys, message: message(:mutual_exclusion)) + raise Grape::Exceptions::Validation.new(params: keys, message: @mutual_exclusion_exception_message) end end end diff --git a/lib/grape/validations/validators/except_values_validator.rb b/lib/grape/validations/validators/except_values_validator.rb index 298eb0ab9..be42840ce 100644 --- a/lib/grape/validations/validators/except_values_validator.rb +++ b/lib/grape/validations/validators/except_values_validator.rb @@ -5,18 +5,26 @@ module Validations module Validators class ExceptValuesValidator < Base def initialize(attrs, options, required, scope, opts) - @except = options.is_a?(Hash) ? options[:value] : options super + except = option_value + raise ArgumentError, 'except_values Proc must have arity of zero (use values: with a one-arity predicate for per-element checks)' if except.is_a?(Proc) && !except.arity.zero? + + # Zero-arity procs (e.g. -> { User.pluck(:role) }) must be called per-request, + # not at definition time, so they are wrapped in a lambda to defer execution. + @excepts_call = except.is_a?(Proc) ? except : -> { except } + @exception_message = message(:except_values) end def validate_param!(attr_name, params) - return unless params.respond_to?(:key?) && params.key?(attr_name) + return unless hash_like?(params) && params.key?(attr_name) - excepts = @except.is_a?(Proc) ? @except.call : @except + excepts = @excepts_call.call return if excepts.nil? param_array = params[attr_name].nil? ? [nil] : Array.wrap(params[attr_name]) - raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: message(:except_values)) if param_array.any? { |param| excepts.include?(param) } + return if param_array.none? { |param| excepts.include?(param) } + + validation_error!(attr_name) end end end diff --git a/lib/grape/validations/validators/length_validator.rb b/lib/grape/validations/validators/length_validator.rb index ad85a2a7b..cce64a0df 100644 --- a/lib/grape/validations/validators/length_validator.rb +++ b/lib/grape/validations/validators/length_validator.rb @@ -5,12 +5,9 @@ module Validations module Validators class LengthValidator < Base def initialize(attrs, options, required, scope, opts) - @min = options[:min] - @max = options[:max] - @is = options[:is] - super + @min, @max, @is = @option.values_at(:min, :max, :is) raise ArgumentError, 'min must be an integer greater than or equal to zero' if !@min.nil? && (!@min.is_a?(Integer) || @min.negative?) raise ArgumentError, 'max must be an integer greater than or equal to zero' if !@max.nil? && (!@max.is_a?(Integer) || @max.negative?) raise ArgumentError, "min #{@min} cannot be greater than max #{@max}" if !@min.nil? && !@max.nil? && @min > @max @@ -27,9 +24,11 @@ def validate_param!(attr_name, params) return unless (!@min.nil? && param.length < @min) || (!@max.nil? && param.length > @max) || (!@is.nil? && param.length != @is) - raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: build_message) + validation_error!(attr_name, build_message) end + private + def build_message if options_key?(:message) @option[:message] diff --git a/lib/grape/validations/validators/mutually_exclusive_validator.rb b/lib/grape/validations/validators/mutually_exclusive_validator.rb index 8e98bbfc6..278cfa550 100644 --- a/lib/grape/validations/validators/mutually_exclusive_validator.rb +++ b/lib/grape/validations/validators/mutually_exclusive_validator.rb @@ -4,11 +4,16 @@ module Grape module Validations module Validators class MutuallyExclusiveValidator < MultipleParamsBase + def initialize(attrs, options, required, scope, opts) + super + @exception_message = message(:mutual_exclusion) + end + def validate_params!(params) keys = keys_in_common(params) return if keys.length <= 1 - raise Grape::Exceptions::Validation.new(params: keys, message: message(:mutual_exclusion)) + raise Grape::Exceptions::Validation.new(params: keys, message: @exception_message) end end end diff --git a/lib/grape/validations/validators/presence_validator.rb b/lib/grape/validations/validators/presence_validator.rb index ae31dc3fb..53ea5d384 100644 --- a/lib/grape/validations/validators/presence_validator.rb +++ b/lib/grape/validations/validators/presence_validator.rb @@ -4,10 +4,15 @@ module Grape module Validations module Validators class PresenceValidator < Base + def initialize(attrs, options, required, scope, opts) + super + @exception_message = message(:presence) + end + def validate_param!(attr_name, params) - return if params.respond_to?(:key?) && params.key?(attr_name) + return if hash_like?(params) && params.key?(attr_name) - raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: message(:presence)) + validation_error!(attr_name) end end end diff --git a/lib/grape/validations/validators/regexp_validator.rb b/lib/grape/validations/validators/regexp_validator.rb index 7d30baf97..ae21d0c14 100644 --- a/lib/grape/validations/validators/regexp_validator.rb +++ b/lib/grape/validations/validators/regexp_validator.rb @@ -4,21 +4,18 @@ module Grape module Validations module Validators class RegexpValidator < Base - def validate_param!(attr_name, params) - return unless params.respond_to?(:key) && params.key?(attr_name) - - value = options_key?(:value) ? @option[:value] : @option - return if Array.wrap(params[attr_name]).all? { |param| param.nil? || scrub(param.to_s).match?(value) } - - raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: message(:regexp)) + def initialize(attrs, options, required, scope, opts) + super + @value = option_value + @exception_message = message(:regexp) end - private + def validate_param!(attr_name, params) + return unless hash_like?(params) && params.key?(attr_name) - def scrub(param) - return param if param.valid_encoding? + return if Array.wrap(params[attr_name]).all? { |param| param.nil? || scrub(param.to_s).match?(@value) } - param.scrub + validation_error!(attr_name) end end end diff --git a/lib/grape/validations/validators/same_as_validator.rb b/lib/grape/validations/validators/same_as_validator.rb index b91c03be3..c45ce8b05 100644 --- a/lib/grape/validations/validators/same_as_validator.rb +++ b/lib/grape/validations/validators/same_as_validator.rb @@ -8,10 +8,7 @@ def validate_param!(attr_name, params) confirmation = options_key?(:value) ? @option[:value] : @option return if params[attr_name] == params[confirmation] - raise Grape::Exceptions::Validation.new( - params: [@scope.full_name(attr_name)], - message: build_message - ) + validation_error!(attr_name, build_message) end private @@ -20,7 +17,7 @@ def build_message if options_key?(:message) @option[:message] else - translate(:same_as, parameter: @option) + translate(:same_as, parameter: option_value) end end end diff --git a/lib/grape/validations/validators/values_validator.rb b/lib/grape/validations/validators/values_validator.rb index 85089e7e2..c97d94e6f 100644 --- a/lib/grape/validations/validators/values_validator.rb +++ b/lib/grape/validations/validators/values_validator.rb @@ -5,44 +5,51 @@ module Validations module Validators class ValuesValidator < Base def initialize(attrs, options, required, scope, opts) - @values = options.is_a?(Hash) ? options[:value] : options super + values = option_value + + # Zero-arity procs return a collection per-request (e.g. DB-backed lists). + # Non-zero-arity procs are per-element predicates, called directly at validation time. + # Non-Proc values are wrapped in a zero-arity lambda for a uniform call interface. + if values.is_a?(Proc) + @values_call = values + @values_is_predicate = !values.arity.zero? + else + @values_call = -> { values } + @values_is_predicate = false + end + @exception_message = message(:values) end def validate_param!(attr_name, params) - return unless params.is_a?(Hash) + return unless hash_like?(params) - val = params[attr_name] + val = scrub(params[attr_name]) return if val.nil? && !required_for_root_scope? - - val = val.scrub if val.respond_to?(:valid_encoding?) && !val.valid_encoding? - - # don't forget that +false.blank?+ is true return if val != false && val.blank? && @allow_blank - return if check_values?(val, attr_name) - raise Grape::Exceptions::Validation.new( - params: [@scope.full_name(attr_name)], - message: message(:values) - ) + validation_error!(attr_name) end private def check_values?(val, attr_name) - values = @values.is_a?(Proc) && @values.arity.zero? ? @values.call : @values - return true if values.nil? - param_array = val.nil? ? [nil] : Array.wrap(val) - return param_array.all? { |param| values.include?(param) } unless values.is_a?(Proc) - begin - param_array.all? { |param| values.call(param) } - rescue StandardError => e - warn "Error '#{e}' raised while validating attribute '#{attr_name}'" - false + if @values_is_predicate + begin + param_array.all? { |param| @values_call.call(param) } + rescue StandardError => e + warn "Error '#{e}' raised while validating attribute '#{attr_name}'" + false + end + else + values = @values_call.call + return true if values.nil? + + param_array.all? { |param| values.include?(param) } end end diff --git a/spec/grape/api/custom_validations_spec.rb b/spec/grape/api/custom_validations_spec.rb index 374416acc..8022dd7ef 100644 --- a/spec/grape/api/custom_validations_spec.rb +++ b/spec/grape/api/custom_validations_spec.rb @@ -16,10 +16,10 @@ let(:default_length_validator) do Class.new(Grape::Validations::Validators::Base) do def validate_param!(attr_name, params) - @option = params[:max].to_i if params.key?(:max) - return if params[attr_name].length <= @option + max = params.key?(:max) ? params[:max].to_i : @option + return if params[attr_name].length <= max - raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: "must be at the most #{@option} characters long") + raise Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: "must be at the most #{max} characters long") end end end @@ -215,13 +215,7 @@ def access_header describe 'using a custom validator with instance variable' do let(:validator_type) do Class.new(Grape::Validations::Validators::Base) do - def validate_param!(_attr_name, _params) - if @instance_variable - raise Grape::Exceptions::Validation.new(params: ['params'], - message: 'This should never happen') - end - @instance_variable = true - end + def validate_param!(_attr_name, _params); end end end let(:app) do @@ -245,8 +239,12 @@ def validate_param!(_attr_name, _params) described_class.deregister(:instance_validator) end - it 'passes validation every time' do + it 'creates one instance per param at definition time, not per request' do expect(validator_type).to receive(:new).twice.and_call_original + app # trigger definition-time instantiation + + # no additional instantiation on subsequent requests + expect(validator_type).not_to receive(:new) get '/', param_to_validate: 'value', another_param_to_validate: 'value' expect(last_response.status).to eq 200 end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 27c9f2736..1d0808271 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -1062,9 +1062,6 @@ def memoized have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(described_class), filters: [], type: :before_validation }), - have_attributes(name: 'endpoint_run_validators.grape', payload: { endpoint: a_kind_of(described_class), - validators: [], - request: a_kind_of(Grape::Request) }), have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(described_class), filters: [], type: :after_validation }), @@ -1091,9 +1088,6 @@ def memoized have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(described_class), filters: [], type: :before_validation }), - have_attributes(name: 'endpoint_run_validators.grape', payload: { endpoint: a_kind_of(described_class), - validators: [], - request: a_kind_of(Grape::Request) }), have_attributes(name: 'endpoint_run_filters.grape', payload: { endpoint: a_kind_of(described_class), filters: [], type: :after_validation }), diff --git a/spec/grape/util/deep_freeze_spec.rb b/spec/grape/util/deep_freeze_spec.rb new file mode 100644 index 000000000..973e243ea --- /dev/null +++ b/spec/grape/util/deep_freeze_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +describe Grape::Util::DeepFreeze do + describe '.deep_freeze' do + subject { described_class.deep_freeze(obj) } + + context 'with a String' do + let(:obj) { +'mutable' } + + it { is_expected.to be_frozen } + end + + context 'with an Array of strings' do + let(:obj) { [+'a', +'b'] } + + it 'freezes the array and each element' do + subject + expect(obj).to be_frozen + expect(obj).to all(be_frozen) + end + end + + context 'with a nested Array' do + let(:obj) { [[+'a'], [+'b']] } + + it 'freezes all nested elements' do + subject + expect(obj).to all(be_frozen) + expect(obj).to all(all(be_frozen)) + end + end + + context 'with a Hash' do + let(:obj) { { +'key' => +'value' } } + + it 'freezes the hash, its keys and its values' do + subject + expect(obj).to be_frozen + obj.each do |k, v| + expect(k).to be_frozen + expect(v).to be_frozen + end + end + end + + context 'with a nested Hash' do + let(:obj) { { a: { +'nested_key' => +'nested_value' } } } + + it 'freezes nested hashes and their contents' do + subject + inner = obj[:a] + expect(inner).to be_frozen + inner.each do |k, v| + expect(k).to be_frozen + expect(v).to be_frozen + end + end + end + + context 'with a Proc' do + let(:obj) { proc {} } + + it 'returns it unchanged without freezing' do + expect(subject).to equal(obj) + expect(obj).not_to be_frozen + end + end + + context 'with an already-frozen object' do + let(:obj) { 'frozen' } + + it 'returns it without raising' do + expect { subject }.not_to raise_error + expect(subject).to equal(obj) + end + end + + context 'with nil' do + let(:obj) { nil } + + it 'returns nil without raising' do + expect { subject }.not_to raise_error + end + end + end +end diff --git a/spec/grape/validations/validators/contract_scope_validator_spec.rb b/spec/grape/validations/validators/contract_scope_validator_spec.rb deleted file mode 100644 index b8d462c31..000000000 --- a/spec/grape/validations/validators/contract_scope_validator_spec.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -describe Grape::Validations::Validators::ContractScopeValidator do - describe '.inherits' do - subject { described_class } - - it { is_expected.to be < Grape::Validations::Validators::Base } - end -end diff --git a/spec/grape/validations/validators/except_values_validator_spec.rb b/spec/grape/validations/validators/except_values_validator_spec.rb index 11b15ccff..9b7442e4d 100644 --- a/spec/grape/validations/validators/except_values_validator_spec.rb +++ b/spec/grape/validations/validators/except_values_validator_spec.rb @@ -191,4 +191,42 @@ end end end + + describe 'lazy evaluation with proc' do + let(:excepts_model) do + Class.new do + class << self + def excepts + @excepts ||= %w[invalid-type1 invalid-type2 invalid-type3] + end + + def add_except(value) + excepts << value + end + end + end + end + let(:app) do + Class.new(Grape::API) do + default_format :json + + params do + requires :type, except_values: -> { ExceptsModel.excepts } + end + get '/except_lambda' do + { type: params[:type] } + end + end + end + + before { stub_const('ExceptsModel', excepts_model) } + + it 'evaluates the proc per-request, not at definition time (e.g. for DB-backed values)' do + app # instantiate at definition time, before the new except is added + ExceptsModel.add_except('invalid-type4') + get('/except_lambda', type: 'invalid-type4') + expect(last_response.status).to eq 400 + expect(last_response.body).to eq({ error: 'type has a value not allowed' }.to_json) + end + end end diff --git a/spec/grape/validations/validators/same_as_validator_spec.rb b/spec/grape/validations/validators/same_as_validator_spec.rb index 62016e5fa..d8781d487 100644 --- a/spec/grape/validations/validators/same_as_validator_spec.rb +++ b/spec/grape/validations/validators/same_as_validator_spec.rb @@ -30,6 +30,35 @@ end end + describe '/value-hash' do + let(:app) do + Class.new(Grape::API) do + params do + requires :password + requires :password_confirmation, same_as: { value: :password } + end + post '/value-hash' do + end + end + end + + context 'is the same' do + it do + post '/value-hash', password: '987654', password_confirmation: '987654' + expect(last_response.status).to eq(201) + expect(last_response.body).to eq('') + end + end + + context 'is not the same' do + it do + post '/value-hash', password: '123456', password_confirmation: 'whatever' + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('password_confirmation is not the same as password') + end + end + end + describe '/custom-message' do let(:app) do Class.new(Grape::API) do