Skip to content
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

Fixed move_params_to_new to allow name to be defined as a Symbol #476

Merged
merged 3 commits into from
Jul 15, 2016

Conversation

anakinj
Copy link
Contributor

@anakinj anakinj commented Jul 15, 2016

I am trying to use a Entity to describe what is expected inside the request body, while doing this I noticed that when parameters have been defined as body params inside the desc block, grape-swagger causes the app to crash.

@LeFnord
Copy link
Member

LeFnord commented Jul 15, 2016

@anakinj … thanks for it, please add a changeling entry

one question, could you access the parameter in the post block?
see this #400 issue

@anakinj
Copy link
Contributor Author

anakinj commented Jul 15, 2016

Noticed a few other problem cases, fixed in the latest push. Changelog also updated.

required: true
}
params body_param: { type: 'String', desc: 'param', documentation: { in: 'body' } },
'body_string_param' => { type: String, desc: 'string_param', documentation: { in: 'body' } }
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this should not be possible, it should be used Ruby >2.x hash style

Copy link
Contributor Author

@anakinj anakinj Jul 15, 2016

Choose a reason for hiding this comment

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

Yeah, and that is not actually not relevant, the problem was that the type was defined as a constant. Will change the test.

@anakinj
Copy link
Contributor Author

anakinj commented Jul 15, 2016

about "one question, could you access the parameter in the post block?" not really sure what you mean but this is what i figured out:
The parameter can be accessed via the params hash, but the declared method will not include it for some reason.

@LeFnord
Copy link
Member

LeFnord commented Jul 15, 2016

what I mean is, can the parameter be accessed in the grape post block, here:

post do
  # can it be accessed?
end

@anakinj
Copy link
Contributor Author

anakinj commented Jul 15, 2016

This definition:

class Endpoint < Grape::API
  format :json
  resource :endpoint do
    desc 'The endpoint' do
      params body_param: { type: 'String', desc: 'param', documentation: { in: 'body' } },
             body_type_as_const_param: { type: String, desc: 'string_param', documentation: { in: 'body' } }
    end
    post do
      { 'params' => params,
        'declared_params' => declared(params) }
    end
  end
  add_swagger_documentation
end

passes the following tests:

context 'param tests' do
  specify do
    post '/endpoint', '{"body_param": "XYZ"}', 'CONTENT_TYPE' => 'application/json'
    expect(last_response.body).to eql('{"params":{"body_param":"XYZ"},"declared_params":{}}')
  end
  specify do
    post '/endpoint', body_param: "XYZ"
    expect(last_response.body).to eql('{"params":{"body_param":"XYZ"},"declared_params":{}}')
  end
end

@LeFnord
Copy link
Member

LeFnord commented Jul 15, 2016

cool … it seems, one could add parameters via params key, but they would not be added to the declared params array, thanks for clarification

@LeFnord
Copy link
Member

LeFnord commented Jul 15, 2016

👍

@LeFnord LeFnord merged commit 9dd244b into ruby-grape:master Jul 15, 2016
@anakinj
Copy link
Contributor Author

anakinj commented Jul 15, 2016

What I am trying to get working is to have the Entity describe the content of the body, and so far with no luck. So with the fixes in this PR and the following definition I almost get what i want:

class Endpoint < Grape::API
  def self.as_body_param(entity_model)
    documentation = {}
    entity_model.documentation.each do |param_name, param_definition|
      documentation[param_name] = param_definition.merge(documentation: { in: 'body'})
    end
    documentation
  end
  resource :endpoint do
    desc 'The endpoint' do
      params TheBodyApi::Endpoint.as_body_param(::Entities::Something)
    end
    post do
      { 'declared_params' => declared(params) }
    end
  end
  add_swagger_documentation
end

It would be nice to be able to somehow tell the endpoint that "The body content should be of type X"

@LeFnord
Copy link
Member

LeFnord commented Jul 15, 2016

wouldn't it be easier to add in: body to the documentation hash of the exposing of the entity self?
it is enough, to do it on one exposing, see: https://github.com/ruby-grape/grape-swagger/blob/master/spec/lib/move_params_spec.rb#L258

@anakinj
Copy link
Contributor Author

anakinj commented Jul 15, 2016

You are right. Thinking to complicated here, but as long as I'm using the current version I have to manipulate the hash a little to workaround the bugs :)

The cleanest solution from swagger point of view would be just to reference a existing model for the body parameter. Now a definition gets generated for every endpoint and model that is defined.

@LeFnord
Copy link
Member

LeFnord commented Jul 15, 2016

yeap, fixing the behavior of the params key is on the road map

LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
…y-grape#476)

* Fixed move_params_to_new to allow name to be defined as a Symbol

* Fixes for parameter type handling

* All param names as symbols
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants