SET IFEQ command implemented in Java + tests #2978

Maayanshani25 · 2025-01-19T10:28:32Z

Issue link

This Pull Request is linked to issue (URL): [/issues/2811]

Description

This pull Request is implemented the IFEQ option in the SET commamnd. Tests were added, and docstring to the command and the related method.

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
Create merge commit if merging release branch into main, squash otherwise.

Maayanshani25 · 2025-01-19T12:07:32Z

@Yury-Fridlyand
I found the implementation of java a bit different than node.
Do you think it's possible to this the also here? #2909 (comment)

Yury-Fridlyand · 2025-01-20T16:17:04Z

Java is not so flexible, unfortunately.
But it is still possible to do if you customize the setter in the SetOptions builder. Probably, you can keep exposed ConditionalSet with 2 options, but under the hood have another one with 3 options.
Need to be careful to avoid breaking changes while doing refactoring.
StandaloneSubscriptionConfigurationBuilder is a nice example of a customized builder.

Maayanshani25 · 2025-01-22T07:55:17Z

So for now I see two options.

The implementation I did in the pr for now. If the user chooses the conditionalSet ONLY_IF_EQUAL, it makes sure comparisonValue isnt null and push them both to the args.
If comparisonValue is set but The condionalSet is not ONLY_IF_EQUAL, throws an error
Removing ONLY_IF_EQUAL from conditionalSet options and use it this way:

if (conditionalSet != null) {
            optionArgs.add(conditionalSet.valkeyApi);
} else if (comparisonValue != null) {
            optionArgs.add("IFEQ", comparisonValue);

}

What do you think is better?

java/client/src/main/java/glide/api/commands/StringBaseCommands.java

java/client/src/main/java/glide/api/models/commands/SetOptions.java

Maayanshani25 · 2025-01-27T17:59:24Z

@Yury-Fridlyand
Is it good? anything else or can you approve?

java/client/src/main/java/glide/api/models/commands/SetOptions.java

Yury-Fridlyand · 2025-01-27T20:31:26Z

LGTM, I applied minor doc updates. Need a second approval.

ikolomi · 2025-01-28T11:18:51Z

java/client/src/main/java/glide/api/commands/StringBaseCommands.java

@@ -223,6 +223,12 @@ public interface StringBaseCommands {
     * String value = client.set("key", "value", options).get();
     * assert value.equals("OK");
     * }</pre>
+     *     <pre>{@code


Apparently addition of IFEQ out-dates the @return doc above.
Since it becomes complicated to explain all the implications introduced by the new options, perhaps the return doc string should only mention that the set options affect the return behavior. In case you prefer to keep the full explanation in the return doc, extend it with IFEQ

ikolomi · 2025-01-28T11:21:01Z

java/client/src/main/java/glide/api/commands/StringBaseCommands.java

@@ -235,8 +241,9 @@ public interface StringBaseCommands {
     * @param options The Set options.
     * @return If the value is successfully set, return <code>"OK"</code>. If value isn't set because
     *     of {@link ConditionalSet#ONLY_IF_EXISTS} or {@link ConditionalSet#ONLY_IF_DOES_NOT_EXIST}
-     *     conditions, return <code>null</code>. If {@link SetOptionsBuilder#returnOldValue(boolean)}
-     *     is set, return the old value as a <code>String</code>.
+     *     or {@link ConditionalSet#ONLY_IF_EQUAL} conditions, return <code>null</code>. If {@link


same as above

ikolomi · 2025-01-28T11:48:47Z

java/client/src/main/java/glide/api/models/commands/SetOptions.java

@@ -49,11 +53,52 @@ public enum ConditionalSet {
         * Only set the key if it does not already exist. Equivalent to <code>NX</code> in the Valkey
         * API.
         */
-        ONLY_IF_DOES_NOT_EXIST("NX");
+        ONLY_IF_DOES_NOT_EXIST("NX"),


Here a confusion starts - are these mutual exclusive? You should doc it

It's not possible for a key to both exist and not exist at the same time, so the user can specify only one condition. In the Valkey API, you can choose SET key value [NX | XX | IFEQ comparison-value].
There is no contradiction between the XX and IFEQ comparison-value conditions but in the API of valkey they chose to apply only one of them.
In Java, it's a bit complicated to enforce this. In Node.js, I created a new type to apply only one of the conditionals, but here it's a bit different. I think it's okay to use the format we did and as Yuri mentioned above:

SetOptions.builder().conditionalSetIfEqualTo("abc").conditionalSet(ONLY_IF_DOES_NOT_EXIST).build(); // should not fail SetOptions.builder().conditionalSet(ONLY_IF_EQUAL).build(); // should fail

In this approach, the last condition set will be the one that applies.

@ikolomi what do you think?

I think we need to enforce the mutual exclusiveness by throwing from the builder's setters

ikolomi · 2025-01-28T11:49:42Z

java/client/src/main/java/glide/api/models/commands/SetOptions.java

+         * @param value The value to compare.
+         * @return This builder instance.
+         */
+        public SetOptionsBuilder conditionalSetIfEqualTo(@NonNull String value) {


I suspect we want these conditions to be mutual exclusive ? If so, we should enforce it by throwing

``
public SetOptionsBuilder conditionalIfExistst() {
}

public SetOptionsBuilder conditionalIfNotExistst() {
}

public SetOptionsBuilder conditionalIfEqual(@nonnull String value) {
}
``

Lest the methods to override the current value + describe in the docs that the conditions are mutual exclusive

ikolomi · 2025-01-28T11:53:31Z

@Maayanshani25 please dont forget to squash

Signed-off-by: Maayan Shani <[email protected]>

…to NonNull Signed-off-by: Maayan Shani <[email protected]>

Signed-off-by: Maayan Shani <[email protected]>

ikolomi · 2025-02-02T08:46:05Z

java/client/src/main/java/glide/api/models/commands/SetOptions.java

+         * @param value The value to compare.
+         * @return This builder instance.
+         */
+        public SetOptionsBuilder conditionalSetIfEqualTo(@NonNull String value) {


``
public SetOptionsBuilder conditionalIfExistst() {
}

public SetOptionsBuilder conditionalIfNotExistst() {
}

public SetOptionsBuilder conditionalIfEqual(@nonnull String value) {
}
``

Lest the methods to override the current value + describe in the docs that the conditions are mutual exclusive

Maayanshani25 requested a review from a team as a code owner January 19, 2025 10:28

Maayanshani25 mentioned this pull request Jan 19, 2025

Support IFEQ option for SET command #2811

Closed

4 tasks

Yury-Fridlyand added the java issues and fixes related to the java client label Jan 20, 2025

Maayanshani25 requested review from avifenesh, Yury-Fridlyand and shohamazon January 22, 2025 08:05

Maayanshani25 force-pushed the ifeq-java branch 2 times, most recently from 8894244 to 772e82f Compare January 22, 2025 15:16

Yury-Fridlyand reviewed Jan 22, 2025

View reviewed changes

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

Maayanshani25 force-pushed the ifeq-java branch 2 times, most recently from 5b73b9f to 97041b8 Compare January 23, 2025 19:45

Yury-Fridlyand reviewed Jan 23, 2025

View reviewed changes

Maayanshani25 force-pushed the ifeq-java branch from b09199f to 384af07 Compare January 26, 2025 06:50

Yury-Fridlyand approved these changes Jan 27, 2025

View reviewed changes

ikolomi self-requested a review January 28, 2025 09:33

ikolomi requested changes Jan 28, 2025

View reviewed changes

Maayanshani25 force-pushed the ifeq-java branch 2 times, most recently from 1becf23 to 9cecbad Compare January 29, 2025 09:15

Maayanshani25 added 6 commits February 2, 2025 18:32

start ifeq in java

77fcf48

Signed-off-by: Maayan Shani <[email protected]>

set option + two tests

d115329

Signed-off-by: Maayan Shani <[email protected]>

add docstring, throw errors, changelog

5b172ce

Signed-off-by: Maayan Shani <[email protected]>

add conditionalSetIfEqualTo builder

6e66667

Signed-off-by: Maayan Shani <[email protected]>

add conditionalSet builder for OXX and NX and change comparisonValue …

137425d

…to NonNull Signed-off-by: Maayan Shani <[email protected]>

Enforce mutual exclusive + yuri fixes

2757f2d

mutual exclusive

05cd90a

Signed-off-by: Maayan Shani <[email protected]>

Maayanshani25 force-pushed the ifeq-java branch from 9cecbad to 05cd90a Compare February 2, 2025 18:32

docstring and lint

2926889

Signed-off-by: Maayan Shani <[email protected]>

ikolomi approved these changes Feb 3, 2025

View reviewed changes

Maayanshani25 merged commit 7b77603 into valkey-io:main Feb 3, 2025
14 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SET IFEQ command implemented in Java + tests #2978

SET IFEQ command implemented in Java + tests #2978

Maayanshani25 commented Jan 19, 2025 •

edited

Loading

Maayanshani25 commented Jan 19, 2025

Yury-Fridlyand commented Jan 20, 2025 •

edited

Loading

Maayanshani25 commented Jan 22, 2025 •

edited

Loading

Maayanshani25 commented Jan 27, 2025

Yury-Fridlyand commented Jan 27, 2025

ikolomi Jan 28, 2025

ikolomi Jan 28, 2025

ikolomi Jan 28, 2025

Maayanshani25 Jan 28, 2025

ikolomi Jan 29, 2025

ikolomi Jan 28, 2025

ikolomi Feb 2, 2025

ikolomi commented Jan 28, 2025

ikolomi Feb 2, 2025

SET IFEQ command implemented in Java + tests #2978

SET IFEQ command implemented in Java + tests #2978

Conversation

Maayanshani25 commented Jan 19, 2025 • edited Loading

Issue link

Description

Checklist

Maayanshani25 commented Jan 19, 2025

Yury-Fridlyand commented Jan 20, 2025 • edited Loading

Maayanshani25 commented Jan 22, 2025 • edited Loading

Maayanshani25 commented Jan 27, 2025

Yury-Fridlyand commented Jan 27, 2025

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

ikolomi commented Jan 28, 2025

Choose a reason for hiding this comment

Maayanshani25 commented Jan 19, 2025 •

edited

Loading

Yury-Fridlyand commented Jan 20, 2025 •

edited

Loading

Maayanshani25 commented Jan 22, 2025 •

edited

Loading