From 019242de6fcc7494850d6bacf19d79aea77273dc Mon Sep 17 00:00:00 2001 From: amal Date: Mon, 25 Oct 2021 14:24:07 -0300 Subject: [PATCH] GIT-718: Update implementation of offset pagination for getRecordings --- README.md | 4 +- .../bigbluebutton_api_controller.rb | 13 +--- app/controllers/concerns/api_helper.rb | 20 +++++++ config/application.rb | 8 +-- .../bigbluebutton_api_controller_test.rb | 59 ++++++++++++------- 5 files changed, 66 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 1f9034e8..ffb3f4ac 100644 --- a/README.md +++ b/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`) diff --git a/app/controllers/bigbluebutton_api_controller.rb b/app/controllers/bigbluebutton_api_controller.rb index c31311a4..68cbc198 100644 --- a/app/controllers/bigbluebutton_api_controller.rb +++ b/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}" diff --git a/app/controllers/concerns/api_helper.rb b/app/controllers/concerns/api_helper.rb index 40410420..2136d6cc 100644 --- a/app/controllers/concerns/api_helper.rb +++ b/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') diff --git a/config/application.rb b/config/application.rb index f38aca2b..b38af266 100644 --- a/config/application.rb +++ b/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 diff --git a/test/controllers/bigbluebutton_api_controller_test.rb b/test/controllers/bigbluebutton_api_controller_test.rb index 017aedd9..49e66bb7 100644 --- a/test/controllers/bigbluebutton_api_controller_test.rb +++ b/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,21 +1331,18 @@ 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 @@ -1357,6 +1350,28 @@ class BigBlueButtonApiControllerTest < ActionDispatch::IntegrationTest 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')