SET IFEQ command implemented in Java + tests by Maayanshani25 · Pull Request #2978 · valkey-io/valkey-glide

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?

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

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

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

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

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

     * 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

     *     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

         * 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

+         * @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 <maayan.shani@mail.huji.ac.il>

…to NonNull Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

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

+         * @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

* start ifeq in java Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> * set option + two tests Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> * add docstring, throw errors, changelog Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> * add conditionalSetIfEqualTo builder Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> * add conditionalSet builder for OXX and NX and change comparisonValue to NonNull Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> * Enforce mutual exclusive + yuri fixes * mutual exclusive Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> * docstring and lint Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il> --------- Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

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 Yury-Fridlyand, avifenesh 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

Comment thread java/client/src/main/java/glide/api/commands/StringBaseCommands.java Outdated

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 suggested 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 <maayan.shani@mail.huji.ac.il>

set option + two tests

d115329

Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

add docstring, throw errors, changelog

5b172ce

Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

add conditionalSetIfEqualTo builder

6e66667

Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

add conditionalSet builder for OXX and NX and change comparisonValue …

137425d

…to NonNull Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

Enforce mutual exclusive + yuri fixes

2757f2d

mutual exclusive

05cd90a

Signed-off-by: Maayan Shani <maayan.shani@mail.huji.ac.il>

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 <maayan.shani@mail.huji.ac.il>

ikolomi approved these changes Feb 3, 2025

View reviewed changes

Maayanshani25 merged commit 7b77603 into valkey-io:main Feb 3, 2025

v3rm0n mentioned this pull request Dec 10, 2025

Set options binary #5047

Merged

6 tasks

Uh oh!

Conversation

Maayanshani25 commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue link

Description

Checklist

Uh oh!

Maayanshani25 commented Jan 19, 2025

Uh oh!

Yury-Fridlyand commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maayanshani25 commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maayanshani25 commented Jan 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yury-Fridlyand commented Jan 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikolomi commented Jan 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Maayanshani25 commented Jan 19, 2025 •

edited

Loading

Yury-Fridlyand commented Jan 20, 2025 •

edited

Loading

Maayanshani25 commented Jan 22, 2025 •

edited

Loading