Fixes invalid merge logic (#198)

* Add failing test

* Process JSON tree in Symbol until dump

* Extract common function

lib/rspec/openapi.rb

@@ -10,6 +10,7 @@
 require 'rspec/openapi/schema_merger'
 require 'rspec/openapi/schema_cleaner'
 require 'rspec/openapi/schema_sorter'
+require 'rspec/openapi/key_transformer'
 
 require 'rspec/openapi/minitest_hooks' if Object.const_defined?('Minitest')
 require 'rspec/openapi/rspec_hooks' if ENV['OPENAPI'] && Object.const_defined?('RSpec')

lib/rspec/openapi/components_updater.rb

@@ -23,38 +23,41 @@ def update!(base, fresh)
       #  0             1         2 ^...............................^
       # ["components", "schema", "Table", "properties", "owner", "properties", "company", "$ref"]
       #  0             1         2 ^...........................................^
-      needle = paths.reject { |path| path.is_a?(Integer) || path == 'oneOf' }
+      needle = paths.reject { |path| path.is_a?(Integer) || path == :oneOf }
       needle = needle.slice(2, needle.size - 3)
       nested_schema = fresh_schemas.dig(*needle)
 
       # Skip if the property using $ref is not found in the parent schema. The property may be removed.
       next if nested_schema.nil?
 
-      schema_name = base.dig(*paths)&.gsub('#/components/schemas/', '')
+      schema_name = base.dig(*paths)&.gsub('#/components/schemas/', '')&.to_sym
       fresh_schemas[schema_name] ||= {}
       RSpec::OpenAPI::SchemaMerger.merge!(fresh_schemas[schema_name], nested_schema)
     end
 
-    RSpec::OpenAPI::SchemaMerger.merge!(base, { 'components' => { 'schemas' => fresh_schemas } })
-    RSpec::OpenAPI::SchemaCleaner.cleanup_components_schemas!(base, { 'components' => { 'schemas' => fresh_schemas } })
+    RSpec::OpenAPI::SchemaMerger.merge!(base, { components: { schemas: fresh_schemas } })
+    RSpec::OpenAPI::SchemaCleaner.cleanup_components_schemas!(base, { components: { schemas: fresh_schemas } })
   end
 
   private
 
   def build_fresh_schemas(references, base, fresh)
     references.inject({}) do |acc, paths|
-      ref_link = dig_schema(base, paths)['$ref']
-      schema_name = ref_link.gsub('#/components/schemas/', '')
+      ref_link = dig_schema(base, paths)[:$ref]
+      puts "ref_link: #{ref_link}"
+      schema_name = ref_link.to_s.gsub('#/components/schemas/', '')
       schema_body = dig_schema(fresh, paths.reject { |path| path.is_a?(Integer) })
 
       RSpec::OpenAPI::SchemaMerger.merge!(acc, { schema_name => schema_body })
     end
   end
 
   def dig_schema(obj, paths)
-    item_schema = obj.dig(*paths, 'schema', 'items')
-    object_schema = obj.dig(*paths, 'schema')
-    one_of_schema = obj.dig(*paths.take(paths.size - 1), 'schema', 'oneOf', paths.last)
+    # Response code can be an integer
+    paths = paths.map { |path| path.is_a?(Integer) ? path : path.to_sym }
+    item_schema = obj.dig(*paths, :schema, :items)
+    object_schema = obj.dig(*paths, :schema)
+    one_of_schema = obj.dig(*paths.take(paths.size - 1), :schema, :oneOf, paths.last)
 
     item_schema || object_schema || one_of_schema
   end
@@ -85,12 +88,12 @@ def find_non_top_level_nested_refs(base, generated_names)
   end
 
   def find_one_of_refs(base, paths)
-    dig_schema(base, paths)&.dig('oneOf')&.map&.with_index do |schema, index|
-      paths + [index] if schema&.dig('$ref')&.start_with?('#/components/schemas/')
+    dig_schema(base, paths)&.dig(:oneOf)&.map&.with_index do |schema, index|
+      paths + [index] if schema&.dig(:$ref)&.start_with?('#/components/schemas/')
     end&.compact
   end
 
   def find_object_refs(base, paths)
-    [paths] if dig_schema(base, paths)&.dig('$ref')&.start_with?('#/components/schemas/')
+    [paths] if dig_schema(base, paths)&.dig(:$ref)&.start_with?('#/components/schemas/')
   end
 end

lib/rspec/openapi/hash_helper.rb

@@ -5,7 +5,7 @@ def paths_to_all_fields(obj)
     case obj
     when Hash
       obj.each.flat_map do |k, v|
-        k = k.to_s
+        k = k.to_sym
         [[k]] + paths_to_all_fields(v).map { |x| [k, *x] }
       end
     when Array
@@ -18,18 +18,20 @@ def paths_to_all_fields(obj)
   end
 
   def matched_paths(obj, selector)
-    selector_parts = selector.split('.').map(&:to_s)
+    selector_parts = selector.split('.').map(&:to_sym)
     paths_to_all_fields(obj).select do |key_parts|
       key_parts.size == selector_parts.size && key_parts.zip(selector_parts).all? do |kp, sp|
-        kp == sp || (sp == '*' && !kp.nil?)
+        kp == sp || (sp == :* && !kp.nil?)
       end
     end
   end
 
   def matched_paths_deeply_nested(obj, begin_selector, end_selector)
     path_depth_sizes = paths_to_all_fields(obj).map(&:size).uniq
     path_depth_sizes.map do |depth|
-      diff = depth - begin_selector.count('.') - end_selector.count('.')
+      begin_selector_count = begin_selector.is_a?(Symbol) ? 0 : begin_selector.count('.')
+      end_selector_count = end_selector.is_a?(Symbol) ? 0 : end_selector.count('.')
+      diff = depth - begin_selector_count - end_selector_count
       if diff >= 0
         selector = "#{begin_selector}.#{'*.' * diff}#{end_selector}"
         matched_paths(obj, selector)

lib/rspec/openapi/key_transformer.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+class << RSpec::OpenAPI::KeyTransformer = Object.new
+  def symbolize(value)
+    case value
+    when Hash
+      value.to_h { |k, v| [k.to_sym, symbolize(v)] }
+    when Array
+      value.map { |v| symbolize(v) }
+    else
+      value
+    end
+  end
+
+  def stringify(value)
+    case value
+    when Hash
+      value.to_h { |k, v| [k.to_s, stringify(v)] }
+    when Array
+      value.map { |v| stringify(v) }
+    else
+      value
+    end
+  end
+end

lib/rspec/openapi/record_builder.rb

@@ -53,7 +53,7 @@ def safe_parse_body(response, media_type)
 
   def extract_headers(request, response)
     request_headers = RSpec::OpenAPI.request_headers.each_with_object([]) do |header, headers_arr|
-      header_key = header.gsub('-', '_').upcase
+      header_key = header.gsub('-', '_').upcase.to_sym
       header_value = request.get_header(['HTTP', header_key].join('_')) || request.get_header(header_key)
       headers_arr << [header, header_value] if header_value
     end

lib/rspec/openapi/schema_cleaner.rb

@@ -27,7 +27,7 @@ def cleanup!(base, spec)
     cleanup_hash!(base, spec, 'paths.*.*')
 
     # cleanup parameters
-    cleanup_array!(base, spec, 'paths.*.*.parameters', %w[name in])
+    cleanup_array!(base, spec, 'paths.*.*.parameters', %i[name in])
 
     # cleanup requestBody
     cleanup_hash!(base, spec, 'paths.*.*.requestBody.content.application/json.schema.properties.*')
@@ -40,7 +40,7 @@ def cleanup!(base, spec)
   end
 
   def cleanup_conflicting_security_parameters!(base)
-    security_schemes = base.dig('components', 'securitySchemes') || {}
+    security_schemes = base.dig(:components, :securitySchemes) || {}
 
     return if security_schemes.empty?
 
@@ -65,22 +65,22 @@ def cleanup_empty_required_array!(base)
     paths_to_objects.each do |path|
       parent = base.dig(*path.take(path.length - 1))
       # "required" array  must not be present if empty
-      parent.delete('required') if parent['required'] && parent['required'].empty?
+      parent.delete(:required) if parent[:required] && parent[:required].empty?
     end
   end
 
   private
 
   def remove_parameters_conflicting_with_security_sceheme!(path_definition, security_scheme, security_scheme_name)
-    return unless path_definition['security']
-    return unless path_definition['parameters']
-    return unless path_definition.dig('security', 0).keys.include?(security_scheme_name)
+    return unless path_definition[:security]
+    return unless path_definition[:parameters]
+    return unless path_definition.dig(:security, 0).keys.include?(security_scheme_name)
 
-    path_definition['parameters'].reject! do |parameter|
-      parameter['in'] == security_scheme['in'] && # same location (ie. header)
-        parameter['name'] == security_scheme['name'] # same name (ie. AUTHORIZATION)
+    path_definition[:parameters].reject! do |parameter|
+      parameter[:in] == security_scheme[:in] && # same location (ie. header)
+        parameter[:name] == security_scheme[:name] # same name (ie. AUTHORIZATION)
     end
-    path_definition.delete('parameters') if path_definition['parameters'].empty?
+    path_definition.delete(:parameters) if path_definition[:parameters].empty?
   end
 
   def cleanup_array!(base, spec, selector, fields_for_identity = [])

lib/rspec/openapi/schema_file.rb

@@ -15,7 +15,7 @@ def edit(&block)
     spec = read
     block.call(spec)
   ensure
-    write(spec)
+    write(RSpec::OpenAPI::KeyTransformer.stringify(spec))
   end
 
   private
@@ -24,7 +24,7 @@ def edit(&block)
   def read
     return {} unless File.exist?(@path)
 
-    YAML.safe_load(File.read(@path)) # this can also parse JSON
+    RSpec::OpenAPI::KeyTransformer.symbolize(YAML.safe_load(File.read(@path))) # this can also parse JSON
   end
 
   # @param [Hash] spec

lib/rspec/openapi/schema_merger.rb

@@ -4,56 +4,44 @@ class << RSpec::OpenAPI::SchemaMerger = Object.new
   # @param [Hash] base
   # @param [Hash] spec
   def merge!(base, spec)
-    spec = normalize_keys(spec)
+    spec = RSpec::OpenAPI::KeyTransformer.symbolize(spec)
+    base.replace(RSpec::OpenAPI::KeyTransformer.symbolize(base))
     merge_schema!(base, spec)
   end
 
   private
 
-  def normalize_keys(spec)
-    case spec
-    when Hash
-      spec.to_h do |key, value|
-        [key.to_s, normalize_keys(value)]
-      end
-    when Array
-      spec.map { |s| normalize_keys(s) }
-    else
-      spec
-    end
-  end
-
   # Not doing `base.replace(deep_merge(base, spec))` to preserve key orders.
   # Also this needs to be aware of OpenAPI details because a Hash-like structure
   # may be an array whose Hash elements have a key name.
   #
   # TODO: Should we probably force-merge `summary` regardless of manual modifications?
   def merge_schema!(base, spec)
-    if (options = base['oneOf'])
+    if (options = base[:oneOf])
       merge_closest_match!(options, spec)
 
       return base
     end
 
     spec.each do |key, value|
       if base[key].is_a?(Hash) && value.is_a?(Hash)
-        merge_schema!(base[key], value) unless base[key].key?('$ref')
+        merge_schema!(base[key], value) unless base[key].key?(:$ref)
       elsif base[key].is_a?(Array) && value.is_a?(Array)
         # parameters need to be merged as if `name` and `in` were the Hash keys.
         merge_arrays(base, key, value)
       else
         # do not ADD `properties` or `required` fields if `additionalProperties` field is present
-        base[key] = value unless base.key?('additionalProperties') && %w[properties required].include?(key)
+        base[key] = value unless base.key?(:additionalProperties) && %i[properties required].include?(key)
       end
     end
     base
   end
 
   def merge_arrays(base, key, value)
     base[key] = case key
-                when 'parameters'
+                when :parameters
                   merge_parameters(base, key, value)
-                when 'required'
+                when :required
                   # Preserve properties that appears in all test cases
                   value & base[key]
                 else
@@ -65,13 +53,13 @@ def merge_arrays(base, key, value)
   def merge_parameters(base, key, value)
     all_parameters = value | base[key]
 
-    unique_base_parameters = base[key].index_by { |parameter| [parameter['name'], parameter['in']] }
+    unique_base_parameters = base[key].index_by { |parameter| [parameter[:name], parameter[:in]] }
     all_parameters = all_parameters.map do |parameter|
-      base_parameter = unique_base_parameters[[parameter['name'], parameter['in']]] || {}
+      base_parameter = unique_base_parameters[[parameter[:name], parameter[:in]]] || {}
       base_parameter ? base_parameter.merge(parameter) : parameter
     end
 
-    all_parameters.uniq! { |param| param.slice('name', 'in') }
+    all_parameters.uniq! { |param| param.slice(:name, :in) }
     base[key] = all_parameters
   end
 
@@ -80,7 +68,7 @@ def merge_parameters(base, key, value)
   def merge_closest_match!(options, spec)
     score, option = options.map { |option| [similarity(option, spec), option] }.max_by(&:first)
 
-    return if option&.key?('$ref')
+    return if option&.key?(:$ref)
 
     if score.to_f > SIMILARITY_THRESHOLD
       merge_schema!(option, spec)
@@ -97,7 +85,7 @@ def similarity(first, second)
       when [Array, Array]
         (first & second).size / [first.size, second.size].max.to_f
       when [Hash, Hash]
-        return 1 if first.merge(second).key?('$ref')
+        return 1 if first.merge(second).key?(:$ref)
 
         intersection = first.keys & second.keys
         total_size = [first.size, second.size].max.to_f

lib/rspec/openapi/schema_sorter.rb

@@ -29,7 +29,7 @@ def deep_sort_by_selector!(base, selector)
   end
 
   def deep_sort_hash!(hash)
-    sorted = hash.entries.sort_by { |k, _| k }.to_h
+    sorted = hash.entries.sort_by { |k, _| k.to_s }.to_h.transform_keys(&:to_sym)
     hash.replace(sorted)
   end
 end

spec/rspec/scheme_merger_spec.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'schema merger spec' do
+  include SpecHelper
+
+  describe 'mixed symbol and strings' do
+    let(:base) do
+      {
+        'n' => 1,
+        'required' => %w[foo bar],
+        'a' => {
+          b1: 1,
+          b2: %w[foo bar],
+          'b3' => {
+            'c1' => 2,
+            c2: 3,
+          },
+        },
+      }
+    end
+
+    let(:spec) do
+      {
+        n: 1,
+        required: ['buz'],
+        a: {
+          'b1' => 1,
+          'b2' => %w[foo bar],
+          b3: {
+            c1: 2,
+            'c2' => 3,
+          },
+        },
+      }
+    end
+
+    it 'normalize keys to symbol' do
+      result = RSpec::OpenAPI::SchemaMerger.merge!(base, spec)
+      expect(result).to eq({
+                             n: 1,
+                             required: [],
+                             a: {
+                               b1: 1,
+                               b2: %w[foo bar],
+                               b3: {
+                                 c1: 2,
+                                 c2: 3,
+                               },
+                             },
+                           })
+    end
+  end
+end