skeeto / elisp-json-rpc

JSON-RPC library for Emacs Lisp
The Unlicense
40 stars 9 forks source link

allow specifying endpoint, named arguments #7

Open whacked opened 8 years ago

whacked commented 8 years ago

From my understanding of json-rpc in json-rpc.el L77-L93, there are 2 limitations that I encountered in my use case.

  1. the request endpoint is hard-coded to the root path / (line 88)
  2. the request parameter list is vectorized by vconcat; I only played a little bit with the request library but it seems like this precludes using named parameters like `((foo . "bar")).

Below is a modified version of json-rpc I am using to work around these 2 issues. The endpoint feature is probably better suited as another attribute into the cl-defstruct. The cons detection section seems inelegant but is the best way for me now.

(setq json-rpc-endpoint "/rpc")
(defun json-rpc2 (connection method &rest mixed-params)
  "Send request of METHOD to CONNECTION, returning result or signalling error."
  (let* ((id (cl-incf (json-rpc-id-counter connection)))
         (params (if (and (= 1 (length mixed-params))
                          (eq 'cons (type-of (first mixed-params))))
                     (first mixed-params)
                   (vconcat mixed-params)))
         (request `(:jsonrpc "2.0" :method ,method :params ,params :id ,id))
         (process (json-rpc-process (json-rpc-ensure connection)))
         (encoded (json-encode request)))
    (with-current-buffer (process-buffer (json-rpc-process connection))
      (erase-buffer))
    (with-temp-buffer
      (insert (concat "POST " json-rpc-endpoint " HTTP/1.1\r\n"))
      (insert (format "Content-Length: %d\r\n\r\n" (string-bytes encoded))
              encoded)
      (process-send-region process (point-min) (point-max)))
    (json-rpc-wait connection)))

Here's an illustration of the use case

server

from flask import Flask
from flask_jsonrpc import JSONRPC

app = Flask(__name__)
jsonrpc = JSONRPC(app, "/rpc")

@jsonrpc.method("App.example")
def example(name = "Bob", car = "Honda"):
    return "Hi %s, you drive a %s" % (name, car)

app.run()

emacs (contd from above)

(setf jrpc-handle (json-rpc-connect "localhost" 5000))
(json-rpc2 jrpc-handle "App.example") ;; => Hi Bob, you drive a Honda
(json-rpc2 jrpc-handle "App.example" "Nikola") ;; => Hi Nikola, you drive a Honda
(json-rpc2 jrpc-handle "App.example" "Nikola" "Tesla") ;; => Hi Nikola, you drive a Tesla
(json-rpc2 jrpc-handle "App.example" '((car . "Tesla"))) ;; => Hi Bob, you drive a Tesla
skeeto commented 8 years ago

You're absolutely correct about #1. It hadn't even occurred to me that you might want to select a different endpoint. The options I see are:

  1. Select the endpoint when establishing the connection. This means you would need a new connection object for each endpoint. That's undesirable.
  2. Add an endpoint argument to json-rpc. However, that would change the API and break existing users, which is unacceptable. Since it uses a &rest argument, there's not even the possibility to slip in a &key argument here.
  3. Add a dynamic variable argument, which is basically what you did in your fix. json-rpc-endpoint would be bound to "/" by default, and callers would let bind around the call to change it. This extends the API nicely without breaking anything. In addition to your change, I would also automatically URL-encode the path so that the caller wouldn't have that responsibility.
  4. Create a new function that behaves just like the existing json-rpc but takes an endpoint argument. json-rpc would then delegate to this new function with "/" as the endpoint. Having this extra function is a little less elegant, and there's the problem of choosing a name for it. However, if given a good name, I'd prefer it over #3.

As for #2, this was intentional as it's required by the JSON-RPC 1.0 spec. The params must be an array of objects.

http://json-rpc.org/wiki/specification

However, I see that JSON-RPC 2.0 also allows it to be an object, to supply named rather than positional parameters. Unfortunately there's no way to add this functionality to the json-rpc function without breaking it. Cons detection won't work because the original caller may intend to pass an object as the first and only argument, not as the params itself.

So to shoot two birds with one stone, I think we need a whole new function adding support for both. It would look like this:

(defun json-rpc-2.0 (connection endpoint method params)
   ...)

Where params must be a vector or non-nil list (and the details of encoding this value are left to json-encode). It would also add a jsonrpc 2.0 member to the request. For consistency I'd also create a json-rpc-1.0 that is just like old json-rpc, but has an endpoint argument.

Sound reasonable?

whacked commented 8 years ago

The (defun json-rpc-2.0 (connection endpoint method params) ...) is reasonable but it entails including the endpoint in every call to json-rpc-2.0, correct? That seems a bit cumbersome especially since you've encapsulated the connection object in the first place. I was hoping there was an analogue of subclassing the cl-defstruct of json-rpc.

The workaroundhack I posted is just a path of least resistance that preserves a similar call signature to the current version, and also some other client libraries such as jquery-jsonrpc. Incidentally, jquery-jsonrpc has a namespace setting, which would be App in the toy example above. I didn't think that warranted an additional abstraction, but conceptually that seems to also fit in the cl-defstruct. From what it seems, cl-defstruct is not "subclass-able" though?

So it would seem like the hard-coded root path is the main issue. To retain flexibility, what about splitting the json-rpc logic at the request building point like at with-current-buffer?