GIT-718: Update implementation of offset pagination for getRecordings

README.md

@@ -164,8 +164,8 @@ These variables are used by the service startup scripts in the Docker images, bu
 * `PROTECTED_RECORDINGS_ENABLED`: Applies to the recording import process. If set to "true", then newly imported recordings will have protected links enabled. Default is "false".
 * `PROTECTED_RECORDINGS_TOKEN_TIMEOUT`: Protected recording link token timeout in minutes. This is the amount of time that the one-time-use link returned in `getRecordings` calls will be valid for. Defaults to 60 minutes (1 hour).
 * `PROTECTED_RECORDINGS_TIMEOUT`: Protected recordings resource access cookie timeout in minutes. This is the amount of time that a user will be granted access to view a recording for after clicking on the one-time-use link. Defaults to 360 minutes (6 hours).
-* `PAGINATION_ENABLED`: Enable pagination feature for GET_RECORDINGS API by setting this to true. Defaults to `false`.
-* `DEFAULT_PAGINATION_LIMIT`: The number of records that will be displayed per page, if the limit is not specified in API params. Defaults to 10.
+* `DEFAULT_PAGINATION_LIMIT`: The number of records that will be displayed per page, this value will be considered if the pagination limit is not specified in GET_RECORDINGS_API params.
+* `MAX_PAGINATION_LIMIT`: The max pagination limit value for get_recordings api. Defaults to 100.
 
 ### Redis Connection (`config/redis_store.yml`)
 

app/controllers/bigbluebutton_api_controller.rb

@@ -268,10 +268,7 @@ def join
   end
 
   def get_recordings
-    if Rails.configuration.x.get_recordings_api_filtered && (params[:recordID].blank? && params[:meetingID].blank?)
-      raise BBBError.new('missingParameters', 'param meetingID or recordID must be included.')
-    end
-
+    validate_get_recordings_params
     query = Recording.includes(playback_formats: [:thumbnails], metadata: []).references(:metadata)
     query = if params[:state].present?
               states = params[:state].split(',')
@@ -293,12 +290,8 @@ def get_recordings
     query = query.with_recording_id_prefixes(params[:recordID].split(',')) if params[:recordID].present?
     query = query.where(meeting_id: params[:meetingID].split(',')) if params[:meetingID].present?
 
-    if Rails.configuration.x.pagination_enabled
-      page = params[:page]&.to_i || 0
-      limit = params[:limit]&.to_i || Rails.configuration.x.default_pagination_limit
-      offset = page * limit
-      query = query.offset(offset).limit(limit)
-    end
+    offset = params[:offset] || 0
+    query = query.offset(offset).limit(params[:limit])
 
     @recordings = query.order(starttime: :desc).all
     @url_prefix = "#{request.protocol}#{request.host}"

app/controllers/concerns/api_helper.rb

@@ -71,6 +71,26 @@ def valid_token?(token)
     decoded_token(token)
   end
 
+  def validate_get_recordings_params
+    if params[:limit].present? && !params[:limit].to_i.between?(1, Rails.configuration.x.max_pagination_limit)
+      raise BBBError.new('invalidParamLimit',
+                         "param limit should be within the range 1 and #{Rails.configuration.x.max_pagination_limit}")
+    end
+
+    if params[:offset].present? && params[:offset].to_i.negative?
+      raise BBBError.new('invalidParamOffset', 'param offset cannot be negative.')
+    end
+
+    if Rails.configuration.x.get_recordings_api_filtered && (params[:recordID].blank? &&
+      params[:meetingID].blank? && limit.nil?)
+      raise BBBError.new('missingParameters', 'param meetingID or recordID or limit must be included.')
+    end
+  end
+
+  def limit
+    params[:limit] || Rails.configuration.x.default_pagination_limit
+  end
+
   def post_req(uri, body)
     http = Net::HTTP.new(uri.host, uri.port)
     http.use_ssl = (uri.scheme == 'https')

config/application.rb

@@ -128,10 +128,10 @@ class Application < Rails::Application
     # Protected recordings resource access cookie timeout in minutes. Defaults to 360 (6 hours)
     config.x.recording_cookie_ttl = ENV.fetch('PROTECTED_RECORDINGS_TIMEOUT', '360').to_i.minutes
 
-    # Enable pagination for get_recordings api. Defaults to false.
-    config.x.pagination_enabled = ENV.fetch('PAGINATION_ENABLED', 'false').casecmp?('true')
+    # Set default pagination limit for get_recordings api.
+    config.x.default_pagination_limit = ENV['DEFAULT_PAGINATION_LIMIT']
 
-    # Set pagination limit for get_recordings api. Defaults to 10.
-    config.x.default_pagination_limit = ENV.fetch('DEFAULT_PAGINATION_LIMIT', '10').to_i
+    # Set max pagination limit for get_recordings api.
+    config.x.max_pagination_limit = ENV.fetch('MAX_PAGINATION_LIMIT', '100').to_i
   end
 end

test/controllers/bigbluebutton_api_controller_test.rb

@@ -1086,7 +1086,7 @@ class BigBlueButtonApiControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_select 'response>returncode', 'FAILED'
     assert_select 'response>messageKey', 'missingParameters'
-    assert_select 'response>message', 'param meetingID or recordID must be included.'
+    assert_select 'response>message', 'param meetingID or recordID or limit must be included.'
   end
 
   test 'getRecordings with only checksum returns all recordings for a post request' do
@@ -1300,30 +1300,26 @@ class BigBlueButtonApiControllerTest < ActionDispatch::IntegrationTest
     assert_select 'response>recordings>recording', 1
   end
 
-  test 'getRecordings returns paginated recordings if pagination is enabled' do
+  test 'getRecordings returns paginated recordings based on the limit given in params' do
     create_list(:recording, 12, state: 'published')
 
-    params = encode_bbb_params('getRecordings', { page: 0, limit: 4 }.to_query)
+    params = encode_bbb_params('getRecordings', { limit: 4 }.to_query)
 
-    Rails.configuration.x.stub(:pagination_enabled, true) do
-      get bigbluebutton_api_get_recordings_url, params: params
-    end
+    get bigbluebutton_api_get_recordings_url, params: params
 
     assert_response :success
     assert_select 'response>returncode', 'SUCCESS'
     assert_select 'response>recordings>recording', 4
   end
 
-  test 'getRecordings returns paginated recordings if pagination is enabled and page & limit value is given' do
-    list = create_list(:recording, 30, state: 'unpublished')
-    offset_zero = list.first(15).last
-    offset_five = list.first(11).last
+  test 'getRecordings returns paginated recordings if offset & limit value is given' do
+    list = create_list(:recording, 20, state: 'unpublished')
+    offset_zero = list.last(4).first
+    offset_five = list.last(8).first
 
-    params = encode_bbb_params('getRecordings', { page: 3, limit: 5 }.to_query)
+    params = encode_bbb_params('getRecordings', { offset: 3, limit: 5 }.to_query)
 
-    Rails.configuration.x.stub(:pagination_enabled, true) do
-      get bigbluebutton_api_get_recordings_url, params: params
-    end
+    get bigbluebutton_api_get_recordings_url, params: params
 
     assert_response :success
     assert_select 'response>returncode', 'SUCCESS'
@@ -1335,28 +1331,47 @@ class BigBlueButtonApiControllerTest < ActionDispatch::IntegrationTest
     assert_equal last_rec_id, offset_five.record_id
   end
 
-  test 'getRecordings returns paginated recordings based on default values of limit=10 & page=0' do
-    create_list(:recording, 10, state: 'published')
-    list = create_list(:recording, 10, state: 'unpublished')
+  test 'getRecordings returns all recordings if values of limit=nil & offset=0' do
+    list = create_list(:recording, 20, state: 'published')
     offset_zero = list.last
     offset_ten = list.first
 
-    Rails.configuration.x.stub(:pagination_enabled, true) do
-      BigBlueButtonApiController.stub_any_instance(:verify_checksum, nil) do
-        get bigbluebutton_api_get_recordings_url
-      end
+    BigBlueButtonApiController.stub_any_instance(:verify_checksum, nil) do
+      get bigbluebutton_api_get_recordings_url
     end
 
     assert_response :success
     assert_select 'response>returncode', 'SUCCESS'
-    assert_select 'response>recordings>recording', 10
+    assert_select 'response>recordings>recording', 20
     response_xml = Nokogiri::XML(@response.body)
     first_rec_id = response_xml.xpath('/response/recordings/recording').first.xpath('.//recordID').text
     last_rec_id = response_xml.xpath('/response/recordings/recording').last.xpath('.//recordID').text
     assert_equal first_rec_id, offset_zero.record_id
     assert_equal last_rec_id, offset_ten.record_id
   end
 
+  test 'getRecordings raises error if value of limit is invalid' do
+    params = encode_bbb_params('getRecordings', { limit: 500 }.to_query)
+    BigBlueButtonApiController.stub_any_instance(:verify_checksum, nil) do
+      get bigbluebutton_api_get_recordings_url, params: params
+    end
+
+    assert_response :success
+    assert_select 'response>returncode', 'FAILED'
+    assert_select 'response>messageKey', 'invalidParamLimit'
+  end
+
+  test 'getRecordings raises error if value of offset is invalid' do
+    params = encode_bbb_params('getRecordings', { offset: -5 }.to_query)
+    BigBlueButtonApiController.stub_any_instance(:verify_checksum, nil) do
+      get bigbluebutton_api_get_recordings_url, params: params
+    end
+
+    assert_response :success
+    assert_select 'response>returncode', 'FAILED'
+    assert_select 'response>messageKey', 'invalidParamOffset'
+  end
+
   test 'getRecordings with get_recordings_api_filtered filters based on recording states' do
     create_list(:recording, 5, state: 'deleted')
     r1 = create(:recording, state: 'published')