From 72816736480f737ac2296915366af9aa05700f63 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 1 Nov 2016 09:59:36 -0400 Subject: [PATCH] Add a default response size and a response size limit. By default, we currently allow a user to retrieve all responses from a thread, which is terrible from a performance standpoint. This change adds a default response size of 100, which was chosen to match typical production pagination parameters, and a response limit of 200. Given that responses can include children comments, 200 is still a large number, but far less disastrous than, say, getting 3000 responses, with all of their children comments. Both of these values are configurable, and can be overriden via environment variables. --- api/comment_threads.rb | 7 ++++++- config/application.yml | 2 ++ locale/en-US.yml | 1 + locale/x-test.yml | 2 ++ spec/api/comment_thread_spec.rb | 17 ++++++++++++++--- spec/presenters/thread_spec.rb | 14 ++++++++------ spec/spec_helper.rb | 10 +++++----- 7 files changed, 38 insertions(+), 15 deletions(-) diff --git a/api/comment_threads.rb b/api/comment_threads.rb index 7ee1a2615ee..04246b1932d 100644 --- a/api/comment_threads.rb +++ b/api/comment_threads.rb @@ -1,3 +1,4 @@ + get "#{APIPREFIX}/threads" do # retrieve threads by course threads = CommentThread.where({"course_id" => params["course_id"]}) if params[:commentable_ids] @@ -46,7 +47,11 @@ error 400, [t(:param_must_be_a_number_greater_than_zero, :param => 'resp_limit')].to_json end else - resp_limit = nil + resp_limit = CommentService.config["thread_response_default_size"] + end + size_limit = CommentService.config["thread_response_size_limit"] + unless (resp_limit <= size_limit) + error 400, [t(:param_exceeds_limit, :param => resp_limit, :limit => size_limit)].to_json end presenter.to_hash(bool_with_responses, resp_skip, resp_limit, bool_recursive).to_json end diff --git a/config/application.yml b/config/application.yml index f2e5f043609..0eccba8ee85 100644 --- a/config/application.yml +++ b/config/application.yml @@ -4,3 +4,5 @@ elasticsearch_server: <%= ENV['SEARCH_SERVER'] || 'http://localhost:9200' %> max_deep_search_comment_count: 5000 default_locale: <%= ENV['SERVICE_LANGUAGE'] || 'en-US' %> manual_pagination_batch_size: <%= ENV['MANUAL_PAGINATION_BATCH_SIZE'] || 500 %> +thread_response_default_size: <%= ENV['THREAD_RESPONSE_DEFAULT_SIZE'] || 100 %> +thread_response_size_limit: <%= ENV['THREAD_RESPONSE_SIZE_LIMIT'] || 200 %> diff --git a/locale/en-US.yml b/locale/en-US.yml index 0bf9981461a..f7de4890b3a 100644 --- a/locale/en-US.yml +++ b/locale/en-US.yml @@ -8,4 +8,5 @@ en-US: blocked_content_with_body_hash: "blocked content with body hash %{hash}" param_must_be_a_non_negative_number: "%{param} must be a non-negative number" param_must_be_a_number_greater_than_zero: "%{param} must be a number greater than zero" + param_exceeds_limit: "%{param} exceeds limit: %{limit}" cannot_specify_group_id_and_group_ids: "Cannot specify both group_id and group_ids as filters." diff --git a/locale/x-test.yml b/locale/x-test.yml index 92c2f0e75f4..ac69875e515 100644 --- a/locale/x-test.yml +++ b/locale/x-test.yml @@ -8,3 +8,5 @@ x-test: blocked_content_with_body_hash: "##x-test## blocked content with body hash %{hash}" param_must_be_a_non_negative_number: "##x-test## %{param} must be a non-negative number" param_must_be_a_number_greater_than_zero: "##x-test## %{param} must be a number greater than zero" + param_exceeds_limit: "##x-test## %{param} exceeds limit: %{limit}" + cannot_specify_group_id_and_group_ids: "##x-test## Cannot specify both group_id and group_ids as filters." diff --git a/spec/api/comment_thread_spec.rb b/spec/api/comment_thread_spec.rb index 19ba3f317a4..ec760793196 100644 --- a/spec/api/comment_thread_spec.rb +++ b/spec/api/comment_thread_spec.rb @@ -542,6 +542,17 @@ def test_unicode_data(text) include_examples 'unicode data' + context 'error conditions' do + subject do + resp_limit = CommentService.config["thread_response_size_limit"] + get "/api/v1/threads/#{thread.id}", resp_limit: resp_limit+1 + end + + it "returns an error when the limit is exceeded" do + expect(subject.status).to eq 400 + end + end + context "response pagination" do before(:each) do User.all.delete @@ -549,7 +560,7 @@ def test_unicode_data(text) @user = create_test_user(999) @threads = {} @comments = {} - [20, 10, 3, 2, 1, 0].each do |n| + [201, 10, 3, 2, 1, 0].each do |n| thread_key = "t#{n}" thread = make_thread(@user, thread_key, DFLT_COURSE_ID, "pdq") @threads[n] = thread @@ -557,7 +568,7 @@ def test_unicode_data(text) # generate n responses in this thread comment_key = "#{thread_key} r#{i}" comment = make_comment(@user, thread, comment_key) - i.times do |j| + 2.times do |j| subcomment_key = "#{comment_key} c#{j}" subcomment = make_comment(@user, comment, subcomment_key) end @@ -572,7 +583,7 @@ def thread_result(id, params) parse(last_response.body) end - it "returns all responses when no skip/limit params given" do + it "limits responses when no skip/limit params given" do @threads.each do |n, thread| res = thread_result thread.id, {} check_thread_response_paging_json thread, res, 0, nil, false diff --git a/spec/presenters/thread_spec.rb b/spec/presenters/thread_spec.rb index 69ed828b890..574d283e24c 100644 --- a/spec/presenters/thread_spec.rb +++ b/spec/presenters/thread_spec.rb @@ -3,6 +3,8 @@ describe ThreadPresenter do context "#to_hash" do + let(:default_resp_limit) { CommentService.config["thread_response_default_size"] } + shared_examples "to_hash arguments" do |thread_type, endorse_responses| before(:each) do User.all.delete @@ -94,18 +96,18 @@ it "handles with_responses=true and recursive=true" do @threads_with_num_comments.each do |thread, num_comments| is_endorsed = num_comments > 0 && endorse_responses - hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, nil, true) + hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, default_resp_limit, true) check_thread_result(@reader, thread, hash) - check_thread_response_paging(thread, hash, 0, nil, false, true) + check_thread_response_paging(thread, hash, 0, default_resp_limit, false, true) end end it "handles with_responses=true and recursive=false" do @threads_with_num_comments.each do |thread, num_comments| is_endorsed = num_comments > 0 && endorse_responses - hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, nil, false) + hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, 0, default_resp_limit, false) check_thread_result(@reader, thread, hash) - check_thread_response_paging(thread, hash) + check_thread_response_paging(thread, hash, 0, default_resp_limit) end end @@ -113,9 +115,9 @@ @threads_with_num_comments.each do |thread, num_comments| is_endorsed = num_comments > 0 && endorse_responses [0, 1, 2, 9, 10, 11, 1000].each do |skip| - hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, skip, nil, true) + hash = ThreadPresenter.new(thread, @reader, false, num_comments, is_endorsed).to_hash(true, skip, default_resp_limit, true) check_thread_result(@reader, thread, hash) - check_thread_response_paging(thread, hash, skip) + check_thread_response_paging(thread, hash, skip, default_resp_limit) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fe263319ee7..67c4e1b6654 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -265,6 +265,10 @@ def check_comment(comment, hash, is_json, recursive=false) def check_discussion_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false) + if resp_limit.nil? + resp_limit = CommentService.config["thread_response_default_size"] + end + all_responses = thread.root_comments.sort({"sk" => 1}).to_a total_responses = all_responses.length hash["resp_total"].should == total_responses @@ -277,11 +281,7 @@ def check_discussion_response_paging(thread, hash, resp_skip=0, resp_limit=nil, check_comment(expected_responses[i], response_hash, is_json, recursive) end hash["resp_skip"].to_i.should == resp_skip - if resp_limit.nil? - hash["resp_limit"].should be_nil - else - hash["resp_limit"].to_i.should == resp_limit - end + hash["resp_limit"].to_i.should == resp_limit end def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false)