Navigation Menu

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

Jump to bottom

JSON served as HTML content_type since v2.1 #1647

Closed
mig-hub opened this issue Oct 2, 2020 · 14 comments · Fixed by #1649
Closed

JSON served as HTML content_type since v2.1 #1647

mig-hub opened this issue Oct 2, 2020 · 14 comments · Fixed by #1649

Comments

@mig-hub
Copy link
Contributor

mig-hub commented Oct 2, 2020

Hello,

Since I switched to version 2.1 then my JSON content type is served as text/html.
Could it be related to this old issue.
I am definitely in "development" RACK_ENV.
Didn't try production yet.
@jkowens
Copy link
Member

jkowens commented Oct 2, 2020

How are you setting the content type to application/json?

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 2, 2020

@jkowens with

content_type :json

But I realized it works for a direct request.
It is only when requested with jQuery.
I am looking into it but it is possible that I don't request JSON explicitly.

But it clearly changed since I've updated to Sinatra 2.1 .
Strange I will check this and close the issue if it is on jQuery side.

@jkowens
Copy link
Member

jkowens commented Oct 2, 2020

Yes, there was a change to add a default_content_type option. I thought it would respect content type if set, so if there is an issue please let us know so we can further investigate.

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 2, 2020 •
edited

Yeah so basically my code is using jQuery.get.
So an ajax call.
It does not request JSON explicitly because it accepts either HTML or JSON and does something different depending on what is loaded. The way I was doing it is to check if the data returned is an object or a string.

So far it was working fine with the content type set to :json but is not working anymore with version 2.1 .
Not sure it can help, but here are the request headers sent by jQuery:

Accept: */*
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,fr;q=0.8
Cache-Control: no-cache
Connection: keep-alive
Host: localhost:8080
Pragma: no-cache
Referer: http://localhost:8080/admin
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36
X-Requested-With: XMLHttpRequest

And here are the response headers:

Connection: Keep-Alive
Content-Encoding: gzip
Content-Length: 144
Content-Type: text/html;charset=utf-8
Date: Fri, 02 Oct 2020 14:05:27 GMT
Server: WEBrick/1.4.2 (Ruby/2.6.5/2019-10-01)
Vary: Accept-Encoding
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 1; mode=block

If I type the URI in the address bar instead, then Sinatra sets it to "application/json" or the vnd version in Firefox.

Not sure it has any influence but the URI does not have ".json" at the end.
There is no extension at all, just the content type.

In the meantime as a quick fix I check if the returned data is a string and if the first characters is "{".
And if it is the case I JSON.parse the string instead.
Not great but it works.

@jkowens
Copy link
Member

jkowens commented Oct 2, 2020

Do you set content_type :json in the route or a before filter?

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 2, 2020

In a before filter.

@jkowens
Copy link
Member

jkowens commented Oct 2, 2020

I'm trying to reproduce the issue, but it seems to be working as expected for me. Here is my test app:

app.rb

require 'sinatra'

before '/api/*' do
  content_type :json
end

get '/' do
  erb :index
end

get '/api/test' do
  {foo: :bar}.to_json
end

views/index.erb

<html>
  <head>
    <script src="https://code.jquery.com/jquery-3.5.1.min.js"></script>
    <script type="text/javascript">
      $(function() {
        $.get("/api/test", function(data) {
          console.log(data)
        });
      });
    </script>
  </head>
</html>

Response Headers:

HTTP/1.1 200 OK
Content-Type: application/json
Content-Length: 13
X-Content-Type-Options: nosniff
Connection: keep-alive
Server: thin

@jkowens
Copy link
Member

jkowens commented Oct 2, 2020

@mig-hub if you can provide a small sample app that demonstrates this issue, that would be super helpful 🙏

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 3, 2020

Sorry I did not reply immediately, different timezone.
I am creating a repo that demonstrates it.
Difference here I assume is that I am using a fairly recent version of jQuery 1, and yours has version 3.
As far as I know jQuery 1 is the version still supporting some older browsers.

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 3, 2020

Small update:
I cannot reproduce it so far in isolation.
It seems to be a weird edge case.
But I found out these facts:

  • jQuery version is not in cause.
  • It is actually not a XHR thing because calling this special handler from the address bar DOES produce the same. I had the wrong impression before because I thought all JSON requests were affected.
  • I confirm It was working fine with Sinatra 2.0.8.1
  • It works fine if I put content_type :json inside the handler. I tried this because I thought maybe another middleware is changing the content type later.

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 3, 2020

Here you go I managed to find what makes the difference.
It seems to be when the Sinatra base app is subclassed.
Here is a small example which demonstrates the issue.

When you visit "/" which is inherited, or "/other" which is not, then for both the content type ends up being HTML.
I don't know if it is a bug but for sure it did not do it on 2.0.
Also I've added a debug puts to show that the inherited before block is executed.
For some reasons the content type is changed again back to HTML between the before block and the handler itself.

@jkowens
Copy link
Member

jkowens commented Oct 3, 2020

Excellent detective work, thank you!

@jkowens jkowens mentioned this issue Oct 3, 2020
Merged
@jkowens
Copy link
Member

jkowens commented Oct 3, 2020

I think I have identified bug introduced in 2.1.0 and I've created PR #1649 to fix. Thanks again 👍

@mig-hub
Copy link
Contributor Author

mig-hub commented Oct 3, 2020

@jkowens ah nice! I’m glad I could help. I wish it took me less time to find what was special but it is not a trivial piece of code so I had to isolate many things to find the culprit.

@jkowens jkowens mentioned this issue Oct 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Respect content type set in superclass before filter jkowens/sinatra
2 participants
@mig-hub @jkowens