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

Add an ability to manually include relationships #1882

Closed
wants to merge 1 commit into from
Closed

Add an ability to manually include relationships #1882

wants to merge 1 commit into from

Conversation

kossnocorp
Copy link

The option allows to explicitly include a certain relationship using the options object:

has_one :job, included: true

Before this commit, there was no ability to do that except via the query: /invoices?include=job.

The PR has no tests, docs, etc., but I going to include that if the PR has a chance to be merged. WDYT?

@mention-bot
Copy link

@kossnocorp, thanks for your PR! By analyzing the annotation information on this pull request, we identified @beauby, @bolshakov and @bf4 to be potential reviewers

The option allows to explicitly include a certain relationship
using the options object:

```ruby
has_one :job, included: true
```

Before this commit there was no ability to do that except
via the query: `/invoices?include=job`.
@NullVoxPopuli
Copy link
Contributor

Hey thanks for the PR!
We've been discussion how to go about his for a bit -- here is some history if you're interested:
#1845
#1797
#1720

I know all the tests currently, pass, but the goal is to keep the API as consistent as possible.

So, I'm wondering, what happens when you have included: true on an association, and then also use block options?

@kossnocorp
Copy link
Author

Not sure what "block options" means, but in such case link is ignored 😞

has_one :job, included: true do
  job = JobsRepository.one_for(object)
  link(:related) { api_v2_job_url(job) } if job
  job
end

@bf4
Copy link
Member

bf4 commented Aug 21, 2016

Before this commit, there was no ability to do that except via the query: /invoices?include=job.

or via the same option created in the controller....

@bf4
Copy link
Member

bf4 commented Aug 21, 2016

What's the behavior suppose to be if include options are given?

@kossnocorp
Copy link
Author

kossnocorp commented Aug 21, 2016

@bf4 it supposed to be equal to:

def index
  render json: invoices, include: [:job]
end

@bf4
Copy link
Member

bf4 commented Aug 21, 2016

@kossnocorp but if someone passes render jsonapi :invoices, include: [:potato] should it still include :job? I think behavior should be that if any include is passed, then the default include is ignored, but I've never wanted this feature in AMS, so I don't know what people asking for it expect. In my apps I keep this behavior in render options.

@kossnocorp
Copy link
Author

@bf4 yes, I think it should.

In my practice, I always include certain relationships due to a few reasons:

  • There are resources that don't make sense without relationships (Time has minutes and seconds).
  • To save on requests number.
  • To save on code (ditch uselessTime minutes and Time seconds endpoints)
  • For security sake.

@NullVoxPopuli
Copy link
Contributor

Those are good reasons. I'm in favor

On Sun, Aug 21, 2016, 11:18 AM Sasha Koss [email protected] wrote:

@bf4 https://github.com/bf4 yes, I think it should.

In my practice, I always include certain relationships due to a few
reasons:

  • There are resources that don't make sense without relationships (Time
    has minutes and seconds).
  • To save on requests number.
  • To save on code (ditch uselessTime minutes and Time seconds
    endpoints)
  • For security sake.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1882 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMJarrVmlC7qXLwZUQTNkNH2a-wqzl7ks5qiGw3gaJpZM4JlJNi
.

@bf4
Copy link
Member

bf4 commented Aug 21, 2016

Ok, assuming this is how the behavior will work, how may levels of the association should it include and should it be able to limit fields (this is discussed in the linked issues above)?

I just really see this convenience becoming a burden on AMS and distraction from important features if we don't get the API right.

re the examples, could you help me here?

There are resources that don't make sense without relationships (Time has minutes and seconds).

Time's not a good example of a relationship. Why would you have a Time resource that has seconds as an association and not an attribute?

To save on requests number.

How does a default include in a serializer save on number of requests? If you want to have some relationship included on some endpoint, then you should just always pass in the desired include when rendering, no? That's what I do.

I do think that the JSON API spec needs to be clearer about what the representation of a resource can be when no fields/includes are passed. I've been considering making any issue around this.

To save on code (ditch uselessTime minutes and Time seconds endpoints)

I don't follow this. We're talking about exchanging a userland option for adding a bunch of complexity and bug surface to the library and complexity in userland for understanding serializer behavior.

I know we've discussed this in some of the linked issues but I'm running out right now.. maybe we should try to consolidate this high-level discussion in one of those issues..

For security sake.

Example please? How does moving code from an option to a default option improve security?

😃

@NullVoxPopuli
Copy link
Contributor

Ok, assuming this is how the behavior will work, how may levels of the association should it include and should it be able to limit fields (this is discussed in the linked issues above)?

I actually think the fields behavior alone would be reason enough to not implement this.
No one can really agree on how they want the default behavior. @kossnocorp what might be better is to use the IncludeDirective class with a before_action in your controllers to merge in default includes / fields per controller / action.
Now that I think of that, I think that should be the direction we steer people towards when this comes up.

To try to answer some of @bf4's questions:

How does a default include in a serializer save on number of requests?

Ember, for example, is pretty aggressive in wanting relationship promises to be fulfilled. So, if the data isn't already there, it will try to make a request to an api resource. matching the model name.

To save on code (ditch uselessTime minutes and Time seconds endpoints)

I also don't allow this.

@kossnocorp for security purposes, you'll probably want to inspect each record about to be rendered to the user. Authorizer does this pattern, I also do this in my skinny_controllers gem (via policies). Basically, you just define some methods that correlate to your actions, and return a boolean if the object can be accessed / updated / whatever or not.

@kossnocorp Basically, this all comes down to how you think about your API approach.

I was in favor of a default include, until @bf4 reminded me about all the fields-related issues we've had...

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Aug 21, 2016

@kossnocorp, I'd recommend either monkey patching AMS (maybe, @bf4, we could have a monkey patch gallery somewhere -- where we showcase what things people have done to help make AMS work for them?), or apply include per controller and merge it into the include options :_)

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Aug 21, 2016

@kossnocorp another approach you could do is change your endpoints per security level.
like, /api/resource and /api/protected/resource ?

I don't know how easy it would be to make ember do this though. :-\

@bf4
Copy link
Member

bf4 commented Aug 22, 2016

Scope was originally intended for auth, hence curren_user as default

B mobile phone

On Aug 21, 2016, at 2:34 PM, L. Preston Sego III [email protected] wrote:

@kossnocorp another approach you could do is change your endpoints per security level.
like, /api/resource and /api/protected/resource ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@jgrau
Copy link
Contributor

jgrau commented Aug 31, 2016

IMO @bf4 has good arguments for why this feature is more problematic than useful. I suggest closing.

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

Successfully merging this pull request may close these issues.

5 participants