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

Allow requests with extra_params and no parameters #87

Merged

Conversation

claudiob
Copy link
Contributor

@claudiob claudiob commented Sep 2, 2013

Without this commit, the following request:

put '/concerts/1' do
  example_request 'Update an existing concert', concert: {year: 2011} do
     ...
  end
end

would receive no PUT body/params, rather than the specified concert: {year: 2011}.

The reason is that the function params was returning nil if example.metadata[:parameters] was undefined, and the only one to define example.metadata[:parameters] was to have previously called parameter.

In other words, requests were not allowed to have so-called "extra parameters" unless they had at least one "parameter" specified.

This commit removes this restrictions, allowing requests to specify all their parameters "inline".

@oestrich
Copy link
Contributor

oestrich commented Sep 2, 2013

I definitely want to add this, however could you add a small test to capture this?

Without this commit, the following request:

post '/orders' do
  example_request 'should take an optional parameter hash', order_type: 'big' do
     ...
  end
end

would **not receive** the POST body/param `order_type: 'big'` specified.

The reason is that the function `params` was returning nil if
example.metadata[:parameters] was undefined, and the only one to
define example.metadata[:parameters] was to have previously called
"parameter". In other words, requests were not allowed to have so-called
"extra parameters" unless they had at least one "parameter" specified.

This commit removes this restrictions, allowing requests to specify all
their parameters "inline".
@claudiob
Copy link
Contributor Author

claudiob commented Sep 2, 2013

@oestrich I add a test that failed before my patch and passes now… is it okay?

@oestrich
Copy link
Contributor

oestrich commented Sep 2, 2013

Yeah, looks good.

oestrich added a commit that referenced this pull request Sep 2, 2013
…arams

Allow requests with extra_params and no parameters
@oestrich oestrich merged commit eac8414 into zipmark:master Sep 2, 2013
@claudiob claudiob deleted the allow-requests-with-only-extra-params branch September 2, 2013 17:38
@@ -52,8 +52,7 @@ def query_string
end

def params
return unless example.metadata[:parameters]
parameters = example.metadata[:parameters].inject({}) do |hash, param|
parameters = example.metadata.fetch(:parameters, {}).inject({}) do |hash, param|
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to break if you already have set the parameters to a String (for example doing params.to_json) even when not using any extra_params

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you give an example of setting params to a string? I'm not sure what you mean by that. If you need to send a string you should be using raw_post

let(:raw_post) { params.to_json }

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow I feel stupid here (facepalm) -- I was setting let(:raw_data) { params.to_json } instead of let(:raw_post) { ....}. I was getting this error doing the following:

post '/users' do
  let(:raw_data) { params.to_json }

  example 'creating a user' do
    do_request raw_data
  end
end

This causes the error where your passing do_request a string and you get an error saying inject method is not defined (which is accurate).

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