diff --git a/.travis.yml b/.travis.yml index 7f7a11b..f37fdf6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,7 +14,6 @@ rvm: - 3.0.0 jobs: allow_failures: - - rvm: 2.7.2 - rvm: 3.0.0 script: - bundle exec rake test diff --git a/Gemfile b/Gemfile index a105dd5..140f576 100644 --- a/Gemfile +++ b/Gemfile @@ -13,6 +13,7 @@ gem "rake", ">= 12.0" gem "minitest", ">= 5.0" gem "sqlite3", "~> #{rails_version_major <= 4 ? '1.3.0' : '1.4.0'}" gem "byebug" +gem "pry-rails" # Test Coverage gem 'coveralls', require: false diff --git a/docs/_guide/3_serializers.md b/docs/_guide/3_serializers.md index 32a0811..d82c76a 100644 --- a/docs/_guide/3_serializers.md +++ b/docs/_guide/3_serializers.md @@ -11,8 +11,8 @@ which can then be converted to JSON or XML. ## NativeSerializer -This serializer uses Rails' native `ActiveModel::Serializers::JSON#as_json` method. Despite the -name, this converts records/recordsets to Ruby primitives (`Array` and `Hash`), not JSON. +This serializer uses Rails' native `ActiveModel::Serializers.serializable_hash` method to convert +records/recordsets to Ruby primitives (`Array` and `Hash`). This is the default serializer, you can configure it using the controller class attributes `native_serializer_config` (or `native_serializer_singular_config` / diff --git a/lib/rest_framework.rb b/lib/rest_framework.rb index e80af8d..ab37e29 100644 --- a/lib/rest_framework.rb +++ b/lib/rest_framework.rb @@ -4,6 +4,7 @@ module RESTFramework require_relative "rest_framework/controller_mixins" require_relative "rest_framework/engine" +require_relative "rest_framework/errors" require_relative "rest_framework/filters" require_relative "rest_framework/paginators" require_relative "rest_framework/routers" diff --git a/lib/rest_framework/controller_mixins/base.rb b/lib/rest_framework/controller_mixins/base.rb index 80b18f6..be695ed 100644 --- a/lib/rest_framework/controller_mixins/base.rb +++ b/lib/rest_framework/controller_mixins/base.rb @@ -1,3 +1,4 @@ +require_relative '../errors' require_relative '../serializers' @@ -45,6 +46,8 @@ def self.included(base) page_size_query_param: 'page_size', max_page_size: nil, serializer_class: nil, + serialize_to_json: true, + serialize_to_xml: true, singleton_controller: nil, skip_actions: nil, }.each do |a, default| @@ -132,39 +135,42 @@ def _get_routes # Helper to render a browsable API for `html` format, along with basic `json`/`xml` formats, and # with support or passing custom `kwargs` to the underlying `render` calls. - def api_response(payload, html_kwargs: nil, json_kwargs: nil, xml_kwargs: nil, **kwargs) + def api_response(payload, html_kwargs: nil, **kwargs) html_kwargs ||= {} - json_kwargs ||= {} - xml_kwargs ||= {} - - # allow blank (no-content) responses - @blank = kwargs[:blank] + json_kwargs = kwargs.delete(:json_kwargs) || {} + xml_kwargs = kwargs.delete(:xml_kwargs) || {} + + # Raise helpful error if payload is nil. Usually this happens when a record is not found (e.g., + # when passing something like `User.find_by(id: some_id)` to `api_response`). The caller should + # actually be calling `find_by!` to raise ActiveRecord::RecordNotFound and allowing the REST + # framework to catch this error and return an appropriate error response. + if payload.nil? + raise RESTFramework::NilPassedToAPIResponseError + end respond_to do |format| - if @blank - format.json {head :no_content} - format.xml {head :no_content} + if payload == '' + format.json {head :no_content} if self.serialize_to_json + format.xml {head :no_content} if self.serialize_to_xml else - if payload.respond_to?(:to_json) - format.json { - jkwargs = kwargs.merge(json_kwargs) - render(json: payload, layout: false, **jkwargs) - } - end - if payload.respond_to?(:to_xml) - format.xml { - xkwargs = kwargs.merge(xml_kwargs) - render(xml: payload, layout: false, **xkwargs) - } - end + format.json { + jkwargs = kwargs.merge(json_kwargs) + render(json: payload, layout: false, **jkwargs) + } if self.serialize_to_json + format.xml { + xkwargs = kwargs.merge(xml_kwargs) + render(xml: payload, layout: false, **xkwargs) + } if self.serialize_to_xml + # TODO: possibly support more formats here if supported? end format.html { @payload = payload - @json_payload = '' - @xml_payload = '' - unless @blank - @json_payload = payload.to_json if payload.respond_to?(:to_json) - @xml_payload = payload.to_xml if payload.respond_to?(:to_xml) + if payload == '' + @json_payload = '' if self.serialize_to_json + @xml_payload = '' if self.serialize_to_xml + else + @json_payload = payload.to_json if self.serialize_to_json + @xml_payload = payload.to_xml if self.serialize_to_xml end @template_logo_text ||= "Rails REST Framework" @title ||= self.controller_name.camelize diff --git a/lib/rest_framework/controller_mixins/models.rb b/lib/rest_framework/controller_mixins/models.rb index 84d674d..82f174a 100644 --- a/lib/rest_framework/controller_mixins/models.rb +++ b/lib/rest_framework/controller_mixins/models.rb @@ -229,7 +229,7 @@ module RESTFramework::DestroyModelMixin def destroy @record = self.get_record @record.destroy! - api_response(nil) + api_response('') end end diff --git a/lib/rest_framework/errors.rb b/lib/rest_framework/errors.rb new file mode 100644 index 0000000..599d840 --- /dev/null +++ b/lib/rest_framework/errors.rb @@ -0,0 +1,26 @@ +# Top-level class for all REST Framework errors. +class RESTFramework::Error < StandardError +end + +class RESTFramework::NilPassedToAPIResponseError < RESTFramework::Error + def message + return <<~MSG.split("\n").join(' ') + Payload of `nil` was passed to `api_response`; this is unsupported. If you want a blank + response, pass `''` (an empty string) as the payload. If this was the result of a `find_by` + (or similar Active Record method) not finding a record, you should use the bang version (e.g., + `find_by!`) to raise `ActiveRecord::RecordNotFound`, which the REST controller will catch and + return an appropriate error response. + MSG + end +end + +class RESTFramework::UnserializableError < RESTFramework::Error + def initialize(object) + @object = object + return super + end + + def message + return "Unable to serialize `#{@object.inspect}` (of type `#{@object.class}`)." + end +end diff --git a/lib/rest_framework/serializers.rb b/lib/rest_framework/serializers.rb index 79308d5..df7a0aa 100644 --- a/lib/rest_framework/serializers.rb +++ b/lib/rest_framework/serializers.rb @@ -1,6 +1,4 @@ class RESTFramework::BaseSerializer - attr_reader :errors - def initialize(object: nil, controller: nil, **kwargs) @object = object @controller = controller @@ -8,9 +6,8 @@ def initialize(object: nil, controller: nil, **kwargs) end -# This serializer uses `.as_json` to serialize objects. Despite the name, `.as_json` is an -# `ActiveModel` method which converts objects to Ruby primitives (with the top-level being either -# an array or a hash). +# This serializer uses `.serializable_hash` to convert objects to Ruby primitives (with the +# top-level being either an array or a hash). class RESTFramework::NativeSerializer < RESTFramework::BaseSerializer class_attribute :config class_attribute :singular_config @@ -22,12 +19,16 @@ def initialize(many: nil, model: nil, **kwargs) if many.nil? # Determine if we are dealing with many objects or just one. - @many = @object.respond_to?(:count) && @object.respond_to?(:each) + @many = @object.is_a?(Enumerable) else @many = many end - @model = model || @controller.send(:get_model) + # Determine model either explicitly, or by inspecting @object or @controller. + @model = model + @model ||= @object.class if @object.is_a?(ActiveRecord::Base) + @model ||= @object[0].class if @many && @object[0].is_a?(ActiveRecord::Base) + @model ||= @controller.send(:get_model) if @controller end # Get controller action, if possible. @@ -67,7 +68,7 @@ def get_controller_native_serializer_config return controller_serializer || @controller.try(:native_serializer_config) end - # Get a configuration passable to `as_json` for the object. + # Get a configuration passable to `serializable_hash` for the object. def get_serializer_config # Return a locally defined serializer config if one is defined. if local_config = self.get_local_native_serializer_config @@ -80,7 +81,7 @@ def get_serializer_config end # If the config wasn't determined, build a serializer config from model fields. - fields = @controller.try(:get_fields) if @controller + fields = @controller.send(:get_fields) if @controller if fields if @model columns, methods = fields.partition { |f| f.in?(@model.column_names) } @@ -99,9 +100,17 @@ def get_serializer_config # Convert the object (record or recordset) to Ruby primitives. def serialize if @object - return @object.as_json(self.get_serializer_config) + begin + if @object.is_a?(Enumerable) + return @object.map { |r| r.serializable_hash(self.get_serializer_config) } + end + return @object.serializable_hash(self.get_serializer_config) + rescue NoMethodError + end end - return nil + + # Raise an error if we cannot serialize the object. + raise RESTFramework::UnserializableError.new(@object) end # Allow a serializer instance to be used as a hash directly in a nested serializer config. @@ -134,10 +143,17 @@ def self.[]=(key, value) end +# :nocov: # Alias NativeModelSerializer -> NativeSerializer. class RESTFramework::NativeModelSerializer < RESTFramework::NativeSerializer - def initialize + def initialize(**kwargs) super - ActiveSupport::Deprecation.new('1.0', 'rest_framework') + ActiveSupport::Deprecation.warn( + <<~MSG.split("\n").join(' ') + RESTFramework::NativeModelSerializer is deprecated and will be removed in future versions of + REST Framework; you should use RESTFramework::NativeSerializer instead. + MSG + ) end end +# :nocov: diff --git a/test/app/controllers/api2/root_controller.rb b/test/app/controllers/api2/root_controller.rb index 4d2de6a..50f1ef5 100644 --- a/test/app/controllers/api2/root_controller.rb +++ b/test/app/controllers/api2/root_controller.rb @@ -1,5 +1,5 @@ class Api2::RootController < Api2Controller - self.extra_actions = {nil: :get, blank: :get, truly_blank: :get} + self.extra_actions = {nil: :get, blank: :get} def root api_response({message: "Welcome to your custom API2 root!"}) @@ -12,8 +12,4 @@ def nil def blank api_response('') end - - def truly_blank - api_response('', blank: true) - end end diff --git a/test/app/controllers/api2/thing_controller.rb b/test/app/controllers/api2/thing_controller.rb index 3978dd2..e121d2e 100644 --- a/test/app/controllers/api2/thing_controller.rb +++ b/test/app/controllers/api2/thing_controller.rb @@ -3,6 +3,15 @@ class Api2::ThingController < Api2Controller self.fields = ['id', 'name'] self.singleton_controller = true + self.extra_actions = {changed_action: {methods: [:get], path: :changed}} + + # Custom action if the thing changed. + def changed_action + record = self.get_record + api_response({changed: record.updated_at}) + end + + protected def get_record return Thing.first diff --git a/test/test/controllers/api2/root_controller_test.rb b/test/test/controllers/api2/root_controller_test.rb new file mode 100644 index 0000000..5d264c9 --- /dev/null +++ b/test/test/controllers/api2/root_controller_test.rb @@ -0,0 +1,37 @@ +require_relative '../base' + + +class Api2::RootControllerTest < ActionController::TestCase + def test_nil_fails + assert_raise RESTFramework::NilPassedToAPIResponseError do + get :nil + end + end + + def test_nil_json_fails + assert_raise RESTFramework::NilPassedToAPIResponseError do + get :nil, as: :json + end + end + + def test_nil_xml_fails + assert_raise RESTFramework::NilPassedToAPIResponseError do + get :nil, as: :xml + end + end + + def test_blank + get :blank + assert_response :success + end + + def test_blank_json + get :blank, as: :json + assert_response :success + end + + def test_blank_xml + get :blank, as: :xml + assert_response :success + end +end diff --git a/test/test/controllers/api2/things_controller_test.rb b/test/test/controllers/api2/things_controller_test.rb index 14dc36b..0ad0f01 100644 --- a/test/test/controllers/api2/things_controller_test.rb +++ b/test/test/controllers/api2/things_controller_test.rb @@ -2,4 +2,10 @@ class Api2::ThingsControllerTest < ActionController::TestCase + def test_list + get :index, as: :json + assert_response :success + assert _parsed_body['results'][0]['name'] + assert_nil _parsed_body['results'][0]['shape'] + end end diff --git a/test/test/integration/api2_routing_test.rb b/test/test/integration/api2_routing_test.rb index 5d6548f..bf491c4 100644 --- a/test/test/integration/api2_routing_test.rb +++ b/test/test/integration/api2_routing_test.rb @@ -10,18 +10,10 @@ def test_can_get_root assert_response :success end - def test_can_get_nil_blank_truly_blank - get '/api2/nil' - assert_response :success - get '/api2/nil.json' - assert_response :success + def test_can_get_blank get '/api2/blank' assert_response :success get '/api2/blank.json' - assert_response :success - get '/api2/truly_blank' - assert_response :success - get '/api2/truly_blank.json' assert_response :no_content end diff --git a/test/test/test_helper.rb b/test/test/test_helper.rb index 3f491fe..7806164 100644 --- a/test/test/test_helper.rb +++ b/test/test/test_helper.rb @@ -47,10 +47,8 @@ class ActiveSupport::TestCase # Load all fixtures. fixtures :all - # Run tests in parallel for Rails >=6, but not when we need code coverage. - if Rails::VERSION::MAJOR >= 6 && !IS_MAIN_TRAVIS_ENV - parallelize(workers: :number_of_processors) - end + # Do not parallelize for now since Coveralls breaks with parallelization. + # parallelize(workers: :number_of_processors) # Helper to get parsed (json) body for both old and new Rails. def _parsed_body