Introducing JSON to Lettuce by tishun · Pull Request #2933 · redis/lettuce

tishun · 2024-07-31T16:11:59Z

Granted, this review turned out much much much bigger than I anticipated.

Instructions for reviewing

Ideally please concentrate on the most important decisions:

API surface as defined by src/main/templates/io/lettuce/core/api/RedisJsonCommands.java
sample use cases in the src/test/java/io/lettuce/core/RedisJsonCommandBuilderUnitTests.java
integration of the Jackson parser in the src/main/java/io/lettuce/core/json/DefaultJsonParser.java
use the testcontainers fw to deploy docker-stack src/test/java/io/lettuce/core/RedisContainerIntegrationTests.java

General considerations

(for more details check the README.md with some more information)

This implementation aims to solve several general problems:

avoid any JSON processing as part of the Netty event loop
provide Lettuce-specific interfaces to handle JSON as DOM
allow the users to plug-in their own implementations of the JSON parser
abstract from the RESP2 vs RESP3 specifics
support both JsonPath v1 and JsonPath v2

Three operating modes facilitating default (basic), advanced and power-user usage

Default mode

Uses Jackson behind the scenes, but provides a driver-specific API to avoid tight coupling with any 3rd party library
Deserializes lazily the data returned by the server to avoid slowing down the event loop
Serializes the data when sending JSON to the server before passing it to the event loop

Advanced mode

Provide a means - though the ClientOptions - to configure a custom JsonParser

Power-user mode

Makes it possible to use the JSON data in raw format, for example work only with byte buffers without parsing

Notes

changes were required to the way the command enumeration as the the JSON.X commands have a dot in their name
changes were required to the CommandBuilder to avoid making it too large and attempt to follow the single responsibility principle

Unrelated changes

There are a unrelated changes such as

adding missing imports in 3 other template files
modifying how the CommandType is used (it was expecting the command name to be a direct derivative of the enum value, but enum values could not have "." symbol in them, as most module commands do)
refactored the common logic for the CommandBuilder abstraction to be shared between different command builders

Supported commands

Command	Status
JSON.ARRAPPEND	✅ Implemented
JSON.ARRINDEX	✅ Implemented
JSON.ARRINSERT	✅ Implemented
JSON.ARRLEN	✅ Implemented
JSON.ARRPOP	✅ Implemented
JSON.ARRTRIM	✅ Implemented
JSON.CLEAR	✅ Implemented
JSON.DEBUG	❌ Not planned
JSON.DEBUG MEMORY	❌ Not planned
JSON.DEL	✅ Implemented
JSON.FORGET	❌ Not planned
JSON.GET	✅ Implemented
JSON.MERGE	✅ Implemented
JSON.MGET	✅ Implemented
JSON.MSET	✅ Implemented
JSON.NUMINCRBY	✅ Implemented
JSON.NUMMULTBY	❌ Not planned
JSON.OBJKEYS	✅ Implemented
JSON.OBJLEN	✅ Implemented
JSON.RESP	❌ Not planned
JSON.SET	✅ Implemented
JSON.STRAPPEND	✅ Implemented
JSON.STRLEN	✅ Implemented
JSON.TOGGLE	✅ Implemented
JSON.TYPE	✅ Implemented

Make sure that:

You have read the contribution guidelines.
You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.

jruaux

This looks really good! It will be a nice addition to Lettuce.
Just a couple things:

I did not see tests against Redis Cluster.
There does not seem to be an implementation of key routing in case of JSON.MGET with the Cluster API.

tishun · 2024-08-07T06:09:03Z

This looks really good! It will be a nice addition to Lettuce. Just a couple things:

I did not see tests against Redis Cluster.

There does not seem to be an implementation of key routing in case of JSON.MGET with the Cluster API.

Hey @jruaux , thanks a lot for taking a look!

Yeah these two points are very valid ones. Let me see if I can address them with an update.

src/main/java/io/lettuce/core/protocol/CommandType.java

sazzad16 · 2024-08-11T14:15:32Z

Hey @tishun, ...tracking... stuff got in here. Is it intended?

tishun · 2024-08-12T08:46:59Z

Hey @tishun, ...tracking... stuff got in here. Is it intended?

No, but this is because the PR is fork/redis-json -> origin/redis-json and origin/redis-json is now behind origin/main

I will try to fix this.

uglide

Overall, this is a good first iteration, but binding all JSON interfaces to generic types of Lettuce codec seems unnecessary and just increases the complexity of the code. Also, we need more developer-friendly overloads for frequently used commands like JSON.SET. See my inline comments for more details.

src/main/java/io/lettuce/core/json/UnproccessedJsonValue.java

src/main/java/io/lettuce/core/api/async/RedisJsonAsyncCommands.java

src/main/java/io/lettuce/core/output/JsonValueListOutput.java

src/main/java/io/lettuce/core/json/DelegateJsonValue.java

src/main/java/io/lettuce/core/api/async/RedisJsonAsyncCommands.java

tishun · 2024-08-21T11:20:05Z

Overall, this is a good first iteration, but binding all JSON interfaces to generic types of Lettuce codec seems unnecessary and just increases the complexity of the code.

There is no way to use the Codec system without having the generics, as we discussed offline. I do - however - agree that the key in a JSON file does not map to the redis key, used as part of most commands. As Such I will refactor the code to only consider the value generic.

Also, we need more developer-friendly overloads for frequently used commands like JSON.SET. See my inline comments for more details.

Agreed, this is one of the leftovers

tishun · 2024-09-09T15:55:51Z

Overall, this is a good first iteration, but binding all JSON interfaces to generic types of Lettuce codec seems unnecessary and just increases the complexity of the code.

There is no way to use the Codec system without having the generics, as we discussed offline.

After some more careful consideration I actually fully agree with you.

The latest version does not have any generics for the JsonValue / JsonParser abstractions as they did not help in any way.

atakavci

great job here i believe.
elegant and easy to read implementation. thanks for the well-organized test stuff as well.
left just a couple of comments and the rest is mostly nits.

src/main/java/io/lettuce/core/AbstractRedisAsyncCommands.java

src/main/java/io/lettuce/core/RedisAsyncCommandsImpl.java

src/main/java/io/lettuce/core/RedisReactiveCommandsImpl.java

src/main/java/io/lettuce/core/api/StatefulRedisConnection.java

src/main/java/io/lettuce/core/api/reactive/RedisReactiveCommands.java

src/test/java/io/lettuce/core/output/NumberListOutputUnitTests.java

src/main/java/io/lettuce/core/json/DelegateJsonValue.java

src/main/java/io/lettuce/core/json/DelegateJsonObject.java

src/main/java/io/lettuce/core/json/UnproccessedJsonValue.java

src/test/java/io/lettuce/core/json/RedisJsonClusterIntegrationTests.java

uglide

Great job!

atakavci

great job!

Started JSON README.md; Parser registry is now part of the Connection; formatting; chained all commands; extracted commands in their own unit RedisJsonCommandBuilder;

…configuration for custom parsers

…guments

…e with existing infra

* Most of the API layer * Add the JSON.TYPE command * Kotlin coroutines added; Started JSON README.md; Parser registry is now part of the Connection; formatting; chained all commands; extracted commands in their own unit RedisJsonCommandBuilder; * Implemented 90% of commands from top 10 * All but SET are implemented * Integrated Jackson, finished up the SET command * Left out a few files by mistake * Adding some JavaDoc * Formatting * Implemented all JSON commands * Introducing test containers to the testing fw * Complete coverage with integration tests, straight scenarios * Added Pathv1 tests * Added RedisCE cluster support for the JSON.MGET and JSON.MSET commands * Handle null values * No longer using K for the JSON object keys * Polishing * JsonType introduced to help typization * Remove the RedisCodec from the JsonValue/JsonParser abstraction, add configuration for custom parsers * Extend API surface with methods that reduce the amount of required arguments * Adding unit tests, addressing changes to README.md * Implemented object-mapping functionality * Addresses Ali's comments * Addressed last bunch of comments from Ali, changed ports to not colide with existing infra * Forgot to change ports and stop debug log * Polishing touches

tishun requested review from atakavci, sazzad16 and uglide July 31, 2024 16:12

tishun marked this pull request as draft August 1, 2024 14:29

jruaux reviewed Aug 6, 2024

View reviewed changes

sazzad16 reviewed Aug 8, 2024

View reviewed changes

src/main/java/io/lettuce/core/protocol/CommandType.java Show resolved Hide resolved

tishun force-pushed the topic/tishun/redis-json branch from 127d29e to fc5d14b Compare August 9, 2024 10:11

yisraelU mentioned this pull request Aug 15, 2024

RedisJson draft profunktor/redis4cats#907

Closed

uglide requested changes Aug 19, 2024

View reviewed changes

yisraelU reviewed Aug 20, 2024

View reviewed changes

src/main/java/io/lettuce/core/api/async/RedisJsonAsyncCommands.java Outdated Show resolved Hide resolved

tishun force-pushed the topic/tishun/redis-json branch from 9b3892b to fd842b3 Compare August 21, 2024 11:12

tishun mentioned this pull request Aug 26, 2024

perf: auto batch flush (more than 2x times performance gain) #2950

Open

tishun marked this pull request as ready for review September 9, 2024 15:59

atakavci reviewed Sep 13, 2024

View reviewed changes

uglide approved these changes Sep 23, 2024

View reviewed changes

atakavci approved these changes Sep 25, 2024

View reviewed changes

tishun added 9 commits September 25, 2024 10:02

Most of the API layer

64561a9

Add the JSON.TYPE command

1b78d19

Kotlin coroutines added;

fcdf5d0

Started JSON README.md; Parser registry is now part of the Connection; formatting; chained all commands; extracted commands in their own unit RedisJsonCommandBuilder;

Implemented 90% of commands from top 10

c3ab46b

All but SET are implemented

e6514c8

Integrated Jackson, finished up the SET command

6148c71

Left out a few files by mistake

81bbe0a

Adding some JavaDoc

51b9dcd

Formatting

dfabd70

tishun added 17 commits September 25, 2024 10:11

Implemented all JSON commands

1bf2838

Introducing test containers to the testing fw

a9934f0

Complete coverage with integration tests, straight scenarios

e56ec77

Added Pathv1 tests

b2ba57a

Added RedisCE cluster support for the JSON.MGET and JSON.MSET commands

582f6ae

Handle null values

133c5bd

No longer using K for the JSON object keys

69e01fb

Polishing

8137d90

JsonType introduced to help typization

9da86ab

Remove the RedisCodec from the JsonValue/JsonParser abstraction, add …

4e4de3e

…configuration for custom parsers

Extend API surface with methods that reduce the amount of required ar…

5ff6851

…guments

Adding unit tests, addressing changes to README.md

96d77e8

Implemented object-mapping functionality

3834d3e

Addresses Ali's comments

4ab9513

Addressed last bunch of comments from Ali, changed ports to not colid…

518dc3e

…e with existing infra

Forgot to change ports and stop debug log

538fdc8

Polishing touches

ae602b0

tishun force-pushed the topic/tishun/redis-json branch from 2b5970c to ae602b0 Compare September 25, 2024 07:21

tishun merged commit f03f593 into redis:redis-json Sep 25, 2024

tishun deleted the topic/tishun/redis-json branch September 25, 2024 07:36

tishun mentioned this pull request Sep 25, 2024

Introducing JSON to Lettuce (#2933) #2992

Merged

4 tasks

This was referenced Dec 1, 2024

Restore API that was accidently deleted when introducing the JSON feature #3065

Merged

Public API methods removed by mistake when introducing RedisJSON #3070

Closed

tishun mentioned this pull request Dec 31, 2024

FEATURE: Support lettuce 6.5.0 redis/lettucemod#62

Closed

Conversation

tishun commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions for reviewing

General considerations

Default mode

Advanced mode

Power-user mode

Notes

Unrelated changes

Supported commands

Uh oh!

jruaux left a comment

Choose a reason for hiding this comment

Uh oh!

tishun commented Aug 7, 2024

Uh oh!

Uh oh!

sazzad16 commented Aug 11, 2024

Uh oh!

tishun commented Aug 12, 2024

Uh oh!

uglide left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tishun commented Aug 21, 2024

Uh oh!

tishun commented Sep 9, 2024

Uh oh!

atakavci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uglide left a comment

Choose a reason for hiding this comment

Uh oh!

atakavci left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tishun commented Jul 31, 2024 •

edited

Loading

uglide left a comment •

edited

Loading