Permalink
Browse files

use persisted? instead of new_record? wherever possible

- persisted? is the API defined in ActiveModel
- makes it easier for extension libraries to conform to ActiveModel
  APIs
  without concern for whether the extended object is specifically
  ActiveRecord

[#5927 state:committed]

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information...
1 parent 4b33bd9 commit f1c13b0dd7b22b5f6289ca1a09f1d7a8c7c8584b @dchelimsky dchelimsky committed with spastorino Nov 9, 2010
Showing with 202 additions and 195 deletions.
  1. +1 −1 activerecord/lib/active_record/aggregations.rb
  2. +1 −1 activerecord/lib/active_record/associations.rb
  3. +11 −10 activerecord/lib/active_record/associations/association_collection.rb
  4. +3 −3 activerecord/lib/active_record/associations/association_proxy.rb
  5. +3 −3 activerecord/lib/active_record/associations/belongs_to_association.rb
  6. +1 −1 activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
  7. +1 −1 activerecord/lib/active_record/associations/has_many_through_association.rb
  8. +6 −6 activerecord/lib/active_record/associations/has_one_association.rb
  9. +1 −1 activerecord/lib/active_record/associations/has_one_through_association.rb
  10. +4 −3 activerecord/lib/active_record/attribute_methods/primary_key.rb
  11. +7 −7 activerecord/lib/active_record/autosave_association.rb
  12. +10 −8 activerecord/lib/active_record/base.rb
  13. +1 −1 activerecord/lib/active_record/locking/optimistic.rb
  14. +1 −1 activerecord/lib/active_record/locking/pessimistic.rb
  15. +7 −5 activerecord/lib/active_record/persistence.rb
  16. +4 −4 activerecord/lib/active_record/session_store.rb
  17. +7 −6 activerecord/lib/active_record/transactions.rb
  18. +1 −1 activerecord/lib/active_record/validations.rb
  19. +1 −1 activerecord/lib/active_record/validations/uniqueness.rb
  20. +6 −6 activerecord/test/cases/associations/belongs_to_associations_test.rb
  21. +13 −13 activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
  22. +6 −6 activerecord/test/cases/associations/has_many_associations_test.rb
  23. +2 −2 activerecord/test/cases/associations/has_one_associations_test.rb
  24. +8 −8 activerecord/test/cases/associations/join_model_test.rb
  25. +43 −43 activerecord/test/cases/autosave_association_test.rb
  26. +6 −6 activerecord/test/cases/base_test.rb
  27. +21 −21 activerecord/test/cases/finder_test.rb
  28. +8 −8 activerecord/test/cases/nested_attributes_test.rb
  29. +5 −5 activerecord/test/cases/relations_test.rb
  30. +2 −2 activerecord/test/cases/session_store/sql_bypass.rb
  31. +8 −8 activerecord/test/cases/transactions_test.rb
  32. +1 −1 activerecord/test/models/pirate.rb
  33. +1 −1 activerecord/test/models/subject.rb
  34. +1 −1 activerecord/test/models/topic.rb
@@ -6,7 +6,7 @@ module Aggregations # :nodoc:
def clear_aggregation_cache #:nodoc:
self.class.reflect_on_all_aggregations.to_a.each do |assoc|
instance_variable_set "@#{assoc.name}", nil
- end unless self.new_record?
+ end if self.persisted?
end
# Active Record implements aggregation through a macro-like class method called +composed_of+
@@ -118,7 +118,7 @@ module Associations # :nodoc:
def clear_association_cache #:nodoc:
self.class.reflect_on_all_associations.to_a.each do |assoc|
instance_variable_set "@#{assoc.name}", nil
- end unless self.new_record?
+ end if self.persisted?
end
private
@@ -127,13 +127,13 @@ def build(attributes = {}, &block)
# Since << flattens its argument list and inserts each record, +push+ and +concat+ behave identically.
def <<(*records)
result = true
- load_target if @owner.new_record?
+ load_target unless @owner.persisted?
transaction do
flatten_deeper(records).each do |record|
raise_on_type_mismatch(record)
add_record_to_target_with_callbacks(record) do |r|
- result &&= insert_record(record) unless @owner.new_record?
+ result &&= insert_record(record) if @owner.persisted?
end
end
end
@@ -291,12 +291,12 @@ def create!(attrs = {})
# This method is abstract in the sense that it relies on
# +count_records+, which is a method descendants have to provide.
def size
- if @owner.new_record? || (loaded? && !@reflection.options[:uniq])
+ if !@owner.persisted? || (loaded? && !@reflection.options[:uniq])
@target.size
elsif !loaded? && @reflection.options[:group]
load_target.size
elsif !loaded? && !@reflection.options[:uniq] && @target.is_a?(Array)
- unsaved_records = @target.select { |r| r.new_record? }
+ unsaved_records = @target.reject { |r| r.persisted? }
unsaved_records.size + count_records
else
count_records
@@ -363,7 +363,7 @@ def replace(other_array)
def include?(record)
return false unless record.is_a?(@reflection.klass)
- return include_in_memory?(record) if record.new_record?
+ return include_in_memory?(record) unless record.persisted?
load_target if @reflection.options[:finder_sql] && !loaded?
return @target.include?(record) if loaded?
exists?(record)
@@ -390,7 +390,7 @@ def construct_counter_sql
end
def load_target
- if !@owner.new_record? || foreign_key_present
+ if @owner.persisted? || foreign_key_present
@lardawge
lardawge Nov 28, 2010 Contributor

This line broke the functionality mentioned in my comments below.

The problem with this commit in general is that #persisted? does not mean the same thing as #new_record? In this case the association never gets loaded in an after_destroy callback because #persisted? is also checking that @owner has not been destroyed... in this case it has... fail! Question is how many other cases through all these changes does checking for a new record mean just that and not if it is destroyed? I am happy to submit a patch for this case but this really should be reverted until it can be fully investigated.

@bearded
bearded Nov 28, 2010

I think you meant line: f1c13b0#L2R549

@lardawge
lardawge Nov 28, 2010 Contributor

For my particular case I am referring to this line...

@bearded
bearded Nov 28, 2010

Oh! Excuse me please... :(

begin
if !loaded?
if @target.is_a?(Array) && @target.any?
@@ -521,7 +521,7 @@ def remove_records(*records)
transaction do
records.each { |record| callback(:before_remove, record) }
- old_records = records.reject { |r| r.new_record? }
+ old_records = records.select { |r| r.persisted? }
yield(records, old_records)
records.each { |record| callback(:after_remove, record) }
end
@@ -546,14 +546,15 @@ def callbacks_for(callback_name)
end
def ensure_owner_is_not_new
- if @owner.new_record?
+ unless @owner.persisted?
raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved"
end
end
def fetch_first_or_last_using_find?(args)
- args.first.kind_of?(Hash) || !(loaded? || @owner.new_record? || @reflection.options[:finder_sql] ||
- @target.any? { |record| record.new_record? } || args.first.kind_of?(Integer))
+ args.first.kind_of?(Hash) || !(loaded? || !@owner.persisted? || @reflection.options[:finder_sql] ||
+ @target.any? { |record| !record.persisted? } || args.first.kind_of?(Integer))
+ # TODO - would prefer @target.none? { |r| r.persisted? }
end
def include_in_memory?(record)
@@ -174,10 +174,10 @@ def sanitize_sql(sql, table_name = @reflection.klass.table_name)
# If the association is polymorphic the type of the owner is also set.
def set_belongs_to_association_for(record)
if @reflection.options[:as]
- record["#{@reflection.options[:as]}_id"] = @owner.id unless @owner.new_record?
+ record["#{@reflection.options[:as]}_id"] = @owner.id if @owner.persisted?
record["#{@reflection.options[:as]}_type"] = @owner.class.base_class.name.to_s
else
- unless @owner.new_record?
+ if @owner.persisted?
primary_key = @reflection.options[:primary_key] || :id
record[@reflection.primary_key_name] = @owner.send(primary_key)
end
@@ -233,7 +233,7 @@ def method_missing(method, *args)
def load_target
return nil unless defined?(@loaded)
- if !loaded? and (!@owner.new_record? || foreign_key_present)
+ if !loaded? and (@owner.persisted? || foreign_key_present)
@target = find_target
end
@@ -14,21 +14,21 @@ def replace(record)
counter_cache_name = @reflection.counter_cache_column
if record.nil?
- if counter_cache_name && !@owner.new_record?
+ if counter_cache_name && @owner.persisted?
@reflection.klass.decrement_counter(counter_cache_name, previous_record_id) if @owner[@reflection.primary_key_name]
end
@target = @owner[@reflection.primary_key_name] = nil
else
raise_on_type_mismatch(record)
- if counter_cache_name && !@owner.new_record? && record.id != @owner[@reflection.primary_key_name]
+ if counter_cache_name && @owner.persisted? && record.id != @owner[@reflection.primary_key_name]
@reflection.klass.increment_counter(counter_cache_name, record.id)
@reflection.klass.decrement_counter(counter_cache_name, @owner[@reflection.primary_key_name]) if @owner[@reflection.primary_key_name]
end
@target = (AssociationProxy === record ? record.target : record)
- @owner[@reflection.primary_key_name] = record_id(record) unless record.new_record?
+ @owner[@reflection.primary_key_name] = record_id(record) if record.persisted?
@updated = true
end
@@ -34,7 +34,7 @@ def count_records
end
def insert_record(record, force = true, validate = true)
- if record.new_record?
+ unless record.persisted?
if force
record.save!
else
@@ -59,7 +59,7 @@ def construct_find_options!(options)
end
def insert_record(record, force = true, validate = true)
- if record.new_record?
+ unless record.persisted?
if force
record.save!
else
@@ -35,18 +35,18 @@ def replace(obj, dont_save = false)
if dependent? && !dont_save
case @reflection.options[:dependent]
when :delete
- @target.delete unless @target.new_record?
+ @target.delete if @target.persisted?
@owner.clear_association_cache
when :destroy
- @target.destroy unless @target.new_record?
+ @target.destroy if @target.persisted?
@owner.clear_association_cache
when :nullify
@target[@reflection.primary_key_name] = nil
- @target.save unless @owner.new_record? || @target.new_record?
+ @target.save if @owner.persisted? && @target.persisted?
end
else
@target[@reflection.primary_key_name] = nil
- @target.save unless @owner.new_record? || @target.new_record?
+ @target.save if @owner.persisted? && @target.persisted?
end
end
@@ -61,7 +61,7 @@ def replace(obj, dont_save = false)
set_inverse_instance(obj, @owner)
@loaded = true
- unless @owner.new_record? or obj.nil? or dont_save
+ unless !@owner.persisted? or obj.nil? or dont_save
return (obj.save ? self : false)
else
return (obj.nil? ? nil : self)
@@ -120,7 +120,7 @@ def new_record(replace_existing)
if replace_existing
replace(record, true)
else
- record[@reflection.primary_key_name] = @owner.id unless @owner.new_record?
+ record[@reflection.primary_key_name] = @owner.id if @owner.persisted?
self.target = record
set_inverse_instance(record, @owner)
end
@@ -21,7 +21,7 @@ def create_through_record(new_value) #nodoc:
if current_object
new_value ? current_object.update_attributes(construct_join_attributes(new_value)) : current_object.destroy
elsif new_value
- if @owner.new_record?
+ unless @owner.persisted?
self.target = new_value
through_association = @owner.send(:association_instance_get, @reflection.through_reflection.name)
through_association.build(construct_join_attributes(new_value))
@@ -3,10 +3,11 @@ module AttributeMethods
module PrimaryKey
extend ActiveSupport::Concern
- # Returns this record's primary key value wrapped in an Array
- # or nil if the record is a new_record?
+ # Returns this record's primary key value wrapped in an Array or nil if
+ # the record is not persisted? or has just been destroyed.
def to_key
- new_record? ? nil : [ id ]
+ key = send(self.class.primary_key)
+ [key] if key
end
module ClassMethods
@@ -208,7 +208,7 @@ def marked_for_destruction?
# Returns whether or not this record has been changed in any way (including whether
# any of its nested autosave associations are likewise changed)
def changed_for_autosave?
- new_record? || changed? || marked_for_destruction? || nested_records_changed_for_autosave?
+ !persisted? || changed? || marked_for_destruction? || nested_records_changed_for_autosave?
end
private
@@ -222,7 +222,7 @@ def associated_records_to_validate_or_save(association, new_record, autosave)
elsif autosave
association.target.find_all { |record| record.changed_for_autosave? }
else
- association.target.find_all { |record| record.new_record? }
+ association.target.find_all { |record| !record.persisted? }
end
end
@@ -248,7 +248,7 @@ def validate_single_association(reflection)
# +reflection+.
def validate_collection_association(reflection)
if association = association_instance_get(reflection.name)
- if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave])
+ if records = associated_records_to_validate_or_save(association, !persisted?, reflection.options[:autosave])
records.each { |record| association_valid?(reflection, record) }
end
end
@@ -277,7 +277,7 @@ def association_valid?(reflection, association)
# Is used as a before_save callback to check while saving a collection
# association whether or not the parent was a new record before saving.
def before_save_collection_association
- @new_record_before_save = new_record?
+ @new_record_before_save = !persisted?
true
end
@@ -299,7 +299,7 @@ def save_collection_association(reflection)
if autosave && record.marked_for_destruction?
association.destroy(record)
- elsif autosave != false && (@new_record_before_save || record.new_record?)
+ elsif autosave != false && (@new_record_before_save || !record.persisted?)
if autosave
saved = association.send(:insert_record, record, false, false)
else
@@ -334,7 +334,7 @@ def save_has_one_association(reflection)
association.destroy
else
key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id
- if autosave != false && (new_record? || association.new_record? || association[reflection.primary_key_name] != key || autosave)
+ if autosave != false && (!persisted? || !association.persisted? || association[reflection.primary_key_name] != key || autosave)
association[reflection.primary_key_name] = key
saved = association.save(:validate => !autosave)
raise ActiveRecord::Rollback if !saved && autosave
@@ -354,7 +354,7 @@ def save_belongs_to_association(reflection)
if autosave && association.marked_for_destruction?
association.destroy
elsif autosave != false
- saved = association.save(:validate => !autosave) if association.new_record? || autosave
+ saved = association.save(:validate => !autosave) if !association.persisted? || autosave
if association.updated?
association_id = association.send(reflection.options[:primary_key] || :id)
@@ -205,7 +205,7 @@ module ActiveRecord #:nodoc:
#
# # No 'Winter' tag exists
# winter = Tag.find_or_initialize_by_name("Winter")
- # winter.new_record? # true
+ # winter.persisted? # false
#
# To find by a subset of the attributes to be used for instantiating a new object, pass a hash instead of
# a list of parameters.
@@ -1393,7 +1393,7 @@ def encode_quoted_value(value) #:nodoc:
def initialize(attributes = nil)
@attributes = attributes_from_column_definition
@attributes_cache = {}
- @new_record = true
+ @persited = false
@readonly = false
@destroyed = false
@marked_for_destruction = false
@@ -1428,7 +1428,7 @@ def initialize_copy(other)
clear_aggregation_cache
clear_association_cache
@attributes_cache = {}
- @new_record = true
+ @persisted = false
ensure_proper_type
populate_with_current_scope_attributes
@@ -1447,7 +1447,8 @@ def initialize_copy(other)
def init_with(coder)
@attributes = coder['attributes']
@attributes_cache, @previously_changed, @changed_attributes = {}, {}, {}
- @new_record = @readonly = @destroyed = @marked_for_destruction = false
+ @readonly = @destroyed = @marked_for_destruction = false
+ @persisted = true
_run_find_callbacks
_run_initialize_callbacks
end
@@ -1488,7 +1489,7 @@ def to_param
# Person.find(5).cache_key # => "people/5-20071224150000" (updated_at available)
def cache_key
case
- when new_record?
+ when !persisted?
"#{self.class.model_name.cache_key}/new"
when timestamp = self[:updated_at]
"#{self.class.model_name.cache_key}/#{id}-#{timestamp.to_s(:number)}"
@@ -1609,8 +1610,9 @@ def column_for_attribute(name)
# Returns true if the +comparison_object+ is the same object, or is of the same type and has the same id.
def ==(comparison_object)
comparison_object.equal?(self) ||
- (comparison_object.instance_of?(self.class) &&
- comparison_object.id == id && !comparison_object.new_record?)
+ persisted? &&
+ (comparison_object.instance_of?(self.class) &&
+ comparison_object.id == id)
end
# Delegates to ==
@@ -1655,7 +1657,7 @@ def readonly!
# Returns the contents of the record as a nicely formatted string.
def inspect
attributes_as_nice_string = self.class.column_names.collect { |name|
- if has_attribute?(name) || new_record?
+ if has_attribute?(name) || !persisted?
"#{name}: #{attribute_for_inspect(name)}"
end
}.compact.join(", ")
@@ -109,7 +109,7 @@ def update(attribute_names = @attributes.keys) #:nodoc:
def destroy #:nodoc:
return super unless locking_enabled?
- unless new_record?
+ if persisted?
lock_col = self.class.locking_column
previous_value = send(lock_col).to_i
@@ -47,7 +47,7 @@ module Pessimistic
# or pass true for "FOR UPDATE" (the default, an exclusive row lock). Returns
# the locked record.
def lock!(lock = true)
- reload(:lock => lock) unless new_record?
+ reload(:lock => lock) if persisted?
self
end
end
Oops, something went wrong.

11 comments on commit f1c13b0

@lardawge
Contributor

For whatever reason, this commit, breaks a lot of my specs...
expect { Model.find 123 }.to raise_error(ActiveRecord::RecordNotFound)
No longer passes...

@spastorino
Member

In which version?

➜  myapp  rails c
Loading development environment (Rails 3.1.0.beta)
>> User.find 45
ActiveRecord::RecordNotFound: Couldn't find User with id=45
@lardawge
Contributor

3.0.3
I kind of missed some details in my initial post... Here are more details:
https://gist.github.com/703988

I put this together only as an example of the setup... not as actual code from the app (can't post it as it is company property). Looks like it is related to after_destroy since I can verify the associations never get destroyed.

@tanelj
tanelj commented on f1c13b0 Nov 20, 2010

Hi!

I notice that there is problem since Rails 3.0.2 when using for example acts_as_audited gem. When trying to destroy an existing object of model that is audited it gets error:
"ActiveRecord::RecordNotSaved: You cannot call create unless the parent is saved"

This error appears after this change:

         def ensure_owner_is_not_new
-          if @owner.new_record?
+          unless @owner.persisted?
             raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved"
         end
@lardawge
Contributor

@tanelj - That's because #persisted? and #new_record? don't mean the same thing and are not interchangeable. Anything in an after_destroy callback is now not going to work... example of one such place where it fails:
f1c13b0#commitcomment-204029

This commit really needs to be rethought and either reverted or thoroughly gone through...
@spastorino Thoughts?

@spastorino
Member

@lardawge can you provide a patch or a failing test?.
Thanks.

@lardawge
Contributor

Looks like José committed a partial revert which fixes my problem... I would love to provide a regression test though...

@spastorino
Member

@lardawge yes we were talking with José and I'm going to do a backport for 3-0-stable and review a bit more that stuff.
But sure please go ahead and provide a regression test :).
Thanks again.

@spastorino
Member

Reverted from 3-0-stable 75015d1.

@dchelimsky
Contributor

Srsly? You reverted the whole thing? Why?

@jhawthorn
Contributor

Reverting this patch also resolved the issue I reported here
https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6037-has_one-through-a-belongs_to-association-will-return-nil-on-deleted-records

I've attached a patch of my test case for that issue. It would be great if that could get in.

Please sign in to comment.