Fix #8 unhandled exception when parsing malformed body by LukasRychtecky · Pull Request #9 · duct-framework/module.web

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

 (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


 (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


 (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


 (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

 (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


 (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

Conversation

LukasRychtecky commented May 21, 2018

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!

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!

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!

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!

weavejester commented Sep 8, 2018

Uh oh!

LukasRychtecky commented Sep 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants