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

fix weird behaviour for optional groups with default params #936

Merged
merged 1 commit into from
Feb 24, 2015
Merged

fix weird behaviour for optional groups with default params #936

merged 1 commit into from
Feb 24, 2015

Conversation

dm1try
Copy link
Member

@dm1try dm1try commented Feb 22, 2015

apply defaults for inner params only if parent param is present

fixes #615 (see for more info)

/cc @dblock take a look

@dblock
Copy link
Member

dblock commented Feb 22, 2015

Looks good, just needs CHANGELOG.

@dm1try dm1try changed the title fix weird behaviour for optional groups fix weird behaviour for optional groups with default params Feb 23, 2015
@dm1try
Copy link
Member Author

dm1try commented Feb 23, 2015

@dblock up

@@ -6,6 +6,7 @@
0.11.0 (2/23/2015)
==================

* [#936](https://github.com/intridea/grape/pull/936): Fixed default params processing for optional groups - [@dm1try](https://github.com/dm1try).
Copy link
Member

Choose a reason for hiding this comment

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

I released 0.11.0 today, so this needs to bump into Next Release, sorry for the hassle.

…faults for inner params only if parent param is present
@u2
Copy link
Contributor

u2 commented Feb 24, 2015

      it 'optional array if empty array params' do
        post '/optional_array', { optional_array: [] }.to_json
        expect(last_response.status).to eq(201)
        expect(last_response.body).to eq({ optional_array: [] }.to_json)
      end

if optional array without default value includes optional param with default value, here shoud return { optional_array: [] }.to_json, right?

     Failure/Error: expect(last_response.body).to eq({ optional_array: [] }.to_json)

       expected: "{\"optional_array\":[]}"
            got: "{\"optional_array\":null}"

       (compared using ==)

@dm1try
Copy link
Member Author

dm1try commented Feb 24, 2015

Yes, it is right but you forgot to provide application/json content type in your spec.
You cannot send an empty array using http query key/value pairs this is why your spec fails.

So

      it 'optional array if empty array params' do
        header 'Content-Type', 'application/json'
        post '/optional_array', { optional_array: [] }.to_json
        expect(last_response.status).to eq(201)
        expect(last_response.body).to eq({ optional_array: [] }.to_json)
      end

should work

@u2
Copy link
Contributor

u2 commented Feb 24, 2015

Thank you.it works.

        subject.params do
          optional :optional_array, type: Array do
            optional :foo_in_optional_array, default: 'bar'
          end
        end
        subject.post '/optional_array_no_declared_params' do
          { no_declared_array: params[:no_declared_array] }
        end

      it 'optional array if params not declared' do
        post '/optional_array_no_declared_params', { no_declared_array: [{id: 2}] }.to_json
        expect(last_response.status).to eq(201)
        expect(last_response.body).to eq({ no_declared_array: [{id: 2}] }.to_json)
      end

And, I can send an array params which not declared in the params block, right?

     Failure/Error: expect(last_response.body).to eq({ no_declared_array: [{id: 2}] }.to_json)

       expected: "{\"no_declared_array\":[{\"id\":2}]}"
            got: "{\"no_declared_array\":null}"

       (compared using ==)

It passed after I add

header 'Content-Type', 'application/json'

it's weird.

@dm1try
Copy link
Member Author

dm1try commented Feb 24, 2015

it's weird.

For me it seems normal. You send the json payload with application/x-www-form-urlencoded content type, it does not make a sense. You should remove .to_json if you use this content type or provide application/json content type as above if you wanna use a json payload.

@u2
Copy link
Contributor

u2 commented Feb 24, 2015

Thank you for your explication. :-)

@dm1try
Copy link
Member Author

dm1try commented Feb 24, 2015

@u2 sure) so #937 can be closed I think.

@u2
Copy link
Contributor

u2 commented Feb 24, 2015

Yes.

dblock added a commit that referenced this pull request Feb 24, 2015
…_groups

fix weird behaviour for optional groups with default params
@dblock dblock merged commit b59999d into ruby-grape:master Feb 24, 2015
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