nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

Webdis do not supports redis replies with the nested arrays. #176

Closed majklik closed 4 years ago

majklik commented 4 years ago

Webdis actually do not supports replies which uses nested arrays. The array element is converted into NULL. I wrote small path which solve this for JSON output. It tioo handle specific output of the XRANGE/XREAD commands like older implementation for HGETALL:

--- src/formats/json.c.orig 2020-06-15 20:54:06.000000000 +0100
+++ src/formats/json.c  2020-06-16 09:00:45.000000000 +0100
@@ -89,9 +89,12 @@
 }

 static json_t *
-json_hgetall_reply(const redisReply *r) {
+json_exand_array(const redisReply *r);
+
+static json_t *
+json_keyvalue_reply(const redisReply *r) {
    /* zip keys and values together in a json object */
-   json_t *jroot;
+   json_t *jroot, *jlist;
    unsigned int i;

    if(r->elements % 2 != 0) {
@@ -103,21 +106,113 @@
        redisReply *k = r->element[i], *v = r->element[i+1];

        /* keys and values need to be strings */
-       if(k->type != REDIS_REPLY_STRING || v->type != REDIS_REPLY_STRING) {
+       if(k->type != REDIS_REPLY_STRING) {
            json_decref(jroot);
            return NULL;
        }
-       json_object_set_new(jroot, k->str, json_string(v->str));
+       switch (v->type) {
+       case REDIS_REPLY_STRING:
+           json_object_set_new(jroot, k->str, json_string(v->str));
+           break;
+       case REDIS_REPLY_INTEGER:
+           json_object_set_new(jroot, k->str, json_integer(v->integer));
+           break;
+       case REDIS_REPLY_ARRAY:
+           if (!(jlist=json_exand_array(v)))
+               jlist = json_null();
+
+           json_object_set_new(jroot, k->str, jlist);
+           break;
+       default:
+           json_decref(jroot);
+           return NULL;
+       } 
+       
    }
    return jroot;
 }

 static json_t *
-json_wrap_redis_reply(const struct cmd *cmd, const redisReply *r) {
+json_exand_array(const redisReply *r) {
+
+   unsigned int i;
+   json_t *jlist, *sublist;
+   redisReply *e;
+
+   jlist = json_array();
+   for(i = 0; i < r->elements; ++i) {
+       e = r->element[i];
+       switch(e->type) {
+       case REDIS_REPLY_STRING:
+           json_array_append_new(jlist, json_string(e->str));
+           break;
+       case REDIS_REPLY_INTEGER:
+           json_array_append_new(jlist, json_integer(e->integer));
+           break;
+       case REDIS_REPLY_ARRAY:
+           if (!(sublist=json_exand_array(e)))
+               sublist = json_null();
+           json_array_append_new(jlist, sublist);
+           break;
+       default:
+           json_array_append_new(jlist, json_null());
+           break;
+       }
+   }
+   return jlist;
+}
+
+static json_t *
+json_singlestream_list(const redisReply *r) {
+
+   unsigned int i;
+   json_t *jlist, *jmsg, *jsubmsg;
+   redisReply *id, *msg;
+   const redisReply *e;
+
+   jlist = json_array();
+   for(i = 0; i < r->elements; i++) {
+       e=r->element[i];
+       if (e->type!=REDIS_REPLY_ARRAY || e->elements<2) continue;
+       id = e->element[0]; msg = e->element[1];
+       if (id->type!=REDIS_REPLY_STRING || id->len<1) continue;
+       if (msg->type!=REDIS_REPLY_ARRAY || msg->elements<2) continue;
+       jmsg = json_object();
+       json_object_set_new(jmsg, "id", json_string(id->str));
+       if (!(jsubmsg=json_keyvalue_reply(msg)))
+           jsubmsg=json_null();
+       json_object_set_new(jmsg, "msg", jsubmsg);
+       json_array_append_new(jlist, jmsg);
+   }
+   return jlist;
+}
+
+static json_t *
+json_xreadstream_list(const redisReply *r) {

    unsigned int i;
-   json_t *jlist, *jroot = json_object(); /* that's what we return */
+   json_t *jobj=NULL, *jlist;
+   redisReply *sid, *msglist;
+   const redisReply *e;
+
+   if (r->elements) jobj = json_object();
+   for(i = 0; i < r->elements; i++) {
+       e=r->element[i];
+       if (e->type!=REDIS_REPLY_ARRAY || e->elements<2) continue;
+       sid = e->element[0]; msglist = e->element[1];
+       if (sid->type!=REDIS_REPLY_STRING || sid->len<1) continue;
+       if (msglist->type!=REDIS_REPLY_ARRAY || msglist->elements<1) continue;
+       if (!(jlist=json_singlestream_list(msglist)))
+           jlist=json_null();
+       json_object_set_new(jobj, sid->str, jlist);
+   }
+   return jobj;
+}
+
+static json_t *
+json_wrap_redis_reply(const struct cmd *cmd, const redisReply *r) {

+   json_t *jlist, *jobj, *jroot = json_object(); /* that's what we return */

    /* copy verb, as jansson only takes a char* but not its length. */
    char *verb;
@@ -153,27 +248,28 @@

        case REDIS_REPLY_ARRAY:
            if(strcasecmp(verb, "HGETALL") == 0) {
-               json_t *jobj = json_hgetall_reply(r);
+               jobj = json_keyvalue_reply(r);
                if(jobj) {
                    json_object_set_new(jroot, verb, jobj);
                    break;
                }
+           }else
+           if(strcasecmp(verb, "XRANGE") == 0 || strcasecmp(verb, "XREVRANGE") == 0) {
+               jobj = json_singlestream_list(r);
+               if(jobj)
+                   json_object_set_new(jroot, verb, jobj);
+               break;
+           } else
+           if(strcasecmp(verb, "XREAD") == 0 || strcasecmp(verb, "XREADGROUP") == 0) {
+               jobj = json_xreadstream_list(r);
+               if(jobj)
+                   json_object_set_new(jroot, verb, jobj);
+               break;
            }
-           jlist = json_array();
-           for(i = 0; i < r->elements; ++i) {
-               redisReply *e = r->element[i];
-               switch(e->type) {
-                   case REDIS_REPLY_STRING:
-                       json_array_append_new(jlist, json_string(e->str));
-                       break;
-                   case REDIS_REPLY_INTEGER:
-                       json_array_append_new(jlist, json_integer(e->integer));
-                       break;
-                   default:
-                       json_array_append_new(jlist, json_null());
-                       break;
-               }
-           }
+           
+           if (!(jlist=json_exand_array(r)))
+               jlist = json_null();
+
            json_object_set_new(jroot, verb, jlist);
            break;
nicolasff commented 4 years ago

Hey @majklik, you are right, this is a known limitation today.

Thanks a lot for the diff! This is not a trivial patch. Webdis was written long before Redis had support for streams and has never been updated to add this support (first commit was when Redis 2.0.4 was the latest stable release 😊).

Thanks also for linking this issue to the GEORADIUS one, but from a quick look at your diff it seems like GEORADIUS would also need some special-casing like these two lines?

+           if(strcasecmp(verb, "XRANGE") == 0 || strcasecmp(verb, "XREVRANGE") == 0) {
[...]
+           if(strcasecmp(verb, "XREAD") == 0 || strcasecmp(verb, "XREADGROUP") == 0) {

And aside from GEORADIUS, are there any other unsupported commands that use the nested maps?

I haven't reviewed this change too carefully yet (and I need to test it thoroughly before it can be integrated), but would you like to re-submit it as a pull request so that you can be duly credited for this work? Having a PR would also let me comment on it and make suggestions for changes. That said if you're just happy with me taking it as is and don't particularly care for this process, I can do that too and just mention your handle in the commit message (or any other name/username/email you'd prefer instead).

Let me know what you think.

nicolasff commented 4 years ago

Closing this in favor of #177.