Skip to content

Has many through supports aliased names #329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kbrock merged 4 commits into from
Jun 6, 2025
Merged

Has many through supports aliased names #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
306eb96
support has_many through with aliased association names
alexgriff May 29, 2025
a26c70c
remove active_hash_ prefix from local var name
alexgriff Jun 4, 2025
a17e921
specs add associations in specific contexts
alexgriff Jun 4, 2025
ada3e8f
specs clearly delete old associations before adding new ones
alexgriff Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/associations/associations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,16 @@ def self.extended(base)

def has_many(association_id, scope = nil, **options, &extension)
if options[:through]
klass_name = association_id.to_s.classify
klass = klass_name.safe_constantize
source_association_name = options[:source]&.to_s || association_id.to_s.singularize
Copy link
Collaborator

@kbrock kbrock Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why, but I do feel this is much easier to read.
If you are having trouble with it, you can add back.


through_klass = reflect_on_association(options[:through])&.klass
klass = through_klass&.reflect_on_association(source_association_name)&.klass

if klass && klass < ActiveHash::Base
define_method(association_id) do
join_models = send(options[:through])
join_models.flat_map do |join_model|
join_model.send(association_id.to_s.singularize)
join_model.send(source_association_name)
end.uniq
end

Expand Down
50 changes: 50 additions & 0 deletions spec/associations/active_record_extensions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,56 @@ def define_doctor_classes

expect(patient.physicians).to contain_exactly(physician1, physician2)
end

describe "with the :source option" do
before do
# NOTE: Removing the Patient#physicians association and adding Patient#doctors
Copy link
Collaborator

@kbrock kbrock Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect

Patient._reflections.delete('physicians')
Patient.class_eval do
define_method(:physicians) { raise NoMethodError, "The #physicians association is removed in this spec, use #doctors" }
define_method(:physicians=) { |_| raise NoMethodError, "The #physicians association is removed in this spec, use #doctors" }
end
Patient.has_many :doctors, through: :appointments, source: :physician
Copy link
Contributor Author

@alexgriff alexgriff Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock what do you think of this approach where we simply add the differently named associations and dont bother removing the "default" ones. I was kind of wanting to not have both associations present on the models in the spec contexts, it felt a bit nicer, but it's not quite necessary and doesn't really effect the functionality being tested.

One other thought is that I could take the approach here but also explicitly remove the associations and added methods, something like:

Appointment._reflections.delete('physician')
Appointment.class_eval do
  define_method(:physician) { raise NoMethodError }
  define_method(:physician=) { raise NoMethodError }
end
Appointment.belongs_to :doctor, class_name: 'Physician', foreign_key: :physician_id

... but that seems like a lot for something that really doesn't "do anything"; specs will pass with or without it. Still, maybe worth it tho?

Copy link
Collaborator

@kbrock kbrock Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Just having a single association does feel cleaner.
I just worry that next year, developers will have to understand why this is being done every time reading this code.

Either way, a comment is our friend. Depending upon how you roll, something to this effect:

# NOTE: the physician association is handled by doctor and no longer a valid association. Removing it.
# NOTE: the physician association is handled by doctor and no longer a valid association. Please don't use in this test

I think I prefer less code, but totally up to you.

end

it "finds ActiveHash records through the join model" do
patient = Patient.create!

physician = Physician.last
Appointment.create!(physician: physician, patient: patient)

expect(patient.doctors).to contain_exactly(physician)
end
end

describe ":through when the join model uses an aliased association" do
before do
# NOTE: Removing the Appointment#physician association and adding Appointment#doctor
Appointment._reflections.delete('physician')
Appointment.class_eval do
define_method(:physician) { raise NoMethodError, "The #physician association is removed in this spec, use #doctor" }
define_method(:physician=) { |_| raise NoMethodError, "The #physician association is removed in this spec, use #doctor" }
end
Appointment.belongs_to :doctor, class_name: 'Physician', foreign_key: :physician_id

# NOTE: Removing the Patient#physicians association and adding Patient#doctors
Patient._reflections.delete('physicians')
Patient.class_eval do
define_method(:physicians) { raise NoMethodError, "The #physicians association is removed in this spec, use #doctors" }
define_method(:physicians=) { |_| raise NoMethodError, "The #physicians association is removed in this spec, use #doctors" }
end
Patient.has_many :doctors, through: :appointments
end

it "finds ActiveHash records through the join model" do
patient = Patient.create!

physician = Physician.last
Appointment.create!(doctor: physician, patient: patient)

expect(patient.doctors).to contain_exactly(physician)
end
end
end

describe "with a lambda" do
Expand Down