Skip to content
Browse files

added a branch where the params verification doesn't rely on exceptio…

…ns and I benchmarked it. It doesn't seem to make too much sense to do that at the moment.
  • Loading branch information...
1 parent e4aa77d commit 43d88e0765ba0a34a60cddfc4bb58f5a59aa825e @mattetti committed May 11, 2012
View
270 benchmarks/old_exception_based_params_verification.rb
@@ -0,0 +1,270 @@
+require 'erb' # used to sanitize the error message and avoid XSS attacks
+
+# ParamsVerification module.
+# Written to verify a service params without creating new objects.
+# This module is used on all requests requiring validation and therefore performance
+# security and maintainability are critical.
+#
+# @api public
+module LegacyParamsVerification
+
+ class ParamError < StandardError; end #:nodoc
+ class NoParamsDefined < ParamError; end #:nodoc
+ class MissingParam < ParamError; end #:nodoc
+ class UnexpectedParam < ParamError; end #:nodoc
+ class InvalidParamType < ParamError; end #:nodoc
+ class InvalidParamValue < ParamError; end #:nodoc
+
+ # An array of validation regular expressions.
+ # The array gets cached but can be accessed via the symbol key.
+ #
+ # @return [Hash] An array with all the validation types as keys and regexps as values.
+ # @api public
+ def self.type_validations
+ @type_validations ||= { :integer => /^-?\d+$/,
+ :float => /^-?(\d*\.\d+|\d+)$/,
+ :decimal => /^-?(\d*\.\d+|\d+)$/,
+ :datetime => /^[-\d:T\s\+]+$/, # "T" is for ISO date format
+ :boolean => /^(1|true|TRUE|T|Y|0|false|FALSE|F|N)$/,
+ #:array => /,/
+ }
+ end
+
+ # Validation against each required WeaselDiesel::Params::Rule
+ # and returns the potentially modified params (with default values)
+ #
+ # @param [Hash] params The params to verify (incoming request params)
+ # @param [WeaselDiesel::Params] service_params A Playco service param compatible object listing required and optional params
+ # @param [Boolean] ignore_unexpected Flag letting the validation know if unexpected params should be ignored
+ #
+ # @return [Hash]
+ # The passed params potentially modified by the default rules defined in the service.
+ #
+ # @example Validate request params against a service's defined param rules
+ # ParamsVerification.validate!(request.params, @service.defined_params)
+ #
+ # @api public
+ def self.validate!(params, service_params, ignore_unexpected=false)
+
+ # Verify that no garbage params are passed, if they are, an exception is raised.
+ # only the first level is checked at this point
+ unless ignore_unexpected
+ unexpected_params?(params, service_params.param_names)
+ end
+
+ # dupe the params so we don't modify the passed value
+ updated_params = params.dup
+ # Required param verification
+ service_params.list_required.each do |rule|
+ updated_params = validate_required_rule(rule, updated_params)
+ end
+
+ # Set optional defaults if any optional
+ service_params.list_optional.each do |rule|
+ updated_params = run_optional_rule(rule, updated_params)
+ end
+
+ # check the namespaced params
+ service_params.namespaced_params.each do |param|
+ param.list_required.each do |rule|
+ updated_params = validate_required_rule(rule, updated_params, param.space_name.to_s)
+ end
+ param.list_optional.each do |rule|
+ updated_params = run_optional_rule(rule, updated_params, param.space_name.to_s)
+ end
+
+ end
+
+ # verify nested params, only 1 level deep tho
+ params.each_pair do |key, value|
+ if value.is_a?(Hash)
+ namespaced = service_params.namespaced_params.find{|np| np.space_name.to_s == key.to_s}
+ raise UnexpectedParam, "Request included unexpected parameter: #{key}" if namespaced.nil?
+ unexpected_params?(params[key], namespaced.param_names)
+ end
+ end
+
+ updated_params
+ end
+
+
+ private
+
+ # Validate a required rule against a list of params passed.
+ #
+ #
+ # @param [WeaselDiesel::Params::Rule] rule The required rule to check against.
+ # @param [Hash] params The request params.
+ # @param [String] namespace Optional param namespace to check the rule against.
+ #
+ # @return [Hash]
+ # A hash representing the potentially modified params after going through the filter.
+ #
+ # @api private
+ def self.validate_required_rule(rule, params, namespace=nil)
+ param_name = rule.name.to_s
+
+ param_value, namespaced_params = extract_param_values(params, param_name, namespace)
+ # puts "verify #{param_name} params, current value: #{param_value}"
+
+ #This is disabled since required params shouldn't have a default, otherwise, why are they required?
+ #if param_value.nil? && rule.options && rule.options[:default]
+ #param_value = rule.options[:default]
+ #end
+
+ # Checks presence
+ if !(namespaced_params || params).keys.include?(param_name)
+ raise MissingParam, "'#{rule.name}' is missing - passed params: #{params.inspect}."
+ # checks null
+ elsif param_value.nil? && !rule.options[:null]
+ raise InvalidParamValue, "Value for parameter '#{param_name}' is missing - passed params: #{params.inspect}."
+ # checks type
+ elsif rule.options[:type]
+ verify_cast(param_name, param_value, rule.options[:type])
+ end
+
+ if rule.options[:options] || rule.options[:in]
+ choices = rule.options[:options] || rule.options[:in]
+ if rule.options[:type]
+ # Force the cast so we can compare properly
+ param_value = params[param_name] = type_cast_value(rule.options[:type], param_value)
+ end
+ raise InvalidParamValue, "Value for parameter '#{param_name}' (#{param_value}) is not in the allowed set of values." unless choices.include?(param_value)
+ # You can have a "in" rule that also applies a min value since they are mutually exclusive
+ elsif rule.options[:minvalue]
+ min = rule.options[:minvalue]
+ raise InvalidParamValue, "Value for parameter '#{param_name}' is lower than the min accepted value (#{min})." if param_value.to_i < min
+ end
+ # Returns the updated params
+
+ # cast the type if a type is defined and if a range of options isn't defined since the casting should have been done already
+ if rule.options[:type] && !(rule.options[:options] || rule.options[:in])
+ # puts "casting #{param_value} into type: #{rule.options[:type]}"
+ params[param_name] = type_cast_value(rule.options[:type], param_value)
+ end
+ params
+ end
+
+
+ # Extract the param valie and the namespaced params
+ # based on a passed namespace and params
+ #
+ # @param [Hash] params The passed params to extract info from.
+ # @param [String] param_name The param name to find the value.
+ # @param [NilClass, String] namespace the params' namespace.
+ # @return [Arrays<Object, String>]
+ #
+ # @api private
+ def self.extract_param_values(params, param_name, namespace=nil)
+ # Namespace check
+ if namespace == '' || namespace.nil?
+ [params[param_name], nil]
+ else
+ # puts "namespace: #{namespace} - params #{params[namespace].inspect}"
+ namespaced_params = params[namespace]
+ if namespaced_params
+ [namespaced_params[param_name], namespaced_params]
+ else
+ [nil, namespaced_params]
+ end
+ end
+ end
+
+ # @param [#WeaselDiesel::Params::Rule] rule The optional rule
+ # @param [Hash] params The request params
+ # @param [String] namespace An optional namespace
+ # @return [Hash] The potentially modified params
+ #
+ # @api private
+ def self.run_optional_rule(rule, params, namespace=nil)
+ param_name = rule.name.to_s
+
+ param_value, namespaced_params = extract_param_values(params, param_name, namespace)
+
+ if param_value.nil? && rule.options[:default]
+ if namespace
+ params[namespace][param_name] = param_value = rule.options[:default]
+ else
+ params[param_name] = param_value = rule.options[:default]
+ end
+ end
+
+ # cast the type if a type is defined and if a range of options isn't defined since the casting should have been done already
+ if rule.options[:type] && !param_value.nil?
+ if namespace
+ params[namespace][param_name] = param_value = type_cast_value(rule.options[:type], param_value)
+ else
+ params[param_name] = param_value = type_cast_value(rule.options[:type], param_value)
+ end
+ end
+
+ choices = rule.options[:options] || rule.options[:in]
+ if choices && param_value && !choices.include?(param_value)
+ raise InvalidParamValue, "Value for parameter '#{param_name}' (#{param_value}) is not in the allowed set of values."
+ end
+
+ params
+ end
+
+ def self.unexpected_params?(params, param_names)
+ # Raise an exception unless no unexpected params were found
+ unexpected_keys = (params.keys - param_names)
+ unless unexpected_keys.empty?
+ raise UnexpectedParam, "Request included unexpected parameter(s): #{unexpected_keys.map{|k| ERB::Util.html_escape(k)}.join(', ')}"
+ end
+ end
+
+
+ def self.type_cast_value(type, value)
+ case type
+ when :integer
+ value.to_i
+ when :float, :decimal
+ value.to_f
+ when :string
+ value.to_s
+ when :boolean
+ if value.is_a? TrueClass
+ true
+ elsif value.is_a? FalseClass
+ false
+ else
+ case value.to_s
+ when /^(1|true|TRUE|T|Y)$/
+ true
+ when /^(0|false|FALSE|F|N)$/
+ false
+ else
+ raise InvalidParamValue, "Could not typecast boolean to appropriate value"
+ end
+ end
+ # An array type is a comma delimited string, we need to cast the passed strings.
+ when :array
+ value.respond_to?(:split) ? value.split(',') : value
+ when :binary, :array, :file
+ value
+ else
+ value
+ end
+ end
+
+ # Checks that the value's type matches the expected type for a given param
+ #
+ # @param [Symbol, String] Param name used if the verification fails and that an error is raised.
+ # @param [#to_s] The value to validate.
+ # @param [Symbol] The expected type, such as :boolean, :integer etc...
+ # @raise [InvalidParamType] Custom exception raised when the validation isn't found or the value doesn't match.
+ #
+ # @return [Nil]
+ # @api public
+ # TODO raising an exception really isn't a good idea since it forces the stack to unwind.
+ # More than likely developers are using exceptions to control the code flow and a different approach should be used.
+ # Catch/throw is a bit more efficient but is still the wrong approach for this specific problem.
+ def self.verify_cast(name, value, expected_type)
+ validation = ParamsVerification.type_validations[expected_type.to_sym]
+ unless validation.nil? || value.to_s =~ validation
+ raise InvalidParamType, "Value for parameter '#{name}' (#{value}) is of the wrong type (expected #{expected_type})"
+ end
+ end
+
+end
View
61 benchmarks/params_verification_benchmarks.rb
@@ -0,0 +1,61 @@
+require 'rubygems'
+require 'benchmark/ips' # gem install benchmark_suite
+$:<< '.'
+require "../lib/params_verification"
+require "../lib/weasel_diesel"
+require "../spec/test_services"
+require "old_exception_based_params_verification"
+
+service = WSList.all.find{|s| s.url == 'services/test.xml'}
+valid_params = {'framework' => 'RSpec', 'version' => '1.02', 'user' => {'id' => '123'}}
+bad_params = {'framework' => 'minitest', 'version' => '1.02', 'user' => {'id' => '123'}}
+
+
+ParamsVerification.validate(valid_params, service.defined_params)
+
+Benchmark.ips do |x|
+ # To reduce overhead, the number of iterations is passed in
+ # and the block must run the code the specific number of times.
+ # Used for when the workload is very small and any overhead
+ # introduces incorrectable errors.
+ x.report("new validation with valid params") do |times|
+ i = 0
+ while i < times
+ ParamsVerification.validate(valid_params, service.defined_params)
+ i += 1
+ end
+ end
+
+ x.report("legacy validation with valid params") do |times|
+ i = 0
+ while i < times
+ begin
+ LegacyParamsVerification.validate!(valid_params, service.defined_params)
+ rescue Exception => e
+ p e
+ end
+ i += 1
+ end
+ end
+
+ x.report("new validation with bad params") do |times|
+ i = 0
+ while i < times
+ ParamsVerification.validate(bad_params, service.defined_params)
+ i += 1
+ end
+ end
+
+ x.report("legacy validation with bad params") do |times|
+ i = 0
+ while i < times
+ begin
+ LegacyParamsVerification.validate!(bad_params, service.defined_params)
+ rescue Exception => e
+ # e
+ end
+ i += 1
+ end
+ end
+
+end
View
102 lib/params_verification.rb
@@ -31,46 +31,52 @@ def self.type_validations
end
# Validation against each required WeaselDiesel::Params::Rule
- # and returns the potentially modified params (with default values)
+ # and returns the potentially modified params (with default values) and the potential error.
#
# @param [Hash] params The params to verify (incoming request params)
# @param [WeaselDiesel::Params] service_params A Playco service param compatible object listing required and optional params
# @param [Boolean] ignore_unexpected Flag letting the validation know if unexpected params should be ignored
#
- # @return [Hash]
- # The passed params potentially modified by the default rules defined in the service.
+ # @return [Array<Hash, Exception, NilClass>] An array with two elements, the params and an exception or nil.
#
# @example Validate request params against a service's defined param rules
- # ParamsVerification.validate!(request.params, @service.defined_params)
+ # ParamsVerification.validate(request.params, @service.defined_params)
#
# @api public
- def self.validate!(params, service_params, ignore_unexpected=false)
+ def self.validate(params, service_params, ignore_unexpected=false)
+ errors = []
+
# Verify that no garbage params are passed, if they are, an exception is raised.
# only the first level is checked at this point
unless ignore_unexpected
- unexpected_params?(params, service_params.param_names)
+ error = unexpected_params_error(params, service_params.param_names)
+ return [params, error] if error
end
# dupe the params so we don't modify the passed value
updated_params = params.dup
# Required param verification
service_params.list_required.each do |rule|
- updated_params = validate_required_rule(rule, updated_params)
+ updated_params, error = validate_required_rule(rule, updated_params)
+ errors << error if error
end
# Set optional defaults if any optional
service_params.list_optional.each do |rule|
- updated_params = run_optional_rule(rule, updated_params)
+ updated_params, error = run_optional_rule(rule, updated_params)
+ errors << error if error
end
# check the namespaced params
service_params.namespaced_params.each do |param|
param.list_required.each do |rule|
- updated_params = validate_required_rule(rule, updated_params, param.space_name.to_s)
+ updated_params, error = validate_required_rule(rule, updated_params, param.space_name.to_s)
+ errors << error if error
end
param.list_optional.each do |rule|
- updated_params = run_optional_rule(rule, updated_params, param.space_name.to_s)
+ updated_params, error = run_optional_rule(rule, updated_params, param.space_name.to_s)
+ errors << error if error
end
end
@@ -79,12 +85,33 @@ def self.validate!(params, service_params, ignore_unexpected=false)
params.each_pair do |key, value|
if value.is_a?(Hash)
namespaced = service_params.namespaced_params.find{|np| np.space_name.to_s == key.to_s}
- raise UnexpectedParam, "Request included unexpected parameter: #{key}" if namespaced.nil?
- unexpected_params?(params[key], namespaced.param_names)
+ errors << UnexpectedParam.new("Request included unexpected parameter: #{key}") if namespaced.nil?
+ error == unexpected_params_error(params[key], namespaced.param_names)
+ errors << error if error
end
end
- updated_params
+ [updated_params, errors.first]
+ end
+
+ # Validation against each required WeaselDiesel::Params::Rule
+ # and returns the potentially modified params (with default values)
+ #
+ # @param [Hash] params The params to verify (incoming request params)
+ # @param [WeaselDiesel::Params] service_params A Playco service param compatible object listing required and optional params
+ # @param [Boolean] ignore_unexpected Flag letting the validation know if unexpected params should be ignored
+ #
+ # @return [Hash]
+ # The passed params potentially modified by the default rules defined in the service.
+ #
+ # @example Validate request params against a service's defined param rules
+ # ParamsVerification.validate!(request.params, @service.defined_params)
+ #
+ # @api public
+ def self.validate!(params, service_params, ignore_unexpected=false)
+ mutated_params, error = validate(params, service_params, ignore_unexpected=false)
+ raise error if error
+ mutated_params
end
@@ -97,30 +124,26 @@ def self.validate!(params, service_params, ignore_unexpected=false)
# @param [Hash] params The request params.
# @param [String] namespace Optional param namespace to check the rule against.
#
- # @return [Hash]
+ # @return [Array<Hash, Exception, NilClass>] An array containing 2 values:
# A hash representing the potentially modified params after going through the filter.
+ # An exception with the details of the error, nil if no errors occur.
#
# @api private
def self.validate_required_rule(rule, params, namespace=nil)
param_name = rule.name.to_s
param_value, namespaced_params = extract_param_values(params, param_name, namespace)
- # puts "verify #{param_name} params, current value: #{param_value}"
-
- #This is disabled since required params shouldn't have a default, otherwise, why are they required?
- #if param_value.nil? && rule.options && rule.options[:default]
- #param_value = rule.options[:default]
- #end
# Checks presence
if !(namespaced_params || params).keys.include?(param_name)
- raise MissingParam, "'#{rule.name}' is missing - passed params: #{params.inspect}."
+ return [params, MissingParam.new("'#{rule.name}' is missing - passed params: #{params.inspect}.")]
# checks null
elsif param_value.nil? && !rule.options[:null]
- raise InvalidParamValue, "Value for parameter '#{param_name}' is missing - passed params: #{params.inspect}."
+ return[params, InvalidParamValue.new("Value for parameter '#{param_name}' is missing - passed params: #{params.inspect}.")]
# checks type
elsif rule.options[:type]
- verify_cast(param_name, param_value, rule.options[:type])
+ error = verify_cast(param_name, param_value, rule.options[:type])
+ return [params, error] if error
end
if rule.options[:options] || rule.options[:in]
@@ -129,11 +152,11 @@ def self.validate_required_rule(rule, params, namespace=nil)
# Force the cast so we can compare properly
param_value = params[param_name] = type_cast_value(rule.options[:type], param_value)
end
- raise InvalidParamValue, "Value for parameter '#{param_name}' (#{param_value}) is not in the allowed set of values." unless choices.include?(param_value)
+ return [params, InvalidParamValue.new("Value for parameter '#{param_name}' (#{param_value}) is not in the allowed set of values.")] unless choices.include?(param_value)
# You can have a "in" rule that also applies a min value since they are mutually exclusive
elsif rule.options[:minvalue]
min = rule.options[:minvalue]
- raise InvalidParamValue, "Value for parameter '#{param_name}' is lower than the min accepted value (#{min})." if param_value.to_i < min
+ return [params, InvalidParamValue.new("Value for parameter '#{param_name}' is lower than the min accepted value (#{min}).")] if param_value.to_i < min
end
# Returns the updated params
@@ -142,7 +165,7 @@ def self.validate_required_rule(rule, params, namespace=nil)
# puts "casting #{param_value} into type: #{rule.options[:type]}"
params[param_name] = type_cast_value(rule.options[:type], param_value)
end
- params
+ [params, nil]
end
@@ -173,7 +196,7 @@ def self.extract_param_values(params, param_name, namespace=nil)
# @param [#WeaselDiesel::Params::Rule] rule The optional rule
# @param [Hash] params The request params
# @param [String] namespace An optional namespace
- # @return [Hash] The potentially modified params
+ # @return [Array<Hash, Exception>] An array with two items, the potentially modified params and an exception if it occurred.
#
# @api private
def self.run_optional_rule(rule, params, namespace=nil)
@@ -200,17 +223,20 @@ def self.run_optional_rule(rule, params, namespace=nil)
choices = rule.options[:options] || rule.options[:in]
if choices && param_value && !choices.include?(param_value)
- raise InvalidParamValue, "Value for parameter '#{param_name}' (#{param_value}) is not in the allowed set of values."
+ return [params, InvalidParamValue.new("Value for parameter '#{param_name}' (#{param_value}) is not in the allowed set of values.")]
end
- params
+ [params, nil]
end
- def self.unexpected_params?(params, param_names)
+ # @return [Exception, NilClass]
+ def self.unexpected_params_error(params, param_names)
# Raise an exception unless no unexpected params were found
unexpected_keys = (params.keys - param_names)
- unless unexpected_keys.empty?
- raise UnexpectedParam, "Request included unexpected parameter(s): #{unexpected_keys.map{|k| ERB::Util.html_escape(k)}.join(', ')}"
+ if unexpected_keys.empty?
+ nil
+ else
+ UnexpectedParam.new("Request included unexpected parameter(s): #{unexpected_keys.map{|k| ERB::Util.html_escape(k)}.join(', ')}")
end
end
@@ -235,6 +261,7 @@ def self.type_cast_value(type, value)
when /^(0|false|FALSE|F|N)$/
false
else
+ # rare exception being raised
raise InvalidParamValue, "Could not typecast boolean to appropriate value"
end
end
@@ -255,15 +282,14 @@ def self.type_cast_value(type, value)
# @param [Symbol] The expected type, such as :boolean, :integer etc...
# @raise [InvalidParamType] Custom exception raised when the validation isn't found or the value doesn't match.
#
- # @return [Nil]
+ # @return [Nil, Exception]
# @api public
- # TODO raising an exception really isn't a good idea since it forces the stack to unwind.
- # More than likely developers are using exceptions to control the code flow and a different approach should be used.
- # Catch/throw is a bit more efficient but is still the wrong approach for this specific problem.
def self.verify_cast(name, value, expected_type)
validation = ParamsVerification.type_validations[expected_type.to_sym]
- unless validation.nil? || value.to_s =~ validation
- raise InvalidParamType, "Value for parameter '#{name}' (#{value}) is of the wrong type (expected #{expected_type})"
+ if validation.nil? || value.to_s =~ validation
+ nil
+ else
+ InvalidParamType.new("Value for parameter '#{name}' (#{value}) is of the wrong type (expected #{expected_type})")
end
end

0 comments on commit 43d88e0

Please sign in to comment.
Something went wrong with that request. Please try again.