Skip to content

Don't overwrite model description with the route description. #804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
# ...
Expand Down
4 changes: 4 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
@@ -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: `<<EntityName>> 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
Expand Down
127 changes: 65 additions & 62 deletions lib/grape-swagger/doc_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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'
Expand All @@ -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
15 changes: 8 additions & 7 deletions lib/grape-swagger/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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] == ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh … ok … will have a look on it later, maybe it can be get together with no model key given

Copy link
Contributor Author

@Bhacaz Bhacaz Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeFnord I did like that because by default if no model key is provided (line 219) the model use for the response will be the one returned by GrapeSwagger::DocMethods::PathString#build called here

@item, path = GrapeSwagger::DocMethods::PathString.build(route, options)


@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
Expand Down
16 changes: 8 additions & 8 deletions spec/support/model_parsers/entity_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -320,7 +320,7 @@ class DocumentedHashAndArrayModel < Grape::Entity
'links' => { 'type' => 'array', 'items' => { 'type' => 'link' } },
'others' => { 'type' => 'text' }
},
'description' => 'This gets Things.'
'description' => 'Something model'
}
}
}
Expand Down
16 changes: 8 additions & 8 deletions spec/support/model_parsers/mock_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This returns something'
'description' => 'ApiError model'
},
'UseItemResponseAsType' => {
'type' => 'object',
Expand All @@ -122,7 +122,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This returns something'
'description' => 'UseItemResponseAsType model'
}
}
end
Expand All @@ -137,7 +137,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This returns something'
'description' => 'UseResponse model'
},
'ApiError' => {
'type' => 'object',
Expand All @@ -147,7 +147,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This returns something'
'description' => 'ApiError model'
}
}
end
Expand All @@ -162,7 +162,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This returns something'
'description' => 'ApiError model'
}
}
end
Expand Down Expand Up @@ -296,7 +296,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'nested route inside namespace'
'description' => 'QueryInput model'
},
'ApiError' => {
'type' => 'object',
Expand All @@ -306,7 +306,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This gets Things.'
'description' => 'ApiError model'
},
'Something' => {
'type' => 'object',
Expand All @@ -316,7 +316,7 @@ class ApiResponse < OpenStruct; end
'description' => "it's a mock"
}
},
'description' => 'This gets Things.'
'description' => 'Something model'
}
}
}
Expand Down
Loading