Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SET IFEQ command implemented in Java + tests #2978

Merged
merged 8 commits into from
Feb 3, 2025
Merged

SET IFEQ command implemented in Java + tests #2978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
77fcf48
start ifeq in java
Maayanshani25 Jan 15, 2025
d115329
set option + two tests
Maayanshani25 Jan 19, 2025
5b172ce
add docstring, throw errors, changelog
Maayanshani25 Jan 22, 2025
6e66667
add conditionalSetIfEqualTo builder
Maayanshani25 Jan 23, 2025
137425d
add conditionalSet builder for OXX and NX and change comparisonValue …
Maayanshani25 Jan 26, 2025
2757f2d
Enforce mutual exclusive + yuri fixes
Maayanshani25 Jan 29, 2025
05cd90a
mutual exclusive
Maayanshani25 Feb 2, 2025
2926889
docstring and lint
Maayanshani25 Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* Java: Shadow `protobuf` dependency ([#2931](https://github.com/valkey-io/valkey-glide/pull/2931))
* Java: Add `RESP2` support ([#2383](https://github.com/valkey-io/valkey-glide/pull/2383))
* Node, Python: Add `IFEQ` option ([#2909](https://github.com/valkey-io/valkey-glide/pull/2909), [#2962](https://github.com/valkey-io/valkey-glide/pull/2962))
* Java: Add `IFEQ` option ([#2978](https://github.com/valkey-io/valkey-glide/pull/2978))

#### Breaking Changes

Expand Down
18 changes: 13 additions & 5 deletions java/client/src/main/java/glide/api/commands/StringBaseCommands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,14 +215,21 @@ 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
* SetOptionsBuilder#returnOldValue(boolean)} is set, return the old value as a <code>String
* </code>.
* @example
* <pre>{@code
* SetOptions options = SetOptions.builder().conditionalSet(ONLY_IF_EXISTS).expiry(Seconds(5L)).build();
* SetOptions options = SetOptions.builder().conditionalSetOnlyIfExists().expiry(Seconds(5L)).build();
* String value = client.set("key", "value", options).get();
* assert value.equals("OK");
* }</pre>
* <pre>{@code
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* client.set("key", "value").get();
* SetOptions options = SetOptions.builder().conditionalSetIfEqualTo("value").build();
* String value = client.set("key", "newValue", options).get();
* assert value.equals("OK");
* }</pre>
*/
CompletableFuture<String> set(String key, String value, SetOptions options);

Expand All @@ -235,8 +242,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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

* SetOptionsBuilder#returnOldValue(boolean)} is set, return the old value as a <code>String
* </code>.
* @example
* <pre>{@code
* SetOptions options = SetOptions.builder().conditionalSet(ONLY_IF_EXISTS).expiry(Seconds(5L)).build();
Expand Down
69 changes: 68 additions & 1 deletion java/client/src/main/java/glide/api/models/commands/SetOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.List;
import lombok.Builder;
import lombok.Getter;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;

/**
Expand All @@ -29,6 +30,9 @@ public final class SetOptions {
*/
private final ConditionalSet conditionalSet;

/** Value to compare when {@link ConditionalSet#ONLY_IF_EQUAL} is set. */
private final String comparisonValue;

/**
* Set command to return the old string stored at <code>key</code>, or <code>null</code> if <code>
* key</code> did not exist. An error is returned and <code>SET</code> aborted if the value stored
Expand All @@ -49,11 +53,71 @@ 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"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Only set the key if the current value of key equals the {@link SetOptions#comparisonValue}.
* Equivalent to <code>IFEQ comparison-value</code> in the Valkey API.
*/
ONLY_IF_EQUAL("IFEQ");

private final String valkeyApi;
}

/**
* Builder class for {@link SetOptions}.
*
* <p>Provides methods to set conditions under which a value should be set.
*
* <p>Note: Calling any of these methods will override the existing values of {@code
* conditionalSet} and {@code comparisonValue}, if they are already set.
*/
public static class SetOptionsBuilder {
/**
* Sets the condition to {@link ConditionalSet#ONLY_IF_EXISTS} for setting the value.
*
* <p>This method overrides any previously set {@code conditionalSet} and {@code
* comparisonValue}.
*
* @return This builder instance
*/
public SetOptionsBuilder conditionalSetOnlyIfExists() {
this.conditionalSet = ConditionalSet.ONLY_IF_EXISTS;
this.comparisonValue = null;
return this;
}

/**
* Sets the condition to {@link ConditionalSet#ONLY_IF_DOES_NOT_EXIST} for setting the value.
*
* <p>This method overrides any previously set {@code conditionalSet} and {@code
* comparisonValue}.
*
* @return This builder instance
*/
public SetOptionsBuilder conditionalSetOnlyIfNotExist() {
this.conditionalSet = ConditionalSet.ONLY_IF_DOES_NOT_EXIST;
this.comparisonValue = null;
return this;
}

/**
* Sets the condition to {@link ConditionalSet#ONLY_IF_EQUAL} for setting the value. The key
* will be set if the provided comparison value matches the existing value.
*
* <p>This method overrides any previously set {@code conditionalSet} and {@code
* comparisonValue}.
*
* @since Valkey 8.1 and above.
* @param value the value to compare
* @return this builder instance
*/
public SetOptionsBuilder conditionalSetOnlyIfEqualTo(@NonNull String value) {
this.conditionalSet = ConditionalSet.ONLY_IF_EQUAL;
this.comparisonValue = value;
return this;
}
}

/** Configuration of value lifetime. */
public static final class Expiry {

Expand Down Expand Up @@ -151,6 +215,9 @@ public String[] toArgs() {
List<String> optionArgs = new ArrayList<>();
if (conditionalSet != null) {
optionArgs.add(conditionalSet.valkeyApi);
if (conditionalSet == ConditionalSet.ONLY_IF_EQUAL) {
optionArgs.add(comparisonValue);
}
}

if (returnOldValue) {
Expand Down
95 changes: 95 additions & 0 deletions java/client/src/test/java/glide/api/GlideClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@
import static glide.api.models.commands.LInsertOptions.InsertPosition.BEFORE;
import static glide.api.models.commands.ScoreFilter.MAX;
import static glide.api.models.commands.SetOptions.ConditionalSet.ONLY_IF_DOES_NOT_EXIST;
import static glide.api.models.commands.SetOptions.ConditionalSet.ONLY_IF_EQUAL;
import static glide.api.models.commands.SetOptions.ConditionalSet.ONLY_IF_EXISTS;
import static glide.api.models.commands.SetOptions.RETURN_OLD_VALUE;
import static glide.api.models.commands.SortBaseOptions.ALPHA_COMMAND_STRING;
Expand Down Expand Up @@ -953,6 +954,100 @@ public void set_with_SetOptions_OnlyIfDoesNotExist_returns_success() {
assertEquals(value, response.get());
}

@SneakyThrows
@Test
public void set_with_SetOptions_OnlyIfEqual_success() {
// setup
String key = "key";
String value = "value";
String newValue = "newValue";

// Set `key` to `value` initially
CompletableFuture<String> initialSetResponse = new CompletableFuture<>();
initialSetResponse.complete("OK");
String[] initialArguments = new String[] {key, value};
when(commandManager.<String>submitNewCommand(eq(pSet), eq(initialArguments), any()))
.thenReturn(initialSetResponse);

CompletableFuture<String> initialResponse = service.set(key, value);
assertNotNull(initialResponse);
assertEquals("OK", initialResponse.get());

// Set `key` to `newValue` with the correct condition
SetOptions setOptions =
SetOptions.builder()
.conditionalSetOnlyIfEqualTo(value) // Key must currently have `value`
.expiry(Expiry.UnixSeconds(60L))
.build();
String[] correctConditionArguments =
new String[] {key, newValue, ONLY_IF_EQUAL.getValkeyApi(), value, "EXAT", "60"};
CompletableFuture<String> correctSetResponse = new CompletableFuture<>();
correctSetResponse.complete("OK");
when(commandManager.<String>submitNewCommand(eq(pSet), eq(correctConditionArguments), any()))
.thenReturn(correctSetResponse);

CompletableFuture<String> correctResponse = service.set(key, newValue, setOptions);
assertNotNull(correctResponse);
assertEquals("OK", correctResponse.get());

// Verify that the key is now set to `newValue`
CompletableFuture<String> fetchValueResponse = new CompletableFuture<>();
fetchValueResponse.complete(newValue);
when(commandManager.<String>submitNewCommand(eq(Get), eq(new String[] {key}), any()))
.thenReturn(fetchValueResponse);

CompletableFuture<String> finalValue = service.get(key);
assertEquals(newValue, finalValue.get());
}

@SneakyThrows
@Test
public void set_with_SetOptions_OnlyIfEqual_fails() {
// Key-Value setup
String key = "key";
String value = "value";
String newValue = "newValue";

// Set `key` to `value` initially
CompletableFuture<String> initialSetResponse = new CompletableFuture<>();
initialSetResponse.complete("OK");
String[] initialArguments = new String[] {key, value};
when(commandManager.<String>submitNewCommand(eq(pSet), eq(initialArguments), any()))
.thenReturn(initialSetResponse);

CompletableFuture<String> initialResponse = service.set(key, value);
assertNotNull(initialResponse);
assertEquals("OK", initialResponse.get());

// Attempt to set `key` to `newValue` with the wrong condition
SetOptions wrongConditionOptions =
SetOptions.builder()
.conditionalSetOnlyIfEqualTo(newValue) // Incorrect: current value of key is `value`
.expiry(Expiry.UnixSeconds(60L))
.build();

String[] wrongConditionArguments =
new String[] {key, newValue, ONLY_IF_EQUAL.getValkeyApi(), newValue, "EXAT", "60"};

CompletableFuture<String> failedSetResponse = new CompletableFuture<>();
failedSetResponse.complete(null);
when(commandManager.<String>submitNewCommand(eq(pSet), eq(wrongConditionArguments), any()))
.thenReturn(failedSetResponse);

CompletableFuture<String> failedResponse = service.set(key, newValue, wrongConditionOptions);
assertNotNull(failedResponse);
assertNull(failedResponse.get()); // Ensure the set operation failed

// Verify that the key remains set to `value`
CompletableFuture<String> fetchValueResponse = new CompletableFuture<>();
fetchValueResponse.complete(value);
when(commandManager.<String>submitNewCommand(eq(Get), eq(new String[] {key}), any()))
.thenReturn(fetchValueResponse);

CompletableFuture<String> finalValue = service.get(key);
assertEquals(value, finalValue.get());
}

@SneakyThrows
@Test
public void exists_returns_long_success() {
Expand Down
Loading