Skip to content
Browse files

Refactored uniqueness validator to use Arel instead of hardcoded SQL

  • Loading branch information...
1 parent a9f3c9d commit a30b440debe87722c0c072555db7945181ceffde @bcardarella bcardarella committed with tenderlove Mar 19, 2011
Showing with 11 additions and 26 deletions.
  1. +11 −26 activerecord/lib/active_record/validations/uniqueness.rb
View
37 activerecord/lib/active_record/validations/uniqueness.rb
@@ -14,28 +14,23 @@ def setup(klass)
def validate_each(record, attribute, value)
finder_class = find_finder_class_for(record)
+ table = finder_class.arel_table
coder = record.class.serialized_attributes[attribute.to_s]
if value && coder
value = coder.dump value
end
- sql, params = mount_sql_and_params(finder_class, record.class.quoted_table_name, attribute, value)
-
- relation = finder_class.unscoped.where(sql, *params)
+ relation = build_relation(finder_class, table, attribute, value)
+ relation = relation.and(table[finder_class.primary_key.to_sym].not_eq(record.send(:id))) if record.persisted?
Array.wrap(options[:scope]).each do |scope_item|
scope_value = record.send(scope_item)
- relation = relation.where(scope_item => scope_value)
- end
-
- if record.persisted?
- # TODO : This should be in Arel
- relation = relation.where("#{record.class.quoted_table_name}.#{record.class.primary_key} <> ?", record.send(:id))
+ relation = relation.and(table[scope_item].eq(scope_value))
end
- if relation.exists?
+ if finder_class.unscoped.where(relation).exists?

Tell me please why do you use unscoped here? It kills my default_scope, but still can not understand why it's needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
record.errors.add(attribute, :taken, options.except(:case_sensitive, :scope).merge(:value => value))
end
end
@@ -57,27 +52,17 @@ def find_finder_class_for(record) #:nodoc:
class_hierarchy.detect { |klass| !klass.abstract_class? }
end
- def mount_sql_and_params(klass, table_name, attribute, value) #:nodoc:
+ def build_relation(klass, table, attribute, value) #:nodoc:
column = klass.columns_hash[attribute.to_s]
+ value = column.limit ? value.to_s.mb_chars[0, column.limit] : value.to_s if column.text?
- operator = if value.nil?
- "IS ?"
- elsif column.text?
- value = column.limit ? value.to_s.mb_chars[0, column.limit] : value.to_s
- "#{klass.connection.case_sensitive_equality_operator} ?"
- else
- "= ?"
- end
-
- sql_attribute = "#{table_name}.#{klass.connection.quote_column_name(attribute)}"
-
- if value.nil? || (options[:case_sensitive] || !column.text?)
- sql = "#{sql_attribute} #{operator}"
+ if !options[:case_sensitive] && column.text?
+ relation = table[attribute].matches(value)
else
- sql = "LOWER(#{sql_attribute}) = LOWER(?)"
+ relation = table[attribute].eq(value)
@sikachu
Ruby on Rails member
sikachu added a note Apr 11, 2011

This is currently break the test suite in mysql_test, as ARel will translate this into = predicate which performs case-insensitive search on MySQL. We were getting around it by using klass.connection.case_sensitive_equality_operator.

I'm going to submit a patch into ARel which force outputs BINARY 'xxxx' when encounter MySQL. I think it should be mandatory, whereas if you want a case insensitive search you'd do #matches(value) instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
- [sql, [value]]
+ relation
end
end

9 comments on commit a30b440

@metaskills

This broke the SQL Server adapter which needs the equality operator to be prefixed like:

WHERE [topics].[title] COLLATE Latin1_General_CS_AS_WS = N'I''M UNIQUE!'

If I define methods with the new convention, like so.

def case_sensitive_modifier(node)
  Arel::Nodes::Bin.new(node)
end

def visit_Arel_Nodes_Bin(o)
  "COLLATE Latin1_General_CS_AS_WS = #{visit o.expr}"
end

It will generate invalid SQL.

WHERE [topics].[title] = COLLATE Latin1_General_CS_AS_WS = N'I''m unique!'
@metaskills

OK, nevermind. I just rethought the problem to match the implementation. Hence:

def visit_Arel_Nodes_Bin(o)
  "#{visit o.expr} COLLATE Latin1_General_CS_AS_WS"
end
@tenderlove
Ruby on Rails member

@metaskills I think we're going to need to changes this to do LOWER() on both sides. Would that be a problem for SQL server?

@metaskills

I honestly do not know. I know LOWER is a good function. I also just found out that after doing the implementation above, I have 2 tests that fail like so https://gist.github.com/939990

I fixed it by making my case_sensitive_modifier do this:

def case_sensitive_modifier(node)
  node.acts_like?(:string) ? Arel::Nodes::Bin.new(node) : node
end

The fact that the two failures were for things like this "WHERE [topics].[parent_id] = 2 COLLATE Latin1_General_CS_AS_WS" means that whatever solution you use, I might have to hack and/or get clever. I'll keep an eye out and I hope this information helps.

@tenderlove
Ruby on Rails member

Well, we need to change to LOWER() because the current match function could cause false positives. It's on my TODO list before 3.1 is out, I just haven't done it yet. :-(

@eduardordm

Using a function will require an index with that function in it, specially in Oracle.

@bcardarella

@tenderlove are you advocating going back to the pure SQL implementation? Did I miss that Arel has LOWER() support?

If this is an issue similar to the MySQL equality operator why not address it in Arel?

@tenderlove
Ruby on Rails member

@bcardarella no, we won't go back to the SQL implementation. ARel has generic function support, but I'm going to add a factory method so we don't have to create the node manually.

@pyromaniac

Hi, i think there is a bug in validation. Here is description solution - #2325 check it please and merge if i'm right. Thanks.

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