Fix #1759, Grape integration, adds serialization_context #1754

bf4 · 2016-05-29T21:21:59Z

improves improves serialization_context to take options and not depend
on a request object.
adds descriptive error on missing serialization_context.
Document overriding CollectionSerializer#paginated?.

beauby · 2016-05-30T07:54:48Z

lib/active_model_serializers/adapter/json_api.rb

@@ -505,6 +505,7 @@ def links_for(serializer)
      # prs:
      #   https://github.com/rails-api/active_model_serializers/pull/1041
      def pagination_links_for(serializer)
+        return {} unless instance_options.key?(:serialization_context)


Why not put this test there?

ref:

if is_collection && serializer.paginated? hash[:links] ||= {} hash[:links].update(pagination_links_for(serializer)) end

I did at first, but then I was thinking it's really tightly coupled to the paginationlinks object and it's intent isn't as obvious in the conditional.

But really we shouldn't let an empty serialization_context get in that far. This is a quick fix, but the better fix is at the boundary, I think

bf4 · 2016-05-31T03:51:53Z

I spent too much time on this. Any blockers or anyone want to make changes?

bf4 · 2016-05-31T03:55:22Z

cc @onomated @hut8 @Kosmin @johnhamelink

onomated · 2016-06-01T00:21:57Z

@bf4 Sorry for the delay. Just seeing this, was a holiday weekend. I'll take a look and report any changes back

groyoh · 2016-06-01T07:21:21Z

lib/active_model_serializers/adapter/json_api/pagination_links.rb

        FIRST_PAGE = 1

        attr_reader :collection, :context

        def initialize(collection, adapter_options)
          @collection = collection
          @adapter_options = adapter_options
-          @context = adapter_options.fetch(:serialization_context)
+          @context = adapter_options.fetch(:serialization_context) do
+            fail MissingSerializationContextError, 'JsonApi::PaginationLinks requires a'\


- fail MissingSerializationContextError, 'JsonApi::PaginationLinks requires a'\ + fail MissingSerializationContextError, 'JsonApi::PaginationLinks requires a '\

' is use for defining the string here whereas " is used below. Not sure if typo or if I'm missing something ;)

just because I have things in the others that need double quotes. rubocop caught it. changing to a heredoc

onomated · 2016-06-01T13:08:41Z

lib/grape/helpers/active_model_serializers.rb

@@ -8,6 +11,12 @@ module ActiveModelSerializers
      #
      # Example: To include pagination meta data: render(posts, meta: { page: posts.page, total_pages: posts.total_pages })
      def render(resource, active_model_serializer_options = {})
+        active_model_serializer_options.fetch(:serialization_context) do
+          active_model_serializer_options[:serialization_context] = ::ActiveModelSerializers::SerializationContext.new(
+            original_url: request.url[/\A[^?]+/],


original_url option should be request_url

oops :)

Also 👍 to making a PR to my branch and moving this code to the formatter (and adding yourself in the changelog!) #1754 (comment)

I also wasn't able to duplicate the failure in the grape tests, so if you could do that, that would be fantastic!

(I don't use grape)

onomated · 2016-06-01T13:22:15Z

So the current solution works when an explicit render call is made on a resource. In generally, grape formatters should render the resulting resource without options. For example,

resource :statuses do
      desc 'Return a public timeline.'
      get :public_timeline do
        Status.limit(20)
      end
end

returns the MissingSerializationContextError error since there wasn't an explicit render call. This works if Status.limit(20) is wrapped in a render as render(Status.limit(20)).
Moving the creation of the serialization context to the formatter will fix this, and allow including other AMS options such the serialization scope. As done in the grape_active-model-serializers gem formatter.
I can take a stab at addressing this if you give me write access @bf4, or I can create a new PR on this branch

bf4 · 2016-06-01T15:03:36Z

@onomated if you would make a PR into my branch, that would be fantastic! I didn't check if I had all the objects I need in the formatter

bf4 · 2016-06-09T05:07:12Z

@onomated I just merged in your PR and will finish this up. Great work 💯

beauby · 2016-06-09T09:03:53Z

lib/active_model_serializers/adapter/json_api/pagination_links.rb

@@ -2,14 +2,21 @@ module ActiveModelSerializers
  module Adapter
    class JsonApi < Base
      class PaginationLinks
+        MissingSerializationContextError = Class.new(KeyError)


Should this be defined within PaginationLinks? It seems like it's more general than that.

I'm happy to put a guard wherever it's to be used... Can't think of a better spot...

beauby · 2016-06-09T09:06:26Z

See my 2 remarks. LGTM otherwise.

bf4 · 2016-06-14T02:26:49Z

@onomated bf4#4 (comment) so is this good for you?

onomated · 2016-06-14T09:27:13Z

@bf4 couldn't quite resolve the comment link in the @ mention above, but on the general topic of pagination, it'll be awesome if this was configurable like so paginate: false as opposed to serialization_context: nil. I personally am in the party of not caring about pagination in the json-api rendition. I currently rely on my internal header pagination implemented a while back. With the fixes in this PR, Grape support has pagination support out the box so it just works for new projects but currently no way to turn this off if its not desired.

- improves improves serialization_context to take options and not depend on a `request` object. - adds descriptive error on missing serialization_context. - Document overriding `CollectionSerializer#paginated?`.

* Fix rails-api#1759, Grape integration, adds serialization_context - `serialization_context` is added in grape formatter so grape continues to render models without an explicit call to the `render` helper method - Made it straightforward for subclasses to add other serializer options (such as `serialization_scope`). * Updated Grape tests to include: - paginated collections - implicit Grape serializer (i.e. without explicit invocation of `render` helper method) * Update Changelog with fixes.

I missed that the code was changing global state.

bf4 · 2016-06-14T14:46:45Z

@onomated I just rebased this off of master. Can you confirm it works for you before merge?

onomated · 2016-06-14T14:47:53Z

@bf4 sure thing. Checking out now

onomated · 2016-06-14T14:54:15Z

@bf4 All good! 👍 💯

xn · 2016-06-15T20:09:58Z

Oh snap! Thanks, this is awesome!

bf4 · 2016-06-15T20:20:22Z

Thanks @onomated for your work here. This was actually a blocker for us for releasing 0.10.1

xn · 2016-06-22T19:01:27Z

Just want to leave this here in case someone gets stuck with an error like: NoMethodError: undefined method 'url' for nil:NilClass.

This formatter now depends on Grape::Middleware::Globals
You can get that going like:

module MyApi
  class ApiBase < Grape::API
    use Grape::Middleware::Globals
    require 'grape/active_model_serializers'
    include Grape::ActiveModelSerializers
    mount MyApi::V1::ApiBase

or minimally:

class API < Grape::API
  # @note Make sure this is above you're first +mount+
  use Grape::Middleware::Globals
end

onomated · 2016-06-22T20:14:52Z

@xn, I didn't encounter this as I use some other middleware that inherits from grape globals. Should this be mixed in as part of include Grape::ActiveModelSerializers?

xn · 2016-06-22T20:20:51Z

Well, if you just add it to the docs, then if someone wants/needs to monkey with Grape::Middleware::Globals or write their own for whatever reasons, they are free to do so without having to monkey with this formatter as well.

bf4 · 2016-06-22T21:35:58Z

PRs accepted :)

xn · 2016-06-23T02:15:34Z

@bf4 I'm happy to roll this into a documentation PR. Though I think it is better to allow the developer to meet the dependency, it is a question that should be answered by someone who has a stake in the project's maintainance/architecture.

bf4 · 2016-06-23T02:42:42Z

Not sure I follow. Do you think there is a problem in AMS impl or just docs needed?

B mobile phone

On Jun 22, 2016, at 9:15 PM, Christian Trosclair notifications@github.com wrote:

@bf4 I'm happy to roll this into a documentation PR. Though I think it is better to allow the developer to meet the dependency, it is a question that should be answered by someone who has a stake in the project's maintainance/architecture.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

xn · 2016-06-23T03:16:58Z

AMS is kindly creating a grape formatter. Said formatter is depending on a grape middleware that is not enabled by default. You have a choice to automatically meet the dependency by hardcoding that dependency in the formatter itself or document the dependency to let the developer meet it on own their own terms.

If I am being too circuitous, I apologize. Let me know and I will tell you what I believe should be done.

bf4 · 2016-06-23T03:21:25Z

I don't use grape, so I don't know what's right for you

xn · 2016-06-23T03:26:42Z

Ok, from basic OO principles you should let the developer meet the dependency. So, I'll create a PR for documentation to meet this in the README.

xn · 2016-06-23T03:41:51Z

#1818

beauby reviewed May 30, 2016
View reviewed changes

bf4 added S: Bug V: 0.10.x S: Changelog Missing labels May 31, 2016

bf4 force-pushed the handle_missing_serialization_context_when_paginating branch from 9b77f0a to 6972c22 Compare May 31, 2016 03:50

bf4 changed the title ~~Skip rendering JSONAPI pagination links when no serialization context~~ Fix #1759, Grape integration, adds serialization_context May 31, 2016

bf4 removed the S: Changelog Missing label May 31, 2016

bf4 force-pushed the handle_missing_serialization_context_when_paginating branch from 6972c22 to ac67827 Compare May 31, 2016 03:54

groyoh added the S: LGTM label May 31, 2016

bf4 force-pushed the handle_missing_serialization_context_when_paginating branch from ac67827 to f685c8c Compare May 31, 2016 21:32

groyoh reviewed Jun 1, 2016
View reviewed changes

bf4 force-pushed the handle_missing_serialization_context_when_paginating branch 2 times, most recently from bfa1e35 to 159c529 Compare June 1, 2016 07:45

onomated reviewed Jun 1, 2016
View reviewed changes

groyoh removed the S: LGTM label Jun 1, 2016

bf4 force-pushed the handle_missing_serialization_context_when_paginating branch from 1a8ccef to d2b761a Compare June 9, 2016 06:30

beauby reviewed Jun 9, 2016
View reviewed changes

bf4 and others added 2 commits June 14, 2016 09:45

Fix rails-api#1759, Grape integration, adds serialization_context

a7296e8

- improves improves serialization_context to take options and not depend on a `request` object. - adds descriptive error on missing serialization_context. - Document overriding `CollectionSerializer#paginated?`.

Use with_adapter(&block) to fix failing tests

1a9c622

I missed that the code was changing global state.

bf4 force-pushed the handle_missing_serialization_context_when_paginating branch from d2b761a to 1a9c622 Compare June 14, 2016 14:46

bf4 merged commit 0fed486 into rails-api:master Jun 15, 2016

bf4 deleted the handle_missing_serialization_context_when_paginating branch June 15, 2016 20:19

Fix #1759, Grape integration, adds serialization_context #1754

Fix #1759, Grape integration, adds serialization_context #1754

Conversation

bf4 commented May 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented May 31, 2016

bf4 commented May 31, 2016 • edited

onomated commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onomated commented Jun 1, 2016 • edited

bf4 commented Jun 1, 2016

bf4 commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Jun 9, 2016

bf4 commented Jun 14, 2016

onomated commented Jun 14, 2016 • edited by bf4

bf4 commented Jun 14, 2016

onomated commented Jun 14, 2016

onomated commented Jun 14, 2016

xn commented Jun 15, 2016

bf4 commented Jun 15, 2016

xn commented Jun 22, 2016 • edited

onomated commented Jun 22, 2016

xn commented Jun 22, 2016

bf4 commented Jun 22, 2016

xn commented Jun 23, 2016

bf4 commented Jun 23, 2016

xn commented Jun 23, 2016 • edited

bf4 commented Jun 23, 2016

xn commented Jun 23, 2016

xn commented Jun 23, 2016

bf4 commented May 29, 2016 •

edited

bf4 commented May 31, 2016 •

edited

onomated commented Jun 1, 2016 •

edited

onomated commented Jun 14, 2016 •

edited by bf4

xn commented Jun 22, 2016 •

edited

xn commented Jun 23, 2016 •

edited