Merge branch '36749-explore-projectscontroller-index-latency' into 'master'

Limit page number on explore/projects

See merge request gitlab-org/gitlab!22876

app/controllers/concerns/page_limiter.rb

+# frozen_string_literal: true
+
+# Include this in your controller and call `limit_pages` in order
+# to configure the limiter.
+#
+#   Examples:
+#     class MyController < ApplicationController
+#       include PageLimiter
+#
+#       before_action only: [:index] do
+#         limit_pages(500)
+#       end
+#
+#       # You can override the default response
+#       rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
+#
+#       def page_out_of_bounds(error)
+#         # Page limit number is available as error.message
+#         head :ok
+#       end
+#
+
+module PageLimiter
+  extend ActiveSupport::Concern
+
+  PageLimiterError          = Class.new(StandardError)
+  PageLimitNotANumberError  = Class.new(PageLimiterError)
+  PageLimitNotSensibleError = Class.new(PageLimiterError)
+  PageOutOfBoundsError      = Class.new(PageLimiterError)
+
+  included do
+    rescue_from PageOutOfBoundsError, with: :default_page_out_of_bounds_response
+  end
+
+  def limit_pages(max_page_number)
+    check_page_number!(max_page_number)
+  end
+
+  private
+
+  # If the page exceeds the defined maximum, raise a PageOutOfBoundsError
+  # If the page doesn't exceed the limit, it does nothing.
+  def check_page_number!(max_page_number)
+    raise PageLimitNotANumberError unless max_page_number.is_a?(Integer)
+    raise PageLimitNotSensibleError unless max_page_number > 0
+
+    if params[:page].present? && params[:page].to_i > max_page_number
+      record_page_limit_interception
+      raise PageOutOfBoundsError.new(max_page_number)
+    end
+  end
+
+  # By default just return a HTTP status code and an empty response
+  def default_page_out_of_bounds_response
+    head :bad_request
+  end
+
+  # Record the page limit being hit in Prometheus
+  def record_page_limit_interception
+    dd = DeviceDetector.new(request.user_agent)
+
+    Gitlab::Metrics.counter(:gitlab_page_out_of_bounds,
+      controller: params[:controller],
+      action: params[:action],
+      bot: dd.bot?
+    )
+  end
+end

app/controllers/explore/projects_controller.rb

 # frozen_string_literal: true
 
 class Explore::ProjectsController < Explore::ApplicationController
+  include PageLimiter
   include ParamsBackwardCompatibility
   include RendersMemberAccess
   include SortingHelper
@@ -9,6 +10,13 @@ class Explore::ProjectsController < Explore::ApplicationController
   before_action :set_non_archived_param
   before_action :set_sorting
 
+  # Limit taken from https://gitlab.com/gitlab-org/gitlab/issues/38357
+  before_action only: [:index, :trending, :starred] do
+    limit_pages(200)
+  end
+
+  rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
+
   def index
     @projects = load_projects
 
@@ -53,10 +61,14 @@ class Explore::ProjectsController < Explore::ApplicationController
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def load_projects
+  def load_project_counts
     @total_user_projects_count = ProjectsFinder.new(params: { non_public: true }, current_user: current_user).execute
     @total_starred_projects_count = ProjectsFinder.new(params: { starred: true }, current_user: current_user).execute
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def load_projects
+    load_project_counts
 
     projects = ProjectsFinder.new(current_user: current_user, params: params)
                  .execute
@@ -80,4 +92,21 @@ class Explore::ProjectsController < Explore::ApplicationController
   def sorting_field
     Project::SORTING_PREFERENCE_FIELD
   end
+
+  def page_out_of_bounds(error)
+    load_project_counts
+    @max_page_number = error.message
+
+    respond_to do |format|
+      format.html do
+        render "page_out_of_bounds", status: :bad_request
+      end
+
+      format.json do
+        render json: {
+          html: view_to_html_string("explore/projects/page_out_of_bounds")
+        }, status: :bad_request
+      end
+    end
+  end
 end

app/views/explore/projects/page_out_of_bounds.html.haml

+- @hide_top_links = true
+- page_title    _("Projects")
+- header_title  _("Projects"), dashboard_projects_path
+
+= render_dashboard_gold_trial(current_user)
+
+- if current_user
+  = render 'dashboard/projects_head', project_tab_filter: :explore
+- else
+  = render 'explore/head'
+
+= render 'explore/projects/nav' unless Feature.enabled?(:project_list_filter_bar) && current_user
+
+.nothing-here-block
+  .svg-content
+    = image_tag 'illustrations/profile-page/personal-project.svg', size: '75'
+    .text-content
+      %h5= _("Maximum page reached")
+      %p= _("Sorry, you have exceeded the maximum browsable page number. Please use the API to explore further.")
+
+      = link_to _("Back to page %{number}") % { number: @max_page_number }, request.params.merge(page: @max_page_number), class: 'btn btn-inverted'

changelogs/unreleased/36749-explore-projectscontroller-index-latency.yml

+---
+title: Limit page number on explore/projects
+merge_request: 22876
+author:
+type: performance

locale/gitlab.pot

@@ -2528,6 +2528,9 @@ msgstr ""
 msgid "Average per day: %{average}"
 msgstr ""
 
+msgid "Back to page %{number}"
+msgstr ""
+
 msgid "Background Color"
 msgstr ""
 
@@ -11496,6 +11499,9 @@ msgstr ""
 msgid "Maximum number of mirrors that can be synchronizing at the same time."
 msgstr ""
 
+msgid "Maximum page reached"
+msgstr ""
+
 msgid "Maximum push size (MB)"
 msgstr ""
 
@@ -17410,6 +17416,9 @@ msgstr ""
 msgid "Sorry, no projects matched your search"
 msgstr ""
 
+msgid "Sorry, you have exceeded the maximum browsable page number. Please use the API to explore further."
+msgstr ""
+
 msgid "Sorry, your filter produced no results"
 msgstr ""
 

spec/controllers/concerns/page_limiter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+class PageLimiterSpecController < ApplicationController
+  include PageLimiter
+
+  before_action do
+    limit_pages 200
+  end
+
+  def index
+    head :ok
+  end
+end
+
+describe PageLimiter do
+  let(:controller_class) do
+    PageLimiterSpecController
+  end
+
+  let(:instance) do
+    controller_class.new
+  end
+
+  before do
+    allow(instance).to receive(:params) do
+      {
+        controller: "explore/projects",
+        action: "index"
+      }
+    end
+
+    allow(instance).to receive(:request) do
+      double(:request, user_agent: "Mozilla/5.0 (compatible; Googlebot/2.1; +http://www.google.com/bot.html)")
+    end
+  end
+
+  describe "#limit_pages" do
+    using RSpec::Parameterized::TableSyntax
+
+    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
+      subject { instance.limit_pages(max_page) }
+
+      before do
+        allow(instance).to receive(:params) { { page: actual_page.to_s } }
+      end
+
+      it "returns the expected result" do
+        if result == PageLimiter::PageOutOfBoundsError
+          expect(instance).to receive(:record_page_limit_interception)
+          expect { subject }.to raise_error(result)
+        elsif result&.superclass == PageLimiter::PageLimiterError
+          expect { subject }.to raise_error(result)
+        else
+          expect(subject).to eq(result)
+        end
+      end
+    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)
+    end
+  end
+
+  describe "#record_page_limit_interception" do
+    subject { instance.send(:record_page_limit_interception) }
+
+    it "records a metric counter" do
+      expect(Gitlab::Metrics).to receive(:counter).with(
+        :gitlab_page_out_of_bounds,
+        controller: "explore/projects",
+        action: "index",
+        bot: true
+      )
+
+      subject
+    end
+  end
+end

spec/controllers/explore/projects_controller_spec.rb

@@ -59,6 +59,79 @@ describe Explore::ProjectsController do
     end
   end
 
+  shared_examples "blocks high page numbers" do
+    let(:page_limit) { 200 }
+
+    context "page number is too high" do
+      [:index, :trending, :starred].each do |endpoint|
+        describe "GET #{endpoint}" do
+          render_views
+
+          before do
+            get endpoint, params: { page: page_limit + 1 }
+          end
+
+          it { is_expected.to respond_with(:bad_request) }
+          it { is_expected.to render_template("explore/projects/page_out_of_bounds") }
+
+          it "assigns the page number" do
+            expect(assigns[:max_page_number]).to eq(page_limit.to_s)
+          end
+        end
+
+        describe "GET #{endpoint}.json" do
+          render_views
+
+          before do
+            get endpoint, params: { page: page_limit + 1 }, format: :json
+          end
+
+          it { is_expected.to respond_with(:bad_request) }
+        end
+
+        describe "metrics recording" do
+          after do
+            get endpoint, params: { page: page_limit + 1 }
+          end
+
+          it "records the interception" do
+            expect(Gitlab::Metrics).to receive(:counter).with(
+              :gitlab_page_out_of_bounds,
+              controller: "explore/projects",
+              action: endpoint.to_s,
+              bot: false
+            )
+          end
+        end
+      end
+    end
+
+    context "page number is acceptable" do
+      [:index, :trending, :starred].each do |endpoint|
+        describe "GET #{endpoint}" do
+          render_views
+
+          before do
+            get endpoint, params: { page: page_limit }
+          end
+
+          it { is_expected.to respond_with(:success) }
+          it { is_expected.to render_template("explore/projects/#{endpoint}") }
+        end
+
+        describe "GET #{endpoint}.json" do
+          render_views
+
+          before do
+            get endpoint, params: { page: page_limit }, format: :json
+          end
+
+          it { is_expected.to respond_with(:success) }
+        end
+      end
+    end
+  end
+
   context 'when user is signed in' do
     let(:user) { create(:user) }
 
@@ -67,6 +140,7 @@ describe Explore::ProjectsController do
     end
 
     include_examples 'explore projects'
+    include_examples "blocks high page numbers"
 
     context 'user preference sorting' do
       let(:project) { create(:project) }
@@ -79,6 +153,7 @@ describe Explore::ProjectsController do
 
   context 'when user is not signed in' do
     include_examples 'explore projects'
+    include_examples "blocks high page numbers"
 
     context 'user preference sorting' do
       let(:project) { create(:project) }