Re: [PATCH] First cut at a patch for validates_uniqueness_of error (was: Re: [PATCH] Test case demonstrating validates_uniqueness_of error with cpk)

[Darrin Holst]

Thanks for the patch David, I applied it and pushed it. Although I don't actively maintain this project anymore I'm happy to apply patches with tests (forks are more welcome).

I wouldn't worry about ugliness, you'll find that there are a lot of place that just copy whole chunks of ActiveRecord and just make a small change to make it work for composite keys. That's what makes this project very brittle when there are AR changes.

Thanks,

Darrin

On Wed, Sep 9, 2009 at 5:49 PM, David Yip <yi...@northwestern.edu> wrote:

Hi Darrin,

Here's a first cut at a patch for the validates_uniqueness_of error I mentioned in my previous email.  It's ugly (essentially copies validates_uniqueness_of) but it gets the job done -- or at least it fixes the error I encountered.  Refinements are definitely welcome.

The meat of the patch is in lib/composite_primary_keys/validations/uniqueness.rb:54-64.

For convenience, I'm also attaching the previous patch.

Thanks,

- David

David Yip wrote:

Hi Darrin,

I'm sending this Composite Primary Keys-related email to you because you seem to be the most active committer on the CPK codebase.  If that's not the case, or if you'd rather defer this to someone else for any reason, feel free to do so.

There's an erroneous interaction between CPK 2.3.2 and validates_uniqueness_of on persisted records in ActiveRecord 2.3.3 (and likely past/future versions, as the code in question doesn't seem to have changed much).  The problem seems to be around validations.rb:753, which is as follows:

 unless record.new_record?
 condition_sql << " AND #{record.class.quoted_table_name}.#{record.class.primary_key} <> ?"
 condition_params << record.send(:id)
 end

When CPK is in use, record.class.primary_key returns an Array, as does record.id.  Interpolating these values into the SQL string produces an erroneous SQL statement.

I've attached a test case against CPK @ f4a1e04b that demonstrates the problem.  The schema changes are only present for the sqlite3 test case, but shouldn't be too hard to port to the schemas for other database systems.

Unfortunately I haven't yet come up with any good ways to fix this.  The  offending behavior of validates_uniqueness_of can't be easily overridden, and although I know CPK reimplements large chunks of ActiveRecord (e.g. for calculations) suggesting the same fix here just feels dirty.  So I thought it best to inform you of this issue and see how you'd like to proceed with it.

Thanks,

- David

From e24393bfb76df4a7b7972c3b4e8d3d0451fa17f5 Mon Sep 17 00:00:00 2001
From: David Yip <yi...@northwestern.edu>
Date: Wed, 9 Sep 2009 15:16:00 -0500
Subject: [PATCH 1/2] A test to demonstrate errors that occur when running validates_uniqueness_of on existing records.

---
 test/fixtures/db_definitions/sqlite.sql |    6 ++++++
 test/fixtures/seat.rb                   |    5 +++++
 test/fixtures/seats.yml                 |    4 ++++
 test/test_validations.rb                |   11 +++++++++++
 4 files changed, 26 insertions(+), 0 deletions(-)
 create mode 100644 test/fixtures/seat.rb
 create mode 100644 test/fixtures/seats.yml
 create mode 100644 test/test_validations.rb

diff --git a/test/fixtures/db_definitions/sqlite.sql b/test/fixtures/db_definitions/sqlite.sql
index fd8c566..b670125 100644
--- a/test/fixtures/db_definitions/sqlite.sql
+++ b/test/fixtures/db_definitions/sqlite.sql
@@ -158,3 +158,9 @@ create table room_assignments (
       room_id integer not null
 );

+create table seats (
+  flight_number integer not_null,
+  seat integer not_null,
+  customer integer,
+  primary key (flight_number, seat)
+);
diff --git a/test/fixtures/seat.rb b/test/fixtures/seat.rb
new file mode 100644
index 0000000..efd3339
--- /dev/null
+++ b/test/fixtures/seat.rb
@@ -0,0 +1,5 @@
+class Seat < ActiveRecord::Base
+  set_primary_keys [:flight_number, :seat]
+
+  validates_uniqueness_of :customer
+end
diff --git a/test/fixtures/seats.yml b/test/fixtures/seats.yml
new file mode 100644
index 0000000..2b5582c
--- /dev/null
+++ b/test/fixtures/seats.yml
@@ -0,0 +1,4 @@
+seat1:
+  flight_number: 1
+  seat: 1
+  customer: 1
diff --git a/test/test_validations.rb b/test/test_validations.rb
new file mode 100644
index 0000000..54c08b0
--- /dev/null
+++ b/test/test_validations.rb
@@ -0,0 +1,11 @@
+require 'abstract_unit'
+require 'fixtures/seat'
+
+class TestValidations < ActiveSupport::TestCase
+  fixtures :seats
+
+  def test_uniqueness_validation_on_saved_record
+    s = Seat.find([1,1])
+    assert s.valid?
+  end
+end
--
1.6.2.1


From 07ebd9f5c5a50f372aaf5f0a63c0efecccd83629 Mon Sep 17 00:00:00 2001
From: David Yip <yi...@northwestern.edu>
Date: Wed, 9 Sep 2009 17:22:15 -0500
Subject: [PATCH 2/2] First cut at a patch for key interpolation problems in validates_uniqueness_of.

---
 Manifest.txt                                       |    1 +
 lib/composite_primary_keys.rb                      |    1 +
 lib/composite_primary_keys/base.rb                 |    2 +
 .../validations/uniqueness.rb                      |   77 ++++++++++++++++++++
 4 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100644 lib/composite_primary_keys/validations/uniqueness.rb

diff --git a/Manifest.txt b/Manifest.txt
index 733fd99..7137651 100644
--- a/Manifest.txt
+++ b/Manifest.txt
@@ -24,6 +24,7 @@ lib/composite_primary_keys/connection_adapters/sqlite3_adapter.rb
 lib/composite_primary_keys/fixtures.rb
 lib/composite_primary_keys/migration.rb
 lib/composite_primary_keys/reflection.rb
+lib/composite_primary_keys/validations/uniqueness.rb
 lib/composite_primary_keys/version.rb
 loader.rb
 local/database_connections.rb.sample
diff --git a/lib/composite_primary_keys.rb b/lib/composite_primary_keys.rb
index 097aa0c..afab434 100644
--- a/lib/composite_primary_keys.rb
+++ b/lib/composite_primary_keys.rb
@@ -44,6 +44,7 @@ require 'composite_primary_keys/base'
 require 'composite_primary_keys/calculations'
 require 'composite_primary_keys/migration'
 require 'composite_primary_keys/attribute_methods'
+require 'composite_primary_keys/validations/uniqueness'

 ActiveRecord::Base.class_eval do
  include CompositePrimaryKeys::ActiveRecord::Base
diff --git a/lib/composite_primary_keys/base.rb b/lib/composite_primary_keys/base.rb
index a4c7ff9..1aa3a8a 100644
--- a/lib/composite_primary_keys/base.rb
+++ b/lib/composite_primary_keys/base.rb
@@ -29,6 +29,8 @@ module CompositePrimaryKeys
            include CompositePrimaryKeys::ActiveRecord::AssociationPreload
            include CompositePrimaryKeys::ActiveRecord::Calculations
            include CompositePrimaryKeys::ActiveRecord::AttributeMethods
+
+            extend CompositePrimaryKeys::ActiveRecord::Validations::Uniqueness::ClassMethods
          EOV
        end

diff --git a/lib/composite_primary_keys/validations/uniqueness.rb b/lib/composite_primary_keys/validations/uniqueness.rb
new file mode 100644
index 0000000..1d98c22
--- /dev/null
+++ b/lib/composite_primary_keys/validations/uniqueness.rb
@@ -0,0 +1,77 @@
+module CompositePrimaryKeys
+  module ActiveRecord
+    module Validations
+      module Uniqueness
+        module ClassMethods
+          def validates_uniqueness_of(*attr_names)
+            configuration = { :case_sensitive => true }
+            configuration.update(attr_names.extract_options!)
+
+            validates_each(attr_names,configuration) do |record, attr_name, value|
+              # The check for an existing value should be run from a class that
+              # isn't abstract. This means working down from the current class
+              # (self), to the first non-abstract class. Since classes don't know
+              # their subclasses, we have to build the hierarchy between self and
+              # the record's class.
+              class_hierarchy = [record.class]
+              while class_hierarchy.first != self
+                class_hierarchy.insert(0, class_hierarchy.first.superclass)
+              end
+
+              # Now we can work our way down the tree to the first non-abstract
+              # class (which has a database table to query from).
+              finder_class = class_hierarchy.detect { |klass| !klass.abstract_class? }
+
+              column = finder_class.columns_hash[attr_name.to_s]
+
+              if value.nil?
+                comparison_operator = "IS ?"
+              elsif column.text?
+                comparison_operator = "#{connection.case_sensitive_equality_operator} ?"
+                value = column.limit ? value.to_s.mb_chars[0, column.limit] : value.to_s
+              else
+                comparison_operator = "= ?"
+              end
+
+              sql_attribute = "#{record.class.quoted_table_name}.#{connection.quote_column_name(attr_name)}"
+
+              if value.nil? || (configuration[:case_sensitive] || !column.text?)
+                condition_sql = "#{sql_attribute} #{comparison_operator}"
+                condition_params = [value]
+              else
+                condition_sql = "LOWER(#{sql_attribute}) #{comparison_operator}"
+                condition_params = [value.mb_chars.downcase]
+              end
+
+              if scope = configuration[:scope]
+                Array(scope).map do |scope_item|
+                  scope_value = record.send(scope_item)
+                  condition_sql << " AND " << attribute_condition("#{record.class.quoted_table_name}.#{scope_item}", scope_value)
+                  condition_params << scope_value
+                end
+              end
+
+              unless record.new_record?
+                if record.class.composite?
+                  record.class.primary_keys.each do |key|
+                    condition_sql << " AND #{record.class.quoted_table_name}.#{key} <> ?"
+                    condition_params << record.send(key)
+                  end
+                else
+                  condition_sql << " AND #{record.class.quoted_table_name}.#{record.class.primary_key} <> ?"
+                  condition_params << record.send(:id)
+                end
+              end
+
+              finder_class.with_exclusive_scope do
+                if finder_class.exists?([condition_sql, *condition_params])
+                  record.errors.add(attr_name, :taken, :default => configuration[:message], :value => value)
+                end
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
--
1.6.2.1