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

scoping resource associations with state #9

Closed
bastianlb opened this issue Apr 15, 2017 · 5 comments
Closed

scoping resource associations with state #9

bastianlb opened this issue Apr 15, 2017 · 5 comments

Comments

@bastianlb
Copy link

Hi Lee,
Thanks for the great set of libraries, it has greatly simplified writing a jsonapi compatible application.
Frequently I find myself scoping queries through some sort of user state, for example in this simplified situation:

class Foo < ApplicationRecord
  has_and_belongs_to_many :bars
end

class Bar < ApplicationRecord
  has_and_belongs_to_many :foos
  has_many :users
end

I might want to scope all requests to the Bar index endpoint, to only return bars that are associated to the current_user. However, if I request GET /foo?include=bar I will still get other Bars associated to the Foo that are not relevant to the user.

How would one accomplish this sort of relationship filtering with the jsonapi_suite? My initial thought would be to allow passing some sort of global scope into render_jsonapi which can be used by the association scope in the FooResource, similar to how ActiveRecord handles scopes:

class FooResource < ApplicationResource
  type :foo

  has_many :bars,
    foreign_key: { foos_bars: :foo_id },
    scope: lambda |current_user| { Bar.where(user: current_user) },
    resource: BarResource
end

class FooController < ApplicationController
  def index
    render_jsonapi Foo.all, current_user: @current_user  # could be generalized to any state
  end
end

What are your thoughts on how this could be handled? I'm happy to implement a PR.

@richmolj
Copy link
Contributor

Hey @BastianL, thanks for the kind words. I think there's a better answer here, but wanted to respond while I have a quick minute.

One, you could pass this in with the request, since 0.6.x supports association filtering:

/foos?include=bar&filter[bar][user_id]=123

The filter is applied as normal, just like if we were writing the resource for /bars?filter[user_id]=123:

allow_filter :user_id

I think that's your best immediate-term solution.

For a longer-term one, we actually have access to the controller from the resource. I use this for guards, currently:

allow_filter :whatever, if: :allowed_for_user?

def allowed_for_user?
  # code with current_user here
end

That code is here: https://github.com/jsonapi-suite/jsonapi_compliable/blob/master/lib/jsonapi_compliable/scoping/filterable.rb#L14. resource.context[:object] is the controller.

So one way to give access to the current user is to evaluate all the scope procs within that context. That said, I'm not thrilled about the pattern and not sure I want to double down on it that much.

I think there's a better solution, but that's what's top of my head at the moment.

@bastianlb
Copy link
Author

I have used the filter aspect of the jsonapi successfully in the past, but it seems lacking especially when dealing with has_and_belongs_to many tables, and don't want to expose the intermediate join table to your client. In above example filter would work, but suppose Bar HABTM users! Then there wouldn't be an id to query off without exposing an intermediate table. My actual use case is slightly more complicated :/

class Foo < ApplicationRecord
  has_and_belongs_to_many :bars
end

class Bar < ApplicationRecord
  has_and_belongs_to_many :foos
  has_and_belongs_to_many :bazs
end

class Baz < ApplicationRecord
  has_and_belongs_to_many :bazs
  has_and_belongs_to_many :users
end

I want to to query Foo, but only ever include the Bars that I filter through the Baz - user association, for the current user.. Would love to discuss this with you further next week if you have time. Enjoy your weekend!

@richmolj
Copy link
Contributor

HABTM is definitely a use case that can be improved. I've thought a lot about this and the short answer is, "it's on the list, but not a priority for me".

I agree ideally the join table is not exposed, since this is an implementation detail. However, let's say you now add a column to the join table - a boolean like primary or an integer like rank. Since this join table was omitted in the original design, we now have to expose it to access this data and break backwards compatibility.

Really we want this data treated as metadata of the relationship, not a separate object. JSONAPI does support per-object metadata...but almost no library actually supports it and there's not a fully-fleshed out filter/sort/etc paradigm for it. I think it's a good long term direction but not one I'm ready to spend time embracing at the moment.

There is a solution for your use case, though, I think:

In above example filter would work, but suppose Bar HABTM users! Then there wouldn't be an id to query off without exposing an intermediate table

If I'm understanding correctly, you could:

allow_filter :user_id do |scope, value|
  scope.joins(:users_to_bars).where("users_to_bars.user_id = ?", value)
end

Which at least solves the immediate filter issue.

All that said, I'm personally exposing my join tables at the moment. Yes, it's a little more verbose but at least everything is dead simple and easy to reason about. I'm working to release an isomorphic js client that works similar to ActiveRecord...I'm considering if adding a through to the client, just like ActiveRecord, is the real solution here. Would be simpler (if less ideal/correct) than a server-side solution.

All that summed up - definitely on my radar, but I don't think exposing the join tables is prohibitive at the moment, just less than ideal. So I'm waiting to get more data before settling on a solution there.

@richmolj
Copy link
Contributor

Another solution you might want to consider is middleware setting User.current via Thread.current. This has fallen out of vogue recently but I've never had much of a problem with it and would be another solution to your original use case. I don't think you should have to do it that way, but something to consider.

@bastianlb
Copy link
Author

Thanks for the help! The filter block should actually do nicely, that wasn't on my radar. I'll check it out and close the issue soon.

richmolj added a commit that referenced this issue Jan 6, 2019
Introduce Graphiti::Serializer
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

No branches or pull requests

2 participants