Always return "included" where "include" has been requested (v1.1) #35

Activity

1. danivek commented on Nov 24, 2017

   

   danivek

   on Nov 24, 2017

   Owner

   More actions

   @noetix, great addition 👍

   But actually, I can't know if "include" parameter has been requested or not regarding data (Maybe I'm missing something).

   Maybe 2 possible ways:

   1. Adding a parameter 'include': true to extra data on serialize, so the library has the knowledge to generate the right result.

    Serializer.serialize('article', data, { 'include': true, ... });
   

   2. Doing it outside the library, because it not depends on the request but only on the data.

   const result = Serializer.serialize('article', data);
   result.included = (req.params.include && !result.included) ? [] : result.included;
   

   @noetix @Mattii Any other ideas/suggestions ?
2. noetix commented on Nov 25, 2017

   

   noetix

   on Nov 25, 2017

   Author

   More actions

   You're right, this is tough when you're decoupled from the incoming request.

   Part of the spec is also to only include requested includes. The current implementation would return any includes in your serialized data.

   With that in mind, option 1 with a minor tweak would be my preferred option. Instead of marking include as true, pass a list of resource types that should be included. That way the library can ignore included data that has not been requested.

   e.g. GET /articles?include=comments

   const Serializer = new JSONAPISerializer()
   Serializer.register('article', articleType)
   Serializer.register('author', authorType)
   Serializer.register('comment', commentType)
   
   Serializer.serialize('product', data, { 'include': ['comments'] })
   

   In this example, any relationship data found for author would not be included. Passing in request options would be easy. You could even allow for the configuration when registering type.
3. danivek commented on Nov 25, 2017

   

   danivek

   on Nov 25, 2017

   Last edited by danivek

   Owner

   More actions

   You're right, it's much better than true, but it could be more complicated if you have such deep path { 'include': ['comments.user.roles'] } to filter.

   I always thought that, for your exemple, any relationship data for author would never be populated before passing the data through serializer, so it would never appeared in included. I think that it's the responsability of any database or something else to filter the wanted data before responding it (there is fewer things to manipulate, filter, etc... best performance).

   Do you have such case of embedded author on article, even if it has not been requested before using the serializer ?

   I will investigate further on the possibility of such option. Do you know a library that can do such filtering ?

   Thanks !
4. mattiloh commented on Nov 25, 2017

   

   mattiloh

   on Nov 25, 2017

   Last edited by mattiloh

   Collaborator

   More actions

   Interesting addition, thanks for the report @noetix!
   The include: ['comments'] seems to be analoge to the possibility to whitelist attributes. But it is indeed quite complex, if we want to handle this for nested includes, too.

   E.g. if the database payload for include: ['articles.comments.author'] contains (for any reasons) articles.author, too, we can't just whitelist all author resources, but we have to be sure, that it contains authors of comments, only.

   The next problem is: relationship-names can differ from related resource types. E.g. an author relation is sometimes a generic resource of type user. So the whitelisting won't work with include: ['author'] (if we only want to filter the include array by resource-types).

   That said, I would favour to go with a simple include: true. That makes sure, that the serializer can be conform with the spec.
5. noetix commented on Nov 26, 2017

   

   noetix

   on Nov 26, 2017

   Author

   More actions

   Agreed.

   If its going to be highly complex to filter results that the database layer shouldn't be returning then the problem should stay with the database layer.

   👍 for include: true.
6. danivek commented on Nov 26, 2017

   

   danivek

   on Nov 26, 2017

   Owner

   More actions

   I made a quick try of whitelisting includes with such include: ['articles.comments.author'] option on branch include-filter-test

   Seems to be not as complex as it is.

   @noetix, could you make a try, and tell me if it corresponds to your needs ?
7. jamesdixon commented on Mar 29, 2018

   

   jamesdixon

   on Mar 29, 2018

   More actions

   @danivek any movement on this? There are definitely cases where I don't want to include the data, but would still like the relationship to be present.
8. EmirGluhbegovic commented on Jul 20, 2018

   

   EmirGluhbegovic

   on Jul 20, 2018

   Last edited by EmirGluhbegovic

   More actions

   @danivek Echoing @jamesdixon question
9. danivek commented on Jul 20, 2018

   

   danivek

   on Jul 20, 2018

   Owner

   More actions

   @EmirGluhbegovic, I need some feedbacks on branch include-filter-test

   could you make a try, and tell me if it corresponds to your needs ?
10. alechirsch commented on Aug 19, 2019

    

    alechirsch

    on Aug 19, 2019

    Contributor

    More actions

    @danivek I am also interested in this concept of whitelisting a chain of includes. Wondering what the status of this is.
11. dimitrivdp commented on Aug 3, 2020

    

    dimitrivdp

    on Aug 3, 2020

    More actions

    I have now very often in my code:

    const  result  = Serializer.serialize('comments', data, {});
    result.included = []
    res.send(result)
    
    

    I would love to write

    res.send(Serializer.serialize('comments', data, { included: false }))

    Just because it is less code. ;-)