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

Do not attempt to convert ActionDispatch::Http::Headers to JSON #138

Merged
merged 2 commits into from
Dec 5, 2017

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Dec 4, 2017

Since Rails 5.0, the headers object contains the request object.
rails/rails@34fa665

These include objects for which to_json conversion is not guaranteed.
Until Rails 5.1, these objects contained an IO object, which caused an error in converting to JSON.

However, IO#to_json was added in Rails 5.2(rails/rails@813f8e3).
As a result, there was no explicit error due to conversion, and SystemStackError got to happen.

To avoid this, fixed to convert only the actual http header to JSON.

Fixes #137

Since Rails 5.0, the headers object contains the request object.
rails/rails@34fa665

These include objects for which `to_json` conversion is not guaranteed.
Until Rails 5.1, these objects contained an IO object, which caused an
error in converting to JSON.

However, `IO#to_json` was added in Rails 5.2(rails/rails@813f8e3).
As a result, there was no explicit error due to conversion, and
`SystemStackError` got to happen.

To avoid this, fixed to convert only the actual http header to JSON.

Fixes dejan#137
@modosc
Copy link
Collaborator

modosc commented Dec 4, 2017

hey @y-yagi , thanks for this pr.

i just pushed a branch with the basic tests we use on the other rails versions - currently everything passes. could you add a test that shows this failure case please?

@y-yagi
Copy link
Contributor Author

y-yagi commented Dec 5, 2017

Hi @modosc. Thank you for the push the branch.

You seem to have used Rails 5.1.4 in test for 5.2.0.beta2.
https://github.com/dejan/rails_panel/blob/rails-5-2-support/meta_request/test/functional/rails-5.2.0.beta2/Gemfile#L10

If you change the version, test should fail as shown below.

Run Test
$ cd test/functional/rails-5.2.0.beta2 && bundle install && bundle exec rake test 

# Running:

E

Error:
MetaRequestTest#test_it_shold_have_a_request_id_header:
SystemStackError: stack level too deep
    test/integration/meta_request_test.rb:11:in `block in <class:MetaRequestTest>'


bin/rails test test/integration/meta_request_test.rb:15


E

Error:
MetaRequestTest#test_it_should_have_a_meta_request_version_header:
SystemStackError: stack level too deep
    test/integration/meta_request_test.rb:11:in `block in <class:MetaRequestTest>'


bin/rails test test/integration/meta_request_test.rb:19

E

Error:
MetaRequestTest#test_should_serve_a_meta_request:
SystemStackError: stack level too deep
    test/integration/meta_request_test.rb:11:in `block in <class:MetaRequestTest>'


bin/rails test test/integration/meta_request_test.rb:27

E

Error:
MetaRequestTest#test_should_create_a_request_file:
SystemStackError: stack level too deep
    test/integration/meta_request_test.rb:11:in `block in <class:MetaRequestTest>'


bin/rails test test/integration/meta_request_test.rb:23



Finished in 0.857378s, 4.6654 runs/s, 0.0000 assertions/s.
4 runs, 0 assertions, 0 failures, 4 errors, 0 skips

For the confirmation, added a test of 5.2 to this branch.

@modosc
Copy link
Collaborator

modosc commented Dec 5, 2017

You seem to have used Rails 5.1.4 in test for 5.2.0.beta2.

hah, that explains a lot.

@modosc modosc merged commit 88463c9 into dejan:master Dec 5, 2017
@y-yagi y-yagi deleted the fix_137 branch December 5, 2017 23:47
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.

Doesnt work with rails 5.2.0.beta2
2 participants