NameError (uninitialized constant Mobility::ActiveRecord::VERSION)

[vollnhals]

I did a plain install into my existing rails 5.2 (activerecord) project

gem "mobility", "~> 0.7.5"

and added Mobility to my Event class.

class Event < ApplicationRecord
  extend Mobility

  # relevant for discussion
  multi_tenant :tenant
end

Expected Behavior

My app still works.

Actual Behavior

Event.new does not work anymore.

NameError (uninitialized constant Mobility::ActiveRecord::VERSION)

This happens I think because extend Mobility does include Mobility::ActiveRecord.

When the call to multi_tenant from the activerecord-multi-tenant gem is called, it wants to look up ActiveRecord but it will find Mobility::ActiveRecord now.

So by including mobility into my project, the project broke.

Possible Fix

The same issue was discussed in #69 . But as far a I can see the issue was closed prematurely because the cause is not extend Mobility , it is the include Mobility::ActiveRecord.

Also see citusdata/activerecord-multi-tenant#42 for a work around .

[shioyama]

Thanks, very good bug report! You're right that this was previously encountered in #69. I wonder if the only way to fix it is to rename Mobility::ActiveRecord to something else. I believe this is why e.g. Roda uses Roda::RodaRequest rather than Roda::Request.

[shioyama]

I suppose the solution would be for all class names which potentially conflict with base classes (Mobility::ActiveRecord, Mobility::ActiveModel, Mobility::Arel and Mobility::Sequel), we could append the string "Mobility" to the class, so we'd have:

• Mobility::MobilityActiveRecord
• Mobility::MobilityActiveModel
• Mobility::MobilityArel
• Mobility::MobilitySequel

I've avoided doing this because it feels like it shouldn't be necessary, and I do think that it's the gem owner's responsibility to always call the base class explicitly (Mobility does this everywhere). But OTOH this could cause very strange bugs which might be harder to track down than the one in this issue.

If anyone has any thoughts or ideas, I'd be eager to hear before making this change.

cc @pwim

[pwim]

From the perspective of a user of the library, whether it is named Mobility::ActiveModel or Mobility::MobilityActiveModel is an internal implementation detail, and so something I don't really have an opinion on, beyond any issues the naming might create.

From the perspective of a developer of the library, I understand the aesthetic appeal of Mobility::ActiveModel is much nicer than Mobility::MobilityActiveModel.

You're right that other gems should use ::ActiveRecord::VERSION. However, part of the reason why they don't is that it doesn't seem to be very common for other gems to create their own ActiveRecord module which is then included in models, so they rarely encounter this issue.

If we wanted to avoid this issue by renaming things, I suppose not everything would need to get renamed. I think this is only an issue for modules we add to objects created in the user application. I haven't looked into it in detail, but it might be possible to rename Mobility::ActiveRecord to Mobility::ActiveRecordAdapter, while still keeping classes like Mobility::ActiveRecord:: UniquenessValidator etc.

[shioyama]

I like the Mobility::ActiveRecordAdapter pattern. This is a simple fix, I'll think a bit more and we can release a fix in a minor version.

[maebeale]

@shioyama I'm having this problem as well. is there a recommended temporary fix I could do on my end?

[shioyama]

@maebeale Sorry I haven't looked into this for a while, but one thing is that activerecord-multi-tenant could fix this at no real cost by just referencing ::ActiveRecord instead of ActiveRecord. I try to do that in Mobility (although probably not consistently).

[shioyama]

This was fixed by #464. Everything is now namespaced under Mobility::Plugins or Mobility::Backends, there is no Mobility::ActiveRecord anymore.

This applies to master and was released in 1.0.0.rc1. (I'll be releasing 1.0.0 in the next few days.)