Overriding app.ErrorMiddleware has no effect

[devboy]

I have a buffalo.App for a JSON REST api.
Switching the GO_ENV to production causes all errors to be sent as HTML.
I changed the app.ErrorMiddleware to my custom one yet the defaultErrorMiddleware is still being called while the one I assigned is ignored.

[paganotoni]

@devboy i tried to reproduce this with https://gist.github.com/paganotoni/a6d165e2a12447759cc9a68518b720a3 and it seems that i'm able to override the default error handlers even with GO_ENV=production.

Instead of changing the ErrorMiddleware i'm updating the error handlers, would this strategy work for you?.

[paganotoni]

@devboy can i know the reasons to override the ErrorMiddleware and why using custom error handlers wont work for you ?

[sebd71]

I don't understand the goal of if statement of app.go line 55

buffalo/app.go

Line 55 in 78d649a

if a.ErrorMiddleware != nil {

We just create "a" object (above lines 42-52) without setting ErrorMiddleware.
So if statement is useless to my POV (always false).

As said in blog.gobuffalo.io (here section New Error Handling Middleware), it would be useful to take complete control over all of the error handling. But in fact, it's not that is currently implemented.
Method based on ErrorHandlers may be a good solution to override only 1 or 2 specific error but it would be good to also allow to override error middleware.

If you're agree with that, let me know if you want that I submit a pull request.

[markbates]

@sebd71 @paganotoni can you both review #1563 to fix this problem we have to introduce a breaking change in v0.14.0 to make it work.

[markbates]

Fixed with #1564 (v0.14.0-rc.1 - coming this week)

[sebd71]

Your changes introduce following problems / dangerous behaviors :

1/ you can create deadlock (forever loop)

app.ErrorHandlers.Default(nil)  // Get will return nil producing forever loop

2/ you can reset default handler unintentionally depending on the code order :

app.ErrorHandlers.Default(myDefaultErrorHandler)  // no effect, reset by next lines
app.ErrorHandlers = buffalo.ErrorHandlers{
    404: myErrorHandler,
    500: myErrorHandler,
}
app.ErrorHandlers.Default(myDefaultErrorHandler)  // works fine

So make ErrorHandler private seems to me safest and expose (like you did in middleware) Get / Set functions to manage ErrorHandlers.
Please review the pull request I just push #1578

[markbates]

Thanks for the feedback. I’m not convinced this needs the full rewrite you propose. The first issue can be fixed with a simple nil check. The second one is expected behavior. If I replace the whole ErrorHandlers with a new one, I would expect it to replace any previous changes I made to it.

I need more convincing that the changes you propose are the right ones.

[markbates]

I should also point out that you’re requesting breaking changes. I would recommend you open a proposal ticket and lay out your ideas there so the community can discuss the pros/cons of these changes.