From 602706ef5510b3f07fc5fa988019952d2133320c Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Thu, 4 Jun 2015 00:52:48 -0400 Subject: [PATCH] 6074: Use each instead of find_each, so our order() and limit() constraints are respected. 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. --- .../app/controllers/application_controller.rb | 2 +- .../arvados/v1/collections_controller_test.rb | 26 ++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 1c8450bfdc..e91e3ce03e 100644 --- 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 28de3f8c02..d2901a0100 100644 --- 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 -- 2.30.2