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

PATCHing a reference that is not loaded should return a 403 #260

Open
256dpi opened this issue Aug 27, 2016 · 2 comments
Open

PATCHing a reference that is not loaded should return a 403 #260

256dpi opened this issue Aug 27, 2016 · 2 comments

Comments

@256dpi
Copy link
Contributor

256dpi commented Aug 27, 2016

Hi, I found a small inconsistency while working on fire.

Let's say I have a model that exposes a HasMany relationship that is not loaded:

// Post
jsonapi.Reference{
  Type: "comments",
  Name: "comments",
  IsNotLoaded: true,
}

This will omit the data: [] attribute from responses to the following URL to fetch the relationship:

GET /posts/1/relationships/comments

First of all: Is this even allowed by the spec or should this call return an error?

In any case the said relationship can currently also be updated via a PATCH request that includes an array of resource links. However, since the references aren't even loaded the API probably does not allow to set them either. In that case we must return an error. The spec suggest to return a 403 Forbidden if the method is not allowed: http://jsonapi.org/format/#crud-updating-relationships.

I'm even wondering if the PATCH route should be generated at all?

@wwwdata
Copy link
Member

wwwdata commented Aug 27, 2016

Hm, indeed this is a problem. The main problem is that we just call the FindOne method of the resource which in this case most likely behaves like a normal request with embedded relations.
Currently this could be avoided by checking the url path in the request which should include the url part relationships but a more dedicated resource method to fetch relationships would be better, right?

We did it in this way at first to just simplify the methods needed to be implemented for CRUD and make this relationship route to be automatically available. But also in terms of database optimizations, this approach is not very good. I would suggest to optionally add another method via interface, check for this and then call this dedicated resource method to only fetch the specific relationship data which then also can be done in a different database query. Would you like to work on this?

@256dpi
Copy link
Contributor Author

256dpi commented Aug 27, 2016

Yes, a method on the resource that is responsible for fetching such relations would be the way to go.

I'll look into submitting a PR for fixing the bug first.

@wwwdata wwwdata added this to the Release 1.0 milestone Aug 29, 2016
@sharpner sharpner added the bug label Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants