fukamachi / ningle

Super micro framework for Common Lisp
http://8arrow.org/ningle/
273 stars 25 forks source link

JSON bodies not properly encapsulated #31

Closed mtstickney closed 3 years ago

mtstickney commented 5 years ago

Apologies in advance if this isn't the right project for this issue; this could be considered an issue with http-body, an issue with myway, or just an issue with the way they're used together here.

The ultimate problem is that http-body is returning a single object from JSON-typed requests, instead of a list of parameters as it does for other request types. This causes various errors in routing (somewhere in myway, I think), some obvious and some subtle. A few examples:

;; Test app
(defvar *app* (make-instance 'ningle:<app>))

(setf (ningle:route *app* "/" :method :post :accept '("application/json"))
      (lambda (params)
        (format *debug-io* "Params:~%~S~%" params)
        '(200 () ("\"Hello, world\""))))

(clack:clackup *app*)

Sending non-compound JSON object as body data:

curl --verbose -X POST -d '1' -H "Content-Type: application/json" -H "Accept: application/json" http://localhost:5000

Result:

The value
  1
is not of type
  LIST
   [Condition of type TYPE-ERROR]

Sending compound data reveals the underlying issue:

curl --verbose -X POST -d '{"beep": "boop"}' -H "Accept: application/json" -H "Content-Type: application/json" http://localhost:5000

Result:

Params:
(("beep" . "boop") (:ACCEPT))

You can see that ningle (or myway) is now altering the input data because its appending onto the parsed object alist itself, rather than onto a list encapsulating that object. Weirder, if you leave the server running and perfom the call several times the input data is apparently re-used in a rather odd way:

Params:
(("beep" . "boop") (:ACCEPT))
Params:
(("beep" . "boop") (:ACCEPT) (:ACCEPT))
Params:
(("beep" . "boop") (:ACCEPT) (:ACCEPT) (:ACCEPT))

Ultimately, I think the proper fix here is for http-body to return the result of parsing a JSON request body in a list, rather than returning the decoded object itself -- you could argue that it should try to parse as many objects out of the request as it can, and return a list of all of them, but that's a whole other thing.

However, that's a breaking API change that affects every dependent of http-body, and would probably break any ningle apps that are currently taking JSON input. That would probably be a lot of trouble for a lot of people; I'm not sure if there's a good fix for this that isn't disruptive, short of creating a separate http-body2 package in http-body with the fix, and using that in a similarly forked package or class in ningle. It wouldn't be pretty, but it might at least let you have a deprecation period.

mandus commented 3 years ago

@mtstickney did you ever solved this? I run into the same problem today (just using ningle) - The :ACCEPT shouldn't show up in the Params in the first place I think, and the fact that an extra Accept is pushed on the list for each call mean that there must be some global state leaking between requests?

mtstickney commented 3 years ago

Sort of: in my case I just removed the :accept option from the ningle:route call, because I didn't need it for anything except a sanity check.

After testing a few things, the original error when supplying a non-object JSON body has been resolved (fixed in lack in https://github.com/fukamachi/lack/commit/2a9fc540cf21f0c64c35a1418bf7de262a776e46); the body will be omitted from the params if it isn't alist-like (so a JSON object or an array of arrays, like [[1, 2]]). This avoids the error, although route requirement params are still appended to that object (this is intentional: ningle assumes an alist-like JSON body is meant to be treated as a set of named request parameters).

After digging around for a bit, I suspect (though I'm not certain without more testing) that the duplicated requirement parameters are caused by the closure that ningle-route uses to compile route requirements. I will do some checking and report back.

mtstickney commented 3 years ago

Yep, that's what it was. I've submitted #33 which ought to fix the duplicated requirement parameters.

Since the mixing of request bodies and requirement parameters is intended behavior (although it makes me kinda nervous), and since the error with non-alist-like JSON bodies has been resolved, I'm going to close this.