Create a new instance of a validator per validation. Cache for Virtus::Attribute.build calls.

antmanj-visma · antmanj-visma · commit 251f0de25bb8 · 2017-06-21T22:55:14.000+03:00
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2017-06-13 12:09:49 -0400 using RuboCop version 0.47.0.
+# on 2017-06-17 22:15:11 +0300 using RuboCop version 0.47.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -10,21 +10,21 @@
 Metrics/AbcSize:
   Max: 44
 
-# Offense count: 278
+# Offense count: 279
 # Configuration parameters: CountComments, ExcludedMethods.
 Metrics/BlockLength:
   Max: 3117
 
 # Offense count: 8
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 287
+  Max: 288
 
 # Offense count: 28
 Metrics/CyclomaticComplexity:
   Max: 14
 
-# Offense count: 1108
+# Offense count: 1114
 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
 # URISchemes: http, https
 Metrics/LineLength:
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@
 * [#1632](https://github.com/ruby-grape/grape/pull/1632): Silence warnings - [@thogg4](https://github.com/thogg4).
 * [#1615](https://github.com/ruby-grape/grape/pull/1615): Fix default and type validator when values is a Hash with no value attribute - [@jlfaber](https://github.com/jlfaber).
 * [#1625](https://github.com/ruby-grape/grape/pull/1625): Handle `given` correctly when nested in Array params - [@rnubel](https://github.com/rnubel), [@avellable](https://github.com/avellable).
+* [#1649](https://github.com/ruby-grape/grape/pull/1649): Don't share validator instances between requests - [@anakinj](https://github.com/anakinj).
 * Your contribution here.
 
 ### 0.19.2 (4/12/2017)
diff --git a/README.md b/README.md
@@ -1422,6 +1422,8 @@ params do
 end
 ```
 
+Every validation will have it's own instance of the validator, which means that the validator can have a state.
+
 ### Validation Errors
 
 Validation and coercion errors are collected and an exception of type `Grape::Exceptions::ValidationErrors` is raised. If the exception goes uncaught it will respond with a status of 400 and an error message. The validation errors are grouped by parameter name and can be accessed via `Grape::Exceptions::ValidationErrors#errors`.
diff --git a/benchmark/simple_with_type_coercer.rb b/benchmark/simple_with_type_coercer.rb
@@ -0,0 +1,22 @@
+$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib'))
+require 'grape'
+require 'benchmark/ips'
+
+api = Class.new(Grape::API) do
+  prefix :api
+  version 'v1', using: :path
+  params do
+    requires :param, type: Array[String]
+  end
+  get '/' do
+    'hello'
+  end
+end
+
+env = Rack::MockRequest.env_for('/api/v1?param=value', method: 'GET')
+
+Benchmark.ips do |ips|
+  ips.report('simple_with_type_coercer') do
+    api.call(env)
+  end
+end
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -210,5 +210,6 @@ module ServeFile
 require 'grape/validations/params_scope'
 require 'grape/validations/validators/all_or_none'
 require 'grape/validations/types'
+require 'grape/validations/validator_factory'
 
 require 'grape/version'
diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb
@@ -338,9 +338,11 @@ def lazy_initialize!
       end
     end
 
-    def run_validators(validators, request)
+    def run_validators(validator_factories, request)
       validation_errors = []
 
+      validators = validator_factories.map(&:create_validator)
+
       ActiveSupport::Notifications.instrument('endpoint_run_validators.grape', endpoint: self, validators: validators, request: request) do
         validators.each do |validator|
           begin
diff --git a/lib/grape/validations.rb b/lib/grape/validations.rb
@@ -14,5 +14,9 @@ class << self
     def self.register_validator(short_name, klass)
       validators[short_name] = klass
     end
+
+    def self.deregister_validator(short_name)
+      validators.delete(short_name)
+    end
   end
 end
diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb
@@ -398,8 +398,13 @@ def validate(type, options, attrs, doc_attrs, opts)
 
         raise Grape::Exceptions::UnknownValidator.new(type) unless validator_class
 
-        value = validator_class.new(attrs, options, doc_attrs[:required], self, opts)
-        @api.namespace_stackable(:validations, value)
+        factory = Grape::Validations::ValidatorFactory.new(attributes:      attrs,
+                                                           options:         options,
+                                                           required:        doc_attrs[:required],
+                                                           params_scope:    self,
+                                                           opts:            opts,
+                                                           validator_class: validator_class)
+        @api.namespace_stackable(:validations, factory)
       end
 
       def validate_value_coercion(coerce_type, *values_list)
diff --git a/lib/grape/validations/types/build_coercer.rb b/lib/grape/validations/types/build_coercer.rb
@@ -20,6 +20,12 @@ module Types
       # @return [Virtus::Attribute] object to be used
       #   for coercion and type validation
       def self.build_coercer(type, method = nil)
+        cache_instance(type, method) do
+          create_coercer_instance(type, method)
+        end
+      end
+
+      def self.create_coercer_instance(type, method = nil)
         # Accept pre-rolled virtus attributes without interference
         return type if type.is_a? Virtus::Attribute
 
@@ -56,6 +62,27 @@ def self.build_coercer(type, method = nil)
         # for many common ruby types.
         Virtus::Attribute.build(conversion_type, converter_options)
       end
+
+      def self.cache_instance(type, method, &_block)
+        key = cache_key(type, method)
+
+        return @__cache[key] if @__cache.key?(key)
+
+        instance = yield
+
+        @__cache_write_lock.synchronize do
+          @__cache[key] = instance
+        end
+
+        instance
+      end
+
+      def self.cache_key(type, method)
+        [type, method].compact.map(&:to_s).join('_')
+      end
+
+      instance_variable_set(:@__cache,            {})
+      instance_variable_set(:@__cache_write_lock, Mutex.new)
     end
   end
 end
diff --git a/lib/grape/validations/validator_factory.rb b/lib/grape/validations/validator_factory.rb
@@ -0,0 +1,18 @@
+module Grape
+  module Validations
+    class ValidatorFactory
+      def initialize(**options)
+        @validator_class = options.delete(:validator_class)
+        @options         = options
+      end
+
+      def create_validator
+        @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/base.rb b/lib/grape/validations/validators/base.rb
@@ -20,7 +20,6 @@ def initialize(attrs, options, required, scope, opts = {})
       end
 
       # Validates a given request.
-      # @note This method must be thread-safe.
       # @note Override #validate! unless you need to access the entire request.
       # @param request [Grape::Request] the request currently being handled
       # @raise [Grape::Exceptions::Validation] if validation failed
@@ -31,8 +30,7 @@ def validate(request)
       end
 
       # Validates a given parameter hash.
-      # @note This method must be thread-safe.
-      # @note Override #validate iff you need to access the entire request.
+      # @note Override #validate if you need to access the entire request.
       # @param params [Hash] parameters to validate
       # @raise [Grape::Exceptions::Validation] if validation failed
       # @return [void]
@@ -65,8 +63,8 @@ def self.convert_to_short_name(klass)
       end
 
       def self.inherited(klass)
-        short_name = convert_to_short_name(klass)
-        Validations.register_validator(short_name, klass)
+        return unless klass.name.present?
+        Validations.register_validator(convert_to_short_name(klass), klass)
       end
 
       def message(default_key = nil)
diff --git a/spec/grape/validations/instance_behaivour_spec.rb b/spec/grape/validations/instance_behaivour_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+describe 'Validator with instance variables' do
+  let(:validator_type) do
+    Class.new(Grape::Validations::Base) do
+      def validate_param!(_attr_name, _params)
+        if @instance_variable
+          raise Grape::Exceptions::Validation, params: ['params'],
+                                               message: 'This should never happen'
+        end
+        @instance_variable = true
+      end
+    end
+  end
+
+  before do
+    Grape::Validations.register_validator('instance_validator', validator_type)
+  end
+
+  after do
+    Grape::Validations.deregister_validator('instance_validator')
+  end
+
+  let(:app) do
+    Class.new(Grape::API) do
+      params do
+        optional :param_to_validate, instance_validator: true
+        optional :another_param_to_validate, instance_validator: true
+      end
+      get do
+        'noop'
+      end
+    end
+  end
+
+  it 'passes validation every time' do
+    expect(validator_type).to receive(:new).exactly(4).times.and_call_original
+
+    2.times do
+      get '/', param_to_validate: 'value', another_param_to_validate: 'value'
+      expect(last_response.status).to eq 200
+    end
+  end
+end
diff --git a/spec/grape/validations/types_spec.rb b/spec/grape/validations/types_spec.rb
@@ -90,4 +90,23 @@ class VirtusC
       expect(described_class.custom?(TypesSpec::BarType)).to be_falsy
     end
   end
+
+  describe '::build_coercer' do
+    it 'has internal cache variables' do
+      expect(described_class.instance_variable_get(:@__cache)).to be_a(Hash)
+      expect(described_class.instance_variable_get(:@__cache_write_lock)).to be_a(Mutex)
+    end
+
+    it 'caches the result of the Virtus::Attribute.build method' do
+      original_cache = described_class.instance_variable_get(:@__cache)
+      described_class.instance_variable_set(:@__cache, {})
+
+      coercer = 'TestCoercer'
+      expect(Virtus::Attribute).to receive(:build).once.and_return(coercer)
+      expect(described_class.build_coercer(Array[String])).to eq(coercer)
+      expect(described_class.build_coercer(Array[String])).to eq(coercer)
+
+      described_class.instance_variable_set(:@__cache, original_cache)
+    end
+  end
 end
