Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

AM::MassAssingmentSecurity: improve performance #5431

Merged
merged 1 commit into from

2 participants

@bogdan

According to this article:
http://merbist.com/2012/02/23/quick-dive-into-ruby-orm-object-initialization/

Current implementation of mass assignment security takes a lot of processor time because spawning to many redundant objects in MassAssignmentSecuritySanitizer#debug_protected_attribute_removal

We can get rid of this method.

Benchmark: https://gist.github.com/2036114

----------when something sanitized----------
New:   0.010000   0.000000   0.010000 (  0.006924)
Old:   0.010000   0.000000   0.010000 (  0.009284)
----------when nothing sanitized----------
New:   0.010000   0.000000   0.010000 (  0.003906)
Old:   0.000000   0.000000   0.000000 (  0.005500) 

A little side effect of this patch attributes are processed one by one. Instead of:

      def process_removed_attributes(attrs)

We get:

      def process_removed_attribute(attr)

The only one place where this could cause side effect is when someone wants to create it's own sanitizer(other than built-in Strict and Logger).

We can make a backward compatibility for this but decided that it doesn't make that much sense.

@josevalim josevalim merged commit cc1c4ac into rails:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 14, 2012
  1. @bogdan
This page is out of date. Refresh to see the latest.
View
34 activemodel/lib/active_model/mass_assignment_security/sanitizer.rb
@@ -3,20 +3,18 @@ module MassAssignmentSecurity
class Sanitizer
# Returns all attributes not denied by the authorizer.
def sanitize(attributes, authorizer)
- sanitized_attributes = attributes.reject { |key, value| authorizer.deny?(key) }
- debug_protected_attribute_removal(attributes, sanitized_attributes)
- sanitized_attributes
+ attributes.reject do |attr, value|
+ if authorizer.deny?(attr)
+ process_removed_attribute(attr)
+ true
+ end
+ end
end
protected
- def debug_protected_attribute_removal(attributes, sanitized_attributes)
- removed_keys = attributes.keys - sanitized_attributes.keys
- process_removed_attributes(removed_keys) if removed_keys.any?
- end
-
- def process_removed_attributes(attrs)
- raise NotImplementedError, "#process_removed_attributes(attrs) suppose to be overwritten"
+ def process_removed_attribute(attr)
+ raise NotImplementedError, "#process_removed_attribute(attr) suppose to be overwritten"
end
end
@@ -34,8 +32,8 @@ def logger?
@target.respond_to?(:logger) && @target.logger
end
- def process_removed_attributes(attrs)
- logger.warn "Can't mass-assign protected attributes: #{attrs.join(', ')}" if logger?
+ def process_removed_attribute(attr)
+ logger.warn "Can't mass-assign protected attribute: #{attr}" if logger?
end
end
@@ -44,19 +42,19 @@ def initialize(target = nil)
super()
end
- def process_removed_attributes(attrs)
- return if (attrs - insensitive_attributes).empty?
- raise ActiveModel::MassAssignmentSecurity::Error.new(attrs)
+ def process_removed_attribute(attr)
+ return if insensitive_attributes.include?(attr)
+ raise ActiveModel::MassAssignmentSecurity::Error.new(attr)
end
def insensitive_attributes
- ['id']
+ @insensitive_attributes ||= ['id']
end
end
class Error < StandardError
- def initialize(attrs)
- super("Can't mass-assign protected attributes: #{attrs.join(', ')}")
+ def initialize(attr)
+ super("Can't mass-assign protected attribute: #{attr}")
end
end
end
View
2  activemodel/test/cases/mass_assignment_security_test.rb
@@ -4,7 +4,7 @@
class CustomSanitizer < ActiveModel::MassAssignmentSecurity::Sanitizer
- def process_removed_attributes(attrs)
+ def process_removed_attribute(attr)
raise StandardError
end
Something went wrong with that request. Please try again.