search

TwilightCoders / active_record-mti

ActiveRecord support for PostgreSQL's native inherited tables (multi-table inheritance)
MIT License
98 stars 7 forks source link

Rails 6 Support #12

Open sjaveed opened 4 years ago

sjaveed commented 4 years ago

Hi @voltechs, I'm interested in using this in a Rails 6 project. Wondering if this is still maintained.

Thanks!

voltechs commented 4 years ago

Hi @sjaveed, thanks for reaching out.

The project is semi-maintained. I'm sitting on a huge refactor that is about 98% complete, with one edge case that I'm still trying to find an elegant solution for.

With the refactor, it will work seamlessly (🤞🏼) with 4.2 through 5.2 (at the very least, but I think 6.x as well).

I haven't had time to work on it recently, but I can tell you we use it heavily at my current employer, so there is some weight behind the maintenance here.

I will try to get this wrapped up in the next few weeks.

sjaveed commented 4 years ago

That's so great to hear! I tried the refactor branch (hopefully the right branch) with my Rails 6 app and it didn't seem to like it too much. I think the support for multiple databases in Rails 6 was throwing a wrench in this unfortunately.

I'd love to help fix that but might need a bit of overview of the code and how it's hooking into ActiveRecord to do its thing.

edit: the branch I tried was the overhaul branch - that seemed to be the one with all the latest changes.

sjaveed commented 4 years ago

Ok so good news and bad news @voltechs - I've got the overhaul branch working in Rails 6.0.3.4 and ensured it continues to work in Rails 5.2. The bad news is I've no idea if what I've done actually makes things worse. That's primarily because I'm not sure what all the moving parts are in ActiveRecord internals. I'll create a PR in a couple of hours to show you what I have and get your thoughts on it.

sjaveed commented 4 years ago

Here's the PR: #13

sjaveed commented 4 years ago

I ended up updating that with PR with another fix that showed up later. However, something else is now complicating things. Looks like the Rails STI implementation no longer always calls instantiate - instead there's an optimization in place for the last two years here: https://github.com/rails/rails/blame/master/activerecord/lib/active_record/querying.rb#L63. This means our discriminate_class_for_record method never gets called and we need to either reliably override the inheritance_column or likely need to push a change to rails to put the includes_column? check in a separate method that can be overridden.

This is relevant for the code here: https://github.com/TwilightCoders/active_record-mti/blob/develop/lib/active_record/mti/inheritance.rb#L10

Thoughts?

voltechs commented 4 years ago

Ah shoot. Sorry you spent time on this, the work I have in flight is not up on GitHub. I'll try to wrap it up and push it up soon, but I don't have a concrete timeline.

sjaveed commented 4 years ago

No worries - I was just interested in seeing if there was some simple fixes that could get this working with Rails 6. Looking forward to proper Rails 6 fixes from someone who knows what they're doing 🤣. Would it be possible to keep this issue open until that support gets merged in?