Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

self.reload prone to error #122

Closed
alvinsj opened this issue May 29, 2014 · 10 comments
Closed

self.reload prone to error #122

alvinsj opened this issue May 29, 2014 · 10 comments

Comments

@alvinsj
Copy link

alvinsj commented May 29, 2014

this issue is referring to this line of code.

I noticed active_record model is more or less destroy-safe. Meaning the children/parents of a model(e.g. dependent: destroy) can destroy the referring model, yet if model.destroy got called again before commit, it won't fail.

however, this self.reload like below has failed the program and raising RecordNotFound error.

before_destroy :reload_position
def reload_position
  self.reload
end

please let me know if i missed some code, or if anyone has a workaround for this.

@jpbullalayao
Copy link

I have this same issue, any info on how to work around this would be much appreciated!

@fabn
Copy link
Contributor

fabn commented Oct 10, 2014

@alvinsj @jpbullalayao I'm looking into this while investigating #131. Since my issue will likely need reload as well could you provide a sample test case which trigger the issue?

I'm trying to fix my issue and I'd like to not introduce other issues with reload.

@tfausak
Copy link

tfausak commented Jan 8, 2016

I have also been bit by this. I think the problem is that acts_as_list tries to reload a record even if it's not persisted. #46 introduced this behavior. Here is an example:

require 'active_record' # 4.2.5
require 'acts_as_list' # 0.7.2

ActiveRecord::Base.establish_connection adapter: 'sqlite3', database: ':memory:'

ActiveRecord::Schema.define do
  create_table :lists
  create_table :elements do |table|
    table.integer :list_id
    table.integer :position
  end
end

class List < ActiveRecord::Base
  has_many :elements
end

class Element < ActiveRecord::Base
  belongs_to :list
  acts_as_list scope: :list
end

list = List.new
x = list.elements.build
list.elements.each &:destroy
# ActiveRecord::RecordNotFound: Couldn't find Element without an ID

I can fix this issue by overriding reload_position on Element.

def reload_position
  reload unless new_record?
end

tfausak added a commit to tfausak/acts_as_list that referenced this issue Jan 8, 2016
@swelther
Copy link
Contributor

Seems I'm hit by this bug too. Any news about the fix?

@brendon
Copy link
Owner

brendon commented Apr 14, 2016

I'll look into this. What's the scenario in which one is destroying records not yet persisted in the database?

@swelther
Copy link
Contributor

Not exactly. I try to destroy a record that has a has_many relationship with an acts_as_list model. And during the destroy I get the record not found error 😢

It fails here:

acts_as_list-0.7.3/lib/acts_as_list/active_record/acts/list.rb:481

@swelther
Copy link
Contributor

Oh wait, maybe it is our fault. We overwrite destroy in the related model and have an dependent: :destroy on the relation of the other model. It is a mess. I check this first :)

@brendon
Copy link
Owner

brendon commented Apr 14, 2016

Haha, sounds like fun. Definitely sounds like an out-of-order destroy problem. I've had something similar with ancestry where I had to basically override the deletion routine to make sure it began deleting the leaves of the tree first and then upward rather than all at once in a non-deterministic way.

@swelther
Copy link
Contributor

Yeah this was fun. Observer, circle dependent destroys and before_validation callbacks. But now it works and it was not acts_as_list fault :)

I'm glad I never used ancestry. I probably would go with a delete flag instead of actually destroying stuff to make my life easier :)

@brendon
Copy link
Owner

brendon commented Apr 15, 2016

That's good to hear @swelther :) Glad you got your problem sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants