From: Tom Clegg Date: Fri, 30 May 2014 18:55:21 +0000 (-0400) Subject: 2893: Merge branch '2893-no-symbols-in-db' closes #2893 X-Git-Tag: 1.1.0~2584^2~9 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/21485541dec5b6df36aaba7d4c2a1e96ba65dec6?hp=c7ee5e02cae78d3edff6ed393d776c4995441896 2893: Merge branch '2893-no-symbols-in-db' closes #2893 --- diff --git a/services/api/app/controllers/arvados/v1/nodes_controller.rb b/services/api/app/controllers/arvados/v1/nodes_controller.rb index 990397bf7c..3fbf5fcc6b 100644 --- a/services/api/app/controllers/arvados/v1/nodes_controller.rb +++ b/services/api/app/controllers/arvados/v1/nodes_controller.rb @@ -23,7 +23,7 @@ class Arvados::V1::NodesController < ApplicationController @object.ping({ ip: params[:local_ipv4] || request.env['REMOTE_ADDR'], ping_secret: params[:ping_secret], ec2_instance_id: params[:instance_id] }) - if @object.info[:ping_secret] == params[:ping_secret] + if @object.info['ping_secret'] == params[:ping_secret] render json: @object.as_api_response(:superuser) else raise "Invalid ping_secret after ping" diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index a6c9d31d41..95fd055d49 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -20,6 +20,7 @@ class ArvadosModel < ActiveRecord::Base after_create :log_create after_update :log_update after_destroy :log_destroy + after_find :convert_serialized_symbols_to_strings validate :ensure_serialized_attribute_type validate :normalize_collection_uuids validate :ensure_valid_uuids @@ -263,6 +264,38 @@ class ArvadosModel < ActiveRecord::Base true end + def self.has_symbols? x + if x.is_a? Hash + x.each do |k,v| + return true if has_symbols?(k) or has_symbols?(v) + end + false + elsif x.is_a? Array + x.each do |k| + return true if has_symbols?(k) + end + false + else + (x.class == Symbol) + end + end + + def self.recursive_stringify x + if x.is_a? Hash + Hash[x.collect do |k,v| + [recursive_stringify(k), recursive_stringify(v)] + end] + elsif x.is_a? Array + x.collect do |k| + recursive_stringify k + end + elsif x.is_a? Symbol + x.to_s + else + x + end + end + def ensure_serialized_attribute_type # Specifying a type in the "serialize" declaration causes rails to # raise an exception if a different data type is retrieved from @@ -272,13 +305,31 @@ class ArvadosModel < ActiveRecord::Base # developer. self.class.serialized_attributes.each do |colname, attr| if attr.object_class - unless self.attributes[colname].is_a? attr.object_class - self.errors.add colname.to_sym, "must be a #{attr.object_class.to_s}" + if self.attributes[colname].class != attr.object_class + self.errors.add colname.to_sym, "must be a #{attr.object_class.to_s}, not a #{self.attributes[colname].class.to_s}" + elsif self.class.has_symbols? attributes[colname] + self.errors.add colname.to_sym, "must not contain symbols: #{attributes[colname].inspect}" end end end end + def convert_serialized_symbols_to_strings + # ensure_serialized_attribute_type should prevent symbols from + # getting into the database in the first place. If someone managed + # to get them into the database (perhaps using an older version) + # we'll convert symbols to strings when loading from the + # database. (Otherwise, loading and saving an object with existing + # symbols in a serialized field will crash.) + self.class.serialized_attributes.each do |colname, attr| + if self.class.has_symbols? attributes[colname] + attributes[colname] = self.class.recursive_stringify attributes[colname] + self.send(colname + '=', + self.class.recursive_stringify(attributes[colname])) + end + end + end + def foreign_key_attributes attributes.keys.select { |a| a.match /_uuid$/ } end diff --git a/services/api/app/models/node.rb b/services/api/app/models/node.rb index 512f0e0a59..2ca05f62d5 100644 --- a/services/api/app/models/node.rb +++ b/services/api/app/models/node.rb @@ -37,7 +37,7 @@ class Node < ArvadosModel end def crunch_worker_state - case self.info.andand[:slurm_state] + case self.info.andand['slurm_state'] when 'alloc', 'comp' 'busy' when 'idle' @@ -64,8 +64,8 @@ class Node < ArvadosModel def ping(o) raise "must have :ip and :ping_secret" unless o[:ip] and o[:ping_secret] - if o[:ping_secret] != self.info[:ping_secret] - logger.info "Ping: secret mismatch: received \"#{o[:ping_secret]}\" != \"#{self.info[:ping_secret]}\"" + if o[:ping_secret] != self.info['ping_secret'] + logger.info "Ping: secret mismatch: received \"#{o[:ping_secret]}\" != \"#{self.info['ping_secret']}\"" raise ArvadosModel::UnauthorizedError.new("Incorrect ping_secret") end self.last_ping_at = Time.now @@ -81,16 +81,16 @@ class Node < ArvadosModel # Record instance ID if not already known if o[:ec2_instance_id] - if !self.info[:ec2_instance_id] - self.info[:ec2_instance_id] = o[:ec2_instance_id] + if !self.info['ec2_instance_id'] + self.info['ec2_instance_id'] = o[:ec2_instance_id] if (Rails.configuration.compute_node_ec2_tag_enable rescue true) tag_cmd = ("ec2-create-tags #{o[:ec2_instance_id]} " + "--tag 'Name=#{self.uuid}'") `#{tag_cmd}` end - elsif self.info[:ec2_instance_id] != o[:ec2_instance_id] + elsif self.info['ec2_instance_id'] != o[:ec2_instance_id] logger.debug "Multiple nodes have credentials for #{self.uuid}" - raise "#{self.uuid} is already running at #{self.info[:ec2_instance_id]} so rejecting ping from #{o[:ec2_instance_id]}" + raise "#{self.uuid} is already running at #{self.info['ec2_instance_id']} so rejecting ping from #{o[:ec2_instance_id]}" end end @@ -108,9 +108,9 @@ class Node < ArvadosModel raise "No available node slots" if try_slot == MAX_SLOTS end while true self.hostname = self.class.hostname_for_slot(self.slot_number) - if info[:ec2_instance_id] + if info['ec2_instance_id'] if (Rails.configuration.compute_node_ec2_tag_enable rescue true) - `ec2-create-tags #{self.info[:ec2_instance_id]} --tag 'hostname=#{self.hostname}'` + `ec2-create-tags #{self.info['ec2_instance_id']} --tag 'hostname=#{self.hostname}'` end end end @@ -120,7 +120,7 @@ class Node < ArvadosModel def start!(ping_url_method) ensure_permission_to_save - ping_url = ping_url_method.call({ id: self.uuid, ping_secret: self.info[:ping_secret] }) + ping_url = ping_url_method.call({ id: self.uuid, ping_secret: self.info['ping_secret'] }) if (Rails.configuration.compute_node_ec2run_args and Rails.configuration.compute_node_ami) ec2_args = ["--user-data '#{ping_url}'", @@ -138,23 +138,23 @@ class Node < ArvadosModel ec2run_cmd = '' ec2spot_cmd = '' end - self.info[:ec2_run_command] = ec2run_cmd - self.info[:ec2_spot_command] = ec2spot_cmd - self.info[:ec2_start_command] = ec2spot_cmd + self.info['ec2_run_command'] = ec2run_cmd + self.info['ec2_spot_command'] = ec2spot_cmd + self.info['ec2_start_command'] = ec2spot_cmd logger.info "#{self.uuid} ec2_start_command= #{ec2spot_cmd.inspect}" result = `#{ec2spot_cmd} 2>&1` - self.info[:ec2_start_result] = result + self.info['ec2_start_result'] = result logger.info "#{self.uuid} ec2_start_result= #{result.inspect}" result.match(/INSTANCE\s*(i-[0-9a-f]+)/) do |m| instance_id = m[1] - self.info[:ec2_instance_id] = instance_id + self.info['ec2_instance_id'] = instance_id if (Rails.configuration.compute_node_ec2_tag_enable rescue true) `ec2-create-tags #{instance_id} --tag 'Name=#{self.uuid}'` end end result.match(/SPOTINSTANCEREQUEST\s*(sir-[0-9a-f]+)/) do |m| sir_id = m[1] - self.info[:ec2_sir_id] = sir_id + self.info['ec2_sir_id'] = sir_id if (Rails.configuration.compute_node_ec2_tag_enable rescue true) `ec2-create-tags #{sir_id} --tag 'Name=#{self.uuid}'` end @@ -165,7 +165,7 @@ class Node < ArvadosModel protected def ensure_ping_secret - self.info[:ping_secret] ||= rand(2**256).to_s(36) + self.info['ping_secret'] ||= rand(2**256).to_s(36) end def dnsmasq_update diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index d6dd9dc8a8..d219915195 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -339,20 +339,20 @@ class User < ArvadosModel perm_exists = false login_perms.each do |perm| if perm.properties['username'] == repo_name - perm_exists = true + perm_exists = perm break end end - if !perm_exists + if perm_exists + login_perm = perm_exists + else login_perm = Link.create(tail_uuid: self.uuid, head_uuid: vm[:uuid], link_class: 'permission', name: 'can_login', properties: {'username' => repo_name}) logger.info { "login permission: " + login_perm[:uuid] } - else - login_perm = login_perms.first end return login_perm diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml index fe77407fad..1fb1608f4d 100644 --- a/services/api/test/fixtures/links.yml +++ b/services/api/test/fixtures/links.yml @@ -404,6 +404,29 @@ multilevel_collection_1_readable_by_active: head_uuid: 1fd08fc162a5c6413070a8bd0bffc818+150 properties: {} +has_symbol_keys_in_database_somehow: + uuid: zzzzz-o0j2j-enl1wg58310loc6 + owner_uuid: zzzzz-tpzed-000000000000000 + created_at: 2014-05-28 16:24:02.314722162 Z + modified_by_client_uuid: + modified_by_user_uuid: zzzzz-tpzed-000000000000000 + modified_at: 2014-05-28 16:24:02.314484982 Z + tail_uuid: ~ + link_class: test + name: ~ + head_uuid: ~ + properties: + :foo: "bar" + baz: + - waz + - :waz + - :waz + - 1 + - ~ + - false + - true + updated_at: 2014-05-28 16:24:02.314296411 Z + bug2931_link_with_null_head_uuid: uuid: zzzzz-o0j2j-uru66qok2wruasb owner_uuid: zzzzz-tpzed-000000000000000 diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb index 1fefcb6c68..f02d62bdc6 100644 --- a/services/api/test/functional/arvados/v1/users_controller_test.rb +++ b/services/api/test/functional/arvados/v1/users_controller_test.rb @@ -144,6 +144,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase email: "foo@example.com" } } + assert_response :success response_items = JSON.parse(@response.body)['items'] @@ -795,6 +796,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase def find_obj_in_resp (response_items, object_type, head_kind=nil) return_obj = nil + response_items response_items.each { |x| if !x next diff --git a/services/api/test/unit/arvados_model_test.rb b/services/api/test/unit/arvados_model_test.rb index e9e872f04e..85d07a5746 100644 --- a/services/api/test/unit/arvados_model_test.rb +++ b/services/api/test/unit/arvados_model_test.rb @@ -31,4 +31,48 @@ class ArvadosModelTest < ActiveSupport::TestCase assert a.uuid.length==27, "Auto assigned uuid length is wrong." end + [ {:a => 'foo'}, + {'a' => :foo}, + {:a => ['foo', 'bar']}, + {'a' => [:foo, 'bar']}, + {'a' => ['foo', :bar]}, + {:a => [:foo, :bar]}, + {:a => {'foo' => {'bar' => 'baz'}}}, + {'a' => {:foo => {'bar' => 'baz'}}}, + {'a' => {'foo' => {:bar => 'baz'}}}, + {'a' => {'foo' => {'bar' => :baz}}}, + {'a' => {'foo' => ['bar', :baz]}}, + {'a' => {['foo', :foo] => ['bar', 'baz']}}, + ].each do |x| + test "refuse symbol keys in serialized attribute: #{x.inspect}" do + set_user_from_auth :admin_trustedclient + assert_nothing_raised do + Link.create!(link_class: 'test', + properties: {}) + end + assert_raises ActiveRecord::RecordInvalid do + Link.create!(link_class: 'test', + properties: x) + end + end + end + + test "Stringify symbols coming from serialized attribute in database" do + set_user_from_auth :admin_trustedclient + fixed = Link.find_by_uuid(links(:has_symbol_keys_in_database_somehow).uuid) + assert_equal(["baz", "foo"], fixed.properties.keys.sort, + "Hash symbol keys from DB did not get stringified.") + assert_equal(['waz', 'waz', 'waz', 1, nil, false, true], + fixed.properties['baz'], + "Array symbol values from DB did not get stringified.") + assert_equal true, fixed.save, "Failed to save fixed model back to db." + end + + test "No HashWithIndifferentAccess in database" do + set_user_from_auth :admin_trustedclient + assert_raises ActiveRecord::RecordInvalid do + Link.create!(link_class: 'test', + properties: {'foo' => 'bar'}.with_indifferent_access) + end + end end