From 762e0ab6ac28f783c5d0c9fdec438f13418f9ad6 Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Thu, 4 Oct 2018 15:22:31 -0300 Subject: [PATCH] 13561: Expand index API to include past versions. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- .../app/controllers/application_controller.rb | 5 +++- .../arvados/v1/collections_controller.rb | 20 ++++++++++++- services/api/app/models/arvados_model.rb | 8 +++++- services/api/app/models/collection.rb | 13 +++++---- ...version_uuid_to_collection_search_index.rb | 17 +++++++++++ services/api/db/structure.sql | 4 ++- .../arvados/v1/collections_controller_test.rb | 28 +++++++++++++++++++ 7 files changed, 85 insertions(+), 10 deletions(-) create mode 100644 services/api/db/migrate/20181004131141_add_current_version_uuid_to_collection_search_index.rb diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 05b39c1aed..a6718dd4d1 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -189,7 +189,10 @@ class ApplicationController < ActionController::Base end def find_objects_for_index - @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || 'untrash' == action_name)}) + @objects ||= model_class.readable_by(*@read_users, { + :include_trash => (params[:include_trash] || 'untrash' == action_name), + :include_old_versions => params[:include_old_versions] + }) apply_where_limit_order_params end diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb index 6e77c12a1d..cfd1e9dc20 100644 --- a/services/api/app/controllers/arvados/v1/collections_controller.rb +++ b/services/api/app/controllers/arvados/v1/collections_controller.rb @@ -27,9 +27,14 @@ class Arvados::V1::CollectionsController < ApplicationController end def find_objects_for_index + opts = {} if params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name) - @objects = Collection.readable_by(*@read_users, {include_trash: true}) + opts.update({include_trash: true}) end + if params[:include_old_versions] + opts.update({include_old_versions: true}) + end + @objects = Collection.readable_by(*@read_users, opts) if !opts.empty? super end @@ -223,4 +228,17 @@ class Arvados::V1::CollectionsController < ApplicationController @select ||= model_class.selectable_attributes - ["manifest_text", "unsigned_manifest_text"] end end + + def load_filters_param + super + return if !params[:include_old_versions] + @filters = @filters.map do |col, operator, operand| + # Replace uuid filters when including past versions + if col == 'uuid' + ['current_version_uuid', operator, operand] + else + [col, operator, operand] + end + end + end end diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index c983372ad8..801da17dbe 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -369,7 +369,13 @@ class ArvadosModel < ActiveRecord::Base end self[:name] = new_name - self[:uuid] = nil if uuid_was.nil? && !uuid.nil? + if uuid_was.nil? && !uuid.nil? + self[:uuid] = nil + if self.is_a? Collection + # Reset so that is assigned to the new UUID + self[:current_version_uuid] = nil + end + end conn.exec_query 'SAVEPOINT save_with_unique_name' retry ensure diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb index 349c002ef4..2e79d74603 100644 --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@ -29,7 +29,6 @@ class Collection < ArvadosModel validate :ensure_storage_classes_contain_non_empty_strings validate :old_versions_cannot_be_updated, on: :update before_save :set_file_names - before_create :set_current_version_uuid api_accessible :user, extend: :common do |t| t.add :name @@ -219,10 +218,6 @@ class Collection < ArvadosModel ['current_version_uuid'] end - def set_current_version_uuid - self.current_version_uuid ||= self.uuid - end - def save! *args # Skip if feature is disabled or saving a new record if !Rails.configuration.collection_versioning || new_record? @@ -547,7 +542,7 @@ class Collection < ArvadosModel end def self.searchable_columns operator - super - ["manifest_text", "current_version_uuid"] + super - ["manifest_text"] end def self.full_text_searchable_columns @@ -632,4 +627,10 @@ class Collection < ArvadosModel raise ArvadosModel::PermissionDeniedError.new("previous versions cannot be updated") end end + + def assign_uuid + super + self.current_version_uuid ||= self.uuid + true + end end diff --git a/services/api/db/migrate/20181004131141_add_current_version_uuid_to_collection_search_index.rb b/services/api/db/migrate/20181004131141_add_current_version_uuid_to_collection_search_index.rb new file mode 100644 index 0000000000..63e99191a9 --- /dev/null +++ b/services/api/db/migrate/20181004131141_add_current_version_uuid_to_collection_search_index.rb @@ -0,0 +1,17 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +class AddCurrentVersionUuidToCollectionSearchIndex < ActiveRecord::Migration + disable_ddl_transaction! + + def up + remove_index :collections, :name => 'collections_search_index' + add_index :collections, ["owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid", "portable_data_hash", "uuid", "name", "current_version_uuid"], name: 'collections_search_index', algorithm: :concurrently + end + + def down + remove_index :collections, :name => 'collections_search_index' + add_index :collections, ["owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid", "portable_data_hash", "uuid", "name"], name: 'collections_search_index', algorithm: :concurrently + end +end diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index 67bc6856db..71d630f6e8 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -1634,7 +1634,7 @@ CREATE INDEX collections_full_text_search_idx ON public.collections USING gin (t -- Name: collections_search_index; Type: INDEX; Schema: public; Owner: - -- -CREATE INDEX collections_search_index ON public.collections USING btree (owner_uuid, modified_by_client_uuid, modified_by_user_uuid, portable_data_hash, uuid, name); +CREATE INDEX collections_search_index ON public.collections USING btree (owner_uuid, modified_by_client_uuid, modified_by_user_uuid, portable_data_hash, uuid, name, current_version_uuid); -- @@ -3187,3 +3187,5 @@ INSERT INTO schema_migrations (version) VALUES ('20180919001158'); INSERT INTO schema_migrations (version) VALUES ('20181001175023'); +INSERT INTO schema_migrations (version) VALUES ('20181004131141'); + 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 970368479c..24a1a09cfd 100644 --- a/services/api/test/functional/arvados/v1/collections_controller_test.rb +++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb @@ -45,6 +45,17 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase "basic Collections index included past version") end + test "get index with include_old_versions" do + authorize_with :active + get :index, { + include_old_versions: true + } + assert_response :success + assert(assigns(:objects).andand.any?, "no Collections returned in index") + assert(json_response["items"].any? { |c| c["uuid"] == collections(:collection_owned_by_active_past_version_1).uuid }, + "past version not included on index") + end + test "collections.get returns signed locators, and no unsigned_manifest_text" do permit_unsigned_manifests authorize_with :active @@ -1145,4 +1156,21 @@ EOS end assert_includes(item_uuids, collections(:collection_in_trashed_subproject).uuid) end + + test 'can get collection with past versions' do + authorize_with :active + get :index, { + filters: [['uuid','=',collections(:collection_owned_by_active).uuid]], + include_old_versions: true + } + assert_response :success + assert_equal 2, assigns(:objects).length + assert_equal 2, json_response['items_available'] + assert_equal 2, json_response['items'].count + json_response['items'].each do |c| + assert_equal collections(:collection_owned_by_active).uuid, + c['current_version_uuid'], + 'response includes a version from a different collection' + end + end end -- 2.30.2