support has_many :through associations#296
Conversation
`belongs_to :through` is not a thing, and this code should not have been included in e6a47bc
this will allow me to add new classes in the next commit, but should also speed things up slightly.
has_many :through associationshas_many :through associations
| klass_name = association_id.to_s.classify | ||
| klass = | ||
| begin | ||
| klass_name.constantize |
There was a problem hiding this comment.
This makes me nervous.
I believe has_many is executed within a lock since it is defining a class.
Think this issue explains the issue: rails/rails#34310
In our projects, when we are defining a class, we work hard to not define a second one.
Not sure if this race condition still exists.
There was a problem hiding this comment.
ok, this is the same code path as our belongs_to fix.
I wasn't able to follow the association builder code to find an answer
looks like they do call safe_constantize (sparingly)
There was a problem hiding this comment.
Hmm. I'll create a separate PR converting all our invocations to safe_constantize.
There was a problem hiding this comment.
lol. I was talking more about ActiveSupport::Dependencies.interlock.loading, which probably goes away once zeitwerk is introduced. But sounds good.
There was a problem hiding this comment.
🤦 I definitely did not read carefully enough. Apologies for missing your point, I get it now.
kbrock
left a comment
There was a problem hiding this comment.
@flavorjones the 2 issues I see are not really introduced by this PR but just propagation of existing issues.
If I find out how rails fixes this, then I'll introduce a fix later on. (I'm not hopeful though)
Lets merge since this is a great fix.
- Support `has_many :through` associations active-hash#296 @flavorjones - Rails 7.1 support active-hash#291 @y-yagi - Rails 7.1: fix sqlite3 issue active-hash#303 @flavorjones - Rails 7.1.3: add missing `has_query_constraints?` active-hash#300 @flavorjones - `Array#pluck` supports methods active-hash#299 @iberianpig - Prefer `safe_constantize` over `constantize` active-hash#297 @flavorjones - Treat `nil` and `blank?` as different values active-hash#295 @kbrock - Fix `#where` for string keys active-hash#292 @usernam3
- Ruby 3.3 support active-hash#298 @m-nakamura145 - Support `has_many :through` associations active-hash#296 @flavorjones - Rails 7.1 support active-hash#291 @y-yagi - Rails 7.1: fix sqlite3 issue active-hash#303 @flavorjones - Rails 7.1.3: add missing `has_query_constraints?` active-hash#300 @flavorjones - `Array#pluck` supports methods active-hash#299 @iberianpig - Prefer `safe_constantize` over `constantize` active-hash#297 @flavorjones - Treat `nil` and `blank?` as different values active-hash#295 @kbrock - Fix `#where` for string keys active-hash#292 @usernam3
This PR closes #239.
The first commit removes unused (and broken?) code that I found while looking at the association code. It was part of e6a47bc and should not have been present.
The second commit refactors the extensions spec a bit to minimize the test classes being created for each test; this makes the tests more explicit and speeds them up slightly.
The final commit introduces the test from #239 and overrides the
has_manymethod in any class that includesActiveHash::Associations::ActiveRecordExtensions. The new implementation will adopt new behavior if the:throughoption is present and it points at anActiveHash::Baseclass::throughassociation name on each join model (e.g., "Appointment#physician")Otherwise
has_manywill just callsuper.