mikemintz/rethinkdb-websocket-server

Native reql syntax in query whitelist

mikemintz opened this issue · 3 comments

It might be nice to express queries in the whitelist using the natural JS reql syntax, rather than the protocol AST syntax we currently use. Instead of this:

RQ(
  RQ.INSERT(
    RQ.TABLE("messages"),
    {body: RQ.ref("body"), userId: RQ.ref("userId"), createdAt: RQ.NOW()}
  )
).opt("db", RQ.DB(cfg.dbName))
.validate(...)

we'd allow writing this:

r.db(cfg.dbName)
 .table('messages')
 .insert({body: RP.ref("body"), userId: RP.ref("userId"), createdAt: r.now()})
 .validate(...)

The biggest challenge is being able to log rejected queries in this format, so the application developer can copy/paste into their whitelist. The reason we have the current whitelist syntax is because it's derived directly from the AST sent over the wire protocol.

My hypothesis is that we can go from AST to JS reql syntax with the following algorithm:

(1) Start at the inner-most 0th arg of the nested arrays.

let x = query;
while (Array.isArray(x.args[0])) {
  x = x.args[0];
}

(2) Work our way from the inside to the outside. Each time we go out one level, append the term name of the outer term to the current query, and use arguments after the 0th as its arguments, and options as an object for the last argument. For each argument, recursively apply this algorithm, making special cases for terms like FUNC and MAKE_ARRAY.

x = x.parentTerm;
result += '.' + termNames[x.termId] + '(';
result += x.arguments.slice(1).map(formatTerm).join(', ');
if (x.options) result += ', ' + formatOptions(x.options);
result += ')';

I hadn't realized the javascript driver provides a .toString() method on reql queries. So if we can convert from a json serialized query into a TermBase (not necessarily trivial), this might be straightforward.

It looks like .toString() isn't robust yet in rethinkdb/rethinkdb#2372 but it's probably better to fix that than try to reinvent it here.

I finally got around to implementing this in #6

If anyone has any feedback on the new syntax or implementation details, it would be good to hear before I merge this into master and cut a release.

I'm considering the new syntax to be "experimental" at least until a new RethinkDB JavaScript driver is released with the .toString() fix. If there aren't any serious issues, I'd like to freeze its syntax and ultimately deprecate the older RQ syntax once people have had enough time to migrate.

opsb commented

That's a great improvement to the ergonomics there @mikemintz, nice!