From bd11c0a9ff6ce23f0992af11d7dc7aef32860e22 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Mon, 14 Apr 2014 09:31:42 -0400 Subject: [PATCH] * Refactored to remove load_kind_params filter and instead override load_where_param and load_filters_param in the links and logs controllers to add the _kind functionality. * Removed 'rescue' clause. I think I left that in from debugging. * Fixed :log -> :logs * filters: ['tail_kind','=','arvados#user'] works * Added more tests. * Added equivalent logic and tests for logs controller for object_kind --- .../app/controllers/application_controller.rb | 5 +- .../arvados/v1/links_controller.rb | 50 ++++++++++++------- .../controllers/arvados/v1/logs_controller.rb | 32 ++++++++++++ services/api/app/models/arvados_model.rb | 17 +++---- services/api/app/models/link.rb | 1 + services/api/app/models/log.rb | 9 +++- .../20140325175653_remove_kind_columns.rb | 4 +- services/api/test/fixtures/logs.yml | 3 ++ .../arvados/v1/links_controller_test.rb | 24 ++++++++- .../arvados/v1/logs_controller_test.rb | 27 ++++++++++ 10 files changed, 138 insertions(+), 34 deletions(-) create mode 100644 services/api/test/fixtures/logs.yml diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 9aad195075..0039bb0288 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -118,11 +118,12 @@ class ApplicationController < ActionController::Base end def load_filters_param + @filters ||= [] if params[:filters].is_a? Array - @filters = params[:filters] + @filters += params[:filters] elsif params[:filters].is_a? String and !params[:filters].empty? begin - @filters = Oj.load params[:filters] + @filters += Oj.load params[:filters] raise unless @filters.is_a? Array rescue raise ArgumentError.new("Could not parse \"filters\" param as an array") diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb index 1b79295af7..1b5bf78dbe 100644 --- a/services/api/app/controllers/arvados/v1/links_controller.rb +++ b/services/api/app/controllers/arvados/v1/links_controller.rb @@ -1,33 +1,49 @@ class Arvados::V1::LinksController < ApplicationController - prepend_before_filter :load_kind_params, :only => :index - def create resource_attrs.delete :head_kind resource_attrs.delete :tail_kind super end - def load_kind_params - if params[:tail_uuid] - params[:where] = Oj.load(params[:where]) if params[:where].is_a?(String) - @where ||= {} - @where[:tail_uuid] = params[:tail_uuid] - end + protected - if params[:where] and params[:where].is_a? Hash - if params[:where][:head_kind] - params[:filters] ||= [] - params[:filters] << ['head_uuid', 'is_a', params[:where][:head_kind]] - params[:where].delete :head_kind + # Overrides ApplicationController load_where_param + def load_where_param + super + + # head_kind and tail_kind columns are now virtual, + # equivilent functionality is now provided by + # 'is_a', so fix up any old-style 'where' clauses. + if @where + @filters ||= [] + if @where[:head_kind] + @filters << ['head_uuid', 'is_a', @where[:head_kind]] + @where.delete :head_kind end - if params[:where][:tail_kind] - params[:filters] ||= [] - params[:filters] << ['tail_uuid', 'is_a', params[:where][:tail_kind]] - params[:where].delete :tail_kind + if @where[:tail_kind] + @filters << ['tail_uuid', 'is_a', @where[:tail_kind]] + @where.delete :tail_kind end end + end + # Overrides ApplicationController load_filters_param + def load_filters_param + super + + # head_kind and tail_kind columns are now virtual, + # equivilent functionality is now provided by + # 'is_a', so fix up any old-style 'filter' clauses. + @filters = @filters.map do |k| + if k[0] == 'head_kind' and k[1] == '=' + ['head_uuid', 'is_a', k[2]] + elsif k[0] == 'tail_kind' and k[1] == '=' + ['tail_uuid', 'is_a', k[2]] + else + k + end + end end end diff --git a/services/api/app/controllers/arvados/v1/logs_controller.rb b/services/api/app/controllers/arvados/v1/logs_controller.rb index dffe662e7f..925eee523e 100644 --- a/services/api/app/controllers/arvados/v1/logs_controller.rb +++ b/services/api/app/controllers/arvados/v1/logs_controller.rb @@ -1,2 +1,34 @@ class Arvados::V1::LogsController < ApplicationController + # Overrides ApplicationController load_where_param + def load_where_param + super + + # object_kind and column is now virtual, + # equivilent functionality is now provided by + # 'is_a', so fix up any old-style 'where' clauses. + if @where + @filters ||= [] + if @where[:object_kind] + @filters << ['object_uuid', 'is_a', @where[:object_kind]] + @where.delete :object_kind + end + end + end + + # Overrides ApplicationController load_filters_param + def load_filters_param + super + + # object_kind and column is now virtual, + # equivilent functionality is now provided by + # 'is_a', so fix up any old-style 'filter' clauses. + @filters = @filters.map do |k| + if k[0] == 'object_kind' and k[1] == '=' + ['object_uuid', 'is_a', k[2]] + else + k + end + end + end + end diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 08bb5ea96d..dbb807c0b6 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -222,18 +222,13 @@ class ArvadosModel < ActiveRecord::Base specials = [system_user_uuid, 'd41d8cd98f00b204e9800998ecf8427e+0'] foreign_key_attributes.each do |attr| - begin - if new_record? or send (attr + "_changed?") - attr_value = send attr - r = ArvadosModel::resource_class_for_uuid attr_value if attr_value - r = r.readable_by(current_user) if r and not skip_uuid_read_permission_check.include? attr - if r and r.where(uuid: attr_value).count == 0 and not specials.include? attr_value - errors.add(attr, "'#{attr_value}' not found") - end + if new_record? or send (attr + "_changed?") + attr_value = send attr + r = ArvadosModel::resource_class_for_uuid attr_value if attr_value + r = r.readable_by(current_user) if r and not skip_uuid_read_permission_check.include? attr + if r and r.where(uuid: attr_value).count == 0 and not specials.include? attr_value + errors.add(attr, "'#{attr_value}' not found") end - rescue Exception => e - bt = e.backtrace.join("\n") - errors.add(attr, "'#{attr_value}' error '#{e}'\n#{bt}\n") end end end diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index cf4ffce8ed..26e7183be3 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -8,6 +8,7 @@ class Link < ArvadosModel after_update :maybe_invalidate_permissions_cache after_create :maybe_invalidate_permissions_cache after_destroy :maybe_invalidate_permissions_cache + attr_accessor :head_kind, :tail_kind api_accessible :user, extend: :common do |t| t.add :tail_uuid diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb index 923a681b0d..f8e337b201 100644 --- a/services/api/app/models/log.rb +++ b/services/api/app/models/log.rb @@ -4,17 +4,24 @@ class Log < ArvadosModel include CommonApiTemplate serialize :properties, Hash before_validation :set_default_event_at - attr_accessor :object + attr_accessor :object, :object_kind api_accessible :user, extend: :common do |t| t.add :object_uuid t.add :object, :if => :object + t.add :object_kind t.add :event_at t.add :event_type t.add :summary t.add :properties end + def object_kind + if k = ArvadosModel::resource_class_for_uuid(object_uuid) + k.kind + end + end + def fill_object(thing) self.object_uuid ||= thing.uuid self.summary ||= "#{self.event_type} of #{thing.uuid}" diff --git a/services/api/db/migrate/20140325175653_remove_kind_columns.rb b/services/api/db/migrate/20140325175653_remove_kind_columns.rb index d77130d5c2..eae2a2c87e 100644 --- a/services/api/db/migrate/20140325175653_remove_kind_columns.rb +++ b/services/api/db/migrate/20140325175653_remove_kind_columns.rb @@ -4,13 +4,13 @@ class RemoveKindColumns < ActiveRecord::Migration def up remove_column :links, :head_kind remove_column :links, :tail_kind - remove_column :log, :object_kind + remove_column :logs, :object_kind end def down add_column :links, :head_kind, :string add_column :links, :tail_kind, :string - add_column :log, :object_kind, :string + add_column :logs, :object_kind, :string act_as_system_user do Link.all.each do |l| diff --git a/services/api/test/fixtures/logs.yml b/services/api/test/fixtures/logs.yml new file mode 100644 index 0000000000..d805439a6c --- /dev/null +++ b/services/api/test/fixtures/logs.yml @@ -0,0 +1,3 @@ +log1: + uuid: zzzzz-xxxxx-pshmckwoma9plh7 + object_uuid: zzzzz-tpzed-l1s2piq4t4mps8r \ No newline at end of file diff --git a/services/api/test/functional/arvados/v1/links_controller_test.rb b/services/api/test/functional/arvados/v1/links_controller_test.rb index 4c0c8dc825..09dd1621d6 100644 --- a/services/api/test/functional/arvados/v1/links_controller_test.rb +++ b/services/api/test/functional/arvados/v1/links_controller_test.rb @@ -44,7 +44,7 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase assert_response 422 end - test "head and tail exist" do + test "head and tail exist, head_kind and tail_kind are returned" do link = { link_class: 'test', name: 'stuff', @@ -153,5 +153,27 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase assert_equal found.count, (found.select { |f| f.head_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count end + test "test can still use filter tail_kind" do + authorize_with :admin + get :index, { + filters: [ ['tail_kind', '=', 'arvados#user'] ] + } + assert_response :success + found = assigns(:objects) + assert_not_equal 0, found.count + assert_equal found.count, (found.select { |f| f.tail_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count + end + + test "test can still use filter head_kind" do + authorize_with :admin + get :index, { + filters: [ ['head_kind', '=', 'arvados#user'] ] + } + assert_response :success + found = assigns(:objects) + assert_not_equal 0, found.count + assert_equal found.count, (found.select { |f| f.head_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count + end + end diff --git a/services/api/test/functional/arvados/v1/logs_controller_test.rb b/services/api/test/functional/arvados/v1/logs_controller_test.rb index 9c410996b8..a224e2573f 100644 --- a/services/api/test/functional/arvados/v1/logs_controller_test.rb +++ b/services/api/test/functional/arvados/v1/logs_controller_test.rb @@ -1,6 +1,8 @@ require 'test_helper' class Arvados::V1::LogsControllerTest < ActionController::TestCase + fixtures :logs + test "non-admins can read their own logs" do authorize_with :active post :create, log: {summary: "test log"} @@ -12,4 +14,29 @@ class Arvados::V1::LogsControllerTest < ActionController::TestCase assert_equal("test log", assigns(:object).summary, "loaded wrong log after creation") end + + test "test can still use where object_kind" do + authorize_with :admin + get :index, { + where: { object_kind: 'arvados#user' } + } + assert_response :success + found = assigns(:objects) + assert_not_equal 0, found.count + assert_equal found.count, (found.select { |f| f.object_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count + l = JSON.parse(@response.body) + assert_equal 'arvados#user', l['items'][0]['object_kind'] + end + + test "test can still use filter object_kind" do + authorize_with :admin + get :index, { + filters: [ ['object_kind', '=', 'arvados#user'] ] + } + assert_response :success + found = assigns(:objects) + assert_not_equal 0, found.count + assert_equal found.count, (found.select { |f| f.object_uuid.match /[a-z0-9]{5}-tpzed-[a-z0-9]{15}/}).count + end + end -- 2.30.2