From 2fc603df805b6e6ed7134dd175c352b32b4781ce Mon Sep 17 00:00:00 2001 From: LeFnord Date: Mon, 27 Feb 2017 14:29:21 +0100 Subject: [PATCH] issue #584: do not mutate route.path - adds changelog entry --- CHANGELOG.md | 1 + lib/grape-swagger/doc_methods/move_params.rb | 8 ++++---- lib/grape-swagger/doc_methods/operation_id.rb | 8 ++------ lib/grape-swagger/doc_methods/parse_params.rb | 4 +--- lib/grape-swagger/doc_methods/path_string.rb | 2 +- lib/grape-swagger/endpoint.rb | 14 +++++++------- spec/lib/move_params_spec.rb | 11 ++++++----- spec/lib/path_string_spec.rb | 9 ++++++++- spec/lib/tag_name_description_spec.rb | 13 +++++++------ 9 files changed, 37 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b414bf..a8c54ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ #### Fixes * [#580](https://github.com/ruby-grape/grape-swagger/pull/580): Issue #578: fixes duplicated path params - [@LeFnord](https://github.com/LeFnord). +* [#585](https://github.com/ruby-grape/grape-swagger/pull/585): Issue #584: do not mutate route.path - [@LeFnord](https://github.com/LeFnord). * Your contribution here. diff --git a/lib/grape-swagger/doc_methods/move_params.rb b/lib/grape-swagger/doc_methods/move_params.rb index 10e4108c..426ae037 100644 --- a/lib/grape-swagger/doc_methods/move_params.rb +++ b/lib/grape-swagger/doc_methods/move_params.rb @@ -8,7 +8,7 @@ def can_be_moved?(params, http_verb) move_methods.include?(http_verb) && includes_body_param?(params) end - def to_definition(params, route, definitions) + def to_definition(path, params, route, definitions) @definitions = definitions unify!(params) @@ -16,7 +16,7 @@ def to_definition(params, route, definitions) return (params + correct_array_param(params_to_move)) if should_correct_array?(params_to_move) - params << parent_definition_of_params(params_to_move, route) + params << parent_definition_of_params(params_to_move, path, route) params end @@ -33,8 +33,8 @@ def correct_array_param(param) param end - def parent_definition_of_params(params, route) - definition_name = OperationId.manipulate(parse_model(route.path)) + def parent_definition_of_params(params, path, route) + definition_name = OperationId.manipulate(parse_model(path)) referenced_definition = build_definition(definition_name, params, route.request_method.downcase) definition = @definitions[referenced_definition] diff --git a/lib/grape-swagger/doc_methods/operation_id.rb b/lib/grape-swagger/doc_methods/operation_id.rb index 47c7209a..c194b0ba 100644 --- a/lib/grape-swagger/doc_methods/operation_id.rb +++ b/lib/grape-swagger/doc_methods/operation_id.rb @@ -4,16 +4,12 @@ class OperationId class << self def build(route, path = nil) if route.options[:nickname] - operation_id = route.options[:nickname] + route.options[:nickname] else verb = route.request_method.to_s.downcase - operation = manipulate(path) unless path.nil? - - operation_id = "#{verb}#{operation}" + "#{verb}#{operation}" end - - operation_id end def manipulate(path) diff --git a/lib/grape-swagger/doc_methods/parse_params.rb b/lib/grape-swagger/doc_methods/parse_params.rb index e5db25a8..a4f160c9 100644 --- a/lib/grape-swagger/doc_methods/parse_params.rb +++ b/lib/grape-swagger/doc_methods/parse_params.rb @@ -2,10 +2,8 @@ module GrapeSwagger module DocMethods class ParseParams class << self - def call(param, settings, route, definitions) - path = route.path + def call(param, settings, path, route, definitions) method = route.request_method - additional_documentation = settings.fetch(:documentation, {}) settings.merge!(additional_documentation) data_type = DataType.call(settings) diff --git a/lib/grape-swagger/doc_methods/path_string.rb b/lib/grape-swagger/doc_methods/path_string.rb index 955a56f5..1929f3ce 100644 --- a/lib/grape-swagger/doc_methods/path_string.rb +++ b/lib/grape-swagger/doc_methods/path_string.rb @@ -3,7 +3,7 @@ module DocMethods class PathString class << self def build(route, options = {}) - path = route.path + path = route.path.dup # always removing format path.sub!(/\(\.\w+?\)$/, '') path.sub!('(.:format)', '') diff --git a/lib/grape-swagger/endpoint.rb b/lib/grape-swagger/endpoint.rb index 5e3b77ab..2dc21e0f 100644 --- a/lib/grape-swagger/endpoint.rb +++ b/lib/grape-swagger/endpoint.rb @@ -108,10 +108,10 @@ def method_object(route, options, path) method[:description] = description_object(route) method[:produces] = produces_object(route, options[:produces] || options[:format]) method[:consumes] = consumes_object(route, options[:format]) - method[:parameters] = params_object(route) + method[:parameters] = params_object(route, path) method[:security] = security_object(route) method[:responses] = response_object(route) - method[:tags] = route.options.fetch(:tags, tag_object(route)) + method[:tags] = route.options.fetch(:tags, tag_object(route, path)) method[:operationId] = GrapeSwagger::DocMethods::OperationId.build(route, path) method.delete_if { |_, value| value.blank? } @@ -161,7 +161,7 @@ def consumes_object(route, format) mime_types end - def params_object(route) + def params_object(route, path) 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 == '' @@ -170,11 +170,11 @@ def params_object(route) elsif value[:documentation] expose_params(value[:documentation][:type]) end - GrapeSwagger::DocMethods::ParseParams.call(param, value, route, @definitions) + GrapeSwagger::DocMethods::ParseParams.call(param, value, path, route, @definitions) end if GrapeSwagger::DocMethods::MoveParams.can_be_moved?(parameters, route.request_method) - parameters = GrapeSwagger::DocMethods::MoveParams.to_definition(parameters, route, @definitions) + parameters = GrapeSwagger::DocMethods::MoveParams.to_definition(path, parameters, route, @definitions) end parameters @@ -227,11 +227,11 @@ def apply_success_codes(route) [default_code] end - def tag_object(route) + def tag_object(route, path) version = GrapeSwagger::DocMethods::Version.get(route) version = [version] unless version.is_a?(Array) Array( - route.path.split('{')[0].split('/').reject(&:empty?).delete_if do |i| + path.split('{')[0].split('/').reject(&:empty?).delete_if do |i| i == route.prefix.to_s || version.map(&:to_s).include?(i) end.first ) diff --git a/spec/lib/move_params_spec.rb b/spec/lib/move_params_spec.rb index 76becf97..53a4b4ce 100644 --- a/spec/lib/move_params_spec.rb +++ b/spec/lib/move_params_spec.rb @@ -91,18 +91,19 @@ end describe 'parent_definition_of_params' do + let(:path) { '/in_body' } describe 'POST' do - let(:params) { paths['/in_body'][:post][:parameters] } + let(:params) { paths[path][:post][:parameters] } let(:options) do { method: 'POST' } end - let(:env) { Rack::MockRequest.env_for('/in_body', options) } + let(:env) { Rack::MockRequest.env_for(path, options) } let(:request) { Grape::Request.new(env) } specify do - subject.to_definition(params, request, definitions) + subject.to_definition(path, params, request, definitions) expect(params).to eql( [ { name: 'InBody', in: 'body', required: true, schema: { '$ref' => '#/definitions/postInBody' } } @@ -120,11 +121,11 @@ method: 'PUT' } end - let(:env) { Rack::MockRequest.env_for('/in_body', options) } + let(:env) { Rack::MockRequest.env_for(path, options) } let(:request) { Grape::Request.new(env) } specify do - subject.to_definition(params, request, definitions) + subject.to_definition(path, params, request, definitions) expect(params).to eql( [ { in: 'path', name: 'key', description: nil, type: 'integer', format: 'int32', required: true }, diff --git a/spec/lib/path_string_spec.rb b/spec/lib/path_string_spec.rb index 40c6592d..ac507dee 100644 --- a/spec/lib/path_string_spec.rb +++ b/spec/lib/path_string_spec.rb @@ -6,7 +6,14 @@ specify { expect(subject).to eql GrapeSwagger::DocMethods::PathString } specify { expect(subject).to respond_to :build } - describe 'operation_id_object' do + describe 'path_string_object' do + specify 'The original route path is not mutated' do + route = Struct.new(:version, :path).new + route.path = '/foo/:dynamic/bar' + subject.build(route, add_version: true) + expect(route.path).to eq '/foo/:dynamic/bar' + end + describe 'version' do describe 'defaults: given, true' do let(:options) { { add_version: true } } diff --git a/spec/lib/tag_name_description_spec.rb b/spec/lib/tag_name_description_spec.rb index 5e3dc5e3..7ee0a8ba 100644 --- a/spec/lib/tag_name_description_spec.rb +++ b/spec/lib/tag_name_description_spec.rb @@ -15,11 +15,12 @@ end describe '#build' do - subject { described_class.build(paths) } + let(:object) { described_class.build(paths) } + describe 'empty paths' do let(:paths) { {} } specify do - expect(subject).to eql([]) + expect(object).to eql([]) end end @@ -30,7 +31,7 @@ end specify do - expect(subject).to eql [{ name: 'tags_given', description: 'Operations about tags_givens' }] + expect(object).to eql [{ name: 'tags_given', description: 'Operations about tags_givens' }] end end @@ -40,7 +41,7 @@ end specify do - expect(subject).to eql [{ name: 'tags_given', description: 'Operations about tags_givens' }] + expect(object).to eql [{ name: 'tags_given', description: 'Operations about tags_givens' }] end end @@ -53,7 +54,7 @@ end specify do - expect(subject).to eql [ + expect(object).to eql [ { name: 'tags_given', description: 'Operations about tags_givens' }, { name: 'another_tag_given', description: 'Operations about another_tag_givens' } ] @@ -68,7 +69,7 @@ end specify do - expect(subject).to eql [{ name: 'tags_given', description: 'Operations about tags_givens' }] + expect(object).to eql [{ name: 'tags_given', description: 'Operations about tags_givens' }] end end end