JSON nested arrays support

majklik commented 4 years ago

Add support for nested arrays replies in JSON formatter and some commands outputs are converted to map list in similar way how HGETALL ( XRANGE / XREVRANGE / XCLAIM / XPENDING / XREAD / GEORADIUS ).

nicolasff commented 4 years ago

Awesome, thanks! I'll go over it shortly.

majklik commented 4 years ago

Well, we can continue there with issue https://github.com/nicolasff/webdis/issues/176#issue-639463065 . The original diff was dirty hack for fast work and supports only two-three commands which I need.

A added in this PR support for other nested arrays replies which is usefull to convert imto map list (IMHO). :-)

PS: We are using webdis about 7 years. :-)

nicolasff commented 4 years ago

Hi @majklik,

First off, congratulations on this great pull request. I see in your GitHub profile that this is the first one you've ever opened from this account, and few (if any) contributions to Webdis have been this complex. The code was clear, well-written and easy to follow. Great job!

Going over it a few times, the only real changes I felt were needed we stylistic, in order to maintain a consistent style throughout the code base. None of them actually change any logic, order, or variable names. They were pretty much all about spacing and braces. I also reviewed the Redis documentation for these commands, and tried to ensure that the keys returned by Webdis as it constructs all of these nested objects stay consistent with the naming conventions used throughout the docs. They are listed below, you will see that they are minor.

Formatting:

Added {} after each single-line if. See goto fail for the reason.
Added spaces around operators, e.g. e=r->element[i]; became e = r->element[i];.

Actual changes:

Changed msgsperowner to msgsperconsumer, following the documentation which says list every consumer in the consumer group with at least one pending message, and the number of pending messages it has. – emphasis on list every consumer.
Changed delivers to deliveries, again following the docs: The number of times this message was delivered. "delivers" is not a noun and as such is not well suited for a key in an object.
In the GEORADIUS code, changed member to name. The docs say: the first item in the sub-array is always the name of the returned item.
Also for GEORADIUS, changed coord to coords since coordinates is always plural.

Other: I also renamed json_exand_array to json_expand_array.

You'll find them all in commit 64d545cd647c70828976cd2b8a258fd05356bca8. All were made in a separate commit to leave your original contribution untouched and leave you as the sole author (rather than editing your commit).

One thing I thought was missing with the XCLAIM command, when it's used with JUSTID, but after testing it I realized that you had already covered this case with the extra condition r->element[0]->type == REDIS_REPLY_ARRAY. Well done!

Thanks again for this great submission, I am very grateful for it.

nicolasff commented 4 years ago

An additional note on testing: I used a custom Dockerfile to run Webdis in valgrind and check for leaks (drop this in the Webdis directory):

FROM debian:buster

RUN apt-get update && apt-get upgrade
RUN apt-get install -y clang valgrind make libevent-dev libmsgpack-dev git redis-server
RUN mkdir /tmp/webdis
COPY . /tmp/webdis/
RUN cd /tmp/webdis && find src -name '*.d' -delete && find src -name '*.o' -delete && sed -i 's/-O3/-g -O0/g' Makefile && make clean all install

RUN echo "daemonize yes" >> /etc/redis.conf
RUN sed -i 's/"daemonize":.*true/"daemonize": false/g' /etc/webdis.prod.json
CMD /usr/bin/redis-server /etc/redis.conf >/dev/null 2>/dev/null && valgrind --tool=memcheck --leak-check=full --show-reachable=yes --show-possibly-lost=yes /usr/local/bin/webdis /etc/webdis.prod.json

EXPOSE 7379

After which I ran the Python tests and the following streams tests:

#!/bin/zsh
curl -s -g 'http://localhost:7379/DEL/stream1' | jq
curl -s -g 'http://localhost:7379/XADD/stream1/*/name/Sara/surname/OConnor' | jq
curl -s -g 'http://localhost:7379/XADD/stream1/*/field1/value1/field2/value2/field3/value3' | jq
curl -s -g 'http://localhost:7379/XLEN/stream1' | jq
curl -s -g 'http://localhost:7379/XGROUP/CREATE/stream1/group1/0' | jq
curl -s -g 'http://localhost:7379/XREADGROUP/GROUP/group1/consumer1/COUNT/1/STREAMS/stream1/>' | jq . > xreadgroup.json
jq < xreadgroup.json
local eventid=$(jq '.XREADGROUP.stream1[0].id' < xreadgroup.json | sed 's/"//g')
curl -s -g 'http://localhost:7379/XCLAIM/stream1/group1/consumer1/0/0-0' | jq
curl -s -g 'http://localhost:7379/XCLAIM/stream1/group1/consumer1/0/'$eventid'/JUSTID' | jq
curl -s -g 'http://localhost:7379/XCLAIM/stream1/group1/consumer1/0/'$eventid | jq

Which produces this output:

{
  "DEL": 1
}
{
  "XADD": "1592779353712-0"
}
{
  "XADD": "1592779353721-0"
}
{
  "XLEN": 2
}
{
  "XGROUP": [
    true,
    "OK"
  ]
}
{
  "XREADGROUP": {
    "stream1": [
      {
        "id": "1592779353712-0",
        "msg": {
          "name": "Sara",
          "surname": "OConnor"
        }
      }
    ]
  }
}
{
  "XCLAIM": []
}
{
  "XCLAIM": [
    "1592779353712-0"
  ]
}
{
  "XCLAIM": [
    {
      "id": "1592779353712-0",
      "msg": {
        "name": "Sara",
        "surname": "OConnor"
      }
    }
  ]
}

The leak summary is as expected: the only retained memory is from strdup calls made when we read the config file. These are not true leaks but just memory that is allocated once per process and not free'd on shutdown. So the new code paths look good and I don't expect any memory issues.

nicolasff / webdis

JSON nested arrays support #177