From d7c84d69bb62d61bc671b2d5e0ad4ed42dbeb7c0 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Wed, 1 Mar 2017 16:09:05 -0500 Subject: [PATCH] 11168: Change db serialize from YAML to JSON. --- .../app/controllers/application_controller.rb | 6 ++- .../api_client_authorizations_controller.rb | 4 +- services/api/app/models/arvados_model.rb | 14 +++++++ services/api/app/models/container.rb | 9 ++-- services/api/app/models/job.rb | 4 +- services/api/config/initializers/lograge.rb | 4 +- services/api/lib/create_superuser_token.rb | 13 +++--- services/api/lib/eventbus.rb | 9 ++-- services/api/lib/load_param.rb | 8 ++-- services/api/lib/safe_json.rb | 8 ++++ services/api/lib/serializers.rb | 41 +++++++++++++++++++ .../collections_performance_test.rb | 5 ++- .../api/test/integration/websocket_test.rb | 41 ++++++++++--------- .../test/unit/create_superuser_token_test.rb | 2 +- 14 files changed, 122 insertions(+), 46 deletions(-) create mode 100644 services/api/lib/safe_json.rb create mode 100644 services/api/lib/serializers.rb diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 22851191b6..2072520bb3 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -1,3 +1,5 @@ +require 'safe_json' + module ApiTemplateOverride def allowed_to_render?(fieldset, field, model, options) return false if !super @@ -209,7 +211,7 @@ class ApplicationController < ActionController::Base # The obvious render(json: ...) forces a slow JSON encoder. See # #3021 and commit logs. Might be fixed in Rails 4.1. render({ - text: Oj.dump(response, mode: :compat).html_safe, + text: SafeJSON.dump(response).html_safe, content_type: 'application/json' }.merge opts) end @@ -467,7 +469,7 @@ class ApplicationController < ActionController::Base def load_json_value(hash, key, must_be_class=nil) if hash[key].is_a? String - hash[key] = Oj.strict_load(hash[key], symbol_keys: false) + hash[key] = SafeJSON.load(hash[key]) if must_be_class and !hash[key].is_a? must_be_class raise TypeError.new("parameter #{key.to_s} must be a #{must_be_class.to_s}") end diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb index 76acc701fd..334538ba13 100644 --- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb +++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb @@ -1,3 +1,5 @@ +require 'safe_json' + class Arvados::V1::ApiClientAuthorizationsController < ApplicationController accept_attribute_as_json :scopes, Array before_filter :current_api_client_is_trusted, :except => [:current] @@ -16,7 +18,7 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController new(user_id: system_user.id, api_client_id: params[:api_client_id] || current_api_client.andand.id, created_by_ip_address: remote_ip, - scopes: Oj.strict_load(params[:scopes] || '["all"]')) + scopes: SafeJSON.load(params[:scopes] || '["all"]')) @object.save! show end diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index b2e1bea3ab..b39c6ab2f9 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -1,5 +1,6 @@ require 'has_uuid' require 'record_filters' +require 'serializers' class ArvadosModel < ActiveRecord::Base self.abstract_class = true @@ -454,6 +455,19 @@ class ArvadosModel < ActiveRecord::Base end end + def self.where_serialized(colname, value) + where("#{colname.to_s} IN (?)", [value.to_yaml, SafeJSON.dump(value)]) + end + + Serializer = { + Hash => HashSerializer, + Array => ArraySerializer, + } + + def self.serialize(colname, type) + super(colname, Serializer[type]) + 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 diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index ea48ffa0e0..e6ce8f030c 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -1,4 +1,5 @@ require 'whitelist_update' +require 'safe_json' class Container < ArvadosModel include HasUuid @@ -83,13 +84,13 @@ class Container < ArvadosModel def self.find_reusable(attrs) candidates = Container. - where('command = ?', attrs[:command].to_yaml). + where_serialized(:command, attrs[:command]). where('cwd = ?', attrs[:cwd]). - where('environment = ?', self.deep_sort_hash(attrs[:environment]).to_yaml). + where_serialized(:environment, self.deep_sort_hash(attrs[:environment])). where('output_path = ?', attrs[:output_path]). where('container_image = ?', attrs[:container_image]). - where('mounts = ?', self.deep_sort_hash(attrs[:mounts]).to_yaml). - where('runtime_constraints = ?', self.deep_sort_hash(attrs[:runtime_constraints]).to_yaml) + where_serialized(:mounts, self.deep_sort_hash(attrs[:mounts])). + where_serialized(:runtime_constraints, self.deep_sort_hash(attrs[:runtime_constraints])) # Check for Completed candidates whose output and log are both readable. select_readable_pdh = Collection. diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb index d078a6146b..55ae5c702e 100644 --- a/services/api/app/models/job.rb +++ b/services/api/app/models/job.rb @@ -1,3 +1,5 @@ +require 'safe_json' + class Job < ArvadosModel include HasUuid include KindAndEtag @@ -321,7 +323,7 @@ class Job < ArvadosModel protected def self.sorted_hash_digest h - Digest::MD5.hexdigest(Oj.dump(deep_sort_hash(h))) + Digest::MD5.hexdigest(SafeJSON.dump(deep_sort_hash(h))) end def foreign_key_attributes diff --git a/services/api/config/initializers/lograge.rb b/services/api/config/initializers/lograge.rb index 4b1aea9e70..6d1dd4afc4 100644 --- a/services/api/config/initializers/lograge.rb +++ b/services/api/config/initializers/lograge.rb @@ -1,10 +1,12 @@ +require 'safe_json' + Server::Application.configure do config.lograge.enabled = true config.lograge.formatter = Lograge::Formatters::Logstash.new config.lograge.custom_options = lambda do |event| exceptions = %w(controller action format id) params = event.payload[:params].except(*exceptions) - params_s = Oj.dump(params) + params_s = SafeJSON.dump(params) if params_s.length > Rails.configuration.max_request_log_params_size { params_truncated: params_s[0..Rails.configuration.max_request_log_params_size] + "[...]" } else diff --git a/services/api/lib/create_superuser_token.rb b/services/api/lib/create_superuser_token.rb index 72b1ae7bc9..84100c2efd 100755 --- a/services/api/lib/create_superuser_token.rb +++ b/services/api/lib/create_superuser_token.rb @@ -29,12 +29,13 @@ module CreateSuperUserToken apiClient = ApiClient.find_or_create_by_url_prefix_and_is_trusted("ssh://root@localhost/", true) # Check if there is an unexpired superuser token corresponding to this api client - api_client_auth = ApiClientAuthorization.where( - 'user_id = ? AND - api_client_id = ? AND - scopes = ? AND - (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)', - system_user.id, apiClient.id, ['all'].to_yaml).first + api_client_auth = + ApiClientAuthorization. + where(user_id: system_user.id). + where(api_client_id: apiClient.id). + where_serialized(:scopes, ['all']). + where('(expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)'). + first # none exist; create one with the supplied token if !api_client_auth diff --git a/services/api/lib/eventbus.rb b/services/api/lib/eventbus.rb index 5e413d5cab..11b178d985 100644 --- a/services/api/lib/eventbus.rb +++ b/services/api/lib/eventbus.rb @@ -3,10 +3,11 @@ Thread.abort_on_exception = true require 'eventmachine' -require 'oj' require 'faye/websocket' -require 'record_filters' require 'load_param' +require 'oj' +require 'record_filters' +require 'safe_json' require 'set' require 'thread' @@ -79,7 +80,7 @@ class EventBus end def send_message(ws, obj) - ws.send(Oj.dump(obj, mode: :compat)) + ws.send(SafeJSON.dump(obj)) end # Push out any pending events to the connection +ws+ @@ -181,7 +182,7 @@ class EventBus begin begin # Parse event data as JSON - p = (Oj.strict_load event.data).symbolize_keys + p = SafeJSON.load(event.data).symbolize_keys filter = Filter.new(p) rescue Oj::Error => e send_message(ws, {status: 400, message: "malformed request"}) diff --git a/services/api/lib/load_param.rb b/services/api/lib/load_param.rb index dee0f23b1d..3e1e8b5eba 100644 --- a/services/api/lib/load_param.rb +++ b/services/api/lib/load_param.rb @@ -17,7 +17,7 @@ module LoadParam @where = params[:where] elsif params[:where].is_a? String begin - @where = Oj.strict_load(params[:where]) + @where = SafeJSON.load(params[:where]) raise unless @where.is_a? Hash rescue raise ArgumentError.new("Could not parse \"where\" param as an object") @@ -33,7 +33,7 @@ module LoadParam @filters += params[:filters] elsif params[:filters].is_a? String and !params[:filters].empty? begin - f = Oj.strict_load params[:filters] + f = SafeJSON.load(params[:filters]) if not f.nil? raise unless f.is_a? Array @filters += f @@ -72,7 +72,7 @@ module LoadParam (case params[:order] when String if params[:order].starts_with? '[' - od = Oj.strict_load(params[:order]) + od = SafeJSON.load(params[:order]) raise unless od.is_a? Array od else @@ -142,7 +142,7 @@ module LoadParam @select = params[:select] when String begin - @select = Oj.strict_load params[:select] + @select = SafeJSON.load(params[:select]) raise unless @select.is_a? Array or @select.nil? rescue raise ArgumentError.new("Could not parse \"select\" param as an array") diff --git a/services/api/lib/safe_json.rb b/services/api/lib/safe_json.rb new file mode 100644 index 0000000000..dbf53e089b --- /dev/null +++ b/services/api/lib/safe_json.rb @@ -0,0 +1,8 @@ +class SafeJSON + def self.dump(o) + return Oj.dump(o, mode: :compat) + end + def self.load(s) + Oj.strict_load(s, symbol_keys: false) + end +end diff --git a/services/api/lib/serializers.rb b/services/api/lib/serializers.rb new file mode 100644 index 0000000000..d7a81e12f9 --- /dev/null +++ b/services/api/lib/serializers.rb @@ -0,0 +1,41 @@ +require 'safe_json' + +class HashSerializer + def self.load(s) + if s.nil? + {} + elsif s[0] == "{" + SafeJSON.load(s) + elsif s[0..2] == "---" + Psych.safe_load(s) + else + raise "invalid serialized data #{s[0..5].inspect}" + end + end + def self.dump(h) + SafeJSON.dump(h) + end + def self.object_class + ::Hash + end +end + +class ArraySerializer + def self.load(s) + if s.nil? + [] + elsif s[0] == "[" + SafeJSON.load(s) + elsif s[0..2] == "---" + Psych.safe_load(s) + else + raise "invalid serialized data #{s[0..5].inspect}" + end + end + def self.dump(a) + SafeJSON.dump(a) + end + def self.object_class + ::Array + end +end diff --git a/services/api/test/integration/collections_performance_test.rb b/services/api/test/integration/collections_performance_test.rb index f6f39fe526..c284750c97 100644 --- a/services/api/test/integration/collections_performance_test.rb +++ b/services/api/test/integration/collections_performance_test.rb @@ -1,3 +1,4 @@ +require 'safe_json' require 'test_helper' require 'helpers/manifest_examples' require 'helpers/time_block' @@ -14,7 +15,7 @@ class CollectionsApiPerformanceTest < ActionDispatch::IntegrationTest api_token: api_token(:active)) end json = time_block "JSON encode #{bigmanifest.length>>20}MiB manifest" do - Oj.dump({"manifest_text" => bigmanifest}) + SafeJSON.dump({"manifest_text" => bigmanifest}) end time_block 'create' do post '/arvados/v1/collections', {collection: json}, auth(:active) @@ -45,7 +46,7 @@ class CollectionsApiPerformanceTest < ActionDispatch::IntegrationTest bytes_per_block: 2**26, api_token: api_token(:active)) json = time_block "JSON encode #{hugemanifest.length>>20}MiB manifest" do - Oj.dump({manifest_text: hugemanifest}) + SafeJSON.dump({manifest_text: hugemanifest}) end vmpeak "post" do post '/arvados/v1/collections', {collection: json}, auth(:active) diff --git a/services/api/test/integration/websocket_test.rb b/services/api/test/integration/websocket_test.rb index a9993b2fc3..841a80859b 100644 --- a/services/api/test/integration/websocket_test.rb +++ b/services/api/test/integration/websocket_test.rb @@ -1,6 +1,7 @@ -require 'test_helper' -require 'oj' require 'database_cleaner' +require 'oj' +require 'safe_json' +require 'test_helper' DatabaseCleaner.strategy = :deletion @@ -112,7 +113,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest ws_helper do |ws| ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data status = d["status"] ws.close end @@ -130,7 +131,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data status = d["status"] ws.close end @@ -152,7 +153,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -189,7 +190,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -229,7 +230,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -268,7 +269,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -312,7 +313,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -350,7 +351,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -394,7 +395,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -442,7 +443,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -487,7 +488,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -557,7 +558,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -581,7 +582,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data status = d["status"] ws.close end @@ -599,7 +600,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data status = d["status"] ws.close end @@ -617,7 +618,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data status = d["status"] ws.close end @@ -640,7 +641,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when (1..Rails.configuration.websocket_max_filters) assert_equal 200, d["status"] @@ -675,7 +676,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] @@ -712,7 +713,7 @@ class WebsocketTest < ActionDispatch::IntegrationTest end ws.on :message do |event| - d = Oj.strict_load event.data + d = SafeJSON.load event.data case state when 1 assert_equal 200, d["status"] diff --git a/services/api/test/unit/create_superuser_token_test.rb b/services/api/test/unit/create_superuser_token_test.rb index ba813602b3..bd55b151f6 100644 --- a/services/api/test/unit/create_superuser_token_test.rb +++ b/services/api/test/unit/create_superuser_token_test.rb @@ -89,7 +89,7 @@ class CreateSuperUserTokenTest < ActiveSupport::TestCase active_user_token = api_client_authorizations("admin_vm").api_token ApiClientAuthorization. where(user_id: system_user.id). - update_all(scopes: ["GET /"]) + update_all(scopes: SafeJSON.dump(["GET /"])) fixture_tokens = ApiClientAuthorization.all.collect(&:api_token) new_token = create_superuser_token refute_includes(fixture_tokens, new_token) -- 2.30.2