Java: `JSON.ARRINSERT` and `JSON.ARRLEN` #2476

Yury-Fridlyand · 2024-10-18T21:35:17Z

Issue link

#2430

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Commits will be squashed upon merging.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

…alkey-442 Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

acarbonetto · 2024-10-18T22:47:27Z

java/integTest/src/test/java/glide/modules/JsonTests.java

+                };
+        var res = Json.arrinsert(client, key, "$..a", 0, values).get();
+
+        doc = Json.get(client, key).get();


why are you re-using doc?

Why not? I reuse res as well

acarbonetto · 2024-10-18T22:48:21Z

java/integTest/build.gradle

@@ -11,6 +11,7 @@ dependencies {
    implementation project(':client')

    implementation group: 'org.apache.commons', name: 'commons-lang3', version: '3.13.0'
+    implementation 'com.google.code.gson:gson:2.10.1'


is the license ok?

Apache-2
Anyway it is used for tests only.

I'll align with you to use this google library instead of org.skyscreamer.jsonassert for JSON equality test in my next PR then. We don't need 2 different lib for the same purpose.

acarbonetto · 2024-10-18T22:49:59Z

java/client/src/main/java/glide/api/commands/servermodules/Json.java

+     *
+     * @param client The client to execute the command.
+     * @param key The key of the JSON document.
+     * @return The array length stored at the root of the document. If document root is not an array,


@jamesx-improving are we including this signature? It doesn't seem useful to include a API signature that won't get used.
We should consider removing this.

Which signature?

For this command, path is indeed optional on all versions of documentations, hence it's hard to justify the removal of this signature (and the GlideString variant of it down below)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

yipin-chen · 2024-10-18T23:51:23Z

java/client/src/main/java/glide/api/commands/servermodules/Json.java

@@ -19,9 +19,11 @@
 /** Module for JSON commands. */
 public class Json {

-    public static final String JSON_PREFIX = "JSON.";
+    private static final String JSON_PREFIX = "JSON.";
    public static final String JSON_SET = JSON_PREFIX + "SET";
    public static final String JSON_GET = JSON_PREFIX + "GET";


Change both GET and SET to private too?

It is used somewhere in UT.

Don't worry about it, I'll get it sort out in my next PR. But you do have a point, these don't need to be public.

java/client/src/main/java/glide/api/commands/servermodules/Json.java

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

jamesx-improving · 2024-10-19T00:51:04Z

java/client/src/main/java/glide/api/commands/servermodules/Json.java

@@ -19,9 +19,11 @@
 /** Module for JSON commands. */
 public class Json {

-    public static final String JSON_PREFIX = "JSON.";
+    private static final String JSON_PREFIX = "JSON.";
    public static final String JSON_SET = JSON_PREFIX + "SET";
    public static final String JSON_GET = JSON_PREFIX + "GET";


Don't worry about it, I'll get it sort out in my next PR. But you do have a point, these don't need to be public.

jamesx-improving · 2024-10-19T01:10:34Z

java/integTest/build.gradle

@@ -11,6 +11,7 @@ dependencies {
    implementation project(':client')

    implementation group: 'org.apache.commons', name: 'commons-lang3', version: '3.13.0'
+    implementation 'com.google.code.gson:gson:2.10.1'


I'll align with you to use this google library instead of org.skyscreamer.jsonassert for JSON equality test in my next PR then. We don't need 2 different lib for the same purpose.

jamesx-improving · 2024-10-19T01:17:42Z

java/client/src/main/java/glide/api/commands/servermodules/Json.java

+     *
+     * @param client The client to execute the command.
+     * @param key The key of the JSON document.
+     * @return The array length stored at the root of the document. If document root is not an array,


For this command, path is indeed optional on all versions of documentations, hence it's hard to justify the removal of this signature (and the GlideString variant of it down below)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* `JSON.ARRINSERT` and `JSON.ARRLEN` Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

JSON.ARRINSERT and JSON.ARRLEN

9d8ee5a

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand added the java issues and fixes related to the java client label Oct 18, 2024

Yury-Fridlyand requested a review from a team as a code owner October 18, 2024 21:35

Yury-Fridlyand changed the title ~~JSON.ARRINSERT and JSON.ARRLEN~~ Java: JSON.ARRINSERT and JSON.ARRLEN Oct 18, 2024

Yury-Fridlyand mentioned this pull request Oct 18, 2024

Java: Json support #2430

Closed

22 tasks

Yury-Fridlyand had a problem deploying to AWS_ACTIONS October 18, 2024 21:39 — with GitHub Actions Error

Yury-Fridlyand added 2 commits October 18, 2024 14:40

typo fix

702f775

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Merge remote-tracking branch 'upstream/release-1.2' into java/yuryf-v…

c9c6d83

…alkey-442 Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS October 18, 2024 21:41 — with GitHub Actions Inactive

acarbonetto approved these changes Oct 18, 2024

View reviewed changes

test fix

1df8761

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS October 18, 2024 23:01 — with GitHub Actions Inactive

yipin-chen approved these changes Oct 18, 2024

View reviewed changes

yipin-chen reviewed Oct 18, 2024

View reviewed changes

java/client/src/main/java/glide/api/commands/servermodules/Json.java Outdated Show resolved Hide resolved

yipin-chen reviewed Oct 18, 2024

View reviewed changes

java/client/src/main/java/glide/api/commands/servermodules/Json.java Show resolved Hide resolved

Address PR review.

31ea6ac

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand had a problem deploying to AWS_ACTIONS October 19, 2024 00:58 — with GitHub Actions Failure

typo fix

eac9503

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand had a problem deploying to AWS_ACTIONS October 19, 2024 01:20 — with GitHub Actions Failure

jamesx-improving approved these changes Oct 19, 2024

View reviewed changes

test fix

5e78f0c

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS October 19, 2024 01:26 — with GitHub Actions Inactive

test fix

0782305

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand temporarily deployed to AWS_ACTIONS October 19, 2024 01:45 — with GitHub Actions Inactive

Yury-Fridlyand merged commit 56b4401 into release-1.2 Oct 19, 2024
8 checks passed

Yury-Fridlyand deleted the java/yuryf-valkey-442 branch October 19, 2024 02:23

avifenesh pushed a commit to avifenesh/valkey-glide that referenced this pull request Oct 21, 2024

Java: JSON.ARRINSERT and JSON.ARRLEN (valkey-io#2476)

7c32dd7

* `JSON.ARRINSERT` and `JSON.ARRLEN` Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

avifenesh pushed a commit to avifenesh/valkey-glide that referenced this pull request Oct 21, 2024

Java: JSON.ARRINSERT and JSON.ARRLEN (valkey-io#2476)

7b2f856

* `JSON.ARRINSERT` and `JSON.ARRLEN` Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Muhammad-awawdi-amazon pushed a commit to Muhammad-awawdi-amazon/valkey-glide that referenced this pull request Oct 22, 2024

Java: JSON.ARRINSERT and JSON.ARRLEN (valkey-io#2476)

c6792b0

* `JSON.ARRINSERT` and `JSON.ARRLEN` Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

cyip10 pushed a commit that referenced this pull request Oct 23, 2024

Java: JSON.ARRINSERT and JSON.ARRLEN (#2476)

cff7d6b

* `JSON.ARRINSERT` and `JSON.ARRLEN` Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: `JSON.ARRINSERT` and `JSON.ARRLEN` #2476

Java: `JSON.ARRINSERT` and `JSON.ARRLEN` #2476

Yury-Fridlyand commented Oct 18, 2024

acarbonetto Oct 18, 2024

Yury-Fridlyand Oct 18, 2024 •

edited

Loading

acarbonetto Oct 18, 2024

Yury-Fridlyand Oct 18, 2024

jamesx-improving Oct 19, 2024

acarbonetto Oct 18, 2024

Yury-Fridlyand Oct 18, 2024

jamesx-improving Oct 19, 2024

yipin-chen Oct 18, 2024

Yury-Fridlyand Oct 19, 2024

jamesx-improving Oct 19, 2024

jamesx-improving Oct 19, 2024

jamesx-improving Oct 19, 2024

jamesx-improving Oct 19, 2024

Java: JSON.ARRINSERT and JSON.ARRLEN #2476

Java: JSON.ARRINSERT and JSON.ARRLEN #2476

Conversation

Yury-Fridlyand commented Oct 18, 2024

Issue link

Checklist

Choose a reason for hiding this comment

Yury-Fridlyand Oct 18, 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

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

Java: `JSON.ARRINSERT` and `JSON.ARRLEN` #2476

Java: `JSON.ARRINSERT` and `JSON.ARRLEN` #2476

Yury-Fridlyand Oct 18, 2024 •

edited

Loading