Open manuelmeurer opened 6 years ago
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. 😶)
So I took a stab at it (https://github.com/TwilightCoders/active_record-mti/commit/6edcb2bb00dd0f6aca533a95830106055d83fe4e). I changed the User
model in the specs to use ApplicationRecord
, but specs did not fail. Maybe that's a starting point for you.
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?
@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. 😅
@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?
Good point, @yuki24! Why is the table name set explicitly, @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 :)
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. 😄
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?
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: https://github.com/TwilightCoders/active_record-mti/compare/develop...yuki24:overhaul. There is a number of improvements I have made in there:
Transportation:: Military::Vehicle
model, which was not tested well in the first place)@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'
@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
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! 😄
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
@yuki24 @voltechs Ping! 😄
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!
Sure thing, enjoy your open-source holidays! :sunglasses:
@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:
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.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.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.
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
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 :)
Thanks, @voltechs! Can you say something about the four issues @yuki24 mentioned?
Hey @manuelmeurer, here are my thoughts on @yuki24's concerns!
ActiveRecord
s 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.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.eager_load
(concern no. 2 & 3)
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.@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).
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.
Unfortunately my
AdminUser
class is not recognized as a MTI class. :( I'm running Rails 4.2.10 and this is my setup:When I check
AdminUser.mti_type_column
andAdminUser.tableoid_column
on the console, they are bothnil
, which they shouldn't be according to the specs. What can I do to further help debugging this problem?