delete_all should respect a limit scope #4979

Closed
daveyeu opened this Issue Feb 9, 2012 · 8 comments

Comments

Projects
None yet
7 participants
Contributor

daveyeu commented Feb 9, 2012

Hi there,

So we nearly just hosed a table in production by trying:

Model.limit(100).delete_all

Thankfully, we decided to read up on how delete_all works here and tested it locally, since it turns out that this simply deletes everything in the table.

Now I get that a LIMIT clause doesn't make sense on a DELETE op, but can we add a sensible translation in this case? I would expect it to behave like .destroy_all, which appears to instantiate the records and call #destroy on each. A #delete_all wouldn't have to inflate AR objects here, but instead, could simply query for the ids to delete, and then perform a DELETE FROM ... id IN (...) query.

Thoughts? I think we can prevent some future pain here.

Thanks,

Dave & John

Member

arunagw commented Feb 9, 2012

Rails Version?

jpignata commented Feb 9, 2012

We see the behavior using both Rails 3.1.3 and 3.0.11.

Member

arunagw commented Feb 9, 2012

have you tried this?

Mode.delete_all(:limit => 100)

Member

arunagw commented Feb 9, 2012

Sorry it will not work. It expects as a column.

Najaf commented Feb 10, 2012

+1

I was going to have a crack at implementing this and naively thought that "DELETE FROM foos LIMIT " was valid SQL. However it appears to only work in MySQL, or as compile-time option for sqlite. No mention of it after a cursory glance at pg docs.

@daveyeu , your idea of getting the ids and then doing a "DELETE FROM foos WHERE id in (...)" I think would do what you'd expect Foo.limit(10).delete_all. It would be interesting to hear what rails contributors think.

Contributor

jasonnoble commented Feb 25, 2012

Perhaps delete_all should raise an error if a limit is provided?

/cc @tenderlove

Owner

tenderlove commented Feb 27, 2012

@jasonnoble that seems acceptable to me.

Contributor

frodsan commented Apr 30, 2012

@tenderlove Any news? Maybe something like this:

def delete_all(conditions = nil)
  raise ActiveRecordError.new("delete_all doesn't support limit scope") if self.limit_value

  if conditions
    where(conditions).delete_all
  else
  ...

@daveyeu @arunagw Is destroy_all method serve that function?

@tenderlove tenderlove closed this in 2f68125 May 1, 2012

bploetz pushed a commit to bploetz/rails that referenced this issue May 1, 2012

Merge pull request #6089 from frodsan/delete_all_limit
delete_all raise an error if a limit is provided - fixes #4979
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment