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

SerializationContext: request url info, content type, query params, api version, namespace, etc #1269

Closed
7 tasks done
bf4 opened this issue Oct 14, 2015 · 13 comments
Closed
7 tasks done

Comments

@bf4
Copy link
Member

bf4 commented Oct 14, 2015

following: #1268 (comment)

@jsdir
Copy link

jsdir commented Oct 15, 2015

👍

@vasilakisfil
Copy link
Contributor

Btw why context is always passed down using default Rails render? I am using JSON adapter. Not an expert but I think it's useless to JSON adapter and adds memory for no reason.

@beauby
Copy link
Contributor

beauby commented Oct 20, 2015

@vasilakisfil It does not add that much memory though, does it? If I'm not wrong it's just a reference.

@vasilakisfil
Copy link
Contributor

@beauby I don't know how much memory it adds but it's a bit huge object. And not used at all (again I am using JSON adapter). I can always remove it by setting context: nil but not sure why it's added by default. Maybe have an option for that just like serializable_scope ?

@vasilakisfil
Copy link
Contributor

Just looked up and I really think context shouldn't be included at all :) Why we need it? The only reason I saw was to get the query_parameters. Getting the query_parameters in order to create the pagination links shouldn't be AMS responsibility. If you have a paginated object you can retrieve the current/previous/next page/per_page and size numbers and then create the links. Unless I am missing something.

@tchak
Copy link
Contributor

tchak commented Oct 21, 2015

I d'like to work on this. Several questions:

@tchak
Copy link
Contributor

tchak commented Oct 21, 2015

@vasilakisfil for top level pagination, yes in theory you could provide links from the controller via links option, but in case of relationship level pagination, things are not that simple. You want to build links in the context of the association, it's parent and you need access to request context at the same time.

@bf4
Copy link
Member Author

bf4 commented Oct 21, 2015

@tchak I'm thinking serialization_context is the superior name (disclaimer: I made the suggestion :)

Yes, it should have url helpers.

Part of the purpose of this object is to expose a defined interface for serialization_context (url_helper, request_url, query_params) without any danger of the encapsulated object(s) being used elsewhere not-as-intended. i.e. if we passed around the controller instance, we'd be inclined to use it everywhere (to the great dismay of grape users and general separation of concerns), but with an object, we are forced to decide what the serialization_context can do

@bf4
Copy link
Member Author

bf4 commented Oct 21, 2015

@tchak Just start working on it. You can probably do the bare minimum in one small commit that renames the option and sets it to a SerializationContext object instead of controller.request

@bf4 bf4 changed the title Rename context option to request_context and encapsulate it Rename context option to serialization_context and encapsulate it Oct 21, 2015
@tchak
Copy link
Contributor

tchak commented Oct 21, 2015

@bf4 my initial work is in #1289

@vasilakisfil
Copy link
Contributor

@tchak have you or anyone else here ever worked on APIs that serve tons of resources using json-api from AMS branch 0.10 ? because then you really want to inject your own pagination. Kaminari or will_paginate work really great but if you need performance optimizations the way these do pagination is not good at all. There have been many threads about it on Kaminari about changing the way it counts the rows on SQL and get an estimation instead.

Or to take it an extra step, usually you don't even need (and you shouldn't if you want to optimize on a db with millions of rows) the last page (or total_count/total_pages in non-hypermedia api).

So 👎 from me once more.

@vasilakisfil
Copy link
Contributor

hmm now that I took a look on the code, the implementation is not tight to will_paginate or kaminari so I could include my own pagination module on ActiveRecord::Relation. So it's 👍 but we could explain which methods are needed on the wiki.

@NullVoxPopuli
Copy link
Contributor

@vasilakisfil when you get something implemented, if you could post your findings / maybe even a documentation PR, that would be fantastic. :-)

@bf4 bf4 changed the title Rename context option to serialization_context and encapsulate it SerializationContext: request url info, content type, query params, api version, namespace, etc Jan 15, 2016
@bf4 bf4 added this to the 0.10 milestone Feb 23, 2016
@remear remear closed this as completed Mar 17, 2016
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

7 participants