Feat: Align `go-redis` `JSONCmdable` #459

unknowntpo · 2024-01-27T11:14:50Z

This PR implemented adapter for go-redis JSONCmdable.

codecov-commenter · 2024-01-27T11:33:26Z

Codecov Report

Attention: 111 lines in your changes are missing coverage. Please review.

Comparison is base (222f9ed) 95.99% compared to head (92c36fa) 95.69%.
Report is 44 commits behind head on main.

Files	Patch %	Lines
rueidiscompat/adapter.go	63.54%	66 Missing and 4 partials ⚠️
rueidiscompat/command.go	66.11%	33 Missing and 8 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
- Coverage   95.99%   95.69%   -0.31%     
==========================================
  Files          77       77              
  Lines       32392    32685     +293     
==========================================
+ Hits        31095    31278     +183     
- Misses       1106     1204      +98     
- Partials      191      203      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rueian · 2024-01-27T11:56:03Z

rueidiscompat/adapter.go

+	_cmd := c.client.B().JsonSet().Key(key).Path(path).Value(str(value))
+	switch mode {
+	case "XX":
+		_cmd.Nx()


Suggested change

_cmd.Nx()

_cmd.Xx()

rueian · 2024-01-27T11:56:12Z

rueidiscompat/adapter.go

+	case "XX":
+		_cmd.Nx()
+	case "NX":
+		_cmd.Xx()


Suggested change

_cmd.Xx()

_cmd.Nx()

rueian · 2024-01-27T11:56:46Z

rueidiscompat/command.go

-	err  error
-	val  []any
+	baseCmd[[]any]
+	// val  []any


Suggested change

// val []any

rueian · 2024-01-27T12:01:32Z

rueidiscompat/command.go

+	}
+	vals := make([]any, len(arr))
+	for i, v := range arr {
+		if isJSONObjKeys {


It will be better to move if isJSONObjKeys { to the outside of the loop.

rueian · 2024-01-27T12:02:01Z

rueidiscompat/command.go

+		if isJSONObjKeys {
+			// for JSON.OBJKEYS
+			if v.IsNil() {
+				vals[i] = nil


This is unnecessary, right?

It's necessary because the return value might be Nil

e.g.

redis> JSON.SET doc $ '{"a":[3], "nested": {"a": {"b":2, "c": 1}}}' OK redis> JSON.OBJKEYS doc $..a 1) (nil) 2) 1) "b" 2) "c"

Ref: JSON.OBJKEYS

I get your point, vals[i] is nil by default, no need to set it, I'll fix that.

rueian · 2024-01-27T12:03:14Z

rueidiscompat/command.go

+				continue
+			}
+			// convert to any which underlying type is []any
+			arr, err := v.ToAny()


Suggested change

arr, err := v.ToAny()

vals[i], err = v.ToAny()

rueian · 2024-01-27T12:06:39Z

rueidiscompat/command.go

+	msg, err := res.ToMessage()
+	if err != nil {
+		if err == rueidis.Nil {
+			cmd.SetVal("")


why cmd.SetVal("") when err == rueidis.Nil ?

I'm wrong.
If we delete everything from this key, and call JSON.GET with path $, we would get nil.
So, for JSON.GET, if the error is rueidis.Nil, we should just return it.

json.set del1 $ '{"a": 1, "nested": {"a": 2, "b": 3}}' OK 127.0.0.1:6378> json.get del1 "{\"a\":1,\"nested\":{\"a\":2,\"b\":3}}" 127.0.0.1:6378> json.del del1 $ (integer) 1 127.0.0.1:6378> json.get del1 $ (nil)

the tests in go-redis is wrong.

https://github.com/redis/rueidis/pull/459/files#diff-8e5958b5f75f88e93f6235ef6011f699baa79bb7f32da3c9f9bf2f3507c4ce1aR10818

And for JSON.NUMINCRBY:

JSON.NUMINCRBY returns a bulk string reply specified as a stringified new value for each path, or nil, if the matching JSON value is not a number.

According to the documentation, the array element in reply of JSON.NUMINCRBY will be rueidis.Nil if matching JSON value is not a number, and when it's nil, we should set it to nil.

e.g.

redis> JSON.SET doc . '{"a":"b","b":[{"a":2}, {"a":5}, {"a":"c"}]}' OK redis> JSON.NUMINCRBY doc $..a 2 "[null,4,7,null]"

Ref:

JSON.NUMINCRBY

JSON.GET

rueian · 2024-01-27T12:07:31Z

rueidiscompat/command.go

+	intPtrSlice := make([]*int64, len(arr))
+	for i, e := range arr {
+		if e.IsNil() {
+			intPtrSlice[i] = nil


This also seems unnecessary.

Do you mean intPtrSlice[i] = nil is unnecessary because element of intPtrSlice has default value nil ?

rueian · 2024-01-27T12:07:52Z

rueidiscompat/command.go

+	anySlice := make([]any, len(arr))
+	for i, e := range arr {
+		if e.IsNil() {
+			anySlice[i] = nil


This also seems unnecessary.

unknowntpo force-pushed the feat-align-go-redis-json branch 2 times, most recently from 1126eb6 to b38db7c Compare January 27, 2024 11:18

implement JSONCmdable

a7b4c1c

unknowntpo force-pushed the feat-align-go-redis-json branch from b38db7c to a7b4c1c Compare January 27, 2024 11:20

rueian reviewed Jan 27, 2024

View reviewed changes

fix: JSONSetMode: XX, NX

8d19ea1

rueian added the feature label Jan 27, 2024

unknowntpo added 4 commits January 28, 2024 08:31

docs: remove unused comments

0c30363

refactor: remove unnecessary set nil operation

91cb821

refactor(newSliceCmd): take isJSONObjKeys cond outside of loop

7faf9f7

refactor(JSONCmd): tweak wrongly handled reply

92c36fa

unknowntpo force-pushed the feat-align-go-redis-json branch from 4122c65 to 92c36fa Compare January 28, 2024 09:48

rueian merged commit ffeb7c7 into redis:main Jan 28, 2024

unknowntpo deleted the feat-align-go-redis-json branch January 29, 2024 00:06

DestroyerAlpha mentioned this pull request Jan 30, 2024

Align go-redis JSONCmdable #432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Align `go-redis` `JSONCmdable` #459

Feat: Align `go-redis` `JSONCmdable` #459

unknowntpo commented Jan 27, 2024

codecov-commenter commented Jan 27, 2024 •

edited

Loading

rueian Jan 27, 2024

rueian Jan 27, 2024

rueian Jan 27, 2024

rueian Jan 27, 2024

rueian Jan 27, 2024

unknowntpo Jan 28, 2024

unknowntpo Jan 28, 2024

rueian Jan 27, 2024

rueian Jan 27, 2024

unknowntpo Jan 28, 2024 •

edited

Loading

rueian Jan 27, 2024

unknowntpo Jan 28, 2024

rueian Jan 28, 2024

rueian Jan 27, 2024

Feat: Align go-redis JSONCmdable #459

Feat: Align go-redis JSONCmdable #459

Conversation

unknowntpo commented Jan 27, 2024

codecov-commenter commented Jan 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unknowntpo Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feat: Align `go-redis` `JSONCmdable` #459

Feat: Align `go-redis` `JSONCmdable` #459

codecov-commenter commented Jan 27, 2024 •

edited

Loading

unknowntpo Jan 28, 2024 •

edited

Loading