Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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.
  • Loading branch information...
commit f81c6bc0404ba2a03eed0ec6c08bbac45661305f 1 parent fed72b5
Martin Gamsjaeger snusnu authored Yehuda Katz committed
4 actionpack/lib/action_controller/record_identifier.rb
View
@@ -77,8 +77,8 @@ def dom_id(record, prefix = nil)
# 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.
1  actionpack/test/controller/record_identifier_test.rb
View
@@ -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
14 actionpack/test/lib/controller/fake_models.rb
View
@@ -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 @@ class Question < Struct.new(:name, :id)
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
6 actionpack/test/template/prototype_helper_test.rb
View
@@ -4,7 +4,7 @@
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
18 activemodel/lib/active_model/lint.rb
View
@@ -14,22 +14,28 @@ module ActiveModel
module Lint
module Tests
- # == Responds to <tt>key</tt>
+ # == Responds to <tt>to_key</tt>
#
# 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 <tt>to_param</tt>
#
# 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
8 activemodel/test/cases/lint_test.rb
View
@@ -10,12 +10,14 @@ def to_model
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
8 activerecord/lib/active_record/attribute_methods/primary_key.rb
View
@@ -45,10 +45,12 @@ module InstanceMethods
# 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
15 activerecord/test/cases/pk_test.rb
View
@@ -9,13 +9,18 @@
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
Please sign in to comment.
Something went wrong with that request. Please try again.