Skip to content
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.

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

Refactor association methods + fix issue with #find_by_id #158

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tiagopog
Copy link

@tiagopog tiagopog commented Jan 16, 2018

What's this PR do?

It changes the use of #find_by_id in favor of #find_by. It also refactors some portion of the code related to the associations macros (belongs_to, has_one, has_many).

TODOs

  • Create an abstract method to fetch associations;
  • Use #find_by rather than #find_by_* when fetching single associations;
  • Pass specs.

Background context

Recently we went through a weird situation at Beauty Date while running our seeds file. It turned out that a model based on ActiveHash was not retrieving a belongs_to association for calling the underlying #find_by_id on the related model and returning nil.

I did a little debug and figured out that, unlike the #find_by_*, calling the #find_by method was actually working as expected. I don't know yet the reason for this bug to be happening apparently
only when executing the seed task, but maybe it's something related to the seed context and the metaprogamming applied while defining those dynamic finders. I sure will need to make a deeper debug on it.

For now this fork has fixed the issue and I also think that avoiding unnecessary metaprogramming is a good practice (safer, faster etc).

Manual test

# belongs_to
City.belongs_to :country
City.first.country

# has_many
Country.has_many :cities
Country.first.cities

# has_one
Country.has_one :mayor
Country.first.mayor

elsif klass.respond_to?(:scoped)
klass.scoped(:conditions => {foreign_key => primary_key_value})
def normalize_primary_key(type, association_class)
primary_key = type == :belongs_to ? association_class.primary_key : name.constantize.primary_key

Choose a reason for hiding this comment

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

@tiagopog I'd like to recommend to invoke the primary_key method just once before of .to_sym calling. e.g:

klass = type == :belongs_to ? association_class : name.constantize
klass.primary_key.to_sym

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch, @serradura 👍

end
end
foreign_key.to_sym

Choose a reason for hiding this comment

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

@tiagopog
Copy link
Author

@zilkey uh, I was wondering here whether it's really necessary to keep the support for the old/deprecated Rails' scoped method? As you may notice in the diff I have removed this call within the has_many method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants