From dca6cfe9750d8d1be4f3b63895b8cb73cc6c4cfe Mon Sep 17 00:00:00 2001
From: Peter Amstutz <peter.amstutz@curoverse.com>
Date: Fri, 2 May 2014 15:54:45 -0400
Subject: [PATCH] Fixed botched 'distinct' parameter, now is a boolean instead
 of taking a column.  New tests.  Also changed syntax of 'order' to take a
 JSON array for consistency with 'filters' and 'select'.

---
 .../app/controllers/users_controller.rb       |  8 ++--
 doc/api/methods.html.textile.liquid           | 10 +++--
 .../app/controllers/application_controller.rb |  6 ++-
 .../controllers/arvados/v1/jobs_controller.rb |  2 +-
 .../arvados/v1/schema_controller.rb           |  4 +-
 services/api/lib/load_param.rb                | 20 +++++++---
 services/api/test/integration/select_test.rb  | 39 ++++++++++++++++---
 services/api/test/test_helper.rb              |  2 +-
 8 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/apps/workbench/app/controllers/users_controller.rb b/apps/workbench/app/controllers/users_controller.rb
index 863876137f..5cfa48710f 100644
--- a/apps/workbench/app/controllers/users_controller.rb
+++ b/apps/workbench/app/controllers/users_controller.rb
@@ -74,7 +74,7 @@ class UsersController < ApplicationController
       storage_log = Log.
         filter([[:object_uuid, '=', u.uuid],
                 [:event_type, '=', 'user-storage-report']]).
-        order(:created_at => :desc).
+        order(['created_at desc']).
         limit(1)
       storage_log.each do |log_entry|
         # We expect this block to only execute once since we specified limit(1)
@@ -122,12 +122,12 @@ class UsersController < ApplicationController
 
     @my_jobs = Job.
       limit(10).
-      order('created_at desc').
+      order(['created_at desc']).
       where(created_by: current_user.uuid)
 
     @my_collections = Collection.
       limit(10).
-      order('created_at desc').
+      order(['created_at desc']).
       where(created_by: current_user.uuid)
     collection_uuids = @my_collections.collect &:uuid
 
@@ -151,7 +151,7 @@ class UsersController < ApplicationController
 
     @my_pipelines = PipelineInstance.
       limit(10).
-      order('created_at desc').
+      order(['created_at desc']).
       where(created_by: current_user.uuid)
 
     respond_to do |f|
diff --git a/doc/api/methods.html.textile.liquid b/doc/api/methods.html.textile.liquid
index 9078720c06..3d93e88712 100644
--- a/doc/api/methods.html.textile.liquid
+++ b/doc/api/methods.html.textile.liquid
@@ -14,7 +14,7 @@ h2(#index). Index, list, search
 
 <pre>
 GET https://{{ site.arvados_api_host }}/arvados/v1/groups?filters=[["owner_uuid","=","xyzzy-tpzed-a4lcehql0dv2u25"]]
- 
+
 POST https://{{ site.arvados_api_host }}/arvados/v1/groups
 _method=GET
 filters=[["owner_uuid","=","xyzzy-tpzed-a4lcehql0dv2u25"]]
@@ -24,8 +24,12 @@ filters=[["owner_uuid","=","xyzzy-tpzed-a4lcehql0dv2u25"]]
 
 table(table table-bordered table-condensed).
 |*Parameter name*|*Value*|*Description*|
-|limit|integer|Maximum number of resources to return|
-|filters|array|Conditions for selecting resources to return|
+|limit   |integer|Maximum number of resources to return|
+|offset  |integer|Skip the first 'offset' objects|
+|filters |array  |Conditions for selecting resources to return|
+|order   |array  |List of fields to use to determine sorting order for returned objects|
+|select  |array  |Specify which fields to return|
+|distinct|boolean|true: do not return duplicate objects, default; false: permitted to return duplicates|
 
 h2. Create
 
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 30027c8c78..4d23b0bcbf 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -261,10 +261,10 @@ class ApplicationController < ActionController::Base
     end
 
     @objects = @objects.select(@select.map { |s| "#{table_name}.#{ActiveRecord::Base.connection.quote_column_name s.to_s}" }.join ", ") if @select
-    @objects = @objects.uniq(ActiveRecord::Base.connection.quote_column_name @distinct.to_s) if @distinct
     @objects = @objects.order(@orders.join ", ") if @orders.any?
     @objects = @objects.limit(@limit)
     @objects = @objects.offset(@offset)
+    @objects = @objects.uniq(@distinct) if not @distinct.nil?
   end
 
   def resource_attrs
@@ -444,7 +444,9 @@ class ApplicationController < ActionController::Base
     {
       filters: { type: 'array', required: false },
       where: { type: 'object', required: false },
-      order: { type: 'string', required: false },
+      order: { type: 'array', required: false },
+      select: { type: 'array', required: false },
+      distinct: { type: 'boolean', required: false },
       limit: { type: 'integer', required: false, default: DEFAULT_LIMIT },
       offset: { type: 'integer', required: false, default: 0 },
     }
diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb
index 8ef6acdc38..b0d93a4b8f 100644
--- a/services/api/app/controllers/arvados/v1/jobs_controller.rb
+++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb
@@ -173,7 +173,7 @@ class Arvados::V1::JobsController < ApplicationController
                     cancelled_at: nil,
                     success: nil
                   })
-    params[:order] ||= 'priority desc, created_at'
+    params[:order] ||= ['priority desc', 'created_at']
     find_objects_for_index
     index
   end
diff --git a/services/api/app/controllers/arvados/v1/schema_controller.rb b/services/api/app/controllers/arvados/v1/schema_controller.rb
index a0f4fce1e0..1db5eff259 100644
--- a/services/api/app/controllers/arvados/v1/schema_controller.rb
+++ b/services/api/app/controllers/arvados/v1/schema_controller.rb
@@ -238,8 +238,8 @@ class Arvados::V1::SchemaController < ApplicationController
                   location: "query"
                 },
                 distinct: {
-                  type: "string",
-                  description: "Return each distinct value exactly once for the specified column (may skip rows)",
+                  type: "boolean",
+                  description: "Return each distinct object",
                   location: "query"
                 }
               },
diff --git a/services/api/lib/load_param.rb b/services/api/lib/load_param.rb
index 69d74f4349..11f18f707a 100644
--- a/services/api/lib/load_param.rb
+++ b/services/api/lib/load_param.rb
@@ -69,7 +69,18 @@ module LoadParam
 
     @orders = []
     if params[:order]
-      params[:order].split(',').each do |order|
+      od = []
+      (case params[:order]
+       when String
+         od = Oj.load(params[:order])
+         raise unless od.is_a? Array
+         od
+       when Array
+         params[:order]
+       else
+         []
+       end).each do |order|
+        order = order.to_s
         attr, direction = order.strip.split " "
         direction ||= 'asc'
         if attr.match /^[a-z][_a-z0-9]+$/ and
@@ -79,6 +90,7 @@ module LoadParam
         end
       end
     end
+
     if @orders.empty?
       @orders = default_orders
     end
@@ -104,10 +116,8 @@ module LoadParam
       }
     end
 
-    if params[:distinct].is_a? String
-      @distinct = params[:distinct]
-    end
-
+    @distinct = true if (params[:distinct] == true || params[:distinct] == "true")
+    @distinct = false if (params[:distinct] == false || params[:distinct] == "false")
   end
 
 
diff --git a/services/api/test/integration/select_test.rb b/services/api/test/integration/select_test.rb
index 28651f8b67..cf4d951906 100644
--- a/services/api/test/integration/select_test.rb
+++ b/services/api/test/integration/select_test.rb
@@ -5,18 +5,25 @@ class SelectTest < ActionDispatch::IntegrationTest
     get "/arvados/v1/links", {:format => :json, :select => ['uuid', 'link_class']}, auth(:active)
     assert_response :success
     assert_equal json_response['items'].count, json_response['items'].select { |i|
-      i['uuid'] != nil and i['link_class'] != nil and i['head_uuid'] == nil and i['tail_uuid'] == nil
+      i.count == 2 and i['uuid'] != nil and i['link_class'] != nil
     }.count
   end
 
-  test "should only get distinct values" do
-    get "/arvados/v1/links", {:format => :json, :select => ['link_class'], :distinct => "link_class"}, auth(:active)
+  test "fewer distinct than total count" do
+    get "/arvados/v1/links", {:format => :json, :select => ['link_class'], :distinct => false}, auth(:active)
     assert_response :success
-    assert_equal json_response['items'].uniq.count, json_response['items'].count
+    links = json_response['items']
+
+    get "/arvados/v1/links", {:format => :json, :select => ['link_class'], :distinct => true}, auth(:active)
+    assert_response :success
+    distinct = json_response['items']
+
+    assert distinct.count < links.count, "distinct count should be less than link count"
+    assert_equal links.uniq.count, distinct.count
   end
 
   test "select with order" do
-    get "/arvados/v1/links", {:format => :json, :select => ['uuid'], :order => "uuid asc"}, auth(:active)
+    get "/arvados/v1/links", {:format => :json, :select => ['uuid'], :order => ["uuid asc"]}, auth(:active)
     assert_response :success
 
     assert json_response['items'].length > 0
@@ -28,4 +35,26 @@ class SelectTest < ActionDispatch::IntegrationTest
     end
   end
 
+  test "select two columns with order" do
+    get "/arvados/v1/links", {:format => :json, :select => ['link_class', 'uuid'], :order => ['link_class asc', "uuid desc"]}, auth(:active)
+    assert_response :success
+
+    assert json_response['items'].length > 0
+
+    prev_link_class = ""
+    prev_uuid = "zzzzz-zzzzz-zzzzzzzzzzzzzzz"
+
+    json_response['items'].each do |i|
+      if prev_link_class != i['link_class']
+        prev_uuid = "zzzzz-zzzzz-zzzzzzzzzzzzzzz"
+      end
+
+      assert i['link_class'] >= prev_link_class
+      assert i['uuid'] < prev_uuid
+
+      prev_link_class = i['link_class']
+      prev_uuid = i['uuid']
+    end
+  end
+
 end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 869d87f7db..1bd1b51263 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -4,7 +4,7 @@ require 'rails/test_help'
 
 module ArvadosTestSupport
   def json_response
-    @json_response ||= ActiveSupport::JSON.decode @response.body
+    ActiveSupport::JSON.decode @response.body
   end
 
   def api_token(api_client_auth_name)
-- 
2.30.2