From 338513f0d57f78650623794bb12599fffb282381 Mon Sep 17 00:00:00 2001 From: Jean-Francis Bastien Date: Mon, 27 Jul 2020 15:41:32 -0400 Subject: [PATCH] Don't overwrite model description with the route destription. --- CHANGELOG.md | 2 +- README.md | 3 +- UPGRADING.md | 4 + lib/grape-swagger/doc_methods.rb | 127 +++++++++--------- lib/grape-swagger/endpoint.rb | 15 ++- spec/support/model_parsers/entity_parser.rb | 16 +-- spec/support/model_parsers/mock_parser.rb | 16 +-- .../model_parsers/representable_parser.rb | 16 +-- ...pi_swagger_v2_response_with_models_spec.rb | 53 ++++++++ spec/swagger_v2/reference_entity_spec.rb | 4 +- 10 files changed, 159 insertions(+), 97 deletions(-) create mode 100644 spec/swagger_v2/api_swagger_v2_response_with_models_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index d0246043..75f81cfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ #### Fixes * Your contribution here. - +* [#804](https://github.com/ruby-grape/grape-swagger/pull/804): Don't overwrite model description with the route description. - [@Bhacaz](https://github.com/Bhacaz) ### 1.2.1 (July 15, 2020) diff --git a/README.md b/README.md index a88859bc..6cc1b371 100644 --- a/README.md +++ b/README.md @@ -859,10 +859,11 @@ get '/thing', failure: [ end ``` -By adding a `model` key, e.g. this would be taken. +By adding a `model` key, e.g. this would be taken. Setting an empty string will act like an empty body. ```ruby get '/thing', failure: [ { code: 400, message: 'General error' }, + { code: 403, message: 'Forbidden error', model: '' }, { code: 422, message: 'Invalid parameter entry', model: Entities::ApiError } ] do # ... diff --git a/UPGRADING.md b/UPGRADING.md index def79655..635f8773 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,5 +1,9 @@ ## Upgrading Grape-swagger +### Upgrading to >= 1.3.0 + +- The model (entity) description no longer comes from the route description. It will have a default value: `<> model`. + ### Upgrading to >= 1.2.0 - The entity_name class method is now called on parent classes for inherited entities. Now you can do this diff --git a/lib/grape-swagger/doc_methods.rb b/lib/grape-swagger/doc_methods.rb index 9ab74f33..efb850e3 100644 --- a/lib/grape-swagger/doc_methods.rb +++ b/lib/grape-swagger/doc_methods.rb @@ -17,6 +17,61 @@ module GrapeSwagger module DocMethods + DEFAULTS = + { + info: {}, + models: [], + doc_version: '0.0.1', + target_class: nil, + mount_path: '/swagger_doc', + host: nil, + base_path: nil, + add_base_path: false, + add_version: true, + add_root: false, + hide_documentation_path: true, + format: :json, + authorizations: nil, + security_definitions: nil, + security: nil, + api_documentation: { desc: 'Swagger compatible API description' }, + specific_api_documentation: { desc: 'Swagger compatible API description for specific API' }, + endpoint_auth_wrapper: nil, + swagger_endpoint_guard: nil, + token_owner: nil + }.freeze + + FORMATTER_METHOD = %i[format default_format default_error_formatter].freeze + + def self.output_path_definitions(combi_routes, endpoint, target_class, options) + output = endpoint.swagger_object( + target_class, + endpoint.request, + options + ) + + paths, definitions = endpoint.path_and_definition_objects(combi_routes, options) + tags = tags_from(paths, options) + + output[:tags] = tags unless tags.empty? || paths.blank? + output[:paths] = paths unless paths.blank? + output[:definitions] = definitions unless definitions.blank? + + output + end + + def self.tags_from(paths, options) + tags = GrapeSwagger::DocMethods::TagNameDescription.build(paths) + + if options[:tags] + names = options[:tags].map { |t| t[:name] } + tags.reject! { |t| names.include?(t[:name]) } + tags += options[:tags] + end + + tags + end + def hide_documentation_path @@hide_documentation_path end @@ -26,54 +81,32 @@ def mount_path end def setup(options) - options = defaults.merge(options) + options = DEFAULTS.merge(options) # options could be set on #add_swagger_documentation call, # for available options see #defaults target_class = options[:target_class] guard = options[:swagger_endpoint_guard] - formatter = options[:format] api_doc = options[:api_documentation].dup specific_api_doc = options[:specific_api_documentation].dup class_variables_from(options) - if formatter - %i[format default_format default_error_formatter].each do |method| - send(method, formatter) - end - end + setup_formatter(options[:format]) desc api_doc.delete(:desc), api_doc - output_path_definitions = proc do |combi_routes, endpoint| - output = endpoint.swagger_object( - target_class, - endpoint.request, - options - ) - - paths, definitions = endpoint.path_and_definition_objects(combi_routes, options) - tags = tags_from(paths, options) - - output[:tags] = tags unless tags.empty? || paths.blank? - output[:paths] = paths unless paths.blank? - output[:definitions] = definitions unless definitions.blank? - - output - end - instance_eval(guard) unless guard.nil? get mount_path do header['Access-Control-Allow-Origin'] = '*' header['Access-Control-Request-Method'] = '*' - output_path_definitions.call(target_class.combined_namespace_routes, self) + GrapeSwagger::DocMethods + .output_path_definitions(target_class.combined_namespace_routes, self, target_class, options) end - desc specific_api_doc.delete(:desc), { params: - specific_api_doc.delete(:params) || {} }.merge(specific_api_doc) + desc specific_api_doc.delete(:desc), { params: specific_api_doc.delete(:params) || {}, **specific_api_doc } params do requires :name, type: String, desc: 'Resource name of mounted API' @@ -88,51 +121,21 @@ def setup(options) combined_routes = target_class.combined_namespace_routes[params[:name]] error!({ error: 'named resource not exist' }, 400) if combined_routes.nil? - output_path_definitions.call({ params[:name] => combined_routes }, self) + GrapeSwagger::DocMethods + .output_path_definitions({ params[:name] => combined_routes }, self, target_class, options) end end - def defaults - { - info: {}, - models: [], - doc_version: '0.0.1', - target_class: nil, - mount_path: '/swagger_doc', - host: nil, - base_path: nil, - add_base_path: false, - add_version: true, - add_root: false, - hide_documentation_path: true, - format: :json, - authorizations: nil, - security_definitions: nil, - security: nil, - api_documentation: { desc: 'Swagger compatible API description' }, - specific_api_documentation: { desc: 'Swagger compatible API description for specific API' }, - endpoint_auth_wrapper: nil, - swagger_endpoint_guard: nil, - token_owner: nil - } - end - def class_variables_from(options) @@mount_path = options[:mount_path] @@class_name = options[:class_name] || options[:mount_path].delete('/') @@hide_documentation_path = options[:hide_documentation_path] end - def tags_from(paths, options) - tags = GrapeSwagger::DocMethods::TagNameDescription.build(paths) + def setup_formatter(formatter) + return unless formatter - if options[:tags] - names = options[:tags].map { |t| t[:name] } - tags.reject! { |t| names.include?(t[:name]) } - tags += options[:tags] - end - - tags + FORMATTER_METHOD.each { |method| send(method, formatter) } end end end diff --git a/lib/grape-swagger/endpoint.rb b/lib/grape-swagger/endpoint.rb index ba0ec340..48e0699a 100644 --- a/lib/grape-swagger/endpoint.rb +++ b/lib/grape-swagger/endpoint.rb @@ -78,11 +78,11 @@ def contact_object(infos) def path_and_definition_objects(namespace_routes, options) @paths = {} @definitions = {} + add_definitions_from options[:models] namespace_routes.each_value do |routes| path_item(routes, options) end - add_definitions_from options[:models] [@paths, @definitions] end @@ -207,19 +207,20 @@ def response_object(route, options) next build_file_response(memo[value[:code]]) if file_response?(value[:model]) - response_model = @item - response_model = expose_params_from_model(value[:model]) if value[:model] - if memo.key?(200) && route.request_method == 'DELETE' && value[:model].nil? memo[204] = memo.delete(200) value[:code] = 204 + next end - next if value[:code] == 204 - next unless !response_model.start_with?('Swagger_doc') && (@definitions[response_model] || value[:model]) + # Explicitly request no model with { model: '' } + next if value[:model] == '' - @definitions[response_model][:description] = description_object(route) + response_model = value[:model] ? expose_params_from_model(value[:model]) : @item + next unless @definitions[response_model] + next if response_model.start_with?('Swagger_doc') + @definitions[response_model][:description] ||= "#{response_model} model" memo[value[:code]][:schema] = build_reference(route, value, response_model, options) memo[value[:code]][:examples] = value[:examples] if value[:examples] end diff --git a/spec/support/model_parsers/entity_parser.rb b/spec/support/model_parsers/entity_parser.rb index dfd3359c..db19ead0 100644 --- a/spec/support/model_parsers/entity_parser.rb +++ b/spec/support/model_parsers/entity_parser.rb @@ -145,23 +145,23 @@ class DocumentedHashAndArrayModel < Grape::Entity let(:swagger_nested_type) do { - 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'type' => 'integer', 'format' => 'int32', 'description' => 'status code' }, 'message' => { 'type' => 'string', 'description' => 'error message' } }, 'description' => 'This returns something' }, + 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'type' => 'integer', 'format' => 'int32', 'description' => 'status code' }, 'message' => { 'type' => 'string', 'description' => 'error message' } }, 'description' => 'ApiError model' }, 'ResponseItem' => { 'type' => 'object', 'properties' => { 'id' => { 'type' => 'integer', 'format' => 'int32' }, 'name' => { 'type' => 'string' } } }, - 'UseItemResponseAsType' => { 'type' => 'object', 'properties' => { 'description' => { 'type' => 'string' }, 'responses' => { '$ref' => '#/definitions/ResponseItem' } }, 'description' => 'This returns something' } + 'UseItemResponseAsType' => { 'type' => 'object', 'properties' => { 'description' => { 'type' => 'string' }, 'responses' => { '$ref' => '#/definitions/ResponseItem' } }, 'description' => 'UseItemResponseAsType model' } } end let(:swagger_entity_as_response_object) do { - 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'type' => 'integer', 'format' => 'int32', 'description' => 'status code' }, 'message' => { 'type' => 'string', 'description' => 'error message' } }, 'description' => 'This returns something' }, + 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'type' => 'integer', 'format' => 'int32', 'description' => 'status code' }, 'message' => { 'type' => 'string', 'description' => 'error message' } }, 'description' => 'ApiError model' }, 'ResponseItem' => { 'type' => 'object', 'properties' => { 'id' => { 'type' => 'integer', 'format' => 'int32' }, 'name' => { 'type' => 'string' } } }, - 'UseResponse' => { 'type' => 'object', 'properties' => { 'description' => { 'type' => 'string' }, '$responses' => { 'type' => 'array', 'items' => { '$ref' => '#/definitions/ResponseItem' } } }, 'description' => 'This returns something' } + 'UseResponse' => { 'type' => 'object', 'properties' => { 'description' => { 'type' => 'string' }, '$responses' => { 'type' => 'array', 'items' => { '$ref' => '#/definitions/ResponseItem' } } }, 'description' => 'UseResponse model' } } end let(:swagger_params_as_response_object) do { - 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'This returns something' } + 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'ApiError model' } } end @@ -300,7 +300,7 @@ class DocumentedHashAndArrayModel < Grape::Entity 'type' => 'object', 'required' => ['elements'], 'properties' => { 'elements' => { 'type' => 'array', 'items' => { '$ref' => '#/definitions/QueryInputElement' }, 'description' => 'Set of configuration' } }, - 'description' => 'nested route inside namespace' + 'description' => 'QueryInput model' }, 'QueryInputElement' => { 'type' => 'object', @@ -310,7 +310,7 @@ class DocumentedHashAndArrayModel < Grape::Entity 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'type' => 'integer', 'format' => 'int32', 'description' => 'status code' }, 'message' => { 'type' => 'string', 'description' => 'error message' } }, - 'description' => 'This gets Things.' + 'description' => 'ApiError model' }, 'Something' => { 'type' => 'object', @@ -320,7 +320,7 @@ class DocumentedHashAndArrayModel < Grape::Entity 'links' => { 'type' => 'array', 'items' => { 'type' => 'link' } }, 'others' => { 'type' => 'text' } }, - 'description' => 'This gets Things.' + 'description' => 'Something model' } } } diff --git a/spec/support/model_parsers/mock_parser.rb b/spec/support/model_parsers/mock_parser.rb index c58a6d01..303c7467 100644 --- a/spec/support/model_parsers/mock_parser.rb +++ b/spec/support/model_parsers/mock_parser.rb @@ -112,7 +112,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This returns something' + 'description' => 'ApiError model' }, 'UseItemResponseAsType' => { 'type' => 'object', @@ -122,7 +122,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This returns something' + 'description' => 'UseItemResponseAsType model' } } end @@ -137,7 +137,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This returns something' + 'description' => 'UseResponse model' }, 'ApiError' => { 'type' => 'object', @@ -147,7 +147,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This returns something' + 'description' => 'ApiError model' } } end @@ -162,7 +162,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This returns something' + 'description' => 'ApiError model' } } end @@ -296,7 +296,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'nested route inside namespace' + 'description' => 'QueryInput model' }, 'ApiError' => { 'type' => 'object', @@ -306,7 +306,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This gets Things.' + 'description' => 'ApiError model' }, 'Something' => { 'type' => 'object', @@ -316,7 +316,7 @@ class ApiResponse < OpenStruct; end 'description' => "it's a mock" } }, - 'description' => 'This gets Things.' + 'description' => 'Something model' } } } diff --git a/spec/support/model_parsers/representable_parser.rb b/spec/support/model_parsers/representable_parser.rb index 5bc722cb..123dc9d1 100644 --- a/spec/support/model_parsers/representable_parser.rb +++ b/spec/support/model_parsers/representable_parser.rb @@ -218,22 +218,22 @@ class DocumentedHashAndArrayModel < Representable::Decorator let(:swagger_nested_type) do { - 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'This returns something' }, - 'UseItemResponseAsType' => { 'type' => 'object', 'properties' => { 'description' => { 'description' => '', 'type' => 'string' }, 'responses' => { 'description' => '', 'type' => 'ResponseItem' } }, 'description' => 'This returns something' } + 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'ApiError model' }, + 'UseItemResponseAsType' => { 'type' => 'object', 'properties' => { 'description' => { 'description' => '', 'type' => 'string' }, 'responses' => { 'description' => '', 'type' => 'ResponseItem' } }, 'description' => 'UseItemResponseAsType model' } } end let(:swagger_entity_as_response_object) do { - 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'This returns something' }, + 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'ApiError model' }, 'ResponseItem' => { 'type' => 'object', 'properties' => { 'id' => { 'description' => '', 'type' => 'integer', 'format' => 'int32' }, 'name' => { 'description' => '', 'type' => 'string' } } }, - 'UseResponse' => { 'type' => 'object', 'properties' => { 'description' => { 'description' => '', 'type' => 'string' }, '$responses' => { 'type' => 'array', 'items' => { '$ref' => '#/definitions/ResponseItem' }, 'description' => '' } }, 'description' => 'This returns something' } + 'UseResponse' => { 'type' => 'object', 'properties' => { 'description' => { 'description' => '', 'type' => 'string' }, '$responses' => { 'type' => 'array', 'items' => { '$ref' => '#/definitions/ResponseItem' }, 'description' => '' } }, 'description' => 'UseResponse model' } } end let(:swagger_params_as_response_object) do { - 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'This returns something' } + 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'description' => 'status code', 'type' => 'integer', 'format' => 'int32' }, 'message' => { 'description' => 'error message', 'type' => 'string' } }, 'description' => 'ApiError model' } } end @@ -372,7 +372,7 @@ class DocumentedHashAndArrayModel < Representable::Decorator 'type' => 'object', 'required' => ['elements'], 'properties' => { 'elements' => { 'type' => 'array', 'items' => { '$ref' => '#/definitions/QueryInputElement' }, 'description' => 'Set of configuration' } }, - 'description' => 'nested route inside namespace' + 'description' => 'QueryInput model' }, 'QueryInputElement' => { 'type' => 'object', @@ -382,7 +382,7 @@ class DocumentedHashAndArrayModel < Representable::Decorator 'ApiError' => { 'type' => 'object', 'properties' => { 'code' => { 'type' => 'integer', 'format' => 'int32', 'description' => 'status code' }, 'message' => { 'type' => 'string', 'description' => 'error message' } }, - 'description' => 'This gets Things.' + 'description' => 'ApiError model' }, 'Something' => { 'type' => 'object', @@ -392,7 +392,7 @@ class DocumentedHashAndArrayModel < Representable::Decorator 'links' => { 'type' => 'array', 'items' => { 'description' => '', 'type' => 'link' } }, 'others' => { 'description' => '', 'type' => 'text' } }, - 'description' => 'This gets Things.' + 'description' => 'Something model' } } } diff --git a/spec/swagger_v2/api_swagger_v2_response_with_models_spec.rb b/spec/swagger_v2/api_swagger_v2_response_with_models_spec.rb new file mode 100644 index 00000000..e9995f04 --- /dev/null +++ b/spec/swagger_v2/api_swagger_v2_response_with_models_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'response' do + include_context "#{MODEL_PARSER} swagger example" + + before :all do + module TheApi + class ResponseApiModels < Grape::API + format :json + + desc 'This returns something', + success: [{ code: 200 }], + failure: [ + { code: 400, message: 'NotFound', model: '' }, + { code: 404, message: 'BadRequest', model: Entities::ApiError } + ] + get '/use-response' do + { 'declared_params' => declared(params) } + end + + add_swagger_documentation(models: [Entities::UseResponse]) + end + end + end + + def app + TheApi::ResponseApiModels + end + + describe 'uses entity as response object implicitly with route name' do + subject do + get '/swagger_doc/use-response' + JSON.parse(last_response.body) + end + + specify do + expect(subject['paths']['/use-response']['get']).to eql( + 'description' => 'This returns something', + 'produces' => ['application/json'], + 'responses' => { + '200' => { 'description' => 'This returns something', 'schema' => { '$ref' => '#/definitions/UseResponse' } }, + '400' => { 'description' => 'NotFound' }, + '404' => { 'description' => 'BadRequest', 'schema' => { '$ref' => '#/definitions/ApiError' } } + }, + 'tags' => ['use-response'], + 'operationId' => 'getUseResponse' + ) + expect(subject['definitions']).to eql(swagger_entity_as_response_object) + end + end +end diff --git a/spec/swagger_v2/reference_entity_spec.rb b/spec/swagger_v2/reference_entity_spec.rb index f0f8dfa8..ab7ae247 100644 --- a/spec/swagger_v2/reference_entity_spec.rb +++ b/spec/swagger_v2/reference_entity_spec.rb @@ -103,7 +103,7 @@ def app 'description' => 'Something interesting.' } }, - 'description' => 'This returns kind and something or an error' + 'description' => 'KindCustom model' ) end end @@ -122,7 +122,7 @@ def app 'title' => { 'type' => 'string', 'description' => 'Title of the parent.' }, 'child' => { 'type' => 'string', 'description' => 'Child property.' } }, - 'description' => 'This returns a child entity' + 'description' => 'MyAPI::Child model' ) end end