duct-framework / module.web

Duct module for running web applications
21 stars 20 forks source link

No implementation of method: :write-body-to-stream for internal server error #13

Closed LukasRychtecky closed 6 years ago

LukasRychtecky commented 6 years ago

Hi,

when an exception is raised (in a Ring handler) it's caught by wrap-hide-errors middleware. Unfortunately request in wrap-hide-errors has no :muuntaja/response nor :muuntaja/request attributes and a static response :duct.handler.static/internal-server-error is not marshalled to a string, thus Ring raises an exception No implementation of method: :write-body-to-stream of protocol: #'ring.core.protocols/StreamableResponseBody found for class: clojure.lang.PersistentArrayMap.

I've tried on 0.6.4 version (because of my existing services and it's still default in lein template).

I have got a couple of solutions on my mind:

  1. Make a static response marshalled: {:status 500, :body "{\"error\":\"internal-server-error\"}"}
  2. Encode a response by a hand like this:
    (try
    (handler request)
    (catch Throwable _
    (let [muuntaja-config (mc/create)
           req (mc/format-request muuntaja-config request)]
      (mc/format-response muuntaja-config req (internal-error (error-handler req))))))

What do you think?

weavejester commented 6 years ago

Thanks for the bug report! My thought is that we should have two static error handlers - one which would be a last resort and show plaintext, JSON, or some other fallback. The second "normal" error handler we put before Muuntaja's formatting.

In this way, we display a correctly formatted error message when something goes wrong with the application itself, and we have a fallback error message in case something goes wrong with Muuntaja's formatting.

LukasRychtecky commented 6 years ago

Thanks for quick response.

If I understand it correctly I should add new middleware that will be called at the end of a chain of middlewares, checks for response's body type? It'd be helpful if you can describe it more deeply and I will try to fix it.

weavejester commented 6 years ago

So my thought is something like:

:duct.handler.static/internal-server-error
{:body {:error :internal-server-error}}

:duct.handler.static/uncaught-server-error
{:body "Uncaught error!"}

:duct.core.handler
{:middleware [#ig/ref :duct.middleware.web/hide-internal-errors
              #ig/ref :duct.middleware.web/format
              #ig/ref :duct.middleware.web/hide-uncaught-errors]}

However, I'll need to think of the best way to implement it in practice. I am currently working on Duct, so I should get to fixing this error fairly soon.

LukasRychtecky commented 6 years ago

Fine, so will you take care of this or should I? I temporary fixed the issue in my services by defining

:duct.handler.static/internal-server-error
{:body "{\"error\":\"internal-server-error\"}"}
weavejester commented 6 years ago

I can take care of it, but it might take a week or so for me to get around to it.

weavejester commented 6 years ago

I've looked into this, and after some experimentation, I think I'm going to go with a default JSON response for now, as that's by far the simplest option. It would be nice if the 500 error had content type handling, but realistically any client is going to see the 500 status code and just report an error.