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).

Author: philippthun

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

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)

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

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
 

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

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)