From f81c6bc0404ba2a03eed0ec6c08bbac45661305f Mon Sep 17 00:00:00 2001 From: snusnu Date: Sun, 21 Feb 2010 03:05:28 +0100 Subject: AMo #key is now #to_key and CI is probably happy Obviously #key is a too common name to be included in the AMo interface, #to_key fits better and also relates nicely to #to_param. Thx wycats, koz and josevalim for the suggestion. AR's #to_key implementation now takes customized primary keys into account and there's a testcase for that too. The #to_param AMo lint makes no assumptions on how the method behaves in the presence of composite primary keys. It leaves the decision wether to provide a default, or to raise and thus signal to the user that implementing this method will need his special attention, up to the implementers. All AMo cares about is that #to_param is implemented and returns nil in case of a new_record?. The default CompliantObject used in lint_test provides a naive default implementation that just joins all key attributes with '-'. The #to_key default implementation in lint_test's CompliantObject now returns [id] instead of [1]. This was previously causing the (wrong) tests I added for AR's #to_key implementation to pass. The #to_key tests added with this patch should be better. The CI failure was caused by my lack of knowledge about the test:isolated task. The tests for the record_identifier code in action_controller are using fake non AR models and I forgot to stub the #to_key method over there. This issue didn't come up when running the test task, only test:isolated revealed it. This patch fixes that. All tests pass isolated or not, well, apart from one previously unpended test in action_controller that is unrelated to my patch. --- actionpack/lib/action_controller/record_identifier.rb | 4 ++-- actionpack/test/controller/record_identifier_test.rb | 1 + actionpack/test/lib/controller/fake_models.rb | 14 +++++++------- actionpack/test/template/prototype_helper_test.rb | 6 +++--- activemodel/lib/active_model/lint.rb | 18 ++++++++++++------ activemodel/test/cases/lint_test.rb | 8 +++++--- .../lib/active_record/attribute_methods/primary_key.rb | 8 +++++--- activerecord/test/cases/pk_test.rb | 15 ++++++++++----- 8 files changed, 45 insertions(+), 29 deletions(-) diff --git a/actionpack/lib/action_controller/record_identifier.rb b/actionpack/lib/action_controller/record_identifier.rb index 9c35c5b0c3..3f966b1b64 100644 --- a/actionpack/lib/action_controller/record_identifier.rb +++ b/actionpack/lib/action_controller/record_identifier.rb @@ -77,8 +77,8 @@ module ActionController # make sure yourself that your dom ids are valid, in case you overwrite this method. def record_key_for_dom_id(record) return record.id unless record.respond_to?(:to_model) - key = record.to_model.key - key ? sanitize_dom_id(key.join('-')) : key + key = record.to_model.to_key + key ? sanitize_dom_id(key.join('_')) : key end # Replaces characters that are invalid in HTML DOM ids with valid ones. diff --git a/actionpack/test/controller/record_identifier_test.rb b/actionpack/test/controller/record_identifier_test.rb index 6b6d154faa..813dedc80d 100644 --- a/actionpack/test/controller/record_identifier_test.rb +++ b/actionpack/test/controller/record_identifier_test.rb @@ -5,6 +5,7 @@ class Comment include ActiveModel::Conversion attr_reader :id + def to_key; id ? [id] : nil end def save; @id = 1 end def new_record?; @id.nil? end def name diff --git a/actionpack/test/lib/controller/fake_models.rb b/actionpack/test/lib/controller/fake_models.rb index e4aed14dc0..4475d40f63 100644 --- a/actionpack/test/lib/controller/fake_models.rb +++ b/actionpack/test/lib/controller/fake_models.rb @@ -6,7 +6,7 @@ class Customer < Struct.new(:name, :id) undef_method :to_json - def key + def to_key id ? [id] : nil end @@ -47,7 +47,7 @@ module Quiz extend ActiveModel::Naming include ActiveModel::Conversion - def key + def to_key id ? [id] : nil end @@ -67,7 +67,7 @@ class Post < Struct.new(:title, :author_name, :body, :secret, :written_on, :cost alias_method :secret?, :secret - def key + def to_key id ? [id] : nil end @@ -96,7 +96,7 @@ class Comment attr_reader :id attr_reader :post_id def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end - def key; id ? [id] : nil end + def to_key; id ? [id] : nil end def save; @id = 1; @post_id = 1 end def new_record?; @id.nil? end def to_param; @id; end @@ -116,7 +116,7 @@ class Tag attr_reader :id attr_reader :post_id def initialize(id = nil, post_id = nil); @id, @post_id = id, post_id end - def key; id ? [id] : nil end + def to_key; id ? [id] : nil end def save; @id = 1; @post_id = 1 end def new_record?; @id.nil? end def to_param; @id; end @@ -136,7 +136,7 @@ class CommentRelevance attr_reader :id attr_reader :comment_id def initialize(id = nil, comment_id = nil); @id, @comment_id = id, comment_id end - def key; id ? [id] : nil end + def to_key; id ? [id] : nil end def save; @id = 1; @comment_id = 1 end def new_record?; @id.nil? end def to_param; @id; end @@ -152,7 +152,7 @@ class TagRelevance attr_reader :id attr_reader :tag_id def initialize(id = nil, tag_id = nil); @id, @tag_id = id, tag_id end - def key; id ? [id] : nil end + def to_key; id ? [id] : nil end def save; @id = 1; @tag_id = 1 end def new_record?; @id.nil? end def to_param; @id; end diff --git a/actionpack/test/template/prototype_helper_test.rb b/actionpack/test/template/prototype_helper_test.rb index dab34f1201..45f9d85e32 100644 --- a/actionpack/test/template/prototype_helper_test.rb +++ b/actionpack/test/template/prototype_helper_test.rb @@ -4,7 +4,7 @@ require 'active_model' class Bunny < Struct.new(:Bunny, :id) extend ActiveModel::Naming include ActiveModel::Conversion - def key() id ? [id] : nil end + def to_key() id ? [id] : nil end end class Author @@ -12,7 +12,7 @@ class Author include ActiveModel::Conversion attr_reader :id - def key() id ? [id] : nil end + def to_key() id ? [id] : nil end def save; @id = 1 end def new_record?; @id.nil? end def name @@ -25,7 +25,7 @@ class Article include ActiveModel::Conversion attr_reader :id attr_reader :author_id - def key() id ? [id] : nil end + def to_key() id ? [id] : nil end def save; @id = 1; @author_id = 1 end def new_record?; @id.nil? end def name diff --git a/activemodel/lib/active_model/lint.rb b/activemodel/lib/active_model/lint.rb index c97286e61c..98413ac9fb 100644 --- a/activemodel/lib/active_model/lint.rb +++ b/activemodel/lib/active_model/lint.rb @@ -14,22 +14,28 @@ module ActiveModel module Lint module Tests - # == Responds to key + # == Responds to to_key # # Returns an Enumerable of all (primary) key attributes # or nil if model.new_record? is true - def test_key - assert model.respond_to?(:key), "The model should respond to key" + def test_to_key + assert model.respond_to?(:to_key), "The model should respond to to_key" def model.new_record?() true end - assert model.key.nil? + assert model.to_key.nil? def model.new_record?() false end - assert model.key.respond_to?(:each) + assert model.to_key.respond_to?(:each) end # == Responds to to_param # # Returns a string representing the object's key suitable for use in URLs - # or nil if model.new_record? is true + # or nil if model.new_record? is true. + # + # Implementers can decide to either raise an exception or provide a default + # in case the record uses a composite primary key. There are no tests for this + # behavior in lint because it doesn't make sense to force any of the possible + # implementation strategies on the implementer. However, if the resource is + # a new_record?, then to_param should always return nil. def test_to_param assert model.respond_to?(:to_param), "The model should respond to to_param" def model.new_record?() true end diff --git a/activemodel/test/cases/lint_test.rb b/activemodel/test/cases/lint_test.rb index 58716cbf85..99f8eff5d8 100644 --- a/activemodel/test/cases/lint_test.rb +++ b/activemodel/test/cases/lint_test.rb @@ -10,12 +10,14 @@ class LintTest < ActiveModel::TestCase self end - def key - new_record? ? nil : [1] + def to_key + new_record? ? nil : [id] end def to_param - key ? key.first.to_s : nil + return nil if to_key.nil? + # some default for CPKs, real implementations will differ + to_key.length > 1 ? to_key.join('-') : to_key.first.to_s end def valid?() true end diff --git a/activerecord/lib/active_record/attribute_methods/primary_key.rb b/activerecord/lib/active_record/attribute_methods/primary_key.rb index ba64b8ee2f..8549414a12 100644 --- a/activerecord/lib/active_record/attribute_methods/primary_key.rb +++ b/activerecord/lib/active_record/attribute_methods/primary_key.rb @@ -45,10 +45,12 @@ module ActiveRecord # Returns this record's primary key value wrapped in an Array # or nil if the record is a new_record? # This is done to comply with the AMo interface that expects - # every AMo compliant object to respond_to?(:key) and return + # every AMo compliant object to respond_to?(:to_key) and return # an Enumerable object from that call, or nil if new_record? - def key - new_record? ? nil : [ self.id ] + # This method also takes custom primary keys specified via + # the +set_primary_key+ into account. + def to_key + new_record? ? nil : [ self.send(self.class.primary_key) ] end end diff --git a/activerecord/test/cases/pk_test.rb b/activerecord/test/cases/pk_test.rb index 2502c136cc..5bba1d5625 100644 --- a/activerecord/test/cases/pk_test.rb +++ b/activerecord/test/cases/pk_test.rb @@ -9,13 +9,18 @@ require 'models/mixed_case_monkey' class PrimaryKeysTest < ActiveRecord::TestCase fixtures :topics, :subscribers, :movies, :mixed_case_monkeys - def test_key - # test new record + def test_to_key_with_default_primary_key topic = Topic.new - assert topic.key.nil? - # test existing record + assert topic.to_key.nil? topic = Topic.find(1) - assert_equal topic.key, [1] + assert_equal topic.to_key, [1] + end + + def test_to_key_with_customized_primary_key + keyboard = Keyboard.new + assert keyboard.to_key.nil? + keyboard.save + assert_equal keyboard.to_key, [keyboard.id] end def test_integer_key -- cgit v1.2.3