Class not recognized as MTI class

[manuelmeurer]

Unfortunately my AdminUser class is not recognized as a MTI class. :(
I'm running Rails 4.2.10 and this is my setup:

ActiveRecord::Migration.create_table :users do |t|
  t.string :first_name, :last_name
  t.timestamps null: false
end

ActiveRecord::Migration.create_table :admin_users, inherits: :users do |t|
  t.text :roles, array: true, default: [], null: false
end

# db/structure.rb
CREATE TABLE users (
    id integer NOT NULL,
    first_name character varying,
    last_name character varying,
    created_at timestamp without time zone NOT NULL,
    updated_at timestamp without time zone NOT NULL,
);

CREATE TABLE admin_users (
    roles text[] DEFAULT '{}'::text[] NOT NULL
)
INHERITS (users);

# app/models/application_record.rb
class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true
end

# app/models/user.rb
class User < ApplicationRecord
end

class AdminUser < User
end

When I check AdminUser.mti_type_column and AdminUser.tableoid_column on the console, they are both nil, which they shouldn't be according to the specs.
What can I do to further help debugging this problem?

[voltechs]

Hey @manuelmeurer,

Thanks for reporting this!

What version of ActiveRecord::MTI are you using?
What version of PostgreSQL are you using?

Lastly, could you fork the project and write a failing test case? I'm happy to take a look if you could help out by doing that :)

(Hint: I'm guessing it has to do with the use of ApplicationRecord, which I haven't been using/testing against. 😶)

[voltechs]

So I took a stab at it (6edcb2b). I changed the User model in the specs to use ApplicationRecord, but specs did not fail. Maybe that's a starting point for you.

[manuelmeurer]

My first idea was also that it has something to do with ApplicationRecord but changing it to class User < ActiveRecord::Base does not fix the problem.
I'm using v0.3.0.pre.rc4 of this gem and Postgres v10.1. Maybe it's the Postgres version?

[voltechs]

@manuelmeurer Thanks for doing that! Hmm, PostgreSQL v10.x sounded like a good theory; it looks like the specs passed though. 🤔

Have you got other ActiveRecord::Base extensions (gems) in your project? Maybe enumerate them here and we can pick through them together to suss out a culprit. Maybe ActiveRecord::MTI isn't playing nice with someone (or vice versa). I took great pains to try and make the injection process as noninvasive as possible. 😅

[yuki24]

@voltechs I noticed that in the test app setup, the table name for the Admin model is manually set to admins. If you remove that line, 19 specs out of 34 will fail. I assumed table names don't have to be explicitly set as the README doesn't say anything about it. Does this need to be fixed or the doc is out of date?

[manuelmeurer]

Good point, @yuki24! Why is the table name set explicitly, @voltechs?

[voltechs]

Oh wow. Great find! That was an attempt to make sure setting a table_name explicitly still worked (that I didn't botch that functionality). Looks like I need do have another class to isolate that behavior. I'll get to that right now :)

[voltechs]

Alright @manuelmeurer @yuki24,

I've refactored how MTI handles table_name inference. I also took some time to clean up the specs around a lot of this. Builds are passing (naturally). Additionally, I added an ActiveRecord::MTI.configure feature that should allow you to deviate a bit more conveniently from the convention MTI is expecting.

Even still, the refactor will degrade more gracefully and you should see your desired behavior without having to do anything further. I've updated the README to describe the behavior in more detail.

If you wouldn't mind pointing to develop and let me know if it's working for you. I'll create a new RC and push it up to RubyGems when I get the thumbs up from you. 😄

[manuelmeurer]

Thanks, I'll try it!
I don't quite understand the table name conventions though... why/when do we need table names like "account/developers" for example?
The README is a bit confusing as it is right now I think, since it says In most cases, you shouldn't have to do anything beyond installing the gem. but also # table_name is 'account/developers'. Does ar_mti change the table name automatically?

[yuki24]

Happy holidays, @manuelmeurer @voltechs!

I'm also not sure about the naming conventions either. I was also concerned about the amount of the monkey-patches so I started my own work: develop...yuki24:overhaul. There is a number of improvements I have made in there:

• All tests pass and works well (except for the Transportation:: Military::Vehicle model, which was not tested well in the first place)
• Only monkey-patches MTI models (safer way to patch)
• Reduces the number of calls to the db to fetch table metadata (2 at maximum)
• Does not assume all models are loaded when inheriting
• A lot less code (+100 lines removed)
• Dropped support for older Rails/Ruby versions as a sacrifice (Ruby 2.1 and earlier and Rails 4.1 and earlier)

@voltechs When you have time, could you check out my code here? Happy to help make the code safer and more stable.

@manuelmeurer You could also try using my branch if you wouldn't mind:

gem 'active_record-mti', git: 'https://github.com/yuki24/active_record-mti.git', ref: 'overhaul'

[voltechs]

@yuki24 Happy (belated) Holidays to you too!

Thanks for that overhaul. It's definitely intriguing. I see a lot of cool solutions in there. I'm still picking through the changeset, I'll let you know when I'm finished. As it stands, I could definitely see incorporating some of the changes, some things I'm not entirely on board with or I don't understand yet.

I have a mental block about dropping support for Rails 4.1 and Ruby 2.1, but I might come around :P

[manuelmeurer]

Hi @yuki24, thanks for your work on this!
Unfortunately I am busy with other features right now but I'll get back to this soon and will try out your version then! 😄

[manuelmeurer]

Hi @yuki24, I tried your branch but have run into the problem that the child model doesn't recognize it's own fields, i.e. in the example I posted in the first message of this issue, if I do AdminUser.new(roles: ['foo']), I get ActiveRecord::UnknownAttributeError: unknown attribute 'roles' for AdminUser. Also, AdminUser.table_name is "users", is that correct?

@voltechs Have you had the chance to look at @yuki24`s change in more detail?
Would be great to get them merged as they seem like sensible improvements to the project.

Regarding dropping support for Ruby <= 2.1 and Rails <= 4.1, I'm all for it since both are not supported anymore:
https://www.ruby-lang.org/en/news/2017/04/01/support-of-ruby-2-1-has-ended/
http://guides.rubyonrails.org/maintenance_policy.html

[manuelmeurer]

@yuki24 @voltechs Ping! 😄

[yuki24]

I've been away from open source since New Year's holidays but will come back in the first week March. Thanks for bearing with me!

[manuelmeurer]

Sure thing, enjoy your open-source holidays! 😎

[yuki24]

@manuelmeurer Sorry for the delay, I finally had time to look into this. First of all, I ended up not using Postgresql's MTI in my app because of a number of reasons. With regards to your issue, a quick fix would be to add self.table_name = 'action_users' in the AdminUser class. I know this is a boring fix, but at least it should fix the issue.

There is also a number of issues that I noticed in this gem:

• Right now, this gem directly adds modules to ActiveRecord::Base. What this means is that it could affect pretty much any model in your app if there was a bug. Unfortunately, it is very likely to happen given the amount of monkey-patches and hacks.
• The current code base is written based on the assumption that every model is loaded and registered to ActiveRecord::MTI::Registry before the first SQL query goes out, which is not true in development. Because of that, when there's an AdminUser < User inheritance and you call a User.all without loading AdminUser in development, it wouldn't be able to look up the AdminUser model properly.
• It is almost impossible to figure out the appropriate table for the model without eager-loading and you would have to opt-in to eager-loading even in development (5-20s of loss on every boot)
• The singular_parent_table_name/plural_child_table_name convention makes it extremely difficult to get things working. Because of this, now the child model needs to know about the parent table and the parent table needs to be loaded before the child is loaded.

So my conclusion is that you would have to give up on something to make it work. And even if you could get everything right in Ruby, the restrictions Postgresql's table inheritance brings in would not go away (I saw a helpful StackOver answer but I can't find it - will post it here once I do).

I probably won't make any more changes to my branch. I'm sorry for dropping the ball, but it's hard to maintain things I don't use myself. But please do let me know if you have questions about Ruby or Rails.

[manuelmeurer]

Hi @yuki24,

Thanks for your detailed answer!
I totally understand that you don't want to continue to work on something you don't use yourself.
I don't have time to dig into these issues either, but maybe someday somebody will make it work. :)

Cheers,
Manuel

[voltechs]

Hey all,

I got around to merging in a bunch of the work @yuki24 did. It's much less polluty (it's a word now :P). It's not quite ready for primetime, but it's a huge step forward. Latest is in develop

Thanks for your support guys :)

[manuelmeurer]

Thanks, @voltechs!
Can you say something about the four issues @yuki24 mentioned?

[voltechs]

Hey @manuelmeurer, here are my thoughts on @yuki24's concerns!

• ActiveRecord::Base Pollution (concern no. 1)
  • I'd argue that this isn't a very good argument. This is pretty much table-stakes for any gem that extended any functionality, and is very often found in gems that extend ActiveRecords functionality. Furthermore, if you're concerned about bugs in the library, you shouldn't be using it in production. This is certainly a pre-1.0.0 gem, and I wouldn't be surprised to find bugs, but that's why were have test coverage and a wonderful community to leverage it's open source feature.
  • We could consider taking an approach where you manually "include" or "extend" a module from the ActiveRecord::MTI namespace in the models you're interested in having this behavior, but that's not done for the STI functionality that comes with Rails—this is meant to be a drop-in extension for unlocking MTI in the Rails framework. I'd further argue that the injected functionality isn't that invasive.
• Tablename => Model guessing sans eager_load (concern no. 2 & 3)
  • I don't think it'd be that difficult, actually.
  • The convention (see below) would allow for classifying the table_name and then you could demodularize until you found the class name. For example, user/admins would classify (after singularizing) to User::Admin, if that's not found, it could then pop the top most namespace (User::) off and look for Admin. I could probably draft something up to do that for a subsequent release.
• Inherited Table Name Convention (concern no. 4)
  • Seeing as how there is no concept of (true) table inheritance in Rails, it's difficult to make an argument for what the convention should be. I won't enumerate all the arguments here, but know that I do understand both sides. That being said, this is all a bit moot given that this is (now) entirely configurable.

[yuki24]

@voltechs Thanks!

I'd argue that this isn't a very good argument

If you think so, please do continue to maintain the project! It's just my concern and that shouldn't stop you from what you are doing. And I'm always here as part of the community for help if there's anything I could do for the project.

I don't think it'd be that difficult, actually.

There is one case that is a bit tricky to cover. Let's say there is Admin (mapped to the admin_users table) inherits from User (mapped to users). In development, not all the constants are loaded, so if a collection of users is loaded through the User model (which also pulls in Admin records if there's any), the User model wouldn't be able to look up the Admin model. It would work fine only if the Admin is loaded beforehand, which makes the behaviour unpredictable. How would you go about this?

# app/models/user.rb
class User < AppliactionRecord; end

# app/models/admin_user.rb
class Admin < User
  self.table_name = 'admin_users'
end

$ rails c
Rails.env           # => 'development'
defined?(AdminUser) # => nil

User.all  # => fails to look up the AdminUser constant
AdminUser # => forces Rails to load AdminUser
User.all  # => works as expected

This edge case is easy to miss if the test suite in the gem is eager-loading the dummy Rails app in the test. STI doesn't have this issue as the class name is simply stored in the database and AR just calls the constantize on it (though this suffers from another case where a class name is renamed later).

[voltechs]

Hey @yuki24,

If you think so, please do continue to maintain the project! It's just my concern and that shouldn't stop you from what you are doing. And I'm always here as part of the community for help if there's anything I could do for the project.

I certainly will! I love this project and we use it in production at work, so I can vouch for its stability and utility. I’m a huge fan of PostgreSQL’s true inheritance feature.

Regarding the admin_users case, I can envision supporting that too. That could be totally configurable (I’d need to allow for a setting called parent_prefix and parent_suffix or something) in conjunction with the _ separator. The only requirement would be that the user (developer) be consistent, which—as seasoned engineers—we can all universally agree is a good practice! So that too would be solvable! 😄

I’ll be making another pre-release here shortly and I’m working on getting it rolled out to our production environment. I’ll release a full (minor) version rev in concert with that.