Remove ordering by id during refresh rake task #1231

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

kakra commented May 3, 2013

Ordering by id breaks compatibility with tables having no column named "id" (e.g. if using a legacy database) without real benefit. While ordering makes multiple runs with errors deterministic, I suggest to rely on the database engine to return unordered results in a deterministic way (e.g. on-disk order). If ordering is a requirement, I suggest to put this feature back only without hardcoding the primary key name. Some people may even use composite primary keys which would break this in the same way.

@kakra kakra Remove ordering by id during refresh rake task
Ordering by id breaks compatibility with tables having no column named "id" (e.g. if using a legacy database) without real benefit. While ordering makes multiple runs with errors deterministic, I suggest to rely on the database engine to return unordered results in a deterministic way (e.g. on-disk order). If ordering is a requirement, I suggest to put this feature back only without hardcoding the primary key name. Some people may even use composite primary keys which would break this in the same way.
7f9398c
Contributor

djcp commented May 3, 2013

Thanks! Did you mean to issue this against the 2.7 branch? We're reluctant to accept PRs without tests - but we realize the rake tasks aren't tested. If you could extract this rake task into a class, write some unit tests and re-issue a PR against master we'd be interested. If this isn't something you can take on, I may take a look a refactoring the rake tasks myself.

kakra commented May 3, 2013

Yes, it's against the 2.7 branch because that is which I use (still ruby 1.8.7). Since I need to GTD (lack of time) I forked it and used my own fork as the gem in my application. That lack of time also means I won't be able to deliver a test case within the next 2-3 weeks. Actually, the change is trivial so I didn't expect to need a test case. So it's up to you: Feel free to refactor it yourself, otherwise I'd look into it when there's some more time free for me.

Member

jyurek commented May 10, 2013

Determinism is important, but I think a more correct solution would be to order by primary_key.

kakra commented May 10, 2013

Then that line should probably be

class_for(klass).find(:all, :order => class_for(klass).primary_key).each do |instance|

That may not work for pre-3.0 ActiveRecord classes - not sure about this. And it won't work for models using composite primary keys. One needs to do some array tranposing foobar magic to get that right then.

Member

jyurek commented May 10, 2013

That's true, though I've never heard of people using composite keys. Though I've also not heard of people having to deal with tables that don't have an id lately, either, so I guess maybe my experience isn't the best thing to go on, here.

kakra commented May 10, 2013

Well I'm migrating an old legacy application and currently old and new components need to run side by side... We will change the tables as soon as Rails can take it over with the old component removed. I had to jump through some loops already to make paperclip accept the very strange pathnames and inconsistent naming of the files.

Member

jyurek commented Jul 16, 2013

This is not a problem in master. The method now looks like this:

def each_instance_with_attachment(klass, name)
  class_for(klass).unscoped.where("#{name}_file_name IS NOT NULL").find_each do |instance|
    yield(instance)
  end
end

jyurek closed this Jul 16, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment