Html exceptions only when accepted #592

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

@bestie
Contributor
bestie commented Aug 10, 2013

Rather than be concerned with whether a request is an asynchronous browser
request or not it is better to simply consider the Accept header and only serve
HTML to clients that specifically ask for it.

This way you will not find your pure JSON API application splitting out HTML
error messages to your console when using curl :)

bestie added some commits Aug 10, 2013
@bestie bestie ShowException only serves HTML Accept header contains text/html
Rather than be concerned with whether a request is an asynchronous browser
request or not it is better to simply consider the Accept header and only serve
HTML to clients that specifically ask for it.

This way you will not find your pure JSON API application splitting out HTML
error messages to your console when using curl :)
58490f5
@bestie bestie ShowExceptions minor refactoring
* Load HTML exception template only if needed
* Only #call is public
* Enumerable body concern in one place
6eb5f29
@gioele gioele and 1 other commented on an outdated diff Aug 11, 2013
lib/rack/showexceptions.rb
end
- def prefers_plain_text?(env)
- env["HTTP_X_REQUESTED_WITH"] == "XMLHttpRequest" && (!env["HTTP_ACCEPT"] || !env["HTTP_ACCEPT"].include?("text/html"))
+ private
@gioele
gioele Aug 11, 2013 Contributor

Isn't marking all the following methods as private a too invasive change?

@bestie
bestie Aug 11, 2013 Contributor

I think it would be very odd if anyone was making use of these methods so I thought better to close up the public API to make later changes easier.

I would think it very odd if someone was making use of these methods, though I do appreciate your concern.

@gioele
gioele Aug 12, 2013 Contributor

I see you point and agree with you, but I think that this kind of changes are not OK unless there is a major version bump to notify of possible breakage.

@bestie
Contributor
bestie commented Aug 13, 2013

I've restored the public API to it's previous state, also I've made sure the new method I've added is private.

Hope this is now acceptable. I'm happy to squash commits if that's your preference.

@bestie
Contributor
bestie commented Nov 27, 2013

@spastorino @raggi Any thoughts on this?

I'm having to monkey patch this change in on all my JSON API apps otherwise every time you get an error it's a huge PITA :)

@raggi raggi and 1 other commented on an outdated diff Dec 4, 2013
lib/rack/showexceptions.rb
@@ -17,7 +17,6 @@ class ShowExceptions
def initialize(app)
@app = app
- @template = ERB.new(TEMPLATE)
@raggi
raggi Dec 4, 2013 Member

ERB parsing is not cheap. Why was this moved, it seems orthogonal to the desired changes?

@bestie
bestie Dec 6, 2013 Contributor

Just a refactoring I thought might be beneficial.

Now no ERB object is created unless HTML is about to rendered. This should improve performance if anything.

Happy to revert if this is a deal breaker.

@raggi
raggi Dec 28, 2013 Member

You want to improve boot time performance at the sacrifice of runtime performance? I'd be more worried about the extra garbage, though.

@bestie
bestie Jan 2, 2014 Contributor

Yes that's right, as the template only needs to rendered in the case of an exception isn't it better to take the performance hit only when one is raised rather than on every request?

@raggi
raggi Jan 3, 2014 Member

Initialize is not called every request.

@bestie
bestie Jan 10, 2014 Contributor

Of course you're right. However this change still prevents all template related objects from being created until absolutely necessary.

@raggi
Member
raggi commented Dec 4, 2013

Looks good in principle. Would you mind addressing the comment?

@bestie bestie Undo template refactoring
As this is orthoganol to HTML rendering change.
5b2ee11
@bestie
Contributor
bestie commented Jan 10, 2014

While I stand by my refactorings I agree that the discussed changes to the template rendering are orthogonal to the purpose of this PR.

I have therefore removed it.

@raggi raggi commented on the diff Jan 12, 2014
lib/rack/showexceptions.rb
@@ -85,7 +94,7 @@ def pretty(env, exception)
end
}.compact
- [@template.result(binding)]
+ @template.result(binding)
@raggi
raggi Jan 12, 2014 Member

Why did you remove the array here? Doesn't ERB return strings from result?

Rack SPEC says "Body must respond to #each and yield strings". String does not do this in all supported ruby versions in all locales.

@bestie
bestie Jan 13, 2014 Contributor

I've moved the concern of wrapping the response body string in an array to one place. See line 45 https://github.com/bestie/rack/blob/5b2ee1163aa6f68df5ec712b4d9802d90a7875f4/lib/rack/showexceptions.rb#L45

@raggi raggi commented on the diff Jul 12, 2014
lib/rack/showexceptions.rb
end
- def prefers_plain_text?(env)
- env["HTTP_X_REQUESTED_WITH"] == "XMLHttpRequest" && (!env["HTTP_ACCEPT"] || !env["HTTP_ACCEPT"].include?("text/html"))
+ def accepts_html?(env)
+ env["HTTP_ACCEPT"] && env["HTTP_ACCEPT"].include?("text/html")
@raggi
raggi Jul 12, 2014 Member

We have Rack::Utils.best_q_match and friends now, that are probably better used here.

@raggi raggi commented on the diff Jul 12, 2014
test/spec_showexceptions.rb
@@ -47,7 +47,7 @@ def show_exceptions(app)
res.body.should.include __FILE__
end
- it "responds with HTML on AJAX requests accepting HTML" do
+ it "responds with HTML to requests specifically accepting HTML" do
@raggi
raggi Jul 12, 2014 Member

Is this test description correct? We should give back HTML for /, and if/once we do, then this description is slightly incorrect.

@raggi raggi added this to the Rack 1.5.3 milestone Jul 12, 2014
@raggi raggi added a commit that closed this pull request Jul 13, 2014
@raggi raggi correct typo and refactor tests for coverage
Closes #592
6f1a4a4
@raggi raggi closed this in 6f1a4a4 Jul 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment