Improvements to api_response for formatting and blank responses; remo…

…ve nil for api_response payload; better error handling; improved tests.

.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

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

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` /

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"

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

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
 

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

lib/rest_framework/serializers.rb

@@ -1,16 +1,13 @@
 class RESTFramework::BaseSerializer
-  attr_reader :errors
-
   def initialize(object: nil, controller: nil, **kwargs)
     @object = object
     @controller = controller
   end
 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:

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

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

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

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

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
 

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