Properly parse POSTs with application/json content type

[deadtrickster]

Yason selected over CL-JSON because it parses to hash-table by default. Yason added to depends-on list.

[deadtrickster]

Maybe it will be better to rewrite that region to get rid of (setf (slot-value this 'body-parameters) repetition? Just something like this (setf (slot-value this 'body-parameters) (cond .... ))

[fukamachi]

Thanks for the pull request.

Indeed, Clack.Request didn't think about other Content-Types to POST, not only JSON but also XML. But I don't get if this should be implemented in Clack.Request. It can also be done in Web application frameworks or middlewares.

[deadtrickster]

Thanks for reply Personally I think it can be include because of simple reason - it's quite common scenario. For example I'm building ExtJS application and I was very surprised when data for crud operations just disappear. In short, in my opinion it should just work.

[fukamachi]

For example I'm building ExtJS application and I was very surprised when data for crud operations just disappear. In short, in my opinion it should just work.

Hmm.. You're right. It isn't kind. This will be in the next version.

[PuercoPop]

This patch introduced a bug https://github.com/fukamachi/clack/issues/58, Also it works for every method, not just POST, places the hash inside the body parameters and could overwrite a body parameter sent with the body. The has should be placed in the env plist imho.

Plus I second Fukumachi's idea that that functionality should be implemented as a middleware, that is what middleware's are for, to wrap functionality you want to be present in every request.

[alpha123]

I agree with fukamachi and Puerco that this should definitely be a middleware. There's no advantage to putting this directly in Clack.Request, and it is exactly the use case for a middleware.

[fukamachi]

I see. So, what is the right behavior of Clack.Request without such parsing middleware? Before merging this patch, Clack.Request had ignored a content body.

[alpha123]

I think it should continue to ignore a content body. The old behavior was fine. Clack.Request should just hand the content body over unprocessed to middleware. I think a JSON parsing middleware should probably come with the distribution though, since it is very useful.

[fukamachi]

I thought it should be separated from Clack core at first, but I don't think so now.

Clack is a framework of frameworks. If most framework need the same process, Clack should have the process in it. I think this case of POST JSON request is NOT a rare case. So, if Clack.Request doesn't have it, most web framework should implement this process or use the same middleware.

Are there any reason that the parsing process is separated from Clack.Request? Is there a scene that should not use it?

[PuercoPop]

'clackup already sets up some middlewares by default I don't see why it can't setup a json-middleware if that would exist. I don't think me or alpha123 are request functionality to be removed from clack, just reorganised with better separation of concerns.

Btw as it currently stands it doesn't just handle the case of POST JSON but all the other methods as well, including PUT. And it presents the decoded json as a form parameter, which is something exclusive to the POST verb iirc.

Something along the lines of

(defclass <clack-middleware-json-preprocessor> (<middleware>)
  ())

(defmethod call ((this <clack-middleware-json-preprocessor>) env)
  (when (string-equal (getf env :content-type) "application/json")
    (setf (getf env :json) (yason:parse (getf env :raw-body))))
  (call-next this env))

[alpha123]

I thought it should be separated from Clack core at first, but I don't think so now. Clack is a framework of frameworks. If most framework need the same process, Clack should have the process in it.

I agree. However, it is much nicer architecture-wise to use a middleware for it. A lot of frameworks will need to serve static files too, but that's a middleware. JSON parsing should really be the same.

I think this case of POST JSON request is NOT a rare case.

It's definitely a common case, and I think Clack should include a middleware for it.

So, if Clack.Request doesn't have it, most web framework should implement this process or use the same middleware.

If the middleware is in the standard clack distribution I think they'd all just use that.

Are there any reason that the parsing process is separated from Clack.Request?

Cleaner architecture.

Is there a scene that should not use it?

Maybe if someone wanted to use a different JSON parsing library, or was sending JSON directly to a database or something.

'clackup already sets up some middlewares as default I don't see why I can't setup a json-middleware if > that would exist. I don't think me or alpha123 are request functionality to be removed from clack, just

reorganised with better separation of concerns.

Exactly this. I would be fine if it's a default middleware -- I think this is great functionality, but I want Clack to remain modular instead of throwing extra stuff into Clack.Request.

[fukamachi]

I see. Thanks for the sample code and very understandable example cases. I was concerned about what Clack.Request:body-parameters returns. The middleware is only putting the JSON into :json. I guess you meant to say is Clack.Request:body-parameters returns :json value when it is in the env, right?

[fukamachi]

I have in mind, whether it is user-friendly.

When you remove the middleware, Clack.Request:body-parameters returns nil simply. It is hard to investigate why it doesn't work without any errors.

Of course, loosely-coupled is a design goal of Clack, but they should work well even if they are used separately.

[fukamachi]

I understood the benefits of the separation as a middleware, and I'd like to do so now. However, it worries me a little. Do you have any suggestions? (or don't need to think seriously about that?)

[snmsts]

I think it is better to support as middleware. In my experience, I had had an application that build with clack and that uses cl-json. After this yason patch was merged,I had to rewrite some part of the application to run my application with the newest clack because of nickname conflict cl-json and yason. I think same kind of situation (better library comes out or the library we depend on are not in maintenance anymore)could be happen in future. I understand that clack is a foundation of WAF,so It is better and safe to separate any functionality into middleware and WAF maker should have right to select.

[fukamachi]

Hmm. I didn't notice that :(

[fukamachi]

Okay, let's move the code to middleware-level anyway.

@PuercoPop Would you send a pull request? Or will I?

[PuercoPop]

Fukumachi I'll send it first thing in the morning. (GMT -5 here)

[fukamachi]

Thanks a lot. Good night :)