From 6b1468b8397077b48b0457f81f3a13e03d5e6773 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Fri, 27 Aug 2021 13:28:42 +0200 Subject: [PATCH 1/2] Properly test JSON responses and fix wrong JSON string - Parse JSON response instead of searching for a substring. - Decode 'X-Broker-Api-Originating-Identity' header value and check contained 'user_id' instead of comparing two base64 encoded strings. - Add missing '}' to JSON string. --- .../broker_api_compatibility/broker_api_v2.14_spec.rb | 7 ++++--- .../broker_api_compatibility/broker_api_versions_spec.rb | 2 +- spec/unit/controllers/runtime/routes_controller_spec.rb | 8 ++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb index 8f7bcde2b4d..917240d90b8 100644 --- a/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb +++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb @@ -48,7 +48,6 @@ it 'sends the broker the X-Broker-Api-Originating-Identity header' do user = VCAP::CloudController::User.make - base64_encoded_user_id = Base64.strict_encode64("{\"user_id\":\"#{user.guid}\"}") get("/v2/service_bindings/#{@binding_guid}/parameters", {}.to_json, @@ -56,7 +55,9 @@ expect( a_request(:get, %r{/v2/service_instances/#{@service_instance_guid}/service_bindings/[[:alnum:]-]+}).with do |req| - req.headers['X-Broker-Api-Originating-Identity'] == "cloudfoundry #{base64_encoded_user_id}" + m = req.headers['X-Broker-Api-Originating-Identity'].match /(?\S+) (?\S+)/ + value = MultiJson.load(Base64.strict_decode64(m[:value])) + m[:platform] == 'cloudfoundry' && value['user_id'] == user.guid end ).to have_been_made end @@ -177,7 +178,7 @@ async_bind_service(status: 202) service_binding = VCAP::CloudController::ServiceBinding.find(guid: @binding_guid) - stub_request(:get, service_binding_url(service_binding)).to_return(status: 200, body: '{"credentials": {"foo": true}') + stub_request(:get, service_binding_url(service_binding)).to_return(status: 200, body: '{"credentials": {"foo": true}}') Delayed::Worker.new.work_off diff --git a/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb index 978c383f688..7ce39d50a5a 100644 --- a/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb +++ b/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb @@ -17,7 +17,7 @@ 'broker_api_v2.11_spec.rb' => '99e61dc50ceb635b09b3bd16901a4fa6', 'broker_api_v2.12_spec.rb' => '4023dffdcaae014556dcdba9f7d206bb', 'broker_api_v2.13_spec.rb' => 'ceecc106d1a203002e277a37812b6df3', - 'broker_api_v2.14_spec.rb' => 'a1e7485793ba1916ea2f4080943530a5', + 'broker_api_v2.14_spec.rb' => 'a9fbd1d91bcc9c33c054c7854be1ab5a', 'broker_api_v2.15_spec.rb' => 'c575fd37bc6dc8df4f773719ccef3288', } end diff --git a/spec/unit/controllers/runtime/routes_controller_spec.rb b/spec/unit/controllers/runtime/routes_controller_spec.rb index 35a45dd6f93..98742882fef 100644 --- a/spec/unit/controllers/runtime/routes_controller_spec.rb +++ b/spec/unit/controllers/runtime/routes_controller_spec.rb @@ -601,7 +601,7 @@ module VCAP::CloudController post '/v2/routes?generate_port=true', MultiJson.dump(req), headers_for(user) expect(last_response.status).to eq(201) - expect(last_response.body).to include("\"port\": #{generated_port}") + expect(MultiJson.load(last_response.body)['entity']['port']).to eq(generated_port) expect(last_response.headers).not_to include('X-CF-Warnings') end end @@ -617,7 +617,7 @@ module VCAP::CloudController post '/v2/routes?generate_port=true', MultiJson.dump(req), headers_for(user) expect(last_response.status).to eq(201) - expect(last_response.body).to include("\"port\": #{generated_port}") + expect(MultiJson.load(last_response.body)['entity']['port']).to eq(generated_port) expect(last_response.headers).to include('X-Cf-Warnings') expect(last_response.headers['X-Cf-Warnings']).to include(port_override_warning) end @@ -640,7 +640,7 @@ module VCAP::CloudController post '/v2/routes?generate_port=true', MultiJson.dump(req), headers_for(user) expect(last_response.status).to eq(201) - expect(last_response.body).to include("\"port\": #{generated_port + 1}") + expect(MultiJson.load(last_response.body)['entity']['port']).to eq(generated_port + 1) expect(last_response.headers).not_to include('X-Cf-Warnings') end end @@ -734,7 +734,7 @@ module VCAP::CloudController post '/v2/routes?generate_port=false', MultiJson.dump(req), headers_for(user) expect(last_response.status).to eq(201) - expect(last_response.body).to include("\"port\": #{port}") + expect(MultiJson.load(last_response.body)['entity']['port']).to eq(port) end end end From 293c9c2663e4f1e2114ee59932ce210418f2e1b4 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Thu, 26 Aug 2021 12:02:37 +0200 Subject: [PATCH 2/2] Optimize JSON encoding - Add 'oj' gem. - Introduce optional 'use_optimized_json_encoder' config property. - If 'use_optimized_json_encoder' is true, use Oj (optimized for Rails) and omit 'as_json' calls for all Presenters. --- Gemfile | 1 + Gemfile.lock | 2 ++ config/initializers/json.rb | 20 ++++++++++++++++--- .../config_schemas/base/api_schema.rb | 2 ++ lib/cloud_controller/http_response_error.rb | 3 +++ 5 files changed, 25 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 10cb1412d06..7f8cd5fc63f 100644 --- a/Gemfile +++ b/Gemfile @@ -25,6 +25,7 @@ gem 'net-ssh' gem 'netaddr', '>= 2.0.4' gem 'newrelic_rpm' gem 'nokogiri', '>=1.10.5' +gem 'oj' gem 'palm_civet' gem 'posix-spawn', '~> 0.3.15' gem 'protobuf', '3.6.12' diff --git a/Gemfile.lock b/Gemfile.lock index 7c54024eb8f..545b1779ba9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -308,6 +308,7 @@ GEM racc (~> 1.4) nokogiri (1.12.5-x86_64-linux) racc (~> 1.4) + oj (3.13.2) os (1.0.1) palm_civet (1.1.0) parallel (1.21.0) @@ -551,6 +552,7 @@ DEPENDENCIES netaddr (>= 2.0.4) newrelic_rpm nokogiri (>= 1.10.5) + oj palm_civet parallel_tests pg diff --git a/config/initializers/json.rb b/config/initializers/json.rb index 03d4eee48e2..627915a3b4d 100644 --- a/config/initializers/json.rb +++ b/config/initializers/json.rb @@ -1,7 +1,15 @@ require 'active_support/json/encoding' module CCInitializers - def self.json(_cc_config) + def self.json(cc_config) + if cc_config[:use_optimized_json_encoder] + MultiJson.use(:oj) + Oj::Rails.optimize # Use optimized encoders instead of as_json() methods for available classes. + Oj.default_options = { + bigdecimal_load: :bigdecimal, + } + end + ActiveSupport.json_encoder = Class.new do attr_reader :options @@ -10,10 +18,16 @@ def initialize(options=nil) end def encode(value) + if MultiJson.default_adapter == :oj && value.is_a?(VCAP::CloudController::Presenters::V3::BasePresenter) + v = value.to_hash + else + v = value.as_json(options.dup) + end + if Rails.env.test? - MultiJson.dump(value.as_json(options.dup)) + MultiJson.dump(v) else - MultiJson.dump(value.as_json(options.dup), options.merge(pretty: true)) + MultiJson.dump(v, options.merge(pretty: true)) end end end diff --git a/lib/cloud_controller/config_schemas/base/api_schema.rb b/lib/cloud_controller/config_schemas/base/api_schema.rb index ffce4c23389..18f31fb81b8 100644 --- a/lib/cloud_controller/config_schemas/base/api_schema.rb +++ b/lib/cloud_controller/config_schemas/base/api_schema.rb @@ -354,6 +354,8 @@ class ApiSchema < VCAP::Config write_key: String, dataset: String, }, + + optional(:use_optimized_json_encoder) => bool, } end # rubocop:enable Metrics/BlockLength diff --git a/lib/cloud_controller/http_response_error.rb b/lib/cloud_controller/http_response_error.rb index e9f9cc799aa..1ca1fe15495 100644 --- a/lib/cloud_controller/http_response_error.rb +++ b/lib/cloud_controller/http_response_error.rb @@ -10,6 +10,9 @@ def initialize(message, method, response) begin source = MultiJson.load(response.body) + rescue ArgumentError + # Either Oj should raise Oj::ParseError instead of ArgumentError, or MultiJson should also wrap + # ArgumentError into MultiJson::Adapters::Oj::ParseError rescue MultiJson::ParseError source = response.body end