From 050d96cfa69cc116c744fc3e4752c7f37f96f45c Mon Sep 17 00:00:00 2001 From: serggl Date: Thu, 29 Sep 2016 09:26:06 +0300 Subject: [PATCH] fix incorrect data type linking for request params of entity types --- .gitignore | 3 + lib/grape-swagger/doc_methods/data_type.rb | 10 ++-- lib/grape-swagger/doc_methods/move_params.rb | 10 +++- lib/grape-swagger/doc_methods/parse_params.rb | 17 +++--- lib/grape-swagger/endpoint.rb | 45 +++++++-------- lib/grape-swagger/model_parsers.rb | 7 +++ spec/support/model_parsers/entity_parser.rb | 7 +++ spec/support/model_parsers/mock_parser.rb | 3 + .../model_parsers/representable_parser.rb | 11 +++- .../api_swagger_v2_param_type_body_spec.rb | 57 +++++++++++++++++++ spec/swagger_v2/params_array_spec.rb | 36 ++++++++++++ 11 files changed, 170 insertions(+), 36 deletions(-) diff --git a/.gitignore b/.gitignore index c5370f6a..80edba87 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,9 @@ doc # bundler .bundle +#IDEA temp files +.idea + # jeweler generated pkg diff --git a/lib/grape-swagger/doc_methods/data_type.rb b/lib/grape-swagger/doc_methods/data_type.rb index 5b83a55d..e6598a55 100644 --- a/lib/grape-swagger/doc_methods/data_type.rb +++ b/lib/grape-swagger/doc_methods/data_type.rb @@ -44,10 +44,12 @@ def parse_entity_name(model) if model.respond_to?(:entity_name) model.entity_name else - name = model.to_s - entity_parts = name.split('::') - entity_parts.reject! { |p| p == 'Entity' || p == 'Entities' } - entity_parts.join('::') + entity_parts = model.to_s.split('::').reject { |p| p == 'Entity' || p == 'Entities' } + length = 0 + entity_parts.reverse.take_while do |x| + length += x.length + length < 42 + end.reverse.join end end diff --git a/lib/grape-swagger/doc_methods/move_params.rb b/lib/grape-swagger/doc_methods/move_params.rb index ecff9145..9c7828ac 100644 --- a/lib/grape-swagger/doc_methods/move_params.rb +++ b/lib/grape-swagger/doc_methods/move_params.rb @@ -74,7 +74,15 @@ def document_as_array(param) end def document_as_property(param) - property_keys.each_with_object({}) { |x, memo| memo[x] = param[x] if param[x].present? } + property_keys.each_with_object({}) do |x, memo| + value = param[x] + next if value.blank? + if x == :type && @definitions[value].present? + memo['$ref'] = "#/definitions/#{value}" + else + memo[x] = value + end + end end def build_nested_properties(params, properties = {}) diff --git a/lib/grape-swagger/doc_methods/parse_params.rb b/lib/grape-swagger/doc_methods/parse_params.rb index 53555c0d..e34edb41 100644 --- a/lib/grape-swagger/doc_methods/parse_params.rb +++ b/lib/grape-swagger/doc_methods/parse_params.rb @@ -2,7 +2,7 @@ module GrapeSwagger module DocMethods class ParseParams class << self - def call(param, settings, route) + def call(param, settings, route, definitions) path = route.path method = route.request_method @@ -21,7 +21,7 @@ def call(param, settings, route) # optional properties document_description(settings) document_type_and_format(data_type) - document_array_param(value_type) + document_array_param(value_type, definitions) document_default_value(settings) document_range_values(settings) document_required(settings) @@ -60,7 +60,7 @@ def document_type_and_format(data_type) end end - def document_array_param(value_type) + def document_array_param(value_type, definitions) if value_type[:is_array] if value_type[:documentation].present? param_type = value_type[:documentation][:param_type] @@ -69,10 +69,13 @@ def document_array_param(value_type) collection_format = value_type[:documentation][:collectionFormat] end - array_items = { - type: type || @parsed_param[:type], - format: @parsed_param.delete(:format) - }.delete_if { |_, value| value.blank? } + array_items = {} + if definitions[value_type[:data_type]] + array_items['$ref'] = "#/definitions/#{@parsed_param[:type]}" + else + array_items[:type] = type || @parsed_param[:type] + end + array_items[:format] = @parsed_param.delete(:format) if @parsed_param[:format] @parsed_param[:in] = param_type || 'formData' @parsed_param[:items] = array_items diff --git a/lib/grape-swagger/endpoint.rb b/lib/grape-swagger/endpoint.rb index 2fd2d748..39a021b9 100644 --- a/lib/grape-swagger/endpoint.rb +++ b/lib/grape-swagger/endpoint.rb @@ -164,7 +164,12 @@ def params_object(route) parameters = partition_params(route).map do |param, value| value = { required: false }.merge(value) if value.is_a?(Hash) _, value = default_type([[param, value]]).first if value == '' - GrapeSwagger::DocMethods::ParseParams.call(param, value, route) + if value[:type] + expose_params(value[:type]) + elsif value[:documentation] + expose_params(value[:documentation][:type]) + end + GrapeSwagger::DocMethods::ParseParams.call(param, value, route, @definitions) end if GrapeSwagger::DocMethods::MoveParams.can_be_moved?(parameters, route.request_method) @@ -262,24 +267,28 @@ def param_type_is_array?(param_type) param_types.size == 1 end + def expose_params(value) + if value.is_a?(Class) && GrapeSwagger.model_parsers.find(value) + expose_params_from_model(value) + elsif value.is_a?(String) + begin + expose_params(Object.const_get(value.gsub(/\[|\]/, ''))) # try to load class from its name + rescue NameError + nil + end + end + end + def expose_params_from_model(model) model_name = model_name(model) return model_name if @definitions.key?(model_name) @definitions[model_name] = nil - properties = nil - parser = nil - - GrapeSwagger.model_parsers.each do |klass, ancestor| - next unless model.ancestors.map(&:to_s).include?(ancestor) - parser = klass.new(model, self) - break - end - - properties = parser.call unless parser.nil? - + parser = GrapeSwagger.model_parsers.find(model) raise GrapeSwagger::Errors::UnregisteredParser, "No parser registered for #{model_name}." unless parser + + properties = parser.new(model, self).call raise GrapeSwagger::Errors::SwaggerSpec, "Empty model #{model_name}, swagger 2.0 doesn't support empty definitions." unless properties && properties.any? @definitions[model_name] = { type: 'object', properties: properties } @@ -288,17 +297,7 @@ def expose_params_from_model(model) end def model_name(name) - if name.respond_to?(:entity_name) - name.entity_name - elsif name.to_s.end_with?('Entity', 'Entities') - length = 0 - name.to_s.split('::')[0..-2].reverse.take_while do |x| - length += x.length - length < 42 - end.reverse.join - else - name.name.demodulize.camelize - end + GrapeSwagger::DocMethods::DataType.parse_entity_name(name) end def hidden?(route, options) diff --git a/lib/grape-swagger/model_parsers.rb b/lib/grape-swagger/model_parsers.rb index 6f01d52a..711443ae 100644 --- a/lib/grape-swagger/model_parsers.rb +++ b/lib/grape-swagger/model_parsers.rb @@ -29,5 +29,12 @@ def each yield klass, ancestor end end + + def find(model) + GrapeSwagger.model_parsers.each do |klass, ancestor| + return klass if model.ancestors.map(&:to_s).include?(ancestor) + end + nil + end end end diff --git a/spec/support/model_parsers/entity_parser.rb b/spec/support/model_parsers/entity_parser.rb index c6f32209..d69f7a04 100644 --- a/spec/support/model_parsers/entity_parser.rb +++ b/spec/support/model_parsers/entity_parser.rb @@ -57,6 +57,13 @@ class ApiError < Grape::Entity expose :message, documentation: { type: String, desc: 'error message' } end + module NestedModule + class ApiResponse < Grape::Entity + expose :status, documentation: { type: String } + expose :error, documentation: { type: ::Entities::ApiError } + end + end + class SecondApiError < Grape::Entity expose :code, documentation: { type: Integer } expose :severity, documentation: { type: String } diff --git a/spec/support/model_parsers/mock_parser.rb b/spec/support/model_parsers/mock_parser.rb index 91be4c99..fe9708ef 100644 --- a/spec/support/model_parsers/mock_parser.rb +++ b/spec/support/model_parsers/mock_parser.rb @@ -53,6 +53,9 @@ class ApiError < OpenStruct; end class SecondApiError < OpenStruct; end class RecursiveModel < OpenStruct; end class DocumentedHashAndArrayModel < OpenStruct; end + module NestedModule + class ApiResponse < OpenStruct; end + end end end diff --git a/spec/support/model_parsers/representable_parser.rb b/spec/support/model_parsers/representable_parser.rb index 8093b3d5..2e0fee56 100644 --- a/spec/support/model_parsers/representable_parser.rb +++ b/spec/support/model_parsers/representable_parser.rb @@ -91,6 +91,15 @@ class ApiError < Representable::Decorator property :message, documentation: { type: String, desc: 'error message' } end + module NestedModule + class ApiResponse < Representable::Decorator + include Representable::JSON + + property :status, documentation: { type: String } + property :error, documentation: { type: ::Entities::ApiError } + end + end + class SecondApiError < Representable::Decorator include Representable::JSON @@ -236,7 +245,7 @@ class DocumentedHashAndArrayModel < Representable::Decorator 'prop_file' => { 'description' => 'prop_file description', 'type' => 'file' }, 'prop_float' => { 'description' => 'prop_float description', 'type' => 'number', 'format' => 'float' }, 'prop_integer' => { 'description' => 'prop_integer description', 'type' => 'integer', 'format' => 'int32' }, - 'prop_json' => { 'description' => 'prop_json description', 'type' => 'Representable::JSON' }, + 'prop_json' => { 'description' => 'prop_json description', 'type' => 'RepresentableJSON' }, 'prop_long' => { 'description' => 'prop_long description', 'type' => 'integer', 'format' => 'int64' }, 'prop_password' => { 'description' => 'prop_password description', 'type' => 'string', 'format' => 'password' }, 'prop_string' => { 'description' => 'prop_string description', 'type' => 'string' }, diff --git a/spec/swagger_v2/api_swagger_v2_param_type_body_spec.rb b/spec/swagger_v2/api_swagger_v2_param_type_body_spec.rb index 84aaf9c7..73395eb0 100644 --- a/spec/swagger_v2/api_swagger_v2_param_type_body_spec.rb +++ b/spec/swagger_v2/api_swagger_v2_param_type_body_spec.rb @@ -54,6 +54,17 @@ class BodyParamTypeApi < Grape::API end end + namespace :with_entity_param do + desc 'put in body with entity parameter' + params do + optional :data, type: ::Entities::NestedModule::ApiResponse, documentation: { desc: 'request data' } + end + + post do + { 'declared_params' => declared(params) } + end + end + add_swagger_documentation end end @@ -156,4 +167,50 @@ def app ) end end + + describe 'complex entity given' do + let(:request_parameters_definition) do + [ + { + 'name' => 'WithEntityParam', + 'in' => 'body', + 'required' => true, + 'schema' => { + '$ref' => '#/definitions/postWithEntityParam' + } + } + ] + end + + let(:request_body_parameters_definition) do + { + 'type' => 'object', + 'properties' => { + 'data' => { + '$ref' => '#/definitions/NestedModuleApiResponse', + 'description' => 'request data' + } + }, + 'description' => 'put in body with entity parameter' + } + end + + subject do + get '/swagger_doc/with_entity_param' + JSON.parse(last_response.body) + end + + specify do + expect(subject['paths']['/with_entity_param']['post']['parameters']).to eql(request_parameters_definition) + end + + specify do + # puts JSON.pretty_generate(subject) + expect(subject['definitions']['NestedModuleApiResponse']).not_to be_nil + end + + specify do + expect(subject['definitions']['postWithEntityParam']).to eql(request_body_parameters_definition) + end + end end diff --git a/spec/swagger_v2/params_array_spec.rb b/spec/swagger_v2/params_array_spec.rb index 4f1f7fb2..b8a1b884 100644 --- a/spec/swagger_v2/params_array_spec.rb +++ b/spec/swagger_v2/params_array_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe 'Group Params as Array' do + include_context "#{MODEL_PARSER} swagger example" + def app Class.new(Grape::API) do format :json @@ -57,6 +59,14 @@ def app { 'declared_params' => declared(params) } end + params do + requires :array_of_entities, type: Array[Entities::ApiError] + end + + post '/array_of_entities' do + { 'declared_params' => declared(params) } + end + add_swagger_documentation end end @@ -156,4 +166,30 @@ def app ) end end + + describe 'documentation for entity array parameters' do + let(:parameters) do + [ + { + 'in' => 'formData', + 'name' => 'array_of_entities', + 'type' => 'array', + 'items' => { + '$ref' => '#/definitions/ApiError' + }, + 'required' => true + } + ] + end + + subject do + get '/swagger_doc/array_of_entities' + JSON.parse(last_response.body) + end + + specify do + expect(subject['definitions']['ApiError']).not_to be_blank + expect(subject['paths']['/array_of_entities']['post']['parameters']).to eql(parameters) + end + end end