new_record? returning true after a successful reload #12101

Closed
LoveIsGrief opened this Issue Sep 1, 2013 · 13 comments

Projects

None yet

8 participants

@LoveIsGrief

Hello,

The documentation of new_record? states

Returns true if this object hasn’t been saved yet – that is, a record for the object doesn’t exist in the data store yet; otherwise, returns false.

However when reload is called successfully it should mean that there is an object in the datastore, yet new_record? still returns true.

Simple case to recreate situation

Person = Person.new
# some processing to find out that we already have the item in the datastore
# e.g nested_attribute contains id
person.id = Item.first.id #could also use person.id = known_id
person.reload
person.new_record? # returns true, even if the reload worked

Code that's failing on me

class Person < ActiveRecord::Base
 has_many :items, -> { uniq }, dependent: :destroy
 accepts_nested_attributes_for :items
 attr_accessible :name, :items, :items_attributes

 protected
  def items_attributes=(attributes)
   #Code truncated
   # After checks for existence of items in db by either item.code or item.id
   self.id = id_from_first_item_attribute
   self.reload
   # Assign new attributes
  end
end

class Item < ActiveRecord::Base
 belongs_to :people
 attr_accessible :code
 validates :code, uniqueness: true
end

params = { items_attributes: [ { code: "A97G1A"} ]}
p = Person.new params
p.new_record? # true
#p.save will create a new record

Is this desired behavior? If so could you please update the documentation? If not, then could you please make @new_record somehow dependent on reload or really do what it says in the documentation?

Kind regards,
LoveIsGrief

@rafaelfranca
Member

I think in this case reload that should raise an exception since you object was not persisted yet.

@pftg
Contributor
pftg commented Sep 1, 2013

@rafaelfranca, @LoveIsGrief has setup id manually, so this is expected behavior, because after reload you have already persisted object which loaded from db by id which you have setup.

@LoveIsGrief

In the meanwhile I overrode the method in the Person class (as seen below) and it is working so far.

class Person < ActiveRecord::Base
  # Slight workaround that might add more processing time
  # It's an alias of <tt>exists?</tt> that returns a boolean value
  def new_record?
    !(Person.exists?(self.id) == 1)
  end
end

Any thoughts? Would this add considerable processing time? Could "new_record?" at it's base be written like this?

@pftg
Contributor
pftg commented Sep 3, 2013

As for me, new_record? should not run any sql queries - to be simple and more faster as it is now. You always can invoke exists? for specific cases like you have used it in your example (also you can add memoization).

@LoveIsGrief

The problem is that somewhere during save process new_record? is being called. And as mentioned in my first post, that leads to a new record being created in the db, even if reload was called before.

@rajcybage
Contributor

I have made a changes regarding the checking of persistence while performing reload and throw error if it is not persisted Please look into #12118

@prathamesh-sonpatki
Member

@pftg is right. As per the current implementation of reload, it doesn't check for persistence. And as a pre-existing id is assigned to it manually, it will reload it from db.
I think documentation of new_record? is correct. reload documentation should be changed to include this case also.

@makaroni4
Contributor

@LoveIsGrief @prathamesh-sonpatki documentation is updated.

@LoveIsGrief

@makaroni4 thanks for the update.

As for the rest, I guess a decision has to be made regarding the commit of @rajcybage . Either:

  1. we disallow reload to be called on non-persisted record, which is one side to the coin and would make sense.
    That would make it necessary to revert @makaroni4's commit, though.

  2. we allow reload to work as now and continue debating if new_record? is doing what it should after a reload. After reading the doc

    Returns true if this object hasn't been saved yet – that is, a record for the object doesn't exist in the data store yet; otherwise, returns false.

I disagree with @prathamesh-sonpatki. It is my opinion, that new_record is not doing what it's documented to do after a reload, because it does exist in the data store after a reload. After all, the record was loaded from the data store...

@mgrantley

I came across this issue, because I had some confusion about new_record as well. I think I understand now, and (sorry to butt in) I disagree with @LoveIsGrief. On a reload, no record is loaded from the data store...it's loaded from the database, essentially a reset. I think if anything, "the object doesn't exist in the data store yet" could be changed to "the object doesn't exist in the database"

@LoveIsGrief

Thanks for contributing to the discussion @mgrantley. I do have a general question now. What is the datastore in rails?
That would be the source of my confusion as I thought datastore was synonymous or included the databse.

@mgrantley

No problem @LoveIsGrief. I think the use of the word 'datastore' is kind of confusing because a datastore CAN be a database...but it's also just a general concept of storing data. In this case I think what they meant was the cache. In your example you have an instantiated object that you're reloading with data from the record (in your database) matching the id that you specified. That instantiated object is never saved to the database, so it is still technically a new record since when you save it a new record will be produced. A new record will be produced because the way you have it set up, you can't assign an id to a new object...the id will be assigned at create. I think what you want to do is use the update and exists? methods.

@robin850 robin850 added a commit that referenced this issue Feb 27, 2014
@robin850 robin850 Replace "data store" with database [ci skip]
Active Record is specifically for databases. Refs #12101.
00824b0
@robin850 robin850 added a commit that referenced this issue Feb 27, 2014
@robin850 robin850 Replace "data store" with database [ci skip]
Active Record is specifically for databases. Refs #12101.
82dde71
@robin850 robin850 added a commit that referenced this issue Feb 27, 2014
@robin850 robin850 Replace "data store" with database [ci skip]
Active Record is specifically for databases. Refs #12101.
d682b0d
@LoveIsGrief LoveIsGrief added the stale label May 27, 2014
@rails-bot

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rafaelfranca rafaelfranca removed the stale label Jul 4, 2014
@matthewd matthewd closed this in #16049 Jul 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment