-
Notifications
You must be signed in to change notification settings - Fork 96
sparse fieldsets filtering support #209
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
Conversation
332f882 to
bbd274c
Compare
f7adcc7 to
d577942
Compare
|
neat. What happens if I use an invalid field? The documentation does not specify behaviour here... |
|
good point. I really don't know in the current implementation, nothing happens but we could also return an error json |
|
Since the documentation says |
|
hm, let's see what they respond to it i am curious 😄 |
|
Nothing helpful. Wanna go with an error? |
|
ok, i will check all query parameters and if something is wrong return a 400, bad request error with all fields that are not in the marshalled structs. I like this better anyways |
|
me too - thank you! |
|
So using this PR on a fork right now and it works awesome. We do some security filtering on our resources. If you own the resource you can see more fields than if you do not. This presents a problem for us, since a FindAll/FindPaginated response, in some cases, can have resources owned by the requestor and not owned by the requestor. I probably sound like an interface nut, but besides the |
|
hm, yes @interlock that makes sense. We can add another method to the resource structs that just returns an So we need to do it this way: If there are fields specified via the new method and there are fields specified by the user via the query parameter, we return the intersection of both of them, right? If the user requests a field that either isn't there, or is forbidden for him, because it was filtered out with the new method, we will return an error and pretend that this field never existed, because for that specific user, the api2go resource looks different with lesser fields. |
|
Sounds good to me! |
|
That helps skipping editing the Request.Url.RawQuery we are currently doing. Will that implementation request which fields to show for each marshalled resource on the same request? This is really only important for We can stash what we need to make that work in the |
|
hm you are right. So a method on the struct that will be marshalled is not very helpful because you define the fields in a CRUD method like What other options do we have? I guess we could also put it in the @interlock you suggest to do a function that writes this in the |
|
That would simplify what we currently do, but does not solve filtering a model down based on our white_list of fields. Use caseA public API request to find users in your area: We have a white_list that only returns some information from each users profile. Protecting their phone number and email for example. If the requestors user resource appeared in the list, that record should actually contain all the information in that record. In effect, based on the ownership of the resource, a different white_list should be applied for fields we return. |
|
hm, that is a different thing. Do I understand this correctly that the following can happen:
This is more than a normal filter fields functionality described in the jsonapi spec, which intended to minimize the json payload and therefore speed up the request (as far as I understood the feature). We now want to define on a per-struct (record) basis which fields should be contained in the json. If that is the case, I would fall back to my initial suggestion, to define another method on the struct that get's marshalled, so in your use case the |
|
Another quick fix: Why don't you just define 2 structs. 1 Complete struct with all the fields, and another struct with a reduced fieldset. I guess it should work to also pass a different struct in the edit: Ok, that Idea pretty much sucks if you have a lot of relationships etc etc etc ... can this be solved with a composition of a basic struct + more fields? |
|
Actually lets say that white_list case is moot for now. Discussion with the team has determined we will be a little more strict with our filtering based on that filters the request had. Solves that problem for us and simplifies our security layer 😄 |
|
ok, for this PR i will just implement another check and if there is an invalid field(s) in the query string, output an error(s). |
|
👍 |
d577942 to
eb7372f
Compare
883a614 to
7ee0a7b
Compare
7ee0a7b to
062b207
Compare
api_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the code namespaced with api2go? We can not know if a user already has a INVALID_FIELD_QUERY_PARAM :p
062b207 to
e0b7252
Compare
if the appropriate query parameters are available. The api goes through all the attributes and only includes the ones from the query parameter. For example: `?fields[posts]=title,content` If there are invalid fields in the query, errors with all of them will be returned
e0b7252 to
39eea9c
Compare
sparse fieldsets filtering support
if the appropriate query parameters are available. The api goes through all
the attributes and only includes the ones from the query parameter. For
example:
?fields[posts]=title,contentThis is pretty ugly because of our internal data structure, that hopefully will be better sometime, see #202
It's already working but I now noticed I forgot to implement it for slices of data when a
FindAllquery get's called for example.TODO:
This will resolve #183