serverless-redis-http icon indicating copy to clipboard operation
serverless-redis-http copied to clipboard

RedisJSON warning and differences

Open nounder opened this issue 1 year ago • 6 comments

Readme states following:

The Upstash implementation of RedisJSON contains a number of subtle differences in what is returned in responses. For this reason, it is not advisable to use SRH with Redis Stack if you are testing your Upstash implementation that uses JSON commands.

Is it possible to specify those "subtle differences"? Upstash in their official docs states compatibility for whole JSON module.

nounder avatar May 17 '23 09:05 nounder

Hi, thanks for reaching out. The following is an automated test I run nightly against the upstash/redis package, and as you can see the only failed tasks are JSON ones: https://github.com/hiett/serverless-redis-http/actions/runs/5003096177/jobs/8963888738

This is because Upstash's JSON does the following differently (from what I can figure out): From: https://redis.io/commands/json.set/

See "Update multi-paths" If you run JSON.SET doc $ '{"f1": {"a":1}, "f2":{"a":2}}' And then JSON.SET doc $..a 3 And then JSON.GET doc

You get back "{\"f1\":{\"a\":3},\"f2\":{\"a\":3}}"

If you run the same commands in Upstash, you get back: "{\"a\":3,\"f1\":{\"a\":3},\"f2\":{\"a\":3}}"

Note the additional "a": 3 in the root.


The ordering of responses in array form is also inverted in many cases, and tested to be this in Upstash's tests: https://github.com/upstash/upstash-redis/blob/main/pkg/commands/json_get.test.ts

JSON.SET doc $ '{"a":2, "b": 3, "nested": {"a": 4, "b": null}}' JSON.GET doc $..b

In normal Redis will return "[3,null]"

In Upstash, will return "[null, 3]"


I didn't want to clutter the docs with these details, but I can make a separate markdown page if you feel it would be good. I made the blanket statement not to use it because I don't want people coming to this repository complaining that the functionality is different, when SRH is merely a proxy. It will return the exact results that the actual Redis server gives back. Open to suggestions on what you think the appropriate thing to do is.

There is potential I could implement an "upstash" mode which tries to replicate these behaviours in the SRH layer. However, it would be a lot more to maintain and keep up with on SRH's side, and feels like it's stepping over the line of what a proxy should actually do.

hiett avatar May 17 '23 15:05 hiett

Both cases resolve around recursive descent (via ..). It looks like Redis OG and Upstash Redis have different way of traversing document via JSONPath. Maybe @ademilter or @chronark can shed more light about root cause?

After seeing this JSON warning in this repo's Readme I got bit discouraged to use it. Having read the specific differences, I've come to the conclusion that they don't matter to me and I would start using your code right away. I believe a more precise warning might be less daunting for new users, thus encouraging wider adoption of your work and Upstash itself.

When people start to complain about the differences, enough interest and dev power will be here to help implement "upstash mode" ^^

Looking forward to use your work later today. Thank you!

nounder avatar May 18 '23 07:05 nounder

Fair point, we should add this to our docs.

I believe we do this, because we were following the official specs when implementing the commands. The OSS redis implementation itself sometimes doesn't follow the official api specs. leading to things like the extra 3 showing up in upstash

I'll raise this internally and have the team take a look. And then I'll add docs on our side

chronark avatar May 18 '23 08:05 chronark

As @chronark said, we tried to follow wording in https://redis.io/commands/json.set/ page. But in some cases wording is not very clear and/or is ambiguous.

In this case, original RedisJSON implementation executes multi-path set operation (JSON.SET doc $..a 3) only for existing fields, as if there's an implicit XX flag.

On Upstash, you can achieve the same with JSON.SET doc $..a 3 XX. We have an internal issue for these differences, we will try to resolve them gradually.

mdogan avatar May 18 '23 09:05 mdogan

Thank you all for your inputs. I really appriciate the time spent talking this through. From these, I think this is the plan of action:

  • I am going to update the Readme to be more specific regarding which JSON commands are slightly different with what arguments (linking to another MD file where it contains the above command examples). I will ensure the wording is far softer, so most people will know they're in the clear unless they're specifically dealing with arguments with recursive decent.
  • I'm going to create a branch from this issue and do that. So this will be closed once that gets merged in :)
  • I also agree that if possible, this should be added to the Upstash docs. I totally understand that the Redis docs are vague at times, and the C Redis often doesn't exactly line up. But I think when users see the Redis name, they're expecting functionality they're used to (and functionality that is present on the interactive examples on the Redis docs page).

hiett avatar May 18 '23 12:05 hiett

@mdogan Could you shed some light on the JSON.GET doc $..b returning results the other way around? Is there another flag that is implicitly used?

Working on this markdown page explaining what to look out for. The JSON.SET makes sense, I have explained to use the XX flag explicitly for functionality to be identical, but not sure about the ordering of the JSON.GET response. Thanks

hiett avatar May 18 '23 16:05 hiett