Extracted HttpErrorHandler interface from Global #3444

jroper · 2014-09-25T06:08:10Z

Play will by default try and load one of these from the root package called ErrorHandler
If none is found, it will fallback to one that delegates to Global, so that Global.onError etc methods still work for legacy apps
Added a new mechanism to the Reflect utils to load bindings for either a Scala interface, or a Java interface with an adapter to the Scala interface
Updated documentation, including removing documentation on old Global methods

julienrf · 2014-09-25T08:07:26Z

...src/play-netty-server/src/main/scala/play/core/server/netty/PlayDefaultUpstreamHandler.scala

Why aren’t we sure here that there is an Application?

It may have failed to compile or load (in dev mode). At some point I'd like to change things so that if it does fail to compile or load, an error application is returned that just responds to every request by rendering the error, that will make so much of the logic here so much simpler. But I think that's a separate change.

Yes, I was thinking of doing something like this when I was writing the Akka HTTP backend. Or at least a MaybeLoadedApplication which wraps either a loaded application or a not-loaded application and which the Netty and Akka HTTP servers can send requests too.

julienrf · 2014-09-25T08:12:18Z

I don’t think this is a good idea to reflectively load the HttpErrorHandler. We should reduce reflection usage to the minimum: ApplicationLoader.

julienrf · 2014-09-25T08:14:30Z

framework/src/play/src/test/scala/play/core/test/Fakes.scala

We should not have this file.

Why not? Unfortunately, we need to instantiate RequestHeaders in the tests, and there's no other way to do it. Now that the app doesn't depend so much on global state, we can start moving tests out of the integration test project, but some things are needed to do that, such as a way to instantiate a RequestHeader.

I don’t see the point of having two separate types, Request and FakeRequest. Why not just have Request? Just put the required methods in this type.

Request is also just a trait that doesn't have any public concrete implementations.

While I completely agree with you, providing either Request or RequestHeader case classes are separate changes in their own right that will have a fundamental impact on Play's public API. They need to be carefully thought through, in particular, we'd like to preparse many headers, including the session, and make them strongly typed, but we don't want two sources of truth (both the headers map and the strongly typed parsed values).

In contrast, this FakeRequest is just something introduced for Play's tests, it is not public API, and so can be done with little thought. I hope some day we can get rid of it, but that day isn't today.

jroper · 2014-09-25T22:35:30Z

If you're using runtime dependency injection, you're using reflection anyway everywhere to instantiate everything. If you're using compile time dependency injection, then reflection is just to load the ApplicationLoader.

richdougherty · 2014-09-26T05:57:33Z

documentation/manual/working/javaGuide/main/http/JavaErrorHandling.md

Typo: "malformated" -> "malformed" or "badly formatted"

richdougherty · 2014-09-29T01:40:42Z

LGTM

James and I had a bit of a talk offline about this PR. I'll just mention some of the things I raised with James. I didn't have any solutions to these issues, these were just comments.

Passing around Mode.Mode is a pet peeve of mine. It would be nice to get rid of the separate methods for dev and prod mode and instead bind different error handlers for dev and prod mode. However I don't think we can easily do this at the moment, because by binding different dev and prod mode error handlers we would make the default error handling logic into something that can only be known at runtime. So if a user wants access to the default error handler, they can't access the logic by overriding DefaultErrorHandler. They would need to get the error handler bound at runtime. Unfortunately if they bind their own error handler then they would override any default error handler.
It's kind of annoying to have to thread the error handler all over the place. There are some places that it isn't passed yet (body parsers). Some random ideas to make it easier to do error handling: pass error handlers in the request context when handling requests, have handlers return a different kind of Result or throw an exception to express that they want to display an error using the default error handling logic. These are my own ideas, but I don't like them very much. :) There are other objects (e.g. ExecutionContexts) that we'll need to thread everywhere too, so maybe in the future we'll find a solution to make passing these objects a bit easier, e.g. we might group some common objects together into a single object, so we only have to thread a single object around the place. We just need to be careful we don't accidentally recreate a "God" object like Application or Global!

* Play will by default try and load one of these from the root package called ErrorHandler * If none is found, it will fallback to one that delegates to Global, so that Global.onError etc methods still work for legacy apps * Added a new mechanism to the Reflect utils to load bindings for either a Scala interface, or a Java interface with an adapter to the Scala interface * Updated documentation, including removing documentation on old Global methods

jroper · 2014-09-29T04:53:16Z

@play-pull-request-validator again please

Extracted HttpErrorHandler interface from Global

julienrf reviewed Sep 25, 2014
View reviewed changes

richdougherty reviewed Sep 26, 2014
View reviewed changes

jroper force-pushed the http-error-handler branch from 282b6bb to 6d93117 Compare September 29, 2014 03:05

richdougherty added a commit that referenced this pull request Sep 29, 2014

Merge pull request #3444 from jroper/http-error-handler

11161bf

Extracted HttpErrorHandler interface from Global

richdougherty merged commit 11161bf into playframework:master Sep 29, 2014

dwhjames mentioned this pull request Oct 8, 2014

Use GlobalSettings by default for AssetsBuilder (fixes #2919) #3377

Closed

jroper mentioned this pull request Feb 5, 2015

Read play.editor from config #3892

Closed

jroper deleted the http-error-handler branch March 5, 2015 00:22

gmethvin mentioned this pull request May 1, 2016

Split onNotFound in onDevNotFound and onProdNotFound #6116

Closed

Uh oh!

Extracted HttpErrorHandler interface from Global #3444

Extracted HttpErrorHandler interface from Global #3444

Uh oh!

Conversation

jroper commented Sep 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrf commented Sep 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jroper commented Sep 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richdougherty commented Sep 29, 2014

Uh oh!

jroper commented Sep 29, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants