ring-clojure / ring-json

Ring middleware for handling JSON
313 stars 47 forks source link

Malformed JSON in request body with wrap-json-body results in a 500 status code instead of 400 #7

Closed jeffcharles closed 10 years ago

jeffcharles commented 11 years ago

When posting a request with invalid JSON in the request body, a 500 status code is returned by the web server. I would've expected a 400 status code to be returned because the request could not be understood by the server due to malformed syntax (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.1) rather than a 500 status code which indicates a server-side error.

From looking through ring-json's and Cheshire's source code, I can see that nothing seems to anticipate that malformed JSON could be presented for parsing. Sometimes third party clients have a mistake with the JSON they send to my web service and it would be nice to send them an appropriate response to indicate the error is on their end instead of mine.

Here's the request I sent (missing closing brace in the body is the malformed part):

$ curl -X POST -H "Content-Type: application/json" -d '{"foo": "bar"' -v http://localhost:3000/todos
* About to connect() to localhost port 3000 (#0)
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> POST /todos HTTP/1.1
> User-Agent: curl/7.29.0
> Host: localhost:3000
> Accept: */*
> Content-Type: application/json
> Content-Length: 13
> 
* upload completely sent off: 13 out of 13 bytes
< HTTP/1.1 500 Server Error
< Date: Mon, 06 May 2013 00:59:40 GMT
< Content-Type: text/html;charset=ISO-8859-1
< Content-Length: 5720
< Server: Jetty(7.6.1.v20120215)
< 

Here's the stack trace on the server:

Exception: com.fasterxml.jackson.core.JsonParseException: Unexpected end-of-input: expected close marker for OBJECT (from [Source: java.io.StringReader@1b556b61; line: 1, column: 0])
 at [Source: java.io.StringReader@1b556b61; line: 1, column: 27]
JsonParser.java:1378  com.fasterxml.jackson.core.JsonParser._constructError
ParserMinimalBase.java:599 com.fasterxml.jackson.core.base.ParserMinimalBase._reportError
ParserMinimalBase.java:532 com.fasterxml.jackson.core.base.ParserMinimalBase._reportInvalidEOF
ParserBase.java:479 com.fasterxml.jackson.core.base.ParserBase._handleEOF
ReaderBasedJsonParser.java:1679 com.fasterxml.jackson.core.json.ReaderBasedJsonParser._skipWSOrEnd
ReaderBasedJsonParser.java:561 com.fasterxml.jackson.core.json.ReaderBasedJsonParser.nextToken
parse.clj:42 cheshire.parse/parse*
parse.clj:61 cheshire.parse/parse
core.clj:87 cheshire.core/parse-string
RestFn.java:423 clojure.lang.RestFn.invoke
json.clj:12 ring.middleware.json/read-json
RestFn.java:423 clojure.lang.RestFn.invoke
json.clj:19 ring.middleware.json/wrap-json-body[fn]
json.clj:42 ring.middleware.json/wrap-json-response[fn]
Var.java:415 clojure.lang.Var.invoke
reload.clj:18 ring.middleware.reload/wrap-reload[fn]
stacktrace.clj:15 ring.middleware.stacktrace/wrap-stacktrace-log[fn]
stacktrace.clj:79 ring.middleware.stacktrace/wrap-stacktrace-web[fn]
jetty.clj:18 ring.adapter.jetty/proxy-handler[fn]
(Unknown Source) ring.adapter.jetty.proxy$org.eclipse.jetty.server.handler.AbstractHandler$0.handle
HandlerWrapper.java:111 org.eclipse.jetty.server.handler.HandlerWrapper.handle
Server.java:349 org.eclipse.jetty.server.Server.handle
AbstractHttpConnection.java:452 org.eclipse.jetty.server.AbstractHttpConnection.handleRequest
AbstractHttpConnection.java:894 org.eclipse.jetty.server.AbstractHttpConnection.content
AbstractHttpConnection.java:948 org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.content
HttpParser.java:857 org.eclipse.jetty.http.HttpParser.parseNext
HttpParser.java:235 org.eclipse.jetty.http.HttpParser.parseAvailable
AsyncHttpConnection.java:76 org.eclipse.jetty.server.AsyncHttpConnection.handle
SelectChannelEndPoint.java:609 org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle
SelectChannelEndPoint.java:45 org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run
QueuedThreadPool.java:599 org.eclipse.jetty.util.thread.QueuedThreadPool.runJob
QueuedThreadPool.java:534 org.eclipse.jetty.util.thread.QueuedThreadPool$3.run
Thread.java:680 java.lang.Thread.run

In an ideal world, I'd prefer to be able to specify a function/handler to call when malformed JSON is present in the request body so I can specify what the response should look like. Unfortunately, I'm new to Clojure and Ring so I'm not entirely sure where a logical place to specify that handler would be if that functionality were to be added.

alexkehayias commented 11 years ago

I have this problem as well, looks like issue #11 would allow us to add an on-error handler which would be a good solution.

weavejester commented 11 years ago

To be clear, a 500 error message is only returned if you don't catch the exception. If you catch the exception, you can return whatever error message you like.

I guess it might be useful to have a default option to catch the exceptions and return a 400 message, though.

alexkehayias commented 11 years ago

I think defaulting to a 400 would be best in the case of malformed json only. If we default to 400 for any exception it's misleading. Maybe it's best to just wrap the middleware in a try/catch and handle exceptions yourself as you say in #11.

dyba commented 10 years ago

I'm hoping to revive the discussion with the pull request I submitted. I'm trying to piece together the discussions across #11 and #7. I can't tell what was the resolution. It sounds to me that there is a favorable disposition towards catching malformed JSON and defaulting with a 400 status. Is this correct?

weavejester commented 10 years ago

Fixed in 80b0e76f5d.