From bf743b5f8df280ab20a1814cd0386d58734c112f Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Mon, 26 Jan 2026 18:49:54 +0100 Subject: [PATCH] Don't JSON encode decimals as string - Use Oj consistently for encoding and decoding. - Remove custom encoder in JSON initializer. - Configure Oj to encode BigDecimals as JSON numbers. - Override BigDecimal's 'as_json' method; it's called by ActiveSupport and returns a string by default (before invoking the actual JSON encoder). - Adapt rescued error classes. - Add specs to test JSON encoding/decoding for service instance parameters (create, update, get) and service binding parameters (create, get). --- app/models/runtime/droplet_model.rb | 2 +- app/presenters/v3/base_presenter.rb | 4 + config/initializers/json.rb | 42 +++---- .../storage_cli/storage_cli_client.rb | 2 +- .../service_credential_bindings_spec.rb | 117 ++++++++++++++++-- spec/request/service_instances_spec.rb | 73 +++++++++++ spec/request/service_route_bindings_spec.rb | 54 ++++++++ 7 files changed, 256 insertions(+), 38 deletions(-) diff --git a/app/models/runtime/droplet_model.rb b/app/models/runtime/droplet_model.rb index 1c4e572e357..1757aec6a23 100644 --- a/app/models/runtime/droplet_model.rb +++ b/app/models/runtime/droplet_model.rb @@ -133,7 +133,7 @@ def docker_user begin docker_exec_metadata = Oj.load(execution_metadata) container_user = docker_exec_metadata['user'] - rescue EncodingError + rescue JSON::ParserError # ignore end end diff --git a/app/presenters/v3/base_presenter.rb b/app/presenters/v3/base_presenter.rb index 2be3be98f4f..04b340e3420 100644 --- a/app/presenters/v3/base_presenter.rb +++ b/app/presenters/v3/base_presenter.rb @@ -13,6 +13,10 @@ def initialize(resource, show_secrets: true, censored_message: Censorship::PRIVA @decorators = decorators end + def as_json(*_args) + to_hash + end + private def redact(unredacted_value) diff --git a/config/initializers/json.rb b/config/initializers/json.rb index ba4c372999b..412858b1d66 100644 --- a/config/initializers/json.rb +++ b/config/initializers/json.rb @@ -1,33 +1,23 @@ -require 'active_support/json/encoding' +require 'bigdecimal' + +# Override BigDecimal's 'as_json' to return self so that Oj can handle it according to its configuration. +# By default, ActiveSupport encodes BigDecimal as a string which we do not want. +class BigDecimal + def as_json(*_args) + self + end +end module CCInitializers def self.json(_cc_config) - Oj::Rails.optimize # Use optimized encoders instead of as_json() methods for available classes. Oj.default_options = { - bigdecimal_load: :bigdecimal, - mode: :rails - } - - ActiveSupport.json_encoder = Class.new do - attr_reader :options - - def initialize(options=nil) - @options = options || {} - end - - def encode(value) - v = if value.is_a?(VCAP::CloudController::Presenters::V3::BasePresenter) - value.to_hash - else - value.as_json(options.dup) - end + time_format: :ruby, # Encode Time/DateTime in Ruby-style string format + mode: :rails, # Rails-compatible JSON behavior + bigdecimal_load: :bigdecimal, # Decode JSON decimals as BigDecimal + compat_bigdecimal: true, # Required in :rails mode to avoid Float decoding + bigdecimal_as_decimal: true # Encode BigDecimal as JSON number (not string) + }.freeze - if Rails.env.test? - Oj.dump(v, time_format: :ruby) - else - Oj.dump(v, options.merge(time_format: :ruby)) - end - end - end + Oj.optimize_rails # Use Oj for Rails JSON encoding/decoding end end diff --git a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb index e42a66dab54..f7266f3bb83 100644 --- a/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb +++ b/lib/cloud_controller/blobstore/storage_cli/storage_cli_client.rb @@ -65,7 +65,7 @@ def config_path_for(resource_type) def fetch_json(path) Oj.load(File.read(path)) - rescue Oj::ParseError, EncodingError => e + rescue JSON::ParserError, EncodingError => e raise BlobstoreError.new("Failed to parse storage-cli JSON at #{path}: #{e.message}") end diff --git a/spec/request/service_credential_bindings_spec.rb b/spec/request/service_credential_bindings_spec.rb index d96ba024fb9..68835e7765c 100644 --- a/spec/request/service_credential_bindings_spec.rb +++ b/spec/request/service_credential_bindings_spec.rb @@ -14,6 +14,20 @@ space.add_developer(user) headers_for(user) end + let(:parameters_mixed_data_types_as_json_string) do + '{"boolean":true,"string":"a string","int":123,"float":3.14159,"optional":null,"object":{"a":"b"},"array":["c","d"]}' + end + let(:parameters_mixed_data_types_as_hash) do + { + boolean: true, + string: 'a string', + int: 123, + float: 3.14159, + optional: nil, + object: { a: 'b' }, + array: %w[c d] + } + end describe 'GET /v3/service_credential_bindings' do it_behaves_like 'list query endpoint' do @@ -1072,11 +1086,7 @@ def check_filtered_bindings(*bindings) end end - context 'when the broker will returns params' do - before do - stub_param_broker_request_for_binding(binding, binding_params) - end - + context 'when the broker returns params' do it 'sends a request to the broker including the identity header' do api_call.call(headers_for(user, scopes: %w[cloud_controller.admin])) @@ -1096,6 +1106,24 @@ def check_filtered_bindings(*bindings) expect(last_response).to have_status_code(200) expect(parsed_response).to eq(binding_params) end + + context 'when the broker returns params with mixed data types' do + before do + stub_param_broker_request_for_binding_with_json_string(binding, parameters_mixed_data_types_as_json_string) + end + + it 'correctly parses all data types and returns the desired JSON string' do + allow_any_instance_of(VCAP::CloudController::ServiceBindingRead).to receive(:fetch_parameters).and_wrap_original do |m, instance| + result = m.call(instance) + expect(result).to eq(parameters_mixed_data_types_as_hash) # correct internal representation + result + end + + api_call.call(admin_headers) + expect(last_response).to have_status_code(200) + expect(last_response).to match(/#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/) + end + end end end end @@ -1134,11 +1162,31 @@ def check_filtered_bindings(*bindings) ).to have_been_made.once end - it 'returns the params in the response body' do - api_call.call(admin_headers) + context 'when the broker returns params' do + it 'returns the params in the response body' do + api_call.call(admin_headers) - expect(last_response).to have_status_code(200) - expect(parsed_response).to eq(binding_params) + expect(last_response).to have_status_code(200) + expect(parsed_response).to eq(binding_params) + end + + context 'when the broker returns params with mixed data types' do + before do + stub_param_broker_request_for_binding_with_json_string(binding, parameters_mixed_data_types_as_json_string) + end + + it 'correctly parses all data types and returns the desired JSON string' do + allow_any_instance_of(VCAP::CloudController::ServiceBindingRead).to receive(:fetch_parameters).and_wrap_original do |m, instance| + result = m.call(instance) + expect(result).to eq(parameters_mixed_data_types_as_hash) # correct internal representation + result + end + + api_call.call(admin_headers) + expect(last_response).to have_status_code(200) + expect(last_response).to match(/#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/) + end + end end context "last service key operation is in 'create succeeded' state" do @@ -1643,6 +1691,28 @@ def check_filtered_bindings(*bindings) expect(service_instance.service_bindings.count).to eq(0) end end + + context 'when providing parameters with mixed data types' do + let(:request_body) do + "{\"type\":\"app\",\"name\":\"#{binding_name}\"," \ + "\"relationships\":{\"service_instance\":{\"data\":{\"guid\":\"#{service_instance_guid}\"}},\"app\":{\"data\":{\"guid\":\"#{app_guid}\"}}}," \ + "\"parameters\":#{parameters_mixed_data_types_as_json_string}}" + end + + it 'correctly parses all data types and sends the desired JSON string to the service broker' do + post '/v3/service_credential_bindings', request_body, admin_headers + + expect_any_instance_of(VCAP::Services::ServiceBrokers::V2::Client).to receive(:bind). + with(binding, hash_including(arbitrary_parameters: parameters_mixed_data_types_as_hash)). # correct internal representation + and_call_original + + stub_request(:put, "#{service_instance.service_broker.broker_url}/v2/service_instances/#{service_instance_guid}/service_bindings/#{binding.guid}"). + with(query: { accepts_incomplete: true }, body: /"parameters":#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/). + to_return(status: 201, body: '{}') + + execute_all_jobs(expected_successes: 1, expected_failures: 0) + end + end end end @@ -1877,6 +1947,29 @@ def check_filtered_bindings(*bindings) expect(service_instance.service_keys.count).to eq(0) end end + + context 'when providing parameters with mixed data types' do + let(:key) { VCAP::CloudController::ServiceKey.last } + let(:request_body) do + "{\"type\":\"key\",\"name\":\"#{binding_name}\"," \ + "\"relationships\":{\"service_instance\":{\"data\":{\"guid\":\"#{service_instance_guid}\"}}}," \ + "\"parameters\":#{parameters_mixed_data_types_as_json_string}}" + end + + it 'correctly parses all data types and sends the desired JSON string to the service broker' do + post '/v3/service_credential_bindings', request_body, admin_headers + + expect_any_instance_of(VCAP::Services::ServiceBrokers::V2::Client).to receive(:bind). + with(key, hash_including(arbitrary_parameters: parameters_mixed_data_types_as_hash)). # correct internal representation + and_call_original + + stub_request(:put, "#{service_instance.service_broker.broker_url}/v2/service_instances/#{service_instance_guid}/service_bindings/#{key.guid}"). + with(query: { accepts_incomplete: true }, body: /"parameters":#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/). + to_return(status: 201, body: '{}') + + execute_all_jobs(expected_successes: 1, expected_failures: 0) + end + end end end @@ -2585,11 +2678,15 @@ def operate_on(binding) end def stub_param_broker_request_for_binding(binding, binding_params, status: 200) + stub_param_broker_request_for_binding_with_json_string(binding, binding_params.to_json, status:) + end + + def stub_param_broker_request_for_binding_with_json_string(binding, binding_params_as_json_string, status: 200) instance = binding.service_instance broker_url = instance.service_broker.broker_url broker_binding_url = "#{broker_url}/v2/service_instances/#{instance.guid}/service_bindings/#{binding.guid}" stub_request(:get, /#{broker_binding_url}/). - to_return(status: status, body: { parameters: binding_params }.to_json) + to_return(status: status, body: "{\"parameters\":#{binding_params_as_json_string}}") end end diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb index 15f63e6fd90..b9c69073434 100644 --- a/spec/request/service_instances_spec.rb +++ b/spec/request/service_instances_spec.rb @@ -8,6 +8,20 @@ let(:space) { VCAP::CloudController::Space.make(organization: org, created_at: Time.now.utc - 1.second) } let!(:space_annotation) { VCAP::CloudController::SpaceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'baz', value: 'wow', space: space) } let(:another_space) { VCAP::CloudController::Space.make } + let(:parameters_mixed_data_types_as_json_string) do + '{"boolean":true,"string":"a string","int":123,"float":3.14159,"optional":null,"object":{"a":"b"},"array":["c","d"]}' + end + let(:parameters_mixed_data_types_as_hash) do + { + boolean: true, + string: 'a string', + int: 123, + float: 3.14159, + optional: nil, + object: { a: 'b' }, + array: %w[c d] + } + end describe 'GET /v3/service_instances/:guid' do let(:api_call) { ->(user_headers) { get "/v3/service_instances/#{guid}", nil, user_headers } } @@ -605,6 +619,22 @@ def check_filtered_instances(*instances) end end + context 'when the service broker returns parameters with mixed data types' do + let(:body) { "{\"parameters\":#{parameters_mixed_data_types_as_json_string}}" } + + it 'correctly parses all data types and returns the desired JSON string' do + allow_any_instance_of(VCAP::CloudController::ServiceInstanceRead).to receive(:fetch_parameters).and_wrap_original do |m, instance| + result = m.call(instance) + expect(result).to eq(parameters_mixed_data_types_as_hash) # correct internal representation + result + end + + get "/v3/service_instances/#{instance.guid}/parameters", nil, admin_headers + expect(last_response).to have_status_code(200) + expect(last_response).to match(/#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/) + end + end + it 'sends the correct request to the service broker' do get "/v3/service_instances/#{instance.guid}/parameters", nil, headers_for(user, scopes: %w[cloud_controller.admin]) @@ -1087,6 +1117,28 @@ def check_filtered_instances(*instances) allow(Steno).to receive(:logger).with('cc.api').and_return(mock_logger) end + context 'when providing parameters with mixed data types' do + let(:request_body) do + "{\"type\":\"managed\",\"name\":\"#{name}\"," \ + "\"relationships\":{\"space\":{\"data\":{\"guid\":\"#{space_guid}\"}},\"service_plan\":{\"data\":{\"guid\":\"#{service_plan_guid}\"}}}," \ + "\"parameters\":#{parameters_mixed_data_types_as_json_string}}" + end + + it 'correctly parses all data types and sends the desired JSON string to the service broker' do + post '/v3/service_instances', request_body, space_dev_headers + + expect_any_instance_of(VCAP::Services::ServiceBrokers::V2::Client).to receive(:provision). + with(instance, hash_including(arbitrary_parameters: parameters_mixed_data_types_as_hash)). # correct internal representation + and_call_original + + stub_request(:put, "#{instance.service_broker.broker_url}/v2/service_instances/#{instance.guid}"). + with(query: { 'accepts_incomplete' => true }, body: /"parameters":#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/). + to_return(status: 201, body: '{}') + + execute_all_jobs(expected_successes: 1, expected_failures: 0) + end + end + it 'creates a service instance in the database' do api_call.call(space_dev_headers) @@ -1871,6 +1923,27 @@ def check_filtered_instances(*instances) allow(Steno).to receive(:logger).with('cc.api').and_return(mock_logger) end + context 'when providing parameters with mixed data types' do + let(:request_body) do + "{\"parameters\":#{parameters_mixed_data_types_as_json_string}}" + end + let(:instance) { VCAP::CloudController::ServiceInstance.last } + + it 'correctly parses all data types and sends the desired JSON string to the service broker' do + patch "/v3/service_instances/#{guid}", request_body, space_dev_headers + + expect_any_instance_of(VCAP::Services::ServiceBrokers::V2::Client).to receive(:update). + with(instance, instance.service_plan, hash_including(arbitrary_parameters: parameters_mixed_data_types_as_hash)). # correct internal representation + and_call_original + + stub_request(:patch, "#{instance.service_broker.broker_url}/v2/service_instances/#{instance.guid}"). + with(query: { 'accepts_incomplete' => true }, body: /"parameters":#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/). + to_return(status: 200, body: '{}') + + execute_all_jobs(expected_successes: 1, expected_failures: 0) + end + end + it 'responds with a pollable job' do api_call.call(space_dev_headers) diff --git a/spec/request/service_route_bindings_spec.rb b/spec/request/service_route_bindings_spec.rb index 49f4bf67cf7..af2cde52cc0 100644 --- a/spec/request/service_route_bindings_spec.rb +++ b/spec/request/service_route_bindings_spec.rb @@ -14,6 +14,20 @@ let!(:org_annotation) { VCAP::CloudController::OrganizationAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'foo', value: 'bar', resource_guid: org.guid) } let(:org) { VCAP::CloudController::Organization.make } let(:user) { VCAP::CloudController::User.make } + let(:parameters_mixed_data_types_as_json_string) do + '{"boolean":true,"string":"a string","int":123,"float":3.14159,"optional":null,"object":{"a":"b"},"array":["c","d"]}' + end + let(:parameters_mixed_data_types_as_hash) do + { + boolean: true, + string: 'a string', + int: 123, + float: 3.14159, + optional: nil, + object: { a: 'b' }, + array: %w[c d] + } + end describe 'GET /v3/service_route_bindings' do # Because route bindings don't have names, we can't use the 'paginated response' shared example @@ -1027,6 +1041,27 @@ expect(service_instance.routes.count).to eq(0) end end + + context 'when providing parameters with mixed data types' do + let(:request_body) do + "{\"relationships\":{\"service_instance\":{\"data\":{\"guid\":\"#{service_instance.guid}\"}},\"route\":{\"data\":{\"guid\":\"#{route.guid}\"}}}," \ + "\"parameters\":#{parameters_mixed_data_types_as_json_string}}" + end + + it 'correctly parses all data types and sends the desired JSON string to the service broker' do + post '/v3/service_route_bindings', request_body, admin_headers + + expect_any_instance_of(VCAP::Services::ServiceBrokers::V2::Client).to receive(:bind). + with(binding, hash_including(arbitrary_parameters: parameters_mixed_data_types_as_hash)). # correct internal representation + and_call_original + + stub_request(:put, "#{service_instance.service_broker.broker_url}/v2/service_instances/#{service_instance.guid}/service_bindings/#{binding.guid}"). + with(query: { accepts_incomplete: true }, body: /"parameters":#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/). + to_return(status: 201, body: '{}') + + execute_all_jobs(expected_successes: 1, expected_failures: 0) + end + end end context 'user-provided service instance' do @@ -1680,6 +1715,25 @@ { 'abra' => 'kadabra', 'kadabra' => 'alakazan' } ) end + + context 'when the broker returns params with mixed data types' do + before do + stub_request(:get, broker_fetch_binding_url). + to_return(status: broker_status_code, body: "{\"parameters\":#{parameters_mixed_data_types_as_json_string}}") + end + + it 'correctly parses all data types and returns the desired JSON string' do + allow_any_instance_of(VCAP::CloudController::ServiceBindingRead).to receive(:fetch_parameters).and_wrap_original do |m, instance| + result = m.call(instance) + expect(result).to eq(parameters_mixed_data_types_as_hash) # correct internal representation + result + end + + api_call.call(admin_headers) + expect(last_response).to have_status_code(200) + expect(last_response).to match(/#{Regexp.escape(parameters_mixed_data_types_as_json_string)}/) + end + end end context "when last binding operation is in 'create in progress' state" do