Observers on numerically sorted hierarchical models trigger load error

[elskwid]

Hello @mceachen,

Just uncovered an interesting issue today and wanted to discuss how it should be handled. I was seeing errors from active record where my hierarchical model table didn't exist during a rake run.

Turns out that there was an observer on that same model (the one that has acts_as_tree order: "name" defined). That observer loads the model and the order option then attempts to look up column information on the table.

This isn't a problem unless you're in an environment like Travis CI (or any environment that starts without a database defined).

No database to start

• Execute rake db:setup
• Rake starts
• Environment loads
• Observers are loaded
• Observers load the observed models
• acts_as_tree order: "field_name" needs to include ClosureTree::DeterministicNumericOrdering if order_is_numeric
• order_is_numeric calls ct_class.columns_hash[order_option]
• columns_hash access the ActiveRecord metadata for the table
• The table doesn't exist so the rake task fails

Observers are scheduled for removal in Rails 4 but I've seen you mention the desire to maintain support for older Rails versions for this project.

I am happy to help fix this issue, just not sure how or if you'd like it fixed. My initial reaction was to change order_is_numeric as follows (although I hate && in unless):

def order_is_numeric
  return false unless order_option && self.table_exists?
  c = ct_class.columns_hash[order_option]
  c && c.type == :integer
end

If you'd like a pull request I can put one together and get this moving.

Thanks for a great project!

[mceachen]

I had to do something similarly kludgy with chromotype: I introduced the idea of a migration_safe_on_load that would ignore issues arising from tables not existing yet.

Do you think it's OK to "lie" and say that order isn't numeric just because the table doesn't exist? Would returning nil be better? (Nothing will work otherwise, so we're limping in any event).

def order_is_numeric
  return nil unless self.table_exists?
  return false unless order_option
  c = ct_class.columns_hash[order_option]
  c && c.type == :integer
end

I think I like yours better, but I agree with your hate with && and unless.

Happy to take your pull request! I'm finishing issue 31 right now, but if I can bang it out, I'll update this issue.

[elskwid]

Technically it isn't numeric. I mean, it isn't there! I don't see a difference between nil and false so it's up to you on that one. I would say, if the method should return 'boolean' then have it do so.

I'll put something together and send it your way. I like the explicit returns. Easier to follow.

For what it's worth, I spent some time digging in Rails in the hopes of coming up with a workaround that we could just document and not have to do something janky. You can't disable them from instantiating although you can stop them from notifying. I even experimented with putting checks in application.rb. It worked but made me want to take a shower.

# active record observers
config.active_record.observers = []
if ActiveRecord::Base.connected? && ActiveRecord::Base.connection.table_exists?(:tags)
  config.active_record.observers << :tag_observer
end

[mceachen]

To prevent unnecessary showering, why not use a "mixin" or "concern" module that you subsequently include in your Tag model? You don't have to do janky observers array mucking about then.

I'll add the twiddle in your initial comment now, in any event, though.

Thanks!

[elskwid]

Ah, you're too fast. You coulda had a pull request. We could have had something together!

In the app where this came up I was able to just inline the observer callbacks right into the models, which worked for our application.

If time doesn't work out for you let me know if I can get that pull request done.

[mceachen]

Oh, we have something, sir, we have something.

You are now the proud father of v3.6.7. Thanks for your help.

[mceachen]

(BTW, I negated the unless, just in the hopes that it would make you happy)

[elskwid]

💞