Added support to handle malformed JSON

[dyba]

• Updated README to document :handle-malformed option for wrap-json-body
• Fixes #7 and #11

[dyba]

By the way, if it's not evident yet, I have much to learn about Ring and Clojure. I'm open to a critique of the code I submitted.

[weavejester]

Well, I appreciate the PR, however there are a lot of things wrong with your code. The tests pass, but only through coincidence. Your code doesn't actually work in practice.

At the risk of sounding critical, you don't appear to have a good understanding of what the code is doing. I don't want to discourage you from contributing to open source projects, but you can't make changes to a code base without having a sound idea of what everything does.

The main problem with your changes is that you never return the response from the handler. You execute the handler, but without the parsed JSON, and then you discard the response. Instead, you then return the request.

There's also a lot of weirdness around taking the body from the request, putting it back, then taking it out again. The wrap-malformed-json middleware generally isn't needed - that's not the way to handle this problem.

Consider the core of the problem: we want to know whether or not the JSON is malformed, right? Well, what about this naive solution:

(defn malformed-json? [json-string]
  (try
    (json/parse-string json-string) false
    (catch JsonParseException _ true)))

This will tell us whether or not a JSON error has occurred. However, it doesn't give us the message of the exception, and if we want the parsed JSON, we'll have to run parse-string again.

So instead, we could write:

(defn parse-json [json-string]
  (try
    [true (json/parse-string json-string)]
    (catch JsonParseException ex
      [false {:message (.message ex)}])))

This returns one of two outputs:

[true parsed-json]
[false error-details]

So we can write something like:

(let [[valid-json? value] (parse-json body)]
  (if valid-json?
    (handler (assoc request :body value))
    (malformed-json value)))

Does that make sense? I'm going to be making a commit of my own with this solution sometime later.

[dyba]

Wow, it's so simple! Thanks for the detailed explanation! :smiley:

[dyba]

I appreciate the time you took to provide valuable feedback. :+1:

[weavejester]

No problem. I didn't want to discourage you from contributing to open source projects (especially Clojure ones), but as you can see, I had a slightly different solution in mind... :)

[dyba]

@weavejester not at all. I find encouragement that there are people such as yourself who are willing to constructively impart their wisdom to n00bs like me. I'm on a journey of discovery and continual improvement and I'm fortunate to have had the opportunity to walk with you along the way and learn a lot. :D

[weavejester]

Fixed by 80b0e76f5d