From a7296e8a929016cd6f0c57c1cd015b327074d055 Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Sun, 29 May 2016 16:21:45 -0500 Subject: [PATCH 1/3] Fix #1759, Grape integration, adds serialization_context - 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?`. --- CHANGELOG.md | 2 ++ .../adapter/json_api/pagination_links.rb | 9 ++++- .../serialization_context.rb | 13 ++++++-- lib/grape/helpers/active_model_serializers.rb | 9 +++++ .../serialization_context_test_isolated.rb | 33 +++++++++++++------ .../adapter/json_api/pagination_links_test.rb | 11 +++++++ 6 files changed, 63 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a40d777b..9975f6f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ Features: - [#1426](https://github.com/rails-api/active_model_serializers/pull/1426) Add ActiveModelSerializers.config.default_includes (@empact) Fixes: +- [#1754](https://github.com/rails-api/active_model_serializers/pull/1754) Fixes #1759, Grape integration, improves serialization_context + missing error message on pagination. Document overriding CollectionSerializer#paginated?. (@bf4) - [#1287](https://github.com/rails-api/active_model_serializers/pull/1287) Pass `fields` options from adapter to serializer. (@vasilakisfil) - [#1710](https://github.com/rails-api/active_model_serializers/pull/1710) Prevent association loading when `include_data` option is set to `false`. (@groyoh) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index a07de69a..b15f5ba6 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -2,6 +2,7 @@ module ActiveModelSerializers module Adapter class JsonApi < Base class PaginationLinks + MissingSerializationContextError = Class.new(KeyError) FIRST_PAGE = 1 attr_reader :collection, :context @@ -9,7 +10,13 @@ module ActiveModelSerializers 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, <<-EOF.freeze + JsonApi::PaginationLinks requires a ActiveModelSerializers::SerializationContext. + Please pass a ':serialization_context' option or + override CollectionSerializer#paginated? to return 'false'. + EOF + end end def as_json diff --git a/lib/active_model_serializers/serialization_context.rb b/lib/active_model_serializers/serialization_context.rb index c61a43b9..9ef604f2 100644 --- a/lib/active_model_serializers/serialization_context.rb +++ b/lib/active_model_serializers/serialization_context.rb @@ -1,3 +1,4 @@ +require 'active_support/core_ext/array/extract_options' module ActiveModelSerializers class SerializationContext class << self @@ -22,9 +23,15 @@ module ActiveModelSerializers attr_reader :request_url, :query_parameters, :key_transform - def initialize(request, options = {}) - @request_url = request.original_url[/\A[^?]+/] - @query_parameters = request.query_parameters + def initialize(*args) + options = args.extract_options! + if args.size == 1 + request = args.pop + options[:request_url] = request.original_url[/\A[^?]+/] + options[:query_parameters] = request.query_parameters + end + @request_url = options.delete(:request_url) + @query_parameters = options.delete(:query_parameters) @url_helpers = options.delete(:url_helpers) || self.class.url_helpers @default_url_options = options.delete(:default_url_options) || self.class.default_url_options end diff --git a/lib/grape/helpers/active_model_serializers.rb b/lib/grape/helpers/active_model_serializers.rb index 370c52d8..baaa166d 100644 --- a/lib/grape/helpers/active_model_serializers.rb +++ b/lib/grape/helpers/active_model_serializers.rb @@ -1,4 +1,7 @@ # Helpers can be included in your Grape endpoint as: helpers Grape::Helpers::ActiveModelSerializers + +require 'active_model_serializers/serialization_context' + module Grape module Helpers module ActiveModelSerializers @@ -8,6 +11,12 @@ module Grape # # 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[^?]+/], + query_parameters: request.params + ) + end env[:active_model_serializer_options] = active_model_serializer_options resource end diff --git a/test/active_model_serializers/serialization_context_test_isolated.rb b/test/active_model_serializers/serialization_context_test_isolated.rb index 981d8075..5720e84a 100644 --- a/test/active_model_serializers/serialization_context_test_isolated.rb +++ b/test/active_model_serializers/serialization_context_test_isolated.rb @@ -5,13 +5,19 @@ require 'minitest/mock' class SerializationContextTest < ActiveSupport::TestCase include ActiveSupport::Testing::Isolation - def create_request - request = Minitest::Mock.new - request.expect(:original_url, 'original_url') - request.expect(:query_parameters, 'query_parameters') - end - class WithRails < SerializationContextTest + def create_request + request = ActionDispatch::Request.new({}) + def request.original_url + 'http://example.com/articles?page=2' + end + + def request.query_parameters + { 'page' => 2 } + end + request + end + setup do require 'rails' require 'active_model_serializers' @@ -20,8 +26,8 @@ class SerializationContextTest < ActiveSupport::TestCase end test 'create context with request url and query parameters' do - assert_equal @context.request_url, 'original_url' - assert_equal @context.query_parameters, 'query_parameters' + assert_equal @context.request_url, 'http://example.com/articles' + assert_equal @context.query_parameters, 'page' => 2 end test 'url_helpers is set up for Rails url_helpers' do @@ -36,14 +42,21 @@ class SerializationContextTest < ActiveSupport::TestCase end class WithoutRails < SerializationContextTest + def create_request + { + request_url: 'http://example.com/articles', + query_parameters: { 'page' => 2 } + } + end + setup do require 'active_model_serializers/serialization_context' @context = ActiveModelSerializers::SerializationContext.new(create_request) end test 'create context with request url and query parameters' do - assert_equal @context.request_url, 'original_url' - assert_equal @context.query_parameters, 'query_parameters' + assert_equal @context.request_url, 'http://example.com/articles' + assert_equal @context.query_parameters, 'page' => 2 end test 'url_helpers is a module when Rails is not present' do diff --git a/test/adapter/json_api/pagination_links_test.rb b/test/adapter/json_api/pagination_links_test.rb index 071956ea..46c6b56a 100644 --- a/test/adapter/json_api/pagination_links_test.rb +++ b/test/adapter/json_api/pagination_links_test.rb @@ -161,6 +161,17 @@ module ActiveModelSerializers assert_equal expected_response_without_pagination_links, adapter.serializable_hash end + + def test_raises_descriptive_error_when_serialization_context_unset + render_options = { adapter: :json_api } + adapter = serializable(using_kaminari, render_options) + exception = assert_raises do + adapter.as_json + end + exception_class = ActiveModelSerializers::Adapter::JsonApi::PaginationLinks::MissingSerializationContextError + assert_equal exception_class, exception.class + assert_match(/CollectionSerializer#paginated\?/, exception.message) + end end end end From 580492282f4c771f53e15877bc9afae9f8e2e02d Mon Sep 17 00:00:00 2001 From: Onome Date: Thu, 9 Jun 2016 00:05:44 -0500 Subject: [PATCH 2/3] Fix #1759, Grape integration, adds serialization_context (#4) * Fix #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. --- CHANGELOG.md | 2 + .../formatters/active_model_serializers.rb | 21 +++- lib/grape/helpers/active_model_serializers.rb | 8 -- test/grape_test.rb | 95 ++++++++++++++++++- 4 files changed, 114 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9975f6f0..2b3e680a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ Features: Fixes: - [#1754](https://github.com/rails-api/active_model_serializers/pull/1754) Fixes #1759, Grape integration, improves serialization_context missing error message on pagination. Document overriding CollectionSerializer#paginated?. (@bf4) + Moved serialization_context creation to Grape formatter, so resource serialization works without explicit calls to the `render` helper method. + Added Grape collection tests. (@onomated) - [#1287](https://github.com/rails-api/active_model_serializers/pull/1287) Pass `fields` options from adapter to serializer. (@vasilakisfil) - [#1710](https://github.com/rails-api/active_model_serializers/pull/1710) Prevent association loading when `include_data` option is set to `false`. (@groyoh) diff --git a/lib/grape/formatters/active_model_serializers.rb b/lib/grape/formatters/active_model_serializers.rb index 20537e74..534c5bab 100644 --- a/lib/grape/formatters/active_model_serializers.rb +++ b/lib/grape/formatters/active_model_serializers.rb @@ -2,14 +2,31 @@ # # Serializer options can be passed as a hash from your Grape endpoint using env[:active_model_serializer_options], # or better yet user the render helper in Grape::Helpers::ActiveModelSerializers + +require 'active_model_serializers/serialization_context' + module Grape module Formatters module ActiveModelSerializers def self.call(resource, env) - serializer_options = {} - serializer_options.merge!(env[:active_model_serializer_options]) if env[:active_model_serializer_options] + serializer_options = build_serializer_options(env) ::ActiveModelSerializers::SerializableResource.new(resource, serializer_options).to_json end + + def self.build_serializer_options(env) + ams_options = env[:active_model_serializer_options] || {} + + # Add serialization context + ams_options.fetch(:serialization_context) do + request = env['grape.request'] + ams_options[:serialization_context] = ::ActiveModelSerializers::SerializationContext.new( + request_url: request.url[/\A[^?]+/], + query_parameters: request.params + ) + end + + ams_options + end end end end diff --git a/lib/grape/helpers/active_model_serializers.rb b/lib/grape/helpers/active_model_serializers.rb index baaa166d..afbdab85 100644 --- a/lib/grape/helpers/active_model_serializers.rb +++ b/lib/grape/helpers/active_model_serializers.rb @@ -1,7 +1,5 @@ # Helpers can be included in your Grape endpoint as: helpers Grape::Helpers::ActiveModelSerializers -require 'active_model_serializers/serialization_context' - module Grape module Helpers module ActiveModelSerializers @@ -11,12 +9,6 @@ module Grape # # 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[^?]+/], - query_parameters: request.params - ) - end env[:active_model_serializer_options] = active_model_serializer_options resource end diff --git a/test/grape_test.rb b/test/grape_test.rb index 8ba8d6ab..91753704 100644 --- a/test/grape_test.rb +++ b/test/grape_test.rb @@ -1,6 +1,9 @@ require 'test_helper' require 'grape' require 'grape/active_model_serializers' +require 'kaminari' +require 'kaminari/hooks' +::Kaminari::Hooks.init class ActiveModelSerializers::GrapeTest < ActiveSupport::TestCase include Rack::Test::Methods @@ -21,6 +24,30 @@ class ActiveModelSerializers::GrapeTest < ActiveSupport::TestCase ARModels::Post.all end end + + def self.reset_all + ARModels::Post.delete_all + @all = nil + end + + def self.collection_per + 2 + end + + def self.collection + @collection ||= + begin + Kaminari.paginate_array( + [ + Profile.new(id: 1, name: 'Name 1', description: 'Description 1', comments: 'Comments 1'), + Profile.new(id: 2, name: 'Name 2', description: 'Description 2', comments: 'Comments 2'), + Profile.new(id: 3, name: 'Name 3', description: 'Description 3', comments: 'Comments 3'), + Profile.new(id: 4, name: 'Name 4', description: 'Description 4', comments: 'Comments 4'), + Profile.new(id: 5, name: 'Name 5', description: 'Description 5', comments: 'Comments 5') + ] + ).page(1).per(collection_per) + end + end end class GrapeTest < Grape::API @@ -41,11 +68,28 @@ class ActiveModelSerializers::GrapeTest < ActiveSupport::TestCase posts = Models.all render posts, adapter: :json_api end + + get '/render_collection_with_json_api' do + posts = Models.collection + render posts, adapter: :json_api + end + + get '/render_with_implicit_formatter' do + Models.model1 + end + + get '/render_array_with_implicit_formatter' do + Models.all + end + + get '/render_collection_with_implicit_formatter' do + Models.collection + end end end def app - GrapeTest.new + Grape::Middleware::Globals.new(GrapeTest.new) end def test_formatter_returns_json @@ -77,6 +121,53 @@ class ActiveModelSerializers::GrapeTest < ActiveSupport::TestCase assert last_response.ok? assert_equal serializable_resource.to_json, last_response.body ensure - ARModels::Post.delete_all + Models.reset_all + end + + def test_formatter_handles_collections + get '/grape/render_collection_with_json_api' + assert last_response.ok? + + representation = JSON.parse(last_response.body) + assert representation.include?('data') + assert representation['data'].count == Models.collection_per + assert representation.include?('links') + assert representation['links'].count > 0 + end + + def test_implicit_formatter + ActiveModel::Serializer.config.adapter = :json_api + get '/grape/render_with_implicit_formatter' + + post = Models.model1 + serializable_resource = serializable(post, adapter: :json_api) + + assert last_response.ok? + assert_equal serializable_resource.to_json, last_response.body + end + + def test_implicit_formatter_handles_arrays + ActiveModel::Serializer.config.adapter = :json_api + get '/grape/render_array_with_implicit_formatter' + + posts = Models.all + serializable_resource = serializable(posts, adapter: :json_api) + + assert last_response.ok? + assert_equal serializable_resource.to_json, last_response.body + ensure + Models.reset_all + end + + def test_implicit_formatter_handles_collections + ActiveModel::Serializer.config.adapter = :json_api + get '/grape/render_collection_with_implicit_formatter' + assert last_response.ok? + + representation = JSON.parse(last_response.body) + assert representation.include?('data') + assert representation['data'].count == Models.collection_per + assert representation.include?('links') + assert representation['links'].count > 0 end end From 1a9c62215a26359aa0785932ada32cd70eb64d3e Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 9 Jun 2016 01:29:59 -0500 Subject: [PATCH 3/3] Use with_adapter(&block) to fix failing tests I missed that the code was changing global state. --- test/grape_test.rb | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/test/grape_test.rb b/test/grape_test.rb index 91753704..2fa91806 100644 --- a/test/grape_test.rb +++ b/test/grape_test.rb @@ -136,23 +136,25 @@ class ActiveModelSerializers::GrapeTest < ActiveSupport::TestCase end def test_implicit_formatter - ActiveModel::Serializer.config.adapter = :json_api - get '/grape/render_with_implicit_formatter' - post = Models.model1 serializable_resource = serializable(post, adapter: :json_api) + with_adapter :json_api do + get '/grape/render_with_implicit_formatter' + end + assert last_response.ok? assert_equal serializable_resource.to_json, last_response.body end def test_implicit_formatter_handles_arrays - ActiveModel::Serializer.config.adapter = :json_api - get '/grape/render_array_with_implicit_formatter' - posts = Models.all serializable_resource = serializable(posts, adapter: :json_api) + with_adapter :json_api do + get '/grape/render_array_with_implicit_formatter' + end + assert last_response.ok? assert_equal serializable_resource.to_json, last_response.body ensure @@ -160,11 +162,12 @@ class ActiveModelSerializers::GrapeTest < ActiveSupport::TestCase end def test_implicit_formatter_handles_collections - ActiveModel::Serializer.config.adapter = :json_api - get '/grape/render_collection_with_implicit_formatter' - assert last_response.ok? + with_adapter :json_api do + get '/grape/render_collection_with_implicit_formatter' + end representation = JSON.parse(last_response.body) + assert last_response.ok? assert representation.include?('data') assert representation['data'].count == Models.collection_per assert representation.include?('links')