Allow using nicknames for body definitions (#862)

* Fix typo * Refactor MoveParams.parent_definition_of_params to use OperationId.build rather than OperationId.manipulate This means it'll use route nicknames if those are available. It also means route parameters will be used in the definition names. * Simplify MoveParams.build_body_parameter MoveParams.build_definition returns the passed in name, so name and referenced_definition were always the same value. * Fix Rubocop offenses * Fix old reference to Travis CI in CONTRIBUTING * Fix CHANGELOG * Update CHANGELOG and UPGRADING
ruby-grape · Jul 26, 2022 · 1a3e3f5 · 1a3e3f5
1 parent 06975c8
commit 1a3e3f5
Show file tree

Hide file tree

Showing 13 changed files with 100 additions and 77 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,10 +3,11 @@
 #### Fixes
 
 * [#850](https://github.com/ruby-grape/grape-swagger/pull/850): Fix value of `enum` to be `Array` - [@takahashim](https://github.com/takahashim)
-* [#846] (https://github.com/ruby-grape/grape-swagger/pull/846): Fixes oapi rake tasks, allows generating sepcs for different API versions.
+* [#846](https://github.com/ruby-grape/grape-swagger/pull/846): Fixes oapi rake tasks, allows generating sepcs for different API versions.
 * [#852](https://github.com/ruby-grape/grape-swagger/pull/852): Fix example to work without error - [@takahashim](https://github.com/takahashim)
-* Your contribution here.
 * [#853](https://github.com/ruby-grape/grape-swagger/pull/853): Add webrick gem so that example works in Ruby 3.x - [@takahashim](https://github.com/takahashim)
+* [#862](https://github.com/ruby-grape/grape-swagger/pull/862): Allow using nicknames for body definitions - [@magni-](https://github.com/magni-)
+* Your contribution here.
 
 ### 1.4.2 (October 22, 2021)
 

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -115,7 +115,7 @@ git push origin my-feature-branch -f
 
 ## Check on Your Pull Request
 
-Go back to your pull request after a few minutes and see whether it passed muster with Travis-CI. Everything should look green, otherwise fix issues and amend your commit as described above.
+Go back to your pull request after a few minutes and see whether it passed muster with GitHub Actions. Everything should look green, otherwise fix issues and amend your commit as described above.
 
 ## Be Patient
 

diff --git a/Gemfile b/Gemfile
@@ -6,7 +6,7 @@ ruby RUBY_VERSION
 
 gemspec
 
-gem 'grape', case version = ENV['GRAPE_VERSION'] || '>= 1.5.0'
+gem 'grape', case version = ENV.fetch('GRAPE_VERSION', '>= 1.5.0')
              when 'HEAD'
                { git: 'https://github.com/ruby-grape/grape' }
              else

diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,5 +1,15 @@
 ## Upgrading Grape-swagger
 
+### Upgrading to >= 1.5.0
+
+- The names generated for body parameter definitions and their references has changed. It'll now include the HTTP action as well as any path parameters.
+  - E.g, given a `PUT /things/:id` endpoint, `paths.things/{id}.put.parameters` in the generated Swaggerfile will contain the following:
+  - With `grape-swagger < 1.5.0`: `{ "name": "Things", ..., "schema": { "$ref": "#/definitions/putThings" } }`
+  - With `grape-swagger >= 1.5.0`: `{ "name": "putThingsId", ..., "schema": { "$ref": "#/definitions/putThingsId" } }`
+- If you use the `nickname` option for an endpoint, that nickname will be used for both the parameter name and its definition reference.
+  - E.g., if the endpoint above were nicknamed `put-thing`, the generated Swaggerfile will contain `{ "name": "put-thing", ..., "schema": { "$ref": "#/definitions/put-thing" } }`
+
+
 ### Upgrading to >= 1.4.2
 
 - `additionalProperties` has been deprecated and will be removed in a future version of `grape-swagger`. It has been replaced with `additional_properties`.

diff --git a/grape-swagger.gemspec b/grape-swagger.gemspec
@@ -19,6 +19,5 @@ Gem::Specification.new do |s|
   s.add_runtime_dependency 'grape', '~> 1.3'
 
   s.files         = `git ls-files`.split("\n")
-  s.test_files    = `git ls-files -- {test,spec}/*`.split("\n")
   s.require_paths = ['lib']
 end
diff --git a/lib/grape-swagger/doc_methods/move_params.rb b/lib/grape-swagger/doc_methods/move_params.rb
@@ -38,15 +38,15 @@ def correct_array_param(param)
         end
 
         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]
+          definition_name = OperationId.build(route, path)
+          build_definition(definition_name, params)
+          definition = @definitions[definition_name]
 
           move_params_to_new(definition, params)
 
           definition[:description] = route.description if route.try(:description)
 
-          build_body_parameter(referenced_definition, definition_name, route.options)
+          build_body_parameter(definition_name, route.options)
         end
 
         def move_params_to_new(definition, params)
@@ -142,17 +142,16 @@ def add_to_required(definition, value)
           definition[:required].push(*value)
         end
 
-        def build_body_parameter(reference, name, options)
+        def build_body_parameter(name, options)
           {}.tap do |x|
             x[:name] = options[:body_name] || name
             x[:in] = 'body'
             x[:required] = true
-            x[:schema] = { '$ref' => "#/definitions/#{reference}" }
+            x[:schema] = { '$ref' => "#/definitions/#{name}" }
           end
         end
 
-        def build_definition(name, params, verb = nil)
-          name = "#{verb}#{name}" if verb
+        def build_definition(name, params)
           @definitions[name] = should_expose_as_array?(params) ? array_type : object_type
 
           name

diff --git a/lib/grape-swagger/rake/oapi_tasks.rb b/lib/grape-swagger/rake/oapi_tasks.rb
@@ -98,7 +98,7 @@ def urls_for(api_class)
                  .select { |e| e.include?('doc') }
                  .reject { |e| e.include?(':name') }
                  .map { |e| format_path(e) }
-                 .map { |e| [e, ENV['resource']].join('/').chomp('/') }
+                 .map { |e| [e, ENV.fetch('resource', nil)].join('/').chomp('/') }
       end
 
       def format_path(path)

diff --git a/spec/issues/579_align_put_post_parameters_spec.rb b/spec/issues/579_align_put_post_parameters_spec.rb
@@ -103,8 +103,8 @@ class Spec < Grape::Entity
         [
           { 'in' => 'path', 'name' => 'guid', 'type' => 'string', 'format' => 'guid', 'required' => true },
           {
-            'name' => 'Issue579ImplicitBodyParameter', 'in' => 'body', 'required' => true, 'schema' => {
-              '$ref' => '#/definitions/putIssue579ImplicitBodyParameter'
+            'name' => 'putIssue579ImplicitBodyParameterGuid', 'in' => 'body', 'required' => true, 'schema' => {
+              '$ref' => '#/definitions/putIssue579ImplicitBodyParameterGuid'
             }
           }
         ]
@@ -130,8 +130,8 @@ class Spec < Grape::Entity
         [
           { 'in' => 'path', 'name' => 'guid', 'type' => 'string', 'format' => 'guid', 'required' => true },
           {
-            'name' => 'Issue579ExplicitBodyParameter', 'in' => 'body', 'required' => true, 'schema' => {
-              '$ref' => '#/definitions/putIssue579ExplicitBodyParameter'
+            'name' => 'putIssue579ExplicitBodyParameterGuid', 'in' => 'body', 'required' => true, 'schema' => {
+              '$ref' => '#/definitions/putIssue579ExplicitBodyParameterGuid'
             }
           }
         ]
@@ -157,7 +157,7 @@ class Spec < Grape::Entity
         [
           { 'in' => 'path', 'name' => 'guid', 'type' => 'string', 'format' => 'guid', 'required' => true },
           {
-            'name' => 'Issue579NamespaceParamGuidBodyParameter', 'in' => 'body', 'required' => true, 'schema' => {
+            'name' => 'putIssue579NamespaceParamGuidBodyParameter', 'in' => 'body', 'required' => true, 'schema' => {
               '$ref' => '#/definitions/putIssue579NamespaceParamGuidBodyParameter'
             }
           }

diff --git a/spec/issues/751_deeply_nested_objects_spec.rb b/spec/issues/751_deeply_nested_objects_spec.rb
@@ -75,7 +75,7 @@ def self.vrp_request_service(this)
   end
 
   describe 'Correctness of vrp Points' do
-    let(:get_points_response) { subject['definitions']['postVrpSubmit']['properties']['vrp']['properties']['points'] }
+    let(:get_points_response) { subject['definitions']['vrp']['properties']['vrp']['properties']['points'] }
     specify do
       expect(get_points_response).to eql(
         'type' => 'array',
@@ -111,7 +111,7 @@ def self.vrp_request_service(this)
   end
 
   describe 'Correctness of vrp Services' do
-    let(:get_service_response) { subject['definitions']['postVrpSubmit']['properties']['vrp']['properties']['services'] }
+    let(:get_service_response) { subject['definitions']['vrp']['properties']['vrp']['properties']['services'] }
     specify do
       expect(get_service_response).to include(
         'type' => 'array',

diff --git a/spec/lib/move_params_spec.rb b/spec/lib/move_params_spec.rb
@@ -103,15 +103,30 @@
         subject.to_definition(path, params, route, definitions)
         expect(params).to eql(
           [
-            { name: 'InBody', in: 'body', required: true, schema: { '$ref' => '#/definitions/postInBody' } }
+            { name: 'postInBody', in: 'body', required: true, schema: { '$ref' => '#/definitions/postInBody' } }
           ]
         )
         expect(subject.definitions['postInBody']).not_to include :description
         expect(subject.definitions['postInBody']).to eql expected_post_defs
       end
+
+      context 'with a nickname' do
+        let(:route_options) { { nickname: 'post-body' } }
+
+        specify do
+          subject.to_definition(path, params, route, definitions)
+          expect(params).to eql(
+            [
+              { name: 'post-body', in: 'body', required: true, schema: { '$ref' => '#/definitions/post-body' } }
+            ]
+          )
+          expect(subject.definitions['post-body']).not_to include :description
+          expect(subject.definitions['post-body']).to eql expected_post_defs
+        end
+      end
     end
 
-    describe 'POST' do
+    describe 'PUT' do
       let(:params) { paths['/in_body/{key}'][:put][:parameters] }
       let(:route) { Grape::Router::Route.new('PUT', path.dup, **route_options) }
 
@@ -120,12 +135,28 @@
         expect(params).to eql(
           [
             { in: 'path', name: 'key', description: nil, type: 'integer', format: 'int32', required: true },
-            { name: 'InBody', in: 'body', required: true, schema: { '$ref' => '#/definitions/putInBody' } }
+            { name: 'putInBody', in: 'body', required: true, schema: { '$ref' => '#/definitions/putInBody' } }
           ]
         )
         expect(subject.definitions['putInBody']).not_to include :description
         expect(subject.definitions['putInBody']).to eql expected_put_defs
       end
+
+      context 'with a nickname' do
+        let(:route_options) { { nickname: 'put-body' } }
+
+        specify do
+          subject.to_definition(path, params, route, definitions)
+          expect(params).to eql(
+            [
+              { in: 'path', name: 'key', description: nil, type: 'integer', format: 'int32', required: true },
+              { name: 'put-body', in: 'body', required: true, schema: { '$ref' => '#/definitions/put-body' } }
+            ]
+          )
+          expect(subject.definitions['put-body']).not_to include :description
+          expect(subject.definitions['put-body']).to eql expected_put_defs
+        end
+      end
     end
   end
 
@@ -167,56 +198,39 @@
       let(:params) { [{ in: 'body', name: 'address[street][name]', description: 'street', type: 'string', required: true }] }
       before do
         subject.instance_variable_set(:@definitions, definitions)
-        subject.send(:build_definition, name, params, verb)
+        subject.send(:build_definition, name, params)
       end
 
-      describe 'verb given' do
-        let(:verb) { 'post' }
-        let(:name) { 'Foo' }
-        let(:definitions) { {} }
+      let(:name) { 'FooBar' }
+      let(:definitions) { {} }
 
-        specify do
-          definition = definitions.to_a.first
-          expect(definition.first).to eql 'postFoo'
-          expect(definition.last).to eql(type: 'object', properties: {})
-        end
-      end
-
-      describe 'no verb given' do
-        let(:name) { 'FooBar' }
-        let(:definitions) { {} }
-        let(:verb) { nil }
-
-        specify do
-          definition = definitions.to_a.first
-          expect(definition.first).to eql 'FooBar'
-          expect(definition.last).to eql(type: 'object', properties: {})
-        end
+      specify do
+        definition = definitions.to_a.first
+        expect(definition.first).to eql 'FooBar'
+        expect(definition.last).to eql(type: 'object', properties: {})
       end
     end
 
     describe 'build_body_parameter' do
-      describe 'name given' do
-        let(:name) { 'Foo' }
-        let(:reference) { 'Bar' }
+      let(:name) { 'Foo' }
+      let(:reference) { 'Bar' }
+      let(:expected_param) do
+        { name: name, in: 'body', required: true, schema: { '$ref' => "#/definitions/#{name}" } }
+      end
+      specify do
+        parameter = subject.send(:build_body_parameter, name, {})
+        expect(parameter).to eql expected_param
+      end
+
+      describe 'body_name option specified' do
+        let(:route_options) { { body_name: 'body' } }
         let(:expected_param) do
-          { name: name, in: 'body', required: true, schema: { '$ref' => "#/definitions/#{reference}" } }
+          { name: route_options[:body_name], in: 'body', required: true, schema: { '$ref' => "#/definitions/#{name}" } }
         end
         specify do
-          parameter = subject.send(:build_body_parameter, reference, name, {})
+          parameter = subject.send(:build_body_parameter, name, route_options)
           expect(parameter).to eql expected_param
         end
-
-        describe 'body_name option specified' do
-          let(:route_options) { { body_name: 'body' } }
-          let(:expected_param) do
-            { name: route_options[:body_name], in: 'body', required: true, schema: { '$ref' => "#/definitions/#{reference}" } }
-          end
-          specify do
-            parameter = subject.send(:build_body_parameter, reference, name, route_options)
-            expect(parameter).to eql expected_param
-          end
-        end
       end
     end
 

diff --git a/spec/swagger_v2/api_swagger_v2_additional_properties_spec.rb b/spec/swagger_v2/api_swagger_v2_additional_properties_spec.rb
@@ -37,7 +37,7 @@ class Element < Grape::Entity
     specify do
       expect(subject.dig('paths', '/things', 'post', 'parameters')).to eql(
         [
-          { 'name' => 'Things', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/postThings' } }
+          { 'name' => 'postThings', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/postThings' } }
         ]
       )
     end

diff --git a/spec/swagger_v2/api_swagger_v2_param_type_body_nested_spec.rb b/spec/swagger_v2/api_swagger_v2_param_type_body_nested_spec.rb
@@ -136,7 +136,7 @@ def app
       specify do
         expect(subject['paths']['/simple_nested_params/in_body']['post']['parameters']).to eql(
           [
-            { 'name' => 'SimpleNestedParamsInBody', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/postSimpleNestedParamsInBody' } }
+            { 'name' => 'postSimpleNestedParamsInBody', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/postSimpleNestedParamsInBody' } }
           ]
         )
       end
@@ -177,13 +177,13 @@ def app
         expect(subject['paths']['/simple_nested_params/in_body/{id}']['put']['parameters']).to eql(
           [
             { 'in' => 'path', 'name' => 'id', 'type' => 'integer', 'format' => 'int32', 'required' => true },
-            { 'name' => 'SimpleNestedParamsInBody', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/putSimpleNestedParamsInBody' } }
+            { 'name' => 'putSimpleNestedParamsInBodyId', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/putSimpleNestedParamsInBodyId' } }
           ]
         )
       end
 
       specify do
-        expect(subject['definitions']['putSimpleNestedParamsInBody']).to eql(
+        expect(subject['definitions']['putSimpleNestedParamsInBodyId']).to eql(
           'type' => 'object',
           'properties' => {
             'name' => { 'type' => 'string', 'description' => 'name' },
@@ -214,7 +214,7 @@ def app
         expect(subject['paths']['/multiple_nested_params/in_body']['post']['parameters']).to eql(
           [
             {
-              'name' => 'MultipleNestedParamsInBody',
+              'name' => 'postMultipleNestedParamsInBody',
               'in' => 'body',
               'required' => true,
               'schema' => { '$ref' => '#/definitions/postMultipleNestedParamsInBody' }
@@ -267,13 +267,13 @@ def app
         expect(subject['paths']['/multiple_nested_params/in_body/{id}']['put']['parameters']).to eql(
           [
             { 'in' => 'path', 'name' => 'id', 'type' => 'integer', 'format' => 'int32', 'required' => true },
-            { 'name' => 'MultipleNestedParamsInBody', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/putMultipleNestedParamsInBody' } }
+            { 'name' => 'putMultipleNestedParamsInBodyId', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/putMultipleNestedParamsInBodyId' } }
           ]
         )
       end
 
       specify do
-        expect(subject['definitions']['putMultipleNestedParamsInBody']).to eql(
+        expect(subject['definitions']['putMultipleNestedParamsInBodyId']).to eql(
           'type' => 'object',
           'properties' => {
             'name' => { 'type' => 'string', 'description' => 'name' },
@@ -313,7 +313,7 @@ def app
       specify do
         expect(subject['paths']['/nested_params_array/in_body']['post']['parameters']).to eql(
           [
-            { 'name' => 'NestedParamsArrayInBody', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/postNestedParamsArrayInBody' } }
+            { 'name' => 'postNestedParamsArrayInBody', 'in' => 'body', 'required' => true, 'schema' => { '$ref' => '#/definitions/postNestedParamsArrayInBody' } }
           ]
         )
       end