6074: Use each instead of find_each, so our order() and limit() constraints are respe...

According to http://apidock.com/rails/ActiveRecord/Batches/find_each,
both order and limit are ignored.

The existing test "max_index_database_read does not interfere with
order" had two fatal bugs that prevented it from catching the
find_each problem:

1. It didn't select the 'name' column, so the 'name' order was
   ignored. But the test passed because the name wasn't returned,
   item['name'] was nil, and... nil !~ /pattern/ is true. Both
   problems are fixed here.

   (This explains why it seemed to find 15 names starting with
   Collection_9, rather than just the 11 that exist (_9 and _90..99).)

2. It tested only that the returned results followed the requested
   order, not that the order was followed when deciding what the limit
   should be. All of the items_available were the same size, so _any_
   order would have set the limit at 15 and passed the test.

The second problem is fixed by adding a separate test.

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 1c8450bfdc1fce5d8184b75dbdae4a7ab665b00a..e91e3ce03ec6369addee5d63964ac10e198e251a 100644 (file)
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -306,7 +306,7 @@ class ApplicationController < ActionController::Base
                limit_columns.map { |s| "octet_length(#{s})" }.join(" + "))
       new_limit = 0
       read_total = 0
-      limit_query.find_each do |record|
+      limit_query.each do |record|
         new_limit += 1
         read_total += record.read_length.to_i
         if read_total >= Rails.configuration.max_index_database_read

diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 28de3f8c02c693ec59757385429dd2fc5be5d796..d2901a010065d330478923631b446c4cf68032c8 100644 (file)
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -111,6 +111,23 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
     assert_equal(201, json_response["items_available"])
   end
 
+  test "max_index_database_read size check follows same order as real query" do
+    authorize_with :user1_with_load
+    txt = '.' + ' d41d8cd98f00b204e9800998ecf8427e+0'*1000 + " 0:0:empty.txt\n"
+    c = Collection.create! manifest_text: txt, name: '0000000000000000000'
+    request_capped_index(select: %w(uuid manifest_text name),
+                         order: ['name asc'],
+                         filters: [['name','>=',c.name]]) do |_|
+      txt.length - 1
+    end
+    assert_response :success
+    assert_equal(1, json_response["items"].size)
+    assert_equal(1, json_response["limit"])
+    assert_equal(c.uuid, json_response["items"][0]["uuid"])
+    # The effectiveness of the test depends on >1 item matching the filters.
+    assert_operator(1, :<, json_response["items_available"])
+  end
+
   test "index with manifest_text limited by max_index_database_read" do
     request_capped_index() { |size| (size * 3) + 1 }
     assert_response :success
@@ -128,13 +145,14 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
   end
 
   test "max_index_database_read does not interfere with order" do
-    request_capped_index(order: "name DESC") { |size| (size * 15) + 1 }
+    request_capped_index(select: %w(uuid manifest_text name),
+                         order: "name DESC") { |size| (size * 11) + 1 }
     assert_response :success
-    assert_equal(15, json_response["items"].size)
+    assert_equal(11, json_response["items"].size)
     assert_empty(json_response["items"].reject do |coll|
-                   coll["name"] !~ /^Collection_9/
+                   coll["name"] =~ /^Collection_9/
                  end)
-    assert_equal(15, json_response["limit"])
+    assert_equal(11, json_response["limit"])
     assert_equal(201, json_response["items_available"])
   end