Merge pull request #210 from edx/feature/limit-responses

Add a default response size and a response size limit.

api/comment_threads.rb

+
 get "#{APIPREFIX}/threads" do # retrieve threads by course
   threads = CommentThread.where({"course_id" => params["course_id"]})
   if params[:commentable_ids]
@@ -46,7 +47,11 @@ get "#{APIPREFIX}/threads/:thread_id" do |thread_id|
       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

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 %>

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."

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."

spec/api/comment_thread_spec.rb

@@ -542,6 +542,17 @@ describe "app" do
 
       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 @@ describe "app" do
           @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 @@ describe "app" do
               # 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 @@ describe "app" do
           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

spec/presenters/thread_spec.rb

@@ -3,6 +3,8 @@ require 'spec_helper'
 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 @@ describe ThreadPresenter do
       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 @@ describe ThreadPresenter do
         @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

spec/spec_helper.rb

@@ -265,6 +265,10 @@ end
 
 
 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
 end
 
 def check_question_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false, recursive=false)