Refactor page limiter concern

Various alterations to the PageLimiter concern to simplify
the flow and remove the class variables.

app/controllers/concerns/page_limiter.rb

@@ -6,51 +6,37 @@
 #   Examples:
 #     class MyController < ApplicationController
 #       include PageLimiter
-#       limit_pages 500
 #
-#       # Optionally provide a block to customize the response:
-#       limit_pages 500 do
-#         head :ok
+#       before_action only: [:index] do
+#         limit_pages(500)
 #       end
 #
-#       # Or override the default response method
-#       limit_pages 500
-#
+#       # You can override the default response
 #       def page_out_of_bounds
 #         head :ok
 #       end
 #
+#       # Or provide an entirely different handler:
+#       rescue_from PageOutOfBoundsError, with: :something
+#
 
 module PageLimiter
   extend ActiveSupport::Concern
 
-  PageLimitNotANumberError = Class.new(StandardError)
+  PageLimiterError          = Class.new(StandardError)
+  PageLimitNotANumberError  = Class.new(PageLimiterError)
+  PageLimitNotSensibleError = Class.new(PageLimiterError)
+  PageOutOfBoundsError      = Class.new(PageLimiterError)
 
   included do
-    around_action :check_page_number, if: :max_page_defined?
-  end
-
-  class_methods do
-    def limit_pages(number, &block)
-      set_max_page(number)
-      @page_limiter_block = block
-    end
-
-    def max_page
-      @max_page
-    end
-
-    def page_limiter_block
-      @page_limiter_block
+    attr_accessor :max_page_number
+    helper_method :max_page_number
+    rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
   end
 
-    private
-
-    def set_max_page(value)
-      raise PageLimitNotANumberError unless value.is_a?(Integer)
-
-      @max_page = value
-    end
+  def limit_pages(number)
+    set_max_page_number(number)
+    check_page_number
   end
 
   # Override this method in your controller to customize the response
@@ -60,9 +46,11 @@ module PageLimiter
 
   private
 
-  # Used to see whether the around_action should run or not
-  def max_page_defined?
-    self.class.max_page.present? && self.class.max_page > 0
+  def set_max_page_number(value)
+    raise PageLimitNotANumberError unless value.is_a?(Integer)
+    raise PageLimitNotSensibleError unless value > 0
+
+    self.max_page_number = value
   end
 
   # If the page exceeds the defined maximum, either call the provided
@@ -71,16 +59,9 @@ module PageLimiter
   #
   # If the page doesn't exceed the limit, it yields the controller action.
   def check_page_number
-    if params[:page].present? && params[:page].to_i > self.class.max_page
+    if params[:page].present? && params[:page].to_i > max_page_number
       record_interception
-
-      if self.class.page_limiter_block.present?
-        instance_eval(&self.class.page_limiter_block)
-      else
-        page_out_of_bounds
-      end
-    else
-      yield
+      raise PageOutOfBoundsError
     end
   end
 

app/controllers/explore/projects_controller.rb

@@ -7,13 +7,14 @@ class Explore::ProjectsController < Explore::ApplicationController
   include SortingHelper
   include SortingPreference
 
-  PAGE_LIMIT = 500
-
   before_action :set_non_archived_param
   before_action :set_sorting
 
-  limit_pages PAGE_LIMIT
-  helper_method :max_page_number
+  before_action only: [:index, :trending, :starred] do
+    limit_pages(200)
+  end
+
+  rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
 
   def index
     @projects = load_projects
@@ -91,10 +92,6 @@ class Explore::ProjectsController < Explore::ApplicationController
     Project::SORTING_PREFERENCE_FIELD
   end
 
-  def max_page_number
-    PAGE_LIMIT
-  end
-
   # Overrides the default in the PageLimiter concern
   def page_out_of_bounds
     load_project_counts

spec/controllers/concerns/page_limiter_spec.rb

@@ -5,12 +5,12 @@ require 'spec_helper'
 class PageLimiterSpecController < ApplicationController
   include PageLimiter
 
-  limit_pages 2 do
-    raise "block response"
+  before_action do
+    limit_pages 200
   end
 
-  def page_out_of_bounds
-    raise "method response"
+  def index
+    head :ok
   end
 end
 
@@ -36,129 +36,59 @@ describe PageLimiter do
     end
   end
 
-  describe ".max_page" do
-    subject { controller_class.max_page }
-
-    it { is_expected.to eq(2) }
-  end
-
-  describe ".page_limiter_block" do
-    subject { controller_class.page_limiter_block }
-
-    it "is an executable block" do
-      expect { subject.call }.to raise_error("block response")
-    end
-  end
-
-  describe ".set_max_page" do
-    subject { controller_class.send(:set_max_page, page) }
-
-    context "page is a number" do
-      let(:page) { 2 }
-
-      it { is_expected.to eq(page) }
-    end
-
-    context "page is a string" do
-      let(:page) { "2" }
-
-      it "raises an error" do
-        expect { subject }.to raise_error(PageLimiter::PageLimitNotANumberError)
-      end
-    end
-
-    context "page is nil" do
-      let(:page) { nil }
-
-      it "raises an error" do
-        expect { subject }.to raise_error(PageLimiter::PageLimitNotANumberError)
-      end
-    end
-  end
-
-  describe "#page_out_of_bounds" do
-    subject { instance.page_out_of_bounds }
-
-    it "returns a bad_request header" do
-      expect { subject }.to raise_error("method response")
-    end
-  end
-
-  describe "#max_page_defined?" do
+  describe "#limit_pages" do
     using RSpec::Parameterized::TableSyntax
-    subject { instance.send(:max_page_defined?) }
 
-    where(:max_page, :result) do
-      2     | true
-      nil   | false
-      0     | false
+    where(:max_page, :actual_page, :result) do
+      2   | 1 | nil
+      2   | 2 | nil
+      2   | 3 | PageLimiter::PageOutOfBoundsError
+      nil | 1 | PageLimiter::PageLimitNotANumberError
+      0   | 1 | PageLimiter::PageLimitNotSensibleError
+      -1  | 1 | PageLimiter::PageLimitNotSensibleError
     end
 
     with_them do
-      before do
-        controller_class.instance_variable_set(:@max_page, max_page)
-      end
-
-      # Reset this afterwards to prevent polluting other specs
-      after do
-        controller_class.instance_variable_set(:@max_page, 2)
-      end
-
-      it { is_expected.to be(result) }
-    end
-  end
-
-  describe "#check_page_number" do
-    let(:max_page) { 2 }
-
-    subject { instance.send(:check_page_number) { "test" } }
+      subject { instance.limit_pages(max_page) }
 
       before do
-      allow(instance).to receive(:params) { { page: page.to_s } }
+        allow(instance).to receive(:params) { { page: actual_page.to_s } }
       end
 
-    context "page is over the limit" do
-      let(:page) { max_page + 1 }
-
-      it "records the interception" do
+      it "returns the expected result" do
+        if result == PageLimiter::PageOutOfBoundsError
           expect(instance).to receive(:record_interception)
-        # Need this second expectation to cancel out the exception
-        expect { subject }.to raise_error("block response")
+          expect { subject }.to raise_error(result)
+        elsif result&.superclass == PageLimiter::PageLimiterError
+          expect { subject }.to raise_error(result)
+        else
+          expect(subject).to eq(result)
         end
-
-      context "block is given" do
-        it "calls the block" do
-          expect { subject }.to raise_error("block response")
       end
     end
-
-      context "block is not given" do
-        before do
-          allow(controller_class).to receive(:page_limiter_block) { nil }
   end
 
-        it "calls the #page_out_of_bounds method" do
-          expect { subject }.to raise_error("method response")
-        end
-      end
-    end
-
-    context "page is not over the limit" do
-      let(:page) { max_page }
+  describe "#page_out_of_bounds" do
+    subject { instance.page_out_of_bounds }
 
-      it "yields" do
-        expect(subject).to eq("test")
+    after do
+      subject
     end
+
+    it "returns a bad_request header" do
+      expect(instance).to receive(:head).with(:bad_request)
     end
   end
 
   describe "#default_page_out_of_bounds_response" do
     subject { instance.send(:default_page_out_of_bounds_response) }
 
+    after do
+      subject
+    end
+
     it "returns a bad_request header" do
       expect(instance).to receive(:head).with(:bad_request)
-
-      subject
     end
   end
 

spec/controllers/explore/projects_controller_spec.rb

@@ -60,7 +60,7 @@ describe Explore::ProjectsController do
   end
 
   shared_examples "blocks high page numbers" do
-    let(:page_limit) { Explore::ProjectsController::PAGE_LIMIT }
+    let(:page_limit) { 200 }
 
     context "page number is too high" do
       [:index, :trending, :starred].each do |endpoint|
@@ -95,7 +95,7 @@ describe Explore::ProjectsController do
               :gitlab_page_out_of_bounds,
               controller: "explore/projects",
               action: endpoint.to_s,
-              agent: "Rails Testing"
+              bot: false
             )
           end
         end
@@ -162,11 +162,4 @@ describe Explore::ProjectsController do
       end
     end
   end
-
-  describe "#max_page_number" do
-    subject { controller.send(:max_page_number) }
-
-    it { is_expected.to eq(Explore::ProjectsController::PAGE_LIMIT) }
-    it { is_expected.to be_an(Integer) }
-  end
 end