has_many associations respect nested resources #17

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@sfaxon
Contributor
sfaxon commented Apr 18, 2012

This uses find(:all, :from ...) to build the associated path
fixes #16

@sfaxon sfaxon has_many associations respect nested resources
This uses find(:all, :from ...) to build the associated path
fixes #16
e01716e
@sfaxon

I don't like the way the path is built here, maybe because find(:all ... still feels like the ActiveRecord 2.x days. Any recommendations?

This whole define_method block should probably be something like:

define_method method_name, ->(options = {}) {
  if instance_variable_defined?(ivar_name)
    instance_variable_get(ivar_name)
  elsif attributes.include?(method_name)
    attributes[method_name]
  else
    # finding does not set an instance variable
    association_model.find(:all, :from => "#{element_base_path}/#{method_name}.#{association_model.format.extension}", :params => options[:params]))
  end
}

The disadvantage of this is that every request to method_name would generate a new request, since options[:params] could change the results. That may be acceptable.

@jeremy
Member
jeremy commented Oct 19, 2012

The method_defined? check is confusing :)

The second method definition you propose is clearer.

Perhaps you could cache an instance variable if params aren't set? Only do a new find call if params are passed.

@kritik
kritik commented Jul 25, 2013

I would prefer to use such solution perfectline@e44d4ae. All other stuff could be added later ;) Can I push this commit?

@raszi
raszi commented Feb 23, 2014

This is unfortunately not working with multiple levels:

class SubItem < ActiveResource::Base
end

class Item < ActiveResource::Base
  has_many :sub_items
end

class Root < ActiveResource::Base
  has_many :items
end

Root.first.items.first.sub_items
@rafaelfranca
Member

Closing due to inactivity. Please submit a new PR if you are still interested.

@ches
Contributor
ches commented Nov 30, 2015

Heh, it sucks to hear "inactivity" when no maintainers respond for over 3 years, but it's nice to see some issue triage happening nonetheless, thanks for your time @rafaelfranca.

This and closely related issues were some of my biggest annoyances on a project that used ActiveResource pretty seriously, but I'm no longer working on it so personally may not have much attention to lend to trying to help get acceptable fixes through reviews as I might have before. This PR may have had some implementation shortcomings, but hopefully #16 encompasses the spirit of the problem, because these are things that should be tracked as open, valid issues, IMO.

@rafaelfranca
Member

Heh, it sucks to hear "inactivity" when no maintainers respond for over 3 years

Yes sorry about that. I had to close since I don't know if the original author would rebase this branch and keeping the PR open would be worse. I could not find a better text to explain the situation and I tried to be quick.

Hopefully I can get all PR reviewed soon and the things will be easier to maintainers. I'll be around since Shopify uses Active Resource, but of course this repository is not my main focus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment