Skip to content
This repository

associations should respect nested resources! #16

Open
ollym opened this Issue · 16 comments

10 participants

Oliver Morgan Markus Schwed Seth Faxon Marko Hielscher Piotr Szal Rafał Wojsznis Isaac Datlof Guillermo Iguaran Ches Martin James Le Cuirot
Oliver Morgan
ollym commented

I thought this issue was resolved but It isn't. Consider the following:

Club class: has_many :events
Event class: belongs_to :club

Club.first.events surely would be logical to make a request like /clubs/1/events.xml rather than what it does at the moment which is /events.xml?club_id=9

With the routes in the server like:

resources :clubs do
  resources :events
end

I think it's nuts to do it the way it is done currently, wrong direction!

Markus Schwed
Collaborator
SweeD commented

Oh, you're absolutely right, this should be changed.

Oliver Morgan
ollym commented

I wish I had more time and experience to fix this myself, but whilst you're there fixing it can you add a ActiveRecord-like Relationship class so that:

club.first.events.find(2) => /clubs/1/events/2.xml

Is possible. And if you're feeling particularly generous and helpful, something like:

club.first.events.limit(10) => /clubs/1/events.xml?limit=10

Seth Faxon

@ollym would you try:

class Event < ActiveResource::Base
  self.site = "http://localhost:3000/clubs/:club_id"
end

And keep the has_many on Club. Does this give you the expected behavior? I think the problem is that the belongs_to relationship is not setting up the prefix path. Having belongs_to set the prefix path could introduce some strange behavior if a class belonged to multiple other models. I'm thinking of the case where a Page and a Post might both have_many :comments. I think the cleanest way may be to force people to use a Page::Comment class and a Post::Comment class in this case. If we can assume/force only one belongs_to relationship I could put that patch together pretty quick.

Regarding the limit method, I know it matches a very popular convention, but I don't think ActiveResource should define that method. It too tightly couples the server implementation to ActiveResource, and reduces the usefulness of ActiveResource to consume API's not owned by the ARes developer. I would propose that something like club.first.events(:params => {:limit => 10}) would be more consistant with the way things are currently done. This is something I could add as well.

Oliver Morgan
ollym commented

@sfaxon aggreed, club.first.events(params: { limit: 10}) is perfectly ok :)

As for the fix I don't want to have to define the URL on each of my models, that just seems a waste. Especially including the host in there too.

On a side-note I think it would be good to be able to do:

self.path = 'clubs/:club_id'

And be able to set the host in a parent controller.

self.host = 'http://localhost:3000'

Seth Faxon

What I mentioned above could also be done as:

class Event < ActiveResource::Base
  self.site = "http://localhost:3000"
  self.prefix = "/clubs/:club_id/"
end

The trailing slash on :club_id is currently required, and is probably a bug. If no one objects to the proposed requirement of only one belongs_to per class I can probably tackle both issues at once.

Oliver Morgan
ollym commented

@sfaxon It is slightly inconvenient - why is it not possible to have more than one?

Seth Faxon

@ollym Disregard my previous comment, I misunderstood. I should have used find(:all, :from ....) in a previous commit. I'll put in another pull request shortly.

Seth Faxon sfaxon referenced this issue from a commit in sfaxon/activeresource
Seth Faxon sfaxon has_many associations respect nested resources
This uses find(:all, :from ...) to build the associated path
fixes #16
e01716e
Ken Ip kenips referenced this issue from a commit in kenips/activeresource
Seth Faxon sfaxon has_many associations respect nested resources
This uses find(:all, :from ...) to build the associated path
fixes #16

Conflicts:
	lib/active_resource/base.rb
	test/cases/base_test.rb
720c06a
Marko Hielscher

What about this issue? Is there a reason why this isn't merged?

Piotr Szal

very cool, works for me as a patch. Thx

Rafał Wojsznis

huh, just noticed it's still broken :o thanks for the patch @sfaxon

Isaac Datlof

Is there any plan to fix this 2 year old issue?

Guillermo Iguaran

Patches are welcome :)

Isaac Datlof

Are patches welcome? I see @sfaxon submitted a pull request that is still open from two years ago.

Ches Martin

@guilleiguaran Yeah, let's be honest here and not just quip "patches welcome" -- there are a host of pull requests submitted in the last several months and there's no leader stewarding the project, giving feedback on them and merging anything. Is anyone taking release ownership in ARes at this stage? Is there a mailing list for it anymore?

Guillermo Iguaran

@ike18t as you can see in the PR, the mergers are waiting for changes before of merge it:
#17 (comment)

James Le Cuirot
chewi commented

The code needed to do this properly would resemble ActiveRecord's code, using thread local storage to preserve the resource parameters along a chain, and using method_missing to apply these parameters to method calls such as find, create, update or whatever. This is not trivial to implement. Upon realising that, I switched to Her. Sorry. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.