diff --git a/.rspec b/.rspec index 8c18f1a..740e6ee 100644 --- a/.rspec +++ b/.rspec @@ -1,2 +1,3 @@ --format documentation --color +--require "spec_helper" \ No newline at end of file diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 67cc731..dd62582 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -6,12 +6,13 @@ # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 3 +# Offense count: 7 # Configuration parameters: AllowedMethods. # AllowedMethods: enums Lint/ConstantDefinitionInBlock: Exclude: - 'spec/grape-swagger/entities/response_model_spec.rb' + - 'spec/issues/962_polymorphic_entity_with_custom_documentation_spec.rb' - 'spec/support/shared_contexts/inheritance_api.rb' - 'spec/support/shared_contexts/this_api.rb' @@ -21,6 +22,11 @@ Lint/EmptyBlock: Exclude: - 'spec/grape-swagger/entity/attribute_parser_spec.rb' +# Offense count: 1 +Lint/EmptyClass: + Exclude: + - 'spec/support/shared_contexts/custom_type_parser.rb' + # Offense count: 2 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: AllowedMethods. @@ -37,7 +43,7 @@ Metrics/AbcSize: # Offense count: 2 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 120 + Max: 135 # Offense count: 2 # Configuration parameters: AllowedMethods, AllowedPatterns. @@ -79,11 +85,12 @@ RSpec/ContextWording: - 'spec/support/shared_contexts/inheritance_api.rb' - 'spec/support/shared_contexts/this_api.rb' -# Offense count: 2 +# Offense count: 3 # Configuration parameters: IgnoredMetadata. RSpec/DescribeClass: Exclude: - '**/spec/features/**/*' + - '**/spec/issues/**/*' - '**/spec/requests/**/*' - '**/spec/routing/**/*' - '**/spec/system/**/*' @@ -95,10 +102,11 @@ RSpec/DescribeClass: RSpec/ExampleLength: Max: 213 -# Offense count: 26 +# Offense count: 30 RSpec/LeakyConstantDeclaration: Exclude: - 'spec/grape-swagger/entities/response_model_spec.rb' + - '**/spec/issues/**/*' - 'spec/support/shared_contexts/inheritance_api.rb' - 'spec/support/shared_contexts/this_api.rb' diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b224df..f93dfe2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ #### Fixes * Your contribution here. +* [#86](https://github.com/ruby-grape/grape-swagger-entity/pull/86): Fix Array[Object] documentation type parsing - [@numbata](https://github.com/numbata). * [#87](https://github.com/ruby-grape/grape-swagger-entity/pull/87): Remove hidden attributes from required - [@bogdan](https://github.com/bogdan). * [#88](https://github.com/ruby-grape/grape-swagger-entity/pull/88): Update danger workflows - [@numbata](https://github.com/numbata). * [#89](https://github.com/ruby-grape/grape-swagger-entity/pull/89): Remove ruby-grape-danger - [@dblock](https://github.com/dblock). diff --git a/lib/grape-swagger/entity/attribute_parser.rb b/lib/grape-swagger/entity/attribute_parser.rb index 9da9e3c..9067cc6 100644 --- a/lib/grape-swagger/entity/attribute_parser.rb +++ b/lib/grape-swagger/entity/attribute_parser.rb @@ -20,7 +20,7 @@ def call(entity_options) documentation = entity_options[:documentation] return param if documentation.nil? - add_array_documentation(param, documentation) if documentation[:is_array] + add_array_documentation(param, documentation) if array_type?(documentation) add_attribute_sample(param, documentation, :default) add_attribute_sample(param, documentation, :example) @@ -37,11 +37,26 @@ def call(entity_options) def model_from(entity_options) model = entity_options[:using] if entity_options[:using].present? - model ||= entity_options[:documentation][:type] if could_it_be_a_model?(entity_options[:documentation]) + documentation = entity_options[:documentation] + model ||= documentation[:type] if could_it_be_a_model?(documentation) model end + # Checks if documentation indicates an array type that needs items wrapping. + # Returns true for: + # - is_array: true (explicit flag) + # - type: 'Array[Something]' (array with element type) + # Returns false for: + # - type: 'Array' (bare array - already an array type, no wrapping needed) + def array_type?(documentation) + return false if documentation.nil? + return documentation[:is_array] if documentation.key?(:is_array) + + # Only match Array[ElementType] syntax, not bare 'Array' + documentation[:type].to_s.match?(/\Aarray\[.+\]\z/i) + end + def could_it_be_a_model?(value) return false if value.nil? @@ -57,6 +72,8 @@ def ambiguous_model_type?(type) end def primitive_type?(type) + return false if type.nil? + data_type = GrapeSwagger::DocMethods::DataType.call(type) GrapeSwagger::DocMethods::DataType.request_primitive?(data_type) end @@ -69,7 +86,7 @@ def data_type_from(entity_options) documented_data_type = document_data_type(documentation, data_type) - if documentation[:is_array] + if array_type?(documentation) { type: :array, items: documented_data_type @@ -97,7 +114,9 @@ def document_data_type(documentation, data_type) end def entity_model_type(name, entity_options) - if entity_options[:documentation] && entity_options[:documentation][:is_array] + documentation = entity_options[:documentation] + + if array_type?(documentation) { 'type' => 'array', 'items' => { diff --git a/lib/grape-swagger/entity/parser.rb b/lib/grape-swagger/entity/parser.rb index 3c6a8e2..8b1b70c 100644 --- a/lib/grape-swagger/entity/parser.rb +++ b/lib/grape-swagger/entity/parser.rb @@ -43,37 +43,57 @@ def extract_params(exposure) def parse_grape_entity_params(params, parent_model = nil) return unless params - parsed = params.each_with_object({}) do |(entity_name, entity_options), memo| - documentation_options = entity_options.fetch(:documentation, {}) - in_option = documentation_options.fetch(:in, nil).to_s - hidden_option = documentation_options.fetch(:hidden, nil) - next if in_option == 'header' || hidden_option == true - - entity_name = entity_name.original if entity_name.is_a?(Alias) - final_entity_name = entity_options.fetch(:as, entity_name) - documentation = entity_options[:documentation] - - memo[final_entity_name] = if entity_options[:nesting] - parse_nested(entity_name, entity_options, parent_model) - else - attribute_parser.call(entity_options) - end - - next unless documentation - - memo[final_entity_name][:readOnly] = documentation[:read_only].to_s == 'true' if documentation[:read_only] - memo[final_entity_name][:description] = documentation[:desc] if documentation[:desc] + required = required_params(params) + parsed_params = parse_params(params, parent_model) + + handle_discriminator(parsed_params, required) + end + + def parse_params(params, parent_model) + params.each_with_object({}) do |(entity_name, entity_options), memo| + next if skip_param?(entity_options) + + original_entity_name = entity_name.is_a?(Alias) ? entity_name.original : entity_name + final_entity_name = entity_options.fetch(:as, original_entity_name) + + memo[final_entity_name] = parse_entity_options(entity_options, original_entity_name, parent_model) + add_documentation_to_memo(memo[final_entity_name], entity_options[:documentation]) + end + end + + def skip_param?(entity_options) + documentation_options = entity_options.fetch(:documentation, {}) + in_option = documentation_options.fetch(:in, nil).to_s + hidden_option = documentation_options.fetch(:hidden, nil) + + in_option == 'header' || hidden_option == true + end + + def parse_entity_options(entity_options, entity_name, parent_model) + if entity_options[:nesting] + parse_nested(entity_name, entity_options, parent_model) + else + attribute_parser.call(entity_options) end + end + + def add_documentation_to_memo(memo_entry, documentation) + return unless documentation + + memo_entry[:readOnly] = documentation[:read_only].to_s == 'true' if documentation[:read_only] + memo_entry[:description] = documentation[:desc] if documentation[:desc] + end + def handle_discriminator(parsed, required) discriminator = GrapeSwagger::Entity::Helper.discriminator(model) if discriminator - respond_with_all_of(parsed, params, discriminator) + respond_with_all_of(parsed, required, discriminator) else - [parsed, required_params(params)] + [parsed, required] end end - def respond_with_all_of(parsed, params, discriminator) + def respond_with_all_of(parsed, required, discriminator) parent_name = GrapeSwagger::Entity::Helper.model_name(model.superclass, endpoint) { @@ -83,7 +103,7 @@ def respond_with_all_of(parsed, params, discriminator) }, [ add_discriminator(parsed, discriminator), - required_params(params).push(discriminator.attribute) + required.push(discriminator.attribute) ] ] } @@ -111,7 +131,7 @@ def parse_nested(entity_name, entity_options, parent_model = nil) .map(&:nested_exposures) .flatten .each_with_object({}) do |value, memo| - memo[value.attribute] = value.send(:options) + memo[value.attribute] = value.send(:options) end properties, required = parse_grape_entity_params(params, nested_entities.last) diff --git a/spec/grape-swagger/entities/response_model_spec.rb b/spec/grape-swagger/entities/response_model_spec.rb index a784f25..b2599e4 100644 --- a/spec/grape-swagger/entities/response_model_spec.rb +++ b/spec/grape-swagger/entities/response_model_spec.rb @@ -102,6 +102,8 @@ def app JSON.parse(last_response.body)['definitions'] end + include_context 'this api' + before :all do module TheseApi module Entities diff --git a/spec/grape-swagger/entity/attribute_parser_spec.rb b/spec/grape-swagger/entity/attribute_parser_spec.rb index 71ada22..12ab99a 100644 --- a/spec/grape-swagger/entity/attribute_parser_spec.rb +++ b/spec/grape-swagger/entity/attribute_parser_spec.rb @@ -218,6 +218,29 @@ def self.to_s it { is_expected.to include(type: :array) } it { is_expected.to include(items: { type: 'string' }) } + context 'when using Array[Type] syntax' do + # Array[Type] syntax for primitives preserves the type string as-is. + # For entity models, use the `using:` option with Array[Object] syntax. + let(:entity_options) { { documentation: { type: 'Array[String]', desc: 'Colors' } } } + + it { is_expected.to include(type: :array) } + it { is_expected.to include(items: { type: 'Array[String]' }) } + end + + context 'when using lowercase array[type] syntax' do + let(:entity_options) { { documentation: { type: 'array[integer]', desc: 'Numbers' } } } + + it { is_expected.to include(type: :array) } + it { is_expected.to include(items: { type: 'array[integer]' }) } + end + + context 'when is_array is explicitly false' do + let(:entity_options) { { documentation: { type: 'string', desc: 'Single value', is_array: false } } } + + it { is_expected.to include(type: 'string') } + it { is_expected.not_to include(:items) } + end + context 'when it contains example' do let(:entity_options) do { documentation: { type: 'string', desc: 'Colors', is_array: true, example: %w[green blue] } } @@ -262,6 +285,13 @@ def self.to_s it { is_expected.not_to include('$ref') } end + context 'when using bare Array type' do + let(:entity_options) { { documentation: { type: 'Array', desc: 'Generic array' } } } + + it { is_expected.to include(type: 'array') } + it { is_expected.not_to include(:items) } + end + context 'when it is exposed as a Boolean class' do let(:entity_options) do { documentation: { type: Grape::API::Boolean, example: example_value, default: example_value } } diff --git a/spec/issues/962_polymorphic_entity_with_custom_documentation_spec.rb b/spec/issues/962_polymorphic_entity_with_custom_documentation_spec.rb new file mode 100644 index 0000000..c62ab43 --- /dev/null +++ b/spec/issues/962_polymorphic_entity_with_custom_documentation_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +describe '#962 empty entity with custom documentation type' do + # grape-swagger < 2.1.3 doesn't support empty model definitions (PR #963) + let(:supports_empty_models?) { Gem::Version.new(GrapeSwagger::VERSION) >= Gem::Version.new('2.1.3') } + + context 'when entity has no properties' do + subject(:swagger_doc) do + get '/swagger_doc' + JSON.parse(last_response.body) + end + + before { skip 'grape-swagger < 2.1.3 does not support empty models' unless supports_empty_models? } + + let(:app) do + Class.new(Grape::API) do + namespace :issue962 do + class Foo < Grape::Entity + end + + class Report < Grape::Entity + expose :foo, + as: :bar, + using: Foo, + documentation: { + type: 'Array[object]', + desc: 'The bar in your report', + example: { + 'id' => 'string', + 'status' => 'string' + } + } + end + + desc 'Get a report', success: Report + get '/' do + present({ foo: [] }, with: Report) + end + end + + add_swagger_documentation format: :json + end + end + + it 'parses Array[object] type as array with $ref items' do + expect(swagger_doc['definitions']['Report']['properties']['bar']).to eql({ + 'type' => 'array', + 'description' => 'The bar in your report', + 'items' => { + '$ref' => '#/definitions/Foo' + }, + 'example' => { + 'id' => 'string', + 'status' => 'string' + } + }) + end + + it 'generates empty Foo entity definition' do + expect(swagger_doc['definitions']['Foo']).to eql({ + 'type' => 'object', + 'properties' => {} + }) + end + end + + context 'when entity has only hidden properties' do + subject(:swagger_doc) do + get '/swagger_doc' + JSON.parse(last_response.body) + end + + before { skip 'grape-swagger < 2.1.3 does not support empty models' unless supports_empty_models? } + + let(:app) do + Class.new(Grape::API) do + namespace :issue962 do + class Foo < Grape::Entity + expose :required_prop, documentation: { hidden: true } + expose :optional_prop, documentation: { hidden: true }, if: -> { true } + end + + class Report < Grape::Entity + expose :foo, + as: :bar, + using: Foo, + documentation: { + type: 'Array[object]', + desc: 'The bar in your report', + example: { + 'id' => 'string', + 'status' => 'string' + } + } + end + + desc 'Get a report', success: Report + get '/' do + present({ foo: [] }, with: Report) + end + end + + add_swagger_documentation format: :json + end + end + + it 'parses Array[object] type as array with $ref items' do + expect(swagger_doc['definitions']['Report']['properties']['bar']).to eql({ + 'type' => 'array', + 'description' => 'The bar in your report', + 'items' => { + '$ref' => '#/definitions/Foo' + }, + 'example' => { + 'id' => 'string', + 'status' => 'string' + } + }) + end + + it 'hides all hidden properties' do + expect(swagger_doc['definitions']['Foo']).to eql({ + 'type' => 'object', + 'properties' => {} + }) + end + end +end diff --git a/spec/support/shared_contexts/custom_type_parser.rb b/spec/support/shared_contexts/custom_type_parser.rb index 7095a07..a5ae7ad 100644 --- a/spec/support/shared_contexts/custom_type_parser.rb +++ b/spec/support/shared_contexts/custom_type_parser.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true -CustomType = Class.new +class CustomType +end class CustomTypeParser attr_reader :model, :endpoint