Fix #8 unhandled exception when parsing malformed body #9

LukasRychtecky · 2018-05-21T18:34:00Z

This is just a preview, I want to sure about the code before writting tests.

LukasRychtecky · 2018-05-21T18:34:30Z

src/duct/handler/static.clj

@@ -49,6 +49,11 @@
 (defmethod ig/init-key ::bad-request [_ response]
  (make-handler (assoc response :status 400)))

+(defmethod ig/init-key ::bad-request-malformed [_ response]


New static handler

LukasRychtecky · 2018-05-21T18:35:39Z

src/duct/middleware/web.clj

@@ -124,4 +124,8 @@
    b))

 (defmethod ig/init-key ::format [_ options]
-  #(mm/wrap-format % (deep-merge mc/default-options options)))
+  (let [malformed-handler (or (:malformed-handler options)


A default handler can be overridden by config:

{:duct.middleware.web/format {:malformed-handler #ig/ref :myapp.malformed-handler}

Rather than use or, supply a default value instead.

OK I'll try it.

LukasRychtecky · 2018-05-21T18:36:24Z

src/duct/module/web.clj

@@ -80,6 +81,8 @@

 (def ^:private api-config
  {:duct.handler.static/bad-request           {:body ^:displace {:error :bad-request}}
+   :duct.handler.static/bad-request-malformed {:body "{\"error\": \"Malformed request body\"}"


Hardcoded JSON, I don't want to add another JSON library as dependency.

Since all the other handlers are data, rather than hardcoded, I don't think we should hardcode the JSON. Can we let Muuntaja handle the formatting of the error message?

Probably yes, Muuntaja uses CHeshire itself.

weavejester · 2018-05-21T19:09:46Z

src/duct/middleware/web.clj

@@ -124,4 +124,8 @@
    b))

 (defmethod ig/init-key ::format [_ options]
-  #(mm/wrap-format % (deep-merge mc/default-options options)))
+  (let [malformed-handler (or (:malformed-handler options)


Rather than use or, supply a default value instead.

weavejester · 2018-05-21T19:10:44Z

src/duct/middleware/web.clj

@@ -124,4 +124,8 @@
    b))

 (defmethod ig/init-key ::format [_ options]
-  #(mm/wrap-format % (deep-merge mc/default-options options)))
+  (let [malformed-handler (or (:malformed-handler options)
+                              (ig/ref :duct.handler.static/bad-request-malformed))]


Supplying a reference here won't work. In the current alpha of Duct there is prep-key for such eventualities, but in the current stable version it needs to be hardcoded or set by a module.

I know, I 've created the PR from 52cd93c

OK I rebased from master. Could you suggest how to solve it without ig/ref?

You should add the ref via the duct.module.web/api module, rather than the options of the ig/init-key method.

LukasRychtecky · 2018-05-25T07:46:18Z

src/duct/module/web.clj

@@ -80,6 +82,8 @@

 (def ^:private api-config
  {:duct.handler.static/bad-request           {:body ^:displace {:error :bad-request}}
+   :duct.handler.static/bad-request-malformed {:body (cheshire/generate-string {:error "Malformed request body"})


Generating JSON via Cheshire

Wouldn't it be better to do this as part of the error handling? Otherwise :duct.handler.static/bad-request-malformed is treated differently from all the other handlers.

LukasRychtecky · 2018-05-25T07:46:46Z

src/duct/middleware/web.clj

-(defmethod ig/init-key ::format [_ options]
-  #(mm/wrap-format % (deep-merge mc/default-options options)))
+(defmethod ig/init-key ::format [_ {:keys [malformed-handler]
+                                    :as options


Is it this OK, or you meant it in a different way?

LukasRychtecky · 2018-06-04T17:30:06Z

src/duct/middleware/web.clj

+                                           (let [handler (or (:malformed-handler options)
+                                                             (:default-malformed-handler options))]
+                                             (-> (handler error content-type request)
+                                                 (update :body cheshire/generate-string)


Processing a response in a handler as proposed.

weavejester · 2018-09-08T11:44:45Z

Thanks for the update! There's still a couple of issues:

We shouldn't specify the default handler function in both the module and the middleware.
The handler function you've added isn't a valid Ring handler function.

Instead, attach the Muuntaja error and content type as namespaced keys onto the request map (e.g. :muuntaja/error and muuntaja/content-type), and remove the default error handler. If the error handler is missing, don't wrap the exception and allow it to be thrown.

LukasRychtecky · 2018-09-10T11:13:05Z

@weavejester thanks for feedback. Sorry for a long running PR, but I've been really busy in last weeks. I expect I will finish it soon.

LukasRychtecky commented May 21, 2018

View reviewed changes

weavejester reviewed May 21, 2018

View reviewed changes

LukasRychtecky force-pushed the 8_Malformed-body-fix branch from cee1a6d to 05025a6 Compare May 25, 2018 07:45

LukasRychtecky commented May 25, 2018

View reviewed changes

LukasRychtecky force-pushed the 8_Malformed-body-fix branch from 05025a6 to 76a7e98 Compare June 4, 2018 17:29

LukasRychtecky commented Jun 4, 2018

View reviewed changes

LukasRychtecky force-pushed the 8_Malformed-body-fix branch from 76a7e98 to a029366 Compare August 30, 2018 15:55

LukasRychtecky changed the title ~~NOT-TO-MERGE: Fix #8 unhandled exception when parsing malformed body~~ Fix #8 unhandled exception when parsing malformed body Aug 30, 2018

Fix duct-framework#8 unhandled exception when parsing malformed body

3a95d77

LukasRychtecky force-pushed the 8_Malformed-body-fix branch from a029366 to 3a95d77 Compare August 30, 2018 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #8 unhandled exception when parsing malformed body #9

Fix #8 unhandled exception when parsing malformed body #9

LukasRychtecky commented May 21, 2018

LukasRychtecky May 21, 2018

LukasRychtecky May 21, 2018

weavejester May 21, 2018

LukasRychtecky May 22, 2018

LukasRychtecky May 21, 2018

weavejester May 21, 2018

LukasRychtecky May 22, 2018

weavejester May 21, 2018

weavejester May 21, 2018

LukasRychtecky May 22, 2018

LukasRychtecky Aug 30, 2018

weavejester Aug 31, 2018

LukasRychtecky May 25, 2018

weavejester May 25, 2018

LukasRychtecky May 25, 2018

LukasRychtecky Jun 4, 2018

weavejester commented Sep 8, 2018

LukasRychtecky commented Sep 10, 2018

Fix #8 unhandled exception when parsing malformed body #9

Are you sure you want to change the base?

Fix #8 unhandled exception when parsing malformed body #9

Conversation

LukasRychtecky commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weavejester commented Sep 8, 2018

LukasRychtecky commented Sep 10, 2018