Add support for RedisJSON

[d3rpp]

Part 2 of #650

Changes over previous editions:

To take on board some of the feedback in #650 (specifically the result variant), I needed to make a few changes

Custom implementer macro:

This was done because the redisjson functions in implementation now return results that get unwrapped before the query, following the following structure

• Did Serialise Correctly?
  • YES: Go on with Query -> Return Result of Query
  • NO: Return Result::Err with ErrorKind::Serialize (only active with json feature) and a description given by the serde_json error messages

This change will only affect some RedisJson Commands, the core commands are untouched by this

There will likely be debate over the removed call to ::std::mem::replace; however, this is only because since the items are now wrapped in Result's, the value is limited to the scope of the initial function, and when I try to use the main macro rustc complains that the value doesn't live long enough.

This also called for the introduction of the arg_take function on Cmd, which added a function that takes the value by value and returns the value (instead of &mut Cmd from arg.

Tests

It has all the same tests as the previous one except with an extra one to test that the serde_json errors are being handled correctly (at the top of test_json.rs); this attempts to serialise one of the few cases where serde_json fails to serialise a struct. (When a HashMap does NOT have a string-ifiable key (Strings and Numbers only)

---

I thought I added more but upon review I did not

[djc]

Here's an initial round of review, this is looking much better.

[d3rpp]

lol whoops

[d3rpp]

Still todo: add start and stop to the json_arr_index function

I know there is a trait somewhere where you can take T or Option::<T>::None but I cannot remember what is was

edit: it's Into<Option<T>>, I'll make it an isize since it can only be a number anyway
edit 2: it makes the macro panic, I'll just move it to a second function which takes them instead.

[d3rpp]

I will admit it is not the worlds cleanest solution but its cleaner than

json_arr_index("key", "path", &json!({"hello": "world"}), None, None)

// or

json_arr_index("key", "path", &json!({"hello": "world"}), 0, 0)

[djc]

I don't remember, why did we want to have the JSON commands in separate traits (JsonCommands, AsyncJsonCommands) anyway?

[djc]

If you rewrite it like this:

let mut cmd = cmd("JSON.ARRAPPEND");
cmd.arg(key)
    .arg(path)
    .arg(serde_json::to_string(value)?);
Ok::<_, RedisError>(cmd)

I think you no longer need arg_take()? (Also note that we don't need to specify the Cmd type in the Ok turbofish.)

Also while looking this I noticed all the indentation in this file is tabs instead of spaces, should fix that up (VS Code has a Convert Indentation to Spaces thing.)

[d3rpp]

I'll do some investigation into this, I think I tried something like this, but I may be wrong

[djc]

I tried it for one command and it seemed to work? But maybe I got it wrong...

[d3rpp]

I tried it for one command and it seemed to work? But maybe I got it wrong...

sorry, I've been busy with a hackathon over the weekend, will look into this asap

[djc]

No hurry from my side! Hope you had fun at the hackathon.

[d3rpp]

If you rewrite it like this:

let mut cmd = cmd("JSON.ARRAPPEND");
cmd.arg(key)
    .arg(path)
    .arg(serde_json::to_string(value)?);
Ok::<_, RedisError>(cmd)

I think you no longer need arg_take()? (Also note that we don't need to specify the Cmd type in the Ok turbofish.)

Also while looking this I noticed all the indentation in this file is tabs instead of spaces, should fix that up (VS Code has a Convert Indentation to Spaces thing.)

let mut cmd = cmd("JSON.ARRAPPEND");

cmd.arg(key)
	.arg(path)
	.arg(serde_json::to_string(value)?);

Ok::<_, RedisError>(cmd)

Does indeed seem to work, ill update the rest of them

I've run the command to use spaces instead of tabs

[d3rpp]

I don't remember, why did we want to have the JSON commands in separate traits (JsonCommands, AsyncJsonCommands) anyway?

If we don't it's 2 Separate traits with the same name so If you import both of them the compiler complains unless you alias one of them.

[djc]

I meant, why can't we make these available as part of the normal command traits (but subject to the Cargo feature flag)?

[d3rpp]

I meant, why can't we make these available as part of the normal command traits (but subject to the Cargo feature flag)?

That's what I did in the First PR (#644)

which lead to the creation of separate modules, and to import them at the same time, it needs a different name.

[djc]

Okay, let's keep the additional trait for now.

[djc]

Some spit and polish, but I think this is almost ready to merge!

[djc]

Alright, nice work! @jaymell did you want to take another look at this?

[jaymell]

It looks good to me, though @d3rpp I'm going to request that if possible you squash this down to one commit and give it a nice commit message to save me a bit of work merging 😁 (and of course resolve conflicts)

[d3rpp]

It looks good to me, though @d3rpp I'm going to request that if possible you squash this down to one commit and give it a nice commit message to save me a bit of work merging 😁 (and of course resolve conflicts)

oooooh last time I tried that it was absolutely painful, is there not a squash checkbox next to the merge button?

[d3rpp]

umm, idk
is there a squash button in the drop-down next to the merge button?

this actually makes 0 sense to me

also, the repo is up-to-date, so there shouldn't be any conflicts

[djc]

Yeah, I just squashed it at merge. @d3rpp thanks for persisting in getting this merged!