Merge pull request #81 from tiagopog/improvement/record-count-refacto…

…ring

Improvement/record count refactoring

.travis.yml

@@ -6,5 +6,5 @@ rvm:
   - 2.3.3
 matrix:
 before_install: gem install bundler -v 1.13.6
-script: bundle exec rspec spec/controllers
+script: bundle exec rspec spec
 

lib/jsonapi/utils/response/formatters.rb

@@ -171,7 +171,7 @@ def turn_into_resource(record, options)
           end
         end
 
-        # Apply some result options like pagination params and count to a collection response.
+        # Apply some result options like pagination params and record count to collection responses.
         #
         # @param records [ActiveRecord::Relation, Hash, Array<Hash>]
         #   Object to be formatted into JSON
@@ -192,7 +192,7 @@ def result_options(records, options)
             end
 
             if JSONAPI.configuration.top_level_meta_include_record_count
-              data[:record_count] = count_records(records, options)
+              data[:record_count] = record_count_for(records, options)
             end
           end
         end

lib/jsonapi/utils/support/pagination.rb

@@ -2,6 +2,8 @@ module JSONAPI
   module Utils
     module Support
       module Pagination
+        RecordCountError = Class.new(ArgumentError)
+
         # Apply proper pagination to the records.
         #
         # @param records [ActiveRecord::Relation, Array] collection of records
@@ -33,7 +35,23 @@ def apply_pagination(records, options = {})
         # @api public
         def pagination_params(records, options)
           return {} unless JSONAPI.configuration.top_level_links_include_pagination
-          paginator.links_page_params(record_count: count_records(records, options))
+          paginator.links_page_params(record_count: record_count_for(records, options))
+        end
+
+        # Apply memoization to the record count result avoiding duplicate counts.
+        #
+        # @param records [ActiveRecord::Relation, Array] collection of records
+        #   e.g.: User.all or [{ id: 1, name: 'Tiago' }, { id: 2, name: 'Doug' }]
+        #
+        # @param options [Hash] JU's options
+        #   e.g.: { resource: V2::UserResource, count: 100 }
+        #
+        # @return [Integer]
+        #   e.g.: 42
+        #
+        # @api public
+        def record_count_for(records, options)
+          @record_count ||= count_records(records, options)
         end
 
         private
@@ -130,14 +148,49 @@ def pagination_range
         #
         # @api private
         def count_records(records, options)
-          if options[:count].present?
-            options[:count]
-          elsif records.is_a?(Array)
-            records.length
-          else
-            records = apply_filter(records, options) if params[:filter].present?
-            records.except(:group, :order).count("DISTINCT #{records.table.name}.id")
+          return options[:count].to_i if options[:count].is_a?(Numeric)
+
+          case records
+          when ActiveRecord::Relation then count_records_from_database(records, options)
+          when Array                  then records.length
+          else raise RecordCountError, "Can't count records with the given options"
+          end
+        end
+
+        # Count records from the datatase applying the given request filters
+        # and skipping things like eager loading, grouping and sorting.
+        #
+        # @param records [ActiveRecord::Relation, Array] collection of records
+        #   e.g.: User.all or [{ id: 1, name: 'Tiago' }, { id: 2, name: 'Doug' }]
+        #
+        # @param options [Hash] JU's options
+        #   e.g.: { resource: V2::UserResource, count: 100 }
+        #
+        # @return [Integer]
+        #   e.g.: 42
+        #
+        # @api private
+        def count_records_from_database(records, options)
+          records = apply_filter(records, options) if params[:filter].present?
+          count   = -> (records, except:) do
+            records.except(*except).count(distinct_count_sql(records))
           end
+          count.(records, except: %i(includes group order))
+        rescue ActiveRecord::StatementInvalid
+          count.(records, except: %i(group order))
+        end
+
+        # Build the SQL distinct count with some reflection on the "records" object.
+        #
+        # @param records [ActiveRecord::Relation] collection of records
+        #   e.g.: User.all
+        #
+        # @return [String]
+        #   e.g.: "DISTINCT users.id"
+        #
+        # @api private
+        def distinct_count_sql(records)
+          "DISTINCT #{records.table_name}.#{records.primary_key}"
         end
       end
     end

lib/jsonapi/utils/version.rb

@@ -1,5 +1,5 @@
 module JSONAPI
   module Utils
-    VERSION = '0.7.0'.freeze
+    VERSION = '0.7.1'.freeze
   end
 end

spec/controllers/posts_controller_spec.rb

@@ -1,4 +1,4 @@
-require 'spec_helper'
+require 'rails_helper'
 
 describe PostsController, type: :controller do
   include_context 'JSON API headers'

spec/controllers/profile_controller_spec.rb

@@ -1,4 +1,4 @@
-require 'spec_helper'
+require 'rails_helper'
 
 describe ProfileController, type: :controller do
   include_context 'JSON API headers'

spec/controllers/users_controller_spec.rb

@@ -1,4 +1,4 @@
-require 'spec_helper'
+require 'rails_helper'
 
 describe UsersController, type: :controller do
   include_context 'JSON API headers'

spec/features/record_count_spec.rb

@@ -0,0 +1,97 @@
+require 'rails_helper'
+
+##
+# Configs
+##
+
+# Resource
+class RecordCountTestResource < JSONAPI::Resource; end
+
+# Controller
+class RecordCountTestController < BaseController
+  def explicit_count
+    jsonapi_render json: User.all, options: { count: 42, resource: UserResource }
+  end
+
+  def array_count
+    jsonapi_render json: User.all.to_a, options: { resource: UserResource }
+  end
+
+  def active_record_count
+    jsonapi_render json: User.all, options: { resource: UserResource }
+  end
+
+  def active_record_count_with_eager_load
+    users = User.all.includes(:posts)
+    jsonapi_render json: users, options: { resource: UserResource }
+  end
+
+  def active_record_count_with_eager_load_and_where_clause
+    users = User.all.includes(:posts).where(posts: { id: Post.first.id })
+    jsonapi_render json: users, options: { resource: UserResource }
+  end
+end
+
+# Routes
+def TestApp.draw_record_count_test_routes
+  JSONAPI.configuration.json_key_format = :underscored_key
+
+  TestApp.routes.draw do
+    controller :record_count_test do
+      get :explicit_count
+      get :array_count
+      get :active_record_count
+      get :active_record_count_with_eager_load
+      get :active_record_count_with_eager_load_and_where_clause
+    end
+  end
+end
+
+##
+# Feature tests
+##
+
+describe RecordCountTestController, type: :controller do
+  include_context 'JSON API headers'
+
+  before(:all) do
+    TestApp.draw_record_count_test_routes
+    FactoryGirl.create_list(:user, 3, :with_posts)
+  end
+
+  describe 'explicit count' do
+    it 'returns the count based on the passed "options"' do
+      get :explicit_count
+      expect(response).to have_meta_record_count(42)
+    end
+  end
+
+  describe 'array count' do
+    it 'returns the count based on the array length' do
+      get :array_count
+      expect(response).to have_meta_record_count(User.count)
+    end
+  end
+
+  describe 'active record count' do
+    it 'returns the count based on the AR\'s query result' do
+      get :active_record_count
+      expect(response).to have_meta_record_count(User.count)
+    end
+  end
+
+  describe 'active record count with eager load' do
+    it 'returns the count based on the AR\'s query result' do
+      get :active_record_count_with_eager_load
+      expect(response).to have_meta_record_count(User.count)
+    end
+  end
+
+  describe 'active record count with eager load and where clause' do
+    it 'returns the count based on the AR\'s query result' do
+      get :active_record_count_with_eager_load_and_where_clause
+      count = User.joins(:posts).where(posts: { id: Post.first.id }).count
+      expect(response).to have_meta_record_count(count)
+    end
+  end
+end

spec/jsonapi/utils/support/pagination_spec.rb

@@ -0,0 +1,130 @@
+require 'rails_helper'
+
+describe JSONAPI::Utils::Support::Pagination do
+  subject do
+    OpenStruct.new(params: {}).extend(JSONAPI::Utils::Support::Pagination)
+  end
+
+  before(:all) do
+    FactoryGirl.create_list(:user, 2)
+  end
+
+  let(:options) { {} }
+
+  ##
+  # Public API
+  ##
+
+  describe '#record_count_for' do
+    context 'with array' do
+      let(:records) { User.all.to_a }
+
+      it 'applies memoization on the record count' do
+        expect(records).to receive(:length).and_return(records.length).once
+        2.times { subject.record_count_for(records, options) }
+      end
+    end
+
+    context 'with ActiveRecord object' do
+      let(:records) { User.all }
+
+      it 'applies memoization on the record count' do
+        expect(records).to receive(:except).and_return(records).once
+        2.times { subject.record_count_for(records, options) }
+      end
+    end
+  end
+
+  ##
+  # Private API
+  ##
+
+  describe '#count_records' do
+    shared_examples_for 'counting records' do
+      it 'counts records' do
+        expect(subject.send(:count_records, records, options)).to eq(count)
+      end
+    end
+
+    context 'with count present within the options' do
+      let(:records) { User.all }
+      let(:options) { { count: 999 } }
+      let(:count)   { 999 }
+      it_behaves_like 'counting records'
+    end
+
+    context 'with array' do
+      let(:records) { User.all.to_a }
+      let(:count)   { records.length }
+      it_behaves_like 'counting records'
+    end
+
+    context 'with ActiveRecord object' do
+      let(:records) { User.all }
+      let(:count)   { records.count }
+      it_behaves_like 'counting records'
+    end
+
+    context 'when no strategy can be applied' do
+      let(:records) { Object.new }
+      let(:count)   { }
+
+      it 'raises an error' do
+        expect {
+          subject.send(:count_records, records, options)
+        }.to raise_error(JSONAPI::Utils::Support::Pagination::RecordCountError)
+      end
+    end
+  end
+
+  describe '#count_records_from_database' do
+    shared_examples_for 'skipping eager load SQL when counting records' do
+      it 'skips any eager load for the SQL count query (default)' do
+        expect(records).to receive(:except)
+          .with(:includes, :group, :order)
+          .and_return(User.all)
+          .once
+        expect(records).to receive(:except)
+          .with(:group, :order)
+          .and_return(User.all)
+          .exactly(0)
+          .times
+        subject.send(:count_records_from_database, records, options)
+      end
+    end
+
+    context 'when not eager loading records' do
+      let(:records) { User.all }
+      it_behaves_like 'skipping eager load SQL when counting records'
+    end
+
+    context 'when eager loading records' do
+      let(:records) { User.includes(:posts) }
+      it_behaves_like 'skipping eager load SQL when counting records'
+    end
+
+    context 'when eager loading records and using where clause on associations' do
+      let(:records) { User.includes(:posts).where(posts: { id: 1 }) }
+
+      it 'fallbacks to the SQL count query with eager load' do
+        expect(records).to receive(:except)
+          .with(:includes, :group, :order)
+          .and_raise(ActiveRecord::StatementInvalid)
+          .once
+        expect(records).to receive(:except)
+          .with(:group, :order)
+          .and_return(User.all)
+          .once
+        subject.send(:count_records_from_database, records, options)
+      end
+    end
+  end
+
+  describe '#distinct_count_sql' do
+    let(:records) { OpenStruct.new(table_name: 'foos', primary_key: 'id') }
+
+    it 'builds the distinct count SQL query' do
+      expect(subject.send(:distinct_count_sql, records)).to eq('DISTINCT foos.id')
+    end
+  end
+end

spec/rails_helper.rb

@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+require 'rails/all'
+require 'rails/test_help'
+require 'rspec/rails'
+
+require 'jsonapi-resources'
+require 'jsonapi/utils'
+
+require 'support/models'
+require 'support/factories'
+require 'support/resources'
+require 'support/controllers'
+require 'support/paginators'
+
+require 'support/shared/jsonapi_errors'
+require 'support/shared/jsonapi_request'
+
+require 'test_app'
+
+RSpec.configure do |config|
+  config.before(:all) do
+    TestApp.draw_app_routes
+
+    %w[posts categories profiles users].each do |table_name|
+      ActiveRecord::Base.connection.execute("DELETE FROM #{table_name}; VACUUM;")
+    end
+  end
+end