DEV: Add per_page as public param for TopicQuery (#30716)

This change allows controllers that construct TopicQuery parameters, to pass per_page into the TopicQuery constructor as an option. I can't see why this shouldn't be a public param, so long as we properly validate the value!

Internal discussion at t/145686.

app/controllers/list_controller.rb

@@ -322,7 +322,8 @@ class ListController < ApplicationController
     define_method("top_#{period}") do |options = nil|
       top_options = build_topic_list_options
       top_options.merge!(options) if options
-      top_options[:per_page] = SiteSetting.topics_per_period_in_top_page
+      top_options[:per_page] = top_options[:per_page].presence ||
+        SiteSetting.topics_per_period_in_top_page
 
       user = list_target_user
       list = TopicQuery.new(user, top_options).list_top_for(period)

lib/topic_query.rb

@@ -16,12 +16,14 @@ class TopicQuery
       begin
         int = lambda { |x| Integer === x || (String === x && x.match?(/\A-?[0-9]+\z/)) }
         zero_up_to_max_int = lambda { |x| int.call(x) && x.to_i.between?(0, PG_MAX_INT) }
+        one_up_to_one_hundred = lambda { |x| int.call(x) && x.to_i.between?(1, 100) }
         array_or_string = lambda { |x| Array === x || String === x }
         string = lambda { |x| String === x }
         true_or_false = lambda { |x| x == true || x == false || x == "true" || x == "false" }
 
         {
           page: zero_up_to_max_int,
+          per_page: one_up_to_one_hundred,
           before: zero_up_to_max_int,
           bumped_before: zero_up_to_max_int,
           topic_ids: array_or_string,
@@ -59,6 +61,7 @@ class TopicQuery
     # For these to work in Ember, add them to `controllers/discovery/list.js`
     @public_valid_options ||= %i[
       page
+      per_page
       before
       bumped_before
       topic_ids
@@ -89,8 +92,6 @@ class TopicQuery
         %i[
           except_topic_ids
           limit
-          page
-          per_page
           visible
           guardian
           no_definitions
@@ -503,7 +504,7 @@ class TopicQuery
     unpinned_topics = topics.where("NOT ( #{pinned_clause} )")
     pinned_topics = topics.dup.offset(nil).where(pinned_clause).reorder(pinned_at: :desc)
 
-    per_page = options[:per_page] || per_page_setting
+    per_page = options[:per_page]&.to_i || per_page_setting
     limit = per_page unless options[:limit] == false
     page = options[:page].to_i
 
@@ -561,7 +562,7 @@ class TopicQuery
     end
 
     list = TopicList.new(filter, @user, topics, options.merge(@options))
-    list.per_page = options[:per_page] || per_page_setting
+    list.per_page = options[:per_page]&.to_i || per_page_setting
     list
   end
 
@@ -836,7 +837,7 @@ class TopicQuery
       result = result.where("COALESCE(categories.topic_id, 0) <> topics.id")
     end
 
-    result = result.limit(options[:per_page]) unless options[:limit] == false
+    result = result.limit(options[:per_page]&.to_i) unless options[:limit] == false
     result = result.visible if options[:visible]
     result =
       result.where.not(topics: { id: options[:except_topic_ids] }).references(:topics) if options[
@@ -844,7 +845,7 @@ class TopicQuery
     ]
 
     if options[:page]
-      offset = options[:page].to_i * options[:per_page]
+      offset = options[:page].to_i * options[:per_page]&.to_i
       result = result.offset(offset) if offset > 0
     end
 

spec/lib/topic_query_spec.rb

@@ -89,6 +89,24 @@ RSpec.describe TopicQuery do
     end
   end
 
+  describe ".validate?" do
+    describe "per_page" do
+      it "only allows integers 1-100" do
+        # Invalid values
+        expect(TopicQuery.validate?(:per_page, -1)).to eq(false)
+        expect(TopicQuery.validate?(:per_page, 0)).to eq(false)
+        expect(TopicQuery.validate?(:per_page, 101)).to eq(false)
+        expect(TopicQuery.validate?(:per_page, "invalid")).to eq(false)
+        expect(TopicQuery.validate?(:per_page, [])).to eq(false)
+
+        # Valid values
+        expect(TopicQuery.validate?(:per_page, 1)).to eq(true)
+        expect(TopicQuery.validate?(:per_page, 100)).to eq(true)
+        expect(TopicQuery.validate?(:per_page, "10")).to eq(true)
+      end
+    end
+  end
+
   describe "#list_topics_by" do
     it "allows users to view their own invisible topics" do
       _topic = Fabricate(:topic, user: user)

spec/requests/api/topics_spec.rb

@@ -519,6 +519,12 @@ RSpec.describe "topics" do
         type: :string,
         description: "Defaults to `desc`, add `ascending=true` to sort asc",
       )
+      parameter(
+        name: :per_page,
+        in: :query,
+        type: :integer,
+        description: "Maximum number of topics returned, between 1-100",
+      )
 
       produces "application/json"
       response "200", "topic updated" do
@@ -697,6 +703,7 @@ RSpec.describe "topics" do
 
         let(:order) { "default" }
         let(:ascending) { "false" }
+        let(:per_page) { 20 }
 
         run_test!
       end
@@ -716,6 +723,12 @@ RSpec.describe "topics" do
         type: :string,
         description: "Enum: `all`, `yearly`, `quarterly`, `monthly`, `weekly`, `daily`",
       )
+      parameter(
+        name: :per_page,
+        in: :query,
+        type: :integer,
+        description: "Maximum number of topics returned, between 1-100",
+      )
 
       produces "application/json"
       response "200", "response" do
@@ -896,6 +909,7 @@ RSpec.describe "topics" do
                }
 
         let(:period) { "all" }
+        let(:per_page) { 20 }
 
         run_test!
       end

spec/requests/list_controller_spec.rb

@@ -726,6 +726,21 @@ RSpec.describe ListController do
       get "/top?period=decadely"
       expect(response.status).to eq(400)
     end
+
+    describe "per page" do
+      it "uses value from params when present" do
+        get "/top.json?per_page=5"
+
+        expect(response.parsed_body["topic_list"]["per_page"]).to eq(5)
+      end
+
+      it "uses SiteSetting.topics_per_period_in_top_page when per_page param isn't present" do
+        SiteSetting.topics_per_period_in_top_page = 32
+        get "/top.json"
+
+        expect(response.parsed_body["topic_list"]["per_page"]).to eq(32)
+      end
+    end
   end
 
   describe "category" do