-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Comments
👍 |
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. |
@vasilakisfil It does not add that much memory though, does it? If I'm not wrong it's just a reference. |
@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 ? |
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. |
I d'like to work on this. Several questions:
|
@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. |
@tchak I'm thinking Yes, it should have url helpers. Part of the purpose of this object is to expose a defined interface for |
@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 |
context
option to request_context
and encapsulate itcontext
option to serialization_context
and encapsulate it
@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. |
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. |
@vasilakisfil when you get something implemented, if you could post your findings / maybe even a documentation PR, that would be fantastic. :-) |
context
option to serialization_context
and encapsulate it
following: #1268 (comment)
options[:context]
in the controller etc. tooptions[:serialization_context]
rename context to serialization_context and add url helpers #1289serialization_context
rename context to serialization_context and add url helpers #1289SerializationContext
object that wraps therequest
and exposesoriginal_url
andquery_parameters
rename context to serialization_context and add url helpers #1289media_type
andapi_version
andurl_helpers
The text was updated successfully, but these errors were encountered: