Draft API for connection and template layers for Redis JSON commands

[Dgramada]

Provides a draft version of how the JSON template and connection layer API should look like. The purpose of this PR is to receive feedback and to get a better feeling of what should Spring Data Redis provide for users that would like to use Redis JSON.

• You have read the Spring Data contribution guidelines.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

[onobc]

👋🏻 @Dgramada ,

Thanks for the PR - it is great to see the feature being added. I have left a few comments to address / points to discuss.

[onobc]

We do have a Spring Data Range object org.springframework.data.domain.Range<T>. I wonder if we could that instead?

[onobc]

When you get into the impl PTAL at this Range and see if it something we would want to re-use.

[Dgramada]

If the interfaces are acceptable, I could close this issue and create a separate one that introduces them alongside their implementations and testing. Would you prefer this approach or would you like me to extend this PR?

[onobc]

If the interfaces are acceptable,
I do - they are at a nice simple starting point.

I could close this issue and create a separate one that introduces them alongside their implementations and testing.
Would you prefer this approach or would you like me to extend this PR?

I would just rock this PR (continue on from this point).

Thank you for the quick turnaround - I was on PTO - sorry for the delay.

[onobc]

Since the default methods just ultimately chain to the non-default method, this validation will be duplicated because the non-default impl will have to implement the same checks.

Note

This comment applies to the PR

[onobc]

RedisOperations is set up by having key and value type info (i.e. RedisOperations<K, V>). This menas that value types are always V.

The JsonOperations, in contrast requires type info in its API calls and may deviate from the V at RedisOperations<K, V>.

Imagine RedisOperations<String, Person> and now you would want to store redisOperations.valueOps().set(key, new Order()) because you're doing jsonOps().get(key, Order.class). These concepts don't align.

That being said, the JSON API seems to be able to operate on any types. Me and Mark were chatting about this and will continue the chat tomorrow. Will follow up w/ the outcome here to see if we want to type the JsonOperations w/ K,V or as you have done it w/ K.

[Dgramada]

My concern with JsonOperations<K, V> would be when used with lets say some data structure like List<E>.
Then we would have something like JsonOperations<String, List> and we would lose the list element type and would have to have to provide it as a parameter in a method overload.

[tishun]

Hey @onobc ,

me and @Dgramada discussed this question and I realized I don't fully understand it. Let me make sure we are on the same page first:

• you can only call JSON.GET on a key that was saved as a JSON with JSON.SET. If you call set my-key my-value and then json.get my-key you will get (error) Existing key has wrong Redis type.
• the V generic is relatively ambiguous in this case, as it might have or might not have the same semantics of a typical value one stores in their Redis instance

Overall I had the same kind of conundrum when I was designing the Lettuce APIs : whether the payload is best represented by a simple type (String, byte[]); an complex type (Person.class or V); or an datatype abstraction (JsonObject). IMHO there is no clear and obvious right choice and it all depends on the use case.

What I can say from experience is that the community quickly provided the feedback that they were actually expecting the simplest solution (String) so we had to overload our classes with it.

NB! In Lettuce this also means that we are not able to benefit from early serialization / late deserialization on the client thread and we end up congesting the thread pool that processes IO (since String is final we have no good way to wrap it with this logic).

[onobc]

Thanks for those details @tishun and @Dgramada . We (team) discussed this and what we would like to do is proceed w/ this PR but focus only on the command layer. This can go into main and be available w/ the 4.1 release. The operations aspect of the PR we will move into a branch or gist for safekeeping and exploration - ultimately targeting 4.2 for the operations / API layer in November.

This approach will give us some time to design the API/operations layer while making the commands available in 4.1.

Sound good?

[tishun]

Makes a lot of sense to me personally, @Dgramada what do you think?

[onobc]

👋🏻 @Dgramada I left a couple comments to address. Thanks.

[onobc]

May be helpful to add inclusive for start and exclusive for stop.

[onobc]

Ditto https://github.com/spring-projects/spring-data-redis/pull/3327/changes#r3002905067

[onobc]

I noticed the tests are not running because there is no "JSON.*" command returned from the conditions on the server (i.e. the tests never run).
I believe we need to guard this on version @EnabledOnRedisVersion(8.2) (is 8.2 correct version?).

When they do run, you will find that they run into a StackOverflow due to the DefaultedRedisConnection not implementing the new methods by invoking the delegate. For example, this fixes the recursion on the jsonSet:

	@Override
	public Boolean jsonSet(byte[] key, String value) {
		return this.delegate.jsonSet(key, value);
	}

	@Override
	public Boolean jsonSet(byte[] key, String path, String value, JsonSetOption option) {
		return this.delegate.jsonSet(key, path, value, option);
	}

[Dgramada]

The current testing setup that downloads Redis from source shouldn't have the JSON commands. Redis JSON is a separate module that was included in Redis 8.0.0.

However the current Makefile setup doesn't turn on the necessary flag to include the modules like JSON, Search, Bloom Filter, etc. Furthermore if we decide to test JSON through source this would require adding additional dependencies for the JSON module to work, which is not ideal. You can find the module flag and all the necessary dependencies for everything to work here.

I have an ongoing pull request that migrates the current Makefile setup to use Docker containers instead of downloading Redis from source. Images of Redis 8.0.0 and onwards have all the Redis modules baked in.

[Dgramada]

I would like to mention that this PR is missing the Jedis implementation because it depends on #3315.

Also there is currently 1 test failing which is the jsonArrAppend() test. This might be a bug on the Lettuce side, but I must verify this. If the problem is in Lettuce, we could drop this method and introduce it in the future once it is fixed.

[onobc]

**I would like to mention that this PR is missing the Jedis implementation because it depends on #3315.

Looks like @mp911de just merged that a few hours ago.

Also there is currently 1 test failing which is the jsonArrAppend() test. This might be a bug on the Lettuce side, but I must verify this. If the problem is in Lettuce, we could drop this method and introduce it in the future once it is fixed.

Good plan - lmk what you find.

[onobc]

The current testing setup that downloads Redis from source shouldn't have the JSON commands. Redis JSON is a separate module that was included in Redis 8.0.0.

However the current Makefile setup doesn't turn on the necessary flag to include the modules like JSON, Search, Bloom Filter, etc. Furthermore if we decide to test JSON through source this would require adding additional dependencies for the JSON module to work, which is not ideal. You can find the module flag and all the necessary dependencies for everything to work here.

I have an ongoing pull request that migrates the current Makefile setup to use Docker containers instead of downloading Redis from source. Images of Redis 8.0.0 and onwards have all the Redis modules baked in.

Ahh ok, this makes sense now. Hopefully the docker-compose PR gets approved and merged.

[Dgramada]

Hey @onobc,
A quick update from me. jsonArrAppend() is broken in Lettuce so I will drop the method from the PR.