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.

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

Jump to bottom

Scopes on associations #268

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

Scopes on associations #268

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4391a24
Modify association_proxy's method_missing to allow for scopes.
dturnerTS Jul 3, 2014
0def4c8
update comment
dturnerTS Jul 3, 2014
eb55314
Need to extract parents primary key if its in the collection path
dturnerTS Jul 3, 2014
d3846ac
variable scope fix
dturnerTS Jul 4, 2014
1c36fab
Her::Api::request will strip out params that start with a leading _. …
dturnerTS Aug 22, 2014
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
12 changes: 12 additions & 0 deletions lib/her/model/associations/association.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module Associations
class Association
# @private
attr_accessor :params
attr_reader :klass

# @private
def initialize(parent, opts = {})
Expand All @@ -15,6 +16,17 @@ def initialize(parent, opts = {})
@name = @opts[:name]
end

def call_scope(name, *args, &block)
parent_id_string = "#{@parent.class.to_s.demodulize.downcase}_#{@parent.class.primary_key}"
parent_id = @parent.send(@parent.class.primary_key)
scoped = if klass.collection_path[parent_id_string]
Copy link

Choose a reason for hiding this comment

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

does it mean we need to specify collection_path on the association's model if we want to call the scopes on it? is there any way to get around it?

This is the official document example,

@user.comments.where(approved: 1)
# GET "/users/1/comments?approved=1"
# => [#<Comment id=1>]

wonder if we could just call it without adding that collection_path:

# want this
@user.comments.approved
# GET "/users/1/comments?approved=1"

# current(without specifying collection_path on Comment model)
# GET "/comments?approved=1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure exactly what your asking about.

You don't have to explicitly set a collection path (https://github.com/remiprev/her/blob/master/lib/her/model/paths.rb#L48). If you don't set the collection_path it basically defaults to /class_name.

However, if a user has many comments, and you need to get the comments through /user/:user_id/comments Her requires a collection path to be set. There are some open issues discussing ways to deal with that and its far out side of the scope of this PR.

If you're suggesting that the right behavior is just to ignore whats in the collection path, I'd disagree since that would limit functionality.

Copy link

Choose a reason for hiding this comment

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

sorry I didn't make it clear. I'll switch the example from "comment" to "post", please bear with me for a moment.

class User
  include Her::Model
  has_many :posts
end

class Post
  include Her::Model
  belongs_to :user
  collection_path '/users/:user_id/posts/' # specified collection_path

  scope :approved, -> { where(approved: true) }
end

user = User.find(1)
user.posts # => /users/1/posts

# works great
user.posts.approved # => /users/1/posts?approved=true

# but break this kind of usage
Post.all # => Her::Errors::PathError: Missing :_user_id parameter to build the request path. Path is `/users/:user_id/posts`. Parameters are `{}`

In web app we normally don't show a list of comment, they tend to attach to a specific user so specifying the collection_path totally makes sense. But things like Post we may also want to show a full list of them(/posts), and also posts that belong to a user(/users/:user_id/posts). I was seeking a way to make the "scopes on associations" thing work on this kind of usage.

If remove that collection_path from Post model

class Post
  include Her::Model
  belongs_to :user

  scope :approved, -> { where(approved: true) }
end

user.posts # => /users/1/posts

# here user id got lost in the url path, but I really want it there
user.posts.approved # => /posts?approved=true
# but will allow this kind of usage
Post.all # => /posts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got, it. This is the same issue raised in #253 . The reason that is has not been resolved is that there is no clear answer on what to do when a class has multiplebelongs_to associations.

I strongly believe that this issue is outside of the scope of this pr.

Copy link

Choose a reason for hiding this comment

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

thanks for the reply, seems there're a lot unsolved discussions on the nested resource path topic. Sorry for bothering you all along 🙇 , hope this pull request will be merged..! i'm sure it'll solve lots of real world problems

klass.where(parent_id_string => parent_id)
else
klass
end
scoped.send(name, *args, &block)
end

# @private
def self.proxy(parent, opts = {})
AssociationProxy.new new(parent, opts)
Expand Down
5 changes: 5 additions & 0 deletions lib/her/model/associations/association_proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ def method_missing(name, *args, &block)
return association.fetch.object_id
end

# Does the underlying class of this association support this method
# at the class level, if so its likely to be a scope.
if association.klass.respond_to?(name) && association.klass.singleton_methods(false).include?(name)
return association.call_scope(name, *args, &block)
end
# create a proxy to the fetched object's method
metaclass = (class << self; self; end)
metaclass.install_proxy_methods 'association.fetch', name
Expand Down
15 changes: 14 additions & 1 deletion spec/model/associations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,13 @@
builder.adapter :test do |stub|
stub.get("/users/1") { |env| [200, {}, { :id => 1, :name => "Tobias Fünke", :comments => [{ :comment => { :id => 2, :body => "Tobias, you blow hard!", :user_id => 1 } }, { :comment => { :id => 3, :body => "I wouldn't mind kissing that man between the cheeks, so to speak", :user_id => 1 } }], :role => { :id => 1, :body => "Admin" }, :organization => { :id => 1, :name => "Bluth Company" }, :organization_id => 1 }.to_json] }
stub.get("/users/2") { |env| [200, {}, { :id => 2, :name => "Lindsay Fünke", :organization_id => 2 }.to_json] }
stub.get("/users/1/comments") { |env| [200, {}, [{ :comment => { :id => 4, :body => "They're having a FIRESALE?" } }].to_json] }
stub.get("/users/1/comments") do |env|
if env[:params]["locked"] == "true"
[200, {}, [{ :comment => { :id => 5, :body => "Is this the tiny town from Footloose?", :locked => true } }].to_json]
else
[200, {}, [{ :comment => { :id => 4, :body => "They're having a FIRESALE?" } }].to_json]
end
end
stub.get("/users/2/comments") { |env| [200, {}, [{ :comment => { :id => 4, :body => "They're having a FIRESALE?" } }, { :comment => { :id => 5, :body => "Is this the tiny town from Footloose?" } }].to_json] }
stub.get("/users/2/comments/5") { |env| [200, {}, { :comment => { :id => 5, :body => "Is this the tiny town from Footloose?" } }.to_json] }
stub.get("/users/2/role") { |env| [200, {}, { :id => 2, :body => "User" }.to_json] }
Expand Down Expand Up @@ -119,6 +125,8 @@
spawn_model "Foo::Comment" do
belongs_to :user
parse_root_in_json true
collection_path 'users/:user_id/comments'
scope :locked_scope, lambda { where(locked: true) }
end
spawn_model "Foo::Post" do
belongs_to :admin, :class_name => 'Foo::User'
Expand Down Expand Up @@ -228,6 +236,11 @@
params[:comments].length.should eq(2)
end

it 'supports scopes on assoications' do
locked_comments = @user_with_included_data.comments.locked_scope
Copy link

Choose a reason for hiding this comment

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

i found this call will append another user_id as parameter in the end..

/users/1/comments?locked=true&user_id=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'm not sure there is currently a good way to prevent that from happening. Any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

Copy link

Choose a reason for hiding this comment

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

thanks, confirmed the behavior 👍

locked_comments.first.id.should eq(5)
end

[:create, :save_existing, :destroy].each do |type|
context "after #{type}" do
let(:subject) { self.send("user_with_included_data_after_#{type}")}
Expand Down