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

JSON Test Serialization on DELETE endpoints Regression in 5.1.4 #30570

Closed
brandonhilkert opened this issue Sep 11, 2017 · 9 comments
Closed

JSON Test Serialization on DELETE endpoints Regression in 5.1.4 #30570

brandonhilkert opened this issue Sep 11, 2017 · 9 comments

Comments

@brandonhilkert
Copy link
Contributor

Steps to reproduce

We tried to update to 5.1.4 and 2 tests broke because of the following...

Test app: https://github.com/brandonhilkert/bulk-destroy-test

Delete endpoints with supplied a supplied JSON payload are double serialized when using as: :json in the test.

I added a bulk endpoint here: brandonhilkert/bulk-destroy-test@4cfb003

Expected behavior

Test should pass

Actual behavior

Test:

  test "deletes all posts with ID" do
    assert_difference('Post.count', -1) do
      delete bulk_posts_url,
        params: { ids: [posts(:one).id, posts(:two).id] },
        as:     :json
    end
  end
(byebug) params
<ActionController::Parameters {"{\"ids\":"=>{"980190962,298486374"=>{"}"=>nil}}, "format"=>:json, "controller"=>"posts", "action"=>"bulk", "post"=>{}} permitted: false>

Removing as: :json causes the params to be properly serialized and test passes:

  test "deletes all posts with ID" do
    assert_difference('Post.count', -1) do
      delete bulk_posts_url,
        params: { ids: [posts(:one).id, posts(:two).id] }
    end
  end
(byebug) params
<ActionController::Parameters {"ids"=>["980190962", "298486374"], "format"=>:json, "controller"=>"posts", "action"=>"bulk"} permitted: false>

System configuration

Rails version: 5.1.4

Ruby version: 2.4.1p111

@brandonhilkert brandonhilkert changed the title JSON Test Serialization Regression in 5.1.4 JSON Test Serialization on DELETE endpoints Regression in 5.1.4 Sep 11, 2017
@phedkvist
Copy link
Contributor

I believe you meant to put -2 in assert_difference. If I remove as: :json I get an error using destroy_all, I use posts.each(&:destroy) which passes the test. Regardless I get the same behaviour using as: :json.

@brandonhilkert
Copy link
Contributor Author

destroy vs. destroy_all should make no different. You're right, it should be -2. I've changed it per your suggestions and the test still fails with as: :json and passes when as: :json is removed.

ref: brandonhilkert/bulk-destroy-test@edf9aaa

@morgoth
Copy link
Member

morgoth commented Sep 14, 2017

We have the same problem with 5.1.4.
We're not using as: :json, but set custom request headers like:

{
  "CONTENT_TYPE" => "application/json",
  "HTTP_ACCEPT" => "application/json"
}

@morgoth
Copy link
Member

morgoth commented Sep 18, 2017

@brandonhilkert I found that this is caused by a change in rack-test rack/rack-test#200 which was released in 0.7.0. You can verify it by using Rails 5.1.4 and rack-test 0.6.3

Rails allows 0.7.0 since #29859

@rails-bot
Copy link

rails-bot bot commented Dec 17, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@morgoth
Copy link
Member

morgoth commented Dec 17, 2017

The issue is still present in the rack-test gem, that's still causing a regression in Rails

@rails-bot rails-bot bot removed the stale label Dec 17, 2017
@y-yagi
Copy link
Member

y-yagi commented Mar 18, 2018

Perhaps this issue will be solved with rack/rack-test#223.

@perlun
Copy link

perlun commented Mar 27, 2018

@y-yagi @morgoth I just released rack-test 1.0.0 (yeah, 🎉) - please let me know if the problem persists.

@y-yagi
Copy link
Member

y-yagi commented Mar 27, 2018

Awesome 🎉
I have confirmed that original issue fixed with rack-test 1.0.0.
So I close this. But if anyone still has problem, please reopen it freely. Thanks!

@y-yagi y-yagi closed this as completed Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants