Allow using nicknames for body definitions #862

magni- · 2022-07-14T07:13:20Z

We have an API defined roughly as follows:

resource :things do
  desc "Batch update things" do
    nickname "put-things"
  end
  params do
    # a bunch of params
  end
  put "/" do
    # ...
  end

  segment "/:id" do
    desc "Update thing" do
      nickname "put-thing"
    end
    params do
      # a bunch of other params
    end
    put "/" do
      # ...
    end
  end
end

We noticed the docs for our API were incorrect. Both paths.things.put.parameters.0.schema.$ref and paths.things{id}.put.parameters.1.schema.$ref (parameters.0 is for the path :id parameter) are pointing to the same thing: "#/definitions/putThings. It turns out the put-thing body parameter definition is overriding the previous put-things body parameter definition, rather than defining its own thing. It's been a while since I've dug into the grape-swagger code, so I went with the (IMO) easy fix: using route nicknames if they're available to name the body parameter definitions.

Edit: Based on the changes I had to make to get specs to pass, this change would also have solved our issue without using route nicknames, since the references become putTests and putTestsId.

Related issue: ruby-grape/grape#2035

…ild 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.

MoveParams.build_definition returns the passed in name, so name and referenced_definition were always the same value.

magni- · 2022-07-14T07:57:36Z

I'm not quite sure why the build is erroring on Ruby@head, seems unrelated to the changes in this PR?

NoMethodError:
  undefined method `<<' for {:constraint=>"[^/\\?#.]"}:Hash

# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/node.rb:59:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/node.rb:112:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/node.rb:39:in `block in parse'
# <internal:kernel>:90:in `tap'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/node.rb:39:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/parser.rb:71:in `node'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-grape-1.0.2/lib/mustermann/grape.rb:20:in `block in <class:Grape>'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/parser.rb:91:in `read'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/parser.rb:59:in `block in parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/node.rb:58:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/node.rb:171:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/parser.rb:71:in `node'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/parser.rb:59:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/parser.rb:17:in `parse'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/pattern.rb:91:in `block in to_ast'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/equality_map.rb:43:in `fetch'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/pattern.rb:90:in `to_ast'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/ast/pattern.rb:81:in `compile'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/regexp_based.rb:19:in `initialize'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/pattern.rb:59:in `new'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/pattern.rb:59:in `block in new'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/equality_map.rb:43:in `fetch'
# ./vendor/bundle/ruby/3.2.0+1/gems/mustermann-1.1.1/lib/mustermann/pattern.rb:59:in `new'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/router/pattern.rb:23:in `initialize'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/router/route.rb:53:in `new'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/router/route.rb:53:in `initialize'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:168:in `new'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:168:in `block in to_routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:205:in `block (2 levels) in map_routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:205:in `map'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:205:in `block in map_routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:205:in `map'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:205:in `map_routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:165:in `to_routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:138:in `routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:138:in `collect'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/endpoint.rb:138:in `routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api/instance.rb:97:in `map'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api/instance.rb:97:in `prepare_routes'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/dsl/routing.rb:190:in `routes'
# ./lib/grape-swagger.rb:27:in `combine_routes'
# ./lib/grape-swagger.rb:131:in `add_swagger_documentation'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api.rb:160:in `replay_step_on'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api.rb:151:in `block in add_setup'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api.rb:150:in `each'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api.rb:150:in `add_setup'
# ./vendor/bundle/ruby/3.2.0+1/gems/grape-1.6.2/lib/grape/api.rb:55:in `block (2 levels) in override_all_methods!'
# ./spec/lib/oapi_tasks_spec.rb:22:in `<class:Base>'
# ./spec/lib/oapi_tasks_spec.rb:19:in `<module:Api>'
# ./spec/lib/oapi_tasks_spec.rb:6:in `block in <top (required)>'
# ./spec/lib/oapi_tasks_spec.rb:5:in `<top (required)>'

magni- · 2022-07-20T05:45:41Z

I'm still seeing the same failure on mustermann@2.0.1. It looks like it would work if we added nil before constraint here: https://github.com/ruby-grape/mustermann-grape/blob/v1.0.2/lib/mustermann/grape.rb#L20 ? But the proper fix seems to be an update to mustermann itself, I started a PR here: sinatra/mustermann#134

Either way I don't think this should be a blocker for this PR 🙇🏼

magni- · 2022-07-21T23:08:33Z

@LeFnord anything else I should add to this PR, or does this look good to go? 🙇🏼 Thanks!

magni- · 2022-07-23T06:18:31Z

I'm still seeing the same failure on mustermann@2.0.1. It looks like it would work if we added nil before constraint here: https://github.com/ruby-grape/mustermann-grape/blob/v1.0.2/lib/mustermann/grape.rb#L20 ? But the proper fix seems to be an update to mustermann itself, I started a PR here: sinatra/mustermann#134

Either way I don't think this should be a blocker for this PR 🙇🏼

mustermann v2.0.2 was released and the build is now fully green 🎉

LeFnord · 2022-07-24T19:10:42Z

thanks @magni- … it seems it changes something so maybe add an entry to UPGRADING.md

and one question: should we add gem musterman, '~> 2.0.2' to ensure it works correct?

magni- · 2022-07-25T00:12:23Z

it seems it changes something so maybe add an entry to UPGRADING.md

👍🏼

and one question: should we add gem musterman, '~> 2.0.2' to ensure it works correct?

Good question. The build works fine on all released versions of Ruby, it's just the upcoming 3.2 (which won't be released for another 5 months) which had issues. If we add an explicit requirement, might as well make it to ~> 3 which was just released (the only change from 2.0.2 is dropping support for Ruby 2.5 and below. grape-swagger already requires Ruby 2.7+).

magni- · 2022-07-25T03:13:14Z

@LeFnord I've updated UPGRADING. For the version number, this feels more like a 1.5.0 than a 1.4.3 ?

LeFnord · 2022-07-25T16:08:54Z

@magni- … yeah, definitely will it be 1.5.0 …
what is with mustermann, should it be aded to the Gemfile?

besides that it looks good … great job, tahnk you

magni- · 2022-07-25T23:52:31Z

@magni- … yeah, definitely will it be 1.5.0 … what is with mustermann, should it be aded to the Gemfile?

I think it makes more sense to update the requirement in mustermann-grape instead? https://github.com/ruby-grape/mustermann-grape/blob/master/mustermann-grape.gemspec#L20

LeFnord · 2022-07-26T07:44:37Z

ok makes sense :)

magni- · 2022-07-27T06:29:59Z

@LeFnord could you release this to RubyGems? Thank you! 🙇🏼

LeFnord · 2022-07-28T20:13:45Z

done

* 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

Fix typo

1ff4463

magni- force-pushed the pp/use-nicknames-for-definitions branch from f51c63b to b417562 Compare July 14, 2022 07:25

magni- added 3 commits July 14, 2022 16:46

Refactor MoveParams.parent_definition_of_params to use OperationId.bu…

ddb9fd6

…ild 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

46510d6

MoveParams.build_definition returns the passed in name, so name and referenced_definition were always the same value.

Fix Rubocop offenses

2055195

magni- force-pushed the pp/use-nicknames-for-definitions branch from b417562 to 2055195 Compare July 14, 2022 07:51

magni- added 2 commits July 20, 2022 13:38

Fix old reference to Travis CI in CONTRIBUTING

6682202

Fix CHANGELOG

7d084a2

magni- force-pushed the pp/use-nicknames-for-definitions branch from 888f46d to c1bf305 Compare July 23, 2022 06:15

Update CHANGELOG and UPGRADING

2ddb537

magni- force-pushed the pp/use-nicknames-for-definitions branch from c1bf305 to 2ddb537 Compare July 25, 2022 05:04

LeFnord merged commit 1a3e3f5 into ruby-grape:master Jul 26, 2022

LeFnord mentioned this pull request Jul 26, 2022

Fix rubocop warnings with auto correct #861

Closed

magni- deleted the pp/use-nicknames-for-definitions branch July 26, 2022 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using nicknames for body definitions #862

Allow using nicknames for body definitions #862

magni- commented Jul 14, 2022 •

edited

Loading

magni- commented Jul 14, 2022 •

edited

Loading

magni- commented Jul 20, 2022

magni- commented Jul 21, 2022

magni- commented Jul 23, 2022

LeFnord commented Jul 24, 2022

magni- commented Jul 25, 2022

magni- commented Jul 25, 2022

LeFnord commented Jul 25, 2022 •

edited

Loading

magni- commented Jul 25, 2022

LeFnord commented Jul 26, 2022

magni- commented Jul 27, 2022

LeFnord commented Jul 28, 2022

Allow using nicknames for body definitions #862

Allow using nicknames for body definitions #862

Conversation

magni- commented Jul 14, 2022 • edited Loading

magni- commented Jul 14, 2022 • edited Loading

magni- commented Jul 20, 2022

magni- commented Jul 21, 2022

magni- commented Jul 23, 2022

LeFnord commented Jul 24, 2022

magni- commented Jul 25, 2022

magni- commented Jul 25, 2022

LeFnord commented Jul 25, 2022 • edited Loading

magni- commented Jul 25, 2022

LeFnord commented Jul 26, 2022

magni- commented Jul 27, 2022

LeFnord commented Jul 28, 2022

magni- commented Jul 14, 2022 •

edited

Loading

magni- commented Jul 14, 2022 •

edited

Loading

LeFnord commented Jul 25, 2022 •

edited

Loading