Has many through supports aliased names

[alexgriff]

Bug/Problem

Currently, using a has_many :through to return a collection of ActiveHash records is limited to working successfully in the case where the association_id passed to has_many exactly corresponds to the ActiveHash class name. It must be the pluralized version of the singularized class name.

In this example, Zoo#countries works as expected:

class Country < ActiveHash::Base
  # ...
end

class Animal < ApplicationRecord
  extend ActiveHash::Associations::ActiveRecordExtensions
  
  belongs_to :country
  belongs_to :zoo
end

class Zoo < ApplicationRecord
  extend ActiveHash::Associations::ActiveRecordExtensions

  has_many :animals
  has_many :countries, through: :animals
end

But, If any of the association method names involved in the has_many through: require some type of aliasing where the relationship to the ActiveHash class can't be deduced, the behavior in ActiveHash::Associations::ActiveRecordExtensions will not correctly define the ActiveHash version of the association reader method.

Here, Zoo#nations would break

class Zoo < ApplicationRecord
  extend ActiveHash::Associations::ActiveRecordExtensions

  has_many :animals
  has_many :nations, through: :animals, source: :country
end

And here, Zoo#countries would break

class Animal < ApplicationRecord
  extend ActiveHash::Associations::ActiveRecordExtensions
  
  belongs_to :country_of_origin, class_name: 'Country'
  belongs_to :zoo
end

class Zoo < ApplicationRecord
  extend ActiveHash::Associations::ActiveRecordExtensions

  has_many :animals
  has_many :countries, through: :animals
end

It felt worth supporting these cases because it feels similar to how the class_name option is correctly supported in a belongs_to_active_hash for equivalent use cases around aliased association names.

I made sure the use of source matches how that option is passed in ActiveRecord itself (has_many throughs: use source, and don't support class_name).

Documentation

I looked for a place to document this new behavior in the README, but there wasn't an obvious place to add this without adding a whole new section or something like that. I can try to do so if that feels appropriate.

Specs

I added specs for the new supported cases. To reuse the same spec setup and ephemeral classes, but at the same time to be able to change the association definitions per spec context, I parameterized the define_doctor_classes spec setup method. This adds some complexity in a way I don't love, but still felt better than alternate approaches. I'm game to rethink this, though, if it doesn't feel like the right solution.

[kbrock]

this looks like a nice change.

1. potential shortening of has_many local variable name
2. simplification of tests
3. squashing the commits

(though you may want to squash commits, make my suggested change in a separate commit - in case we decide it doesn't make it any better)

[kbrock]

Is there a way to just define 2 different associations here instead of passing in these parameters/metadata? Or maybe pass a boolean and define one belongs_to vs the other?

Not sure if it is possible.

[alexgriff]

Agreed there's room for improvement here, i'll poke at this a bit more.

[kbrock]

For me, it reads better without the active_hash prefix. This is our code base, so for a method name, sure, but for local variables, it gets a little wordy. What do you think?

Suggested change

	active_hash_source_association_name = options[:source]&.to_s || association_id.to_s.singularize
	through_klass = reflect_on_association(options[:through])&.klass
	klass = through_klass&.reflect_on_association(active_hash_source_association_name)&.klass
	source_association_name = options[:source]&.to_s || association_id.to_s.singularize
	through_klass = reflect_on_association(options[:through])&.klass
	klass = through_klass&.reflect_on_association(source_association_name)&.klass

join_model.send() would also need to be changed.

[alexgriff]

Ya, I'm down to make this change. My thinking was that there are two associations here in the has many through:, only one of which is to an active hash, so was trying to capture that. But, to your point, the existing code even in this same method uses local vars like klass not active_hash_klass. Makes sense to me to follow that.

[alexgriff]

@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?

[kbrock]

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.

[kbrock]

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

[kbrock]

I like this.

Add a comment if that helps you.
Let me know when you are done and I'll merge

[alexgriff]

@kbrock I think I'm all set - thanks for shepherding this through!

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

I get this, but I ended up definitely going "more code" not less, and actually deleting the old associations.

I liked that I could get an actual error messages in there easily enough which also kinda acts like a comment

define_method(:physicians) { raise NoMethodError, "The #physicians association is removed in this spec, use #doctors" }

But happy to change anything

[kbrock]

perfect