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.

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 node + tests #2909

Merged
merged 8 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* Java, Node, Python: Update documentation for CONFIG SET and CONFIG GET ([#2919](https://github.com/valkey-io/valkey-glide/pull/2919))
* Go: Add `BZPopMin` ([#2849](https://github.com/valkey-io/valkey-glide/pull/2849))
* Java: Add `RESP2` support ([#2383](https://github.com/valkey-io/valkey-glide/pull/2383))
* Node: Add `IFEQ` option ([#2909](https://github.com/valkey-io/valkey-glide/pull/2909))

#### Breaking Changes

Expand Down
10 changes: 9 additions & 1 deletion node/src/BaseClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1473,7 +1473,8 @@ export class BaseClient {
* @param value - The value to store with the given key.
* @param options - (Optional) See {@link SetOptions} and {@link DecoderOption}.
* @returns - If the value is successfully set, return OK.
* If value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist` conditions, return null.
* If `conditional` in `options` is not set, the value will be set regardless of prior value existence.
* If value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist` or `onlyIfEqual` conditions, return `null`.
* If `returnOldValue` is set, return the old value as a string.
*
* @example
Expand All @@ -1493,6 +1494,13 @@ export class BaseClient {
* // Example usage of get method to retrieve the value of a key
* const result4 = await client.get("key");
* console.log(result4); // Output: 'new_value' - Value wasn't modified back to being "value" because of "NX" flag.
*
* // Example usage of set method with conditional option IFEQ
Maayanshani25 marked this conversation as resolved.
Show resolved Hide resolved
* await client.set("key", "value we will compare to");
* const result5 = await client.set("key", "new_value", {conditionalSet: "onlyIfEqual", comparisonValue: "value we will compare to"});
* console.log(result5); // Output: 'OK' - Set "new_value" to "key" only if comparisonValue is equal to the current value of "key".
* const result6 = await client.set("key", "another_new_value", {conditionalSet: "onlyIfEqual", comparisonValue: "value we will compare to"});
* console.log(result6); // Output: `null` - Value wasn't set because the comparisonValue is not equal to the current value of "key". Value of "key" remains "new_value".
* ```
*/
public async set(
Expand Down
36 changes: 26 additions & 10 deletions node/src/Commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,29 @@ export function createGetRange(
]);
}

export interface SetOptions {
/**
* `onlyIfDoesNotExist` - Only set the key if it does not already exist.
* Equivalent to `NX` in the Valkey API. `onlyIfExists` - Only set the key if
* it already exist. Equivalent to `EX` in the Valkey API. if `conditional` is
* not set the value will be set regardless of prior value existence. If value
* isn't set because of the condition, return null.
*/
conditionalSet?: "onlyIfExists" | "onlyIfDoesNotExist";
export type SetOptions = (
| {
/**
* `onlyIfDoesNotExist` - Only set the key if it does not already exist.
* `NX` in the Valkey API.
*
* `onlyIfExists` - Only set the key if it already exists.
* `EX` in the Valkey API.
*/
conditionalSet?: "onlyIfExists" | "onlyIfDoesNotExist";
}
| {
/**
* `onlyIfEqual` - Only set the key if the comparison value equals the current value of key.
* `IFEQ` in the Valkey API.
*/
conditionalSet: "onlyIfEqual";
Copy link
Collaborator

Choose a reason for hiding this comment

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

IFEQ is short for "if equal", why do we need the "only"?
It is not aligned with the valkey naming, and I don't think it's to clarify something which is not clear.

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 like the other conditions onlyIfExists == NX and onlyIfDoesNotExist == EX. That is why i used only as a prefix

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmm ok that's fair.

/**
* The value to compare the existing value with.
*/
comparisonValue: GlideString;
}
) & {
/**
* Return the old string stored at key, or nil if key did not exist. An error
* is returned and SET aborted if the value stored at key is not a string.
Expand All @@ -151,7 +165,7 @@ export interface SetOptions {
type: TimeUnit;
count: number;
};
}
};

/**
* @internal
Expand All @@ -168,6 +182,8 @@ export function createSet(
args.push("XX");
} else if (options.conditionalSet === "onlyIfDoesNotExist") {
args.push("NX");
} else if (options.conditionalSet === "onlyIfEqual") {
args.push("IFEQ", options.comparisonValue);
}

if (options.returnOldValue) {
Expand Down
120 changes: 116 additions & 4 deletions node/tests/SharedTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8166,6 +8166,34 @@ export function runBaseTests(config: {
expect(getNotExistingKey).toEqual(null);
}

async function setWithOnlyIfEquals(client: BaseClient) {
const key = uuidv4();
const initialValue = uuidv4();
const newValue = uuidv4();
const setKey = await client.set(key, initialValue);
expect(setKey).toEqual("OK");
const getRes = await client.get(key);
expect(getRes).toEqual(initialValue);

// Attempt to set with a non-matching value (should fail -> return null)
const conditionalSetFailResponse = await client.set(key, newValue, {
conditionalSet: "onlyIfEqual",
comparisonValue: newValue,
});
expect(conditionalSetFailResponse).toEqual(null);

// Attempt to set with a matching value (should succeed -> return OK)
const conditionalSetSuccessResponse = await client.set(key, newValue, {
conditionalSet: "onlyIfEqual",
comparisonValue: initialValue,
});
expect(conditionalSetSuccessResponse).toEqual("OK");

// Retrieve the updated value of the key
const updatedGetResponse = await client.get(key);
expect(updatedGetResponse).toEqual(newValue);
}

async function setWithOnlyIfNotExistOptions(client: BaseClient) {
const key = uuidv4();
const value = uuidv4();
Expand Down Expand Up @@ -8233,9 +8261,54 @@ export function runBaseTests(config: {
expect(await client.get(key)).toEqual(null);
}

async function testSetWithAllCombination(client: BaseClient) {
async function setIfeqWithAllOptions(client: BaseClient) {
const key = uuidv4();
const value = uuidv4();
const initialValue = uuidv4();
const newValue = uuidv4();

await client.set(key, initialValue);
// set with multiple options:
// * only apply SET if the provided value equals oldValue
// * expires after 1 second
// * returns the old value
const setResWithAllOptions = await client.set(key, newValue, {
expiry: {
type: TimeUnit.UnixSeconds,
count: Math.floor(Date.now() / 1000) + 1,
},
conditionalSet: "onlyIfEqual",
comparisonValue: initialValue,
returnOldValue: true,
});
// initialValue should be get from the key
expect(setResWithAllOptions).toEqual(initialValue);
// newValue should be set as the key value
expect(await client.get(key)).toEqual(newValue);
Maayanshani25 marked this conversation as resolved.
Show resolved Hide resolved

// fail command
const wrongValue = "wrong value";
const setResFailedWithAllOptions = await client.set(key, wrongValue, {
expiry: {
type: TimeUnit.UnixSeconds,
count: Math.floor(Date.now() / 1000) + 1,
},
conditionalSet: "onlyIfEqual",
comparisonValue: wrongValue,
returnOldValue: true,
});
// current value of key should be newValue
expect(setResFailedWithAllOptions).toEqual(newValue);
// key should not be set. it remains the same
expect(await client.get(key)).toEqual(newValue);
}

async function testSetWithAllCombination(
client: BaseClient,
cluster: ValkeyCluster,
) {
const key = uuidv4();
const value = uuidv4(); // Initial value
const value2 = uuidv4(); // New value for IFEQ testing
const count = 2;
const expiryCombination = [
{ type: TimeUnit.Seconds, count },
Expand All @@ -8246,6 +8319,7 @@ export function runBaseTests(config: {
];
let exist = false;

// onlyIfDoesNotExist tests
for (const expiryVal of expiryCombination) {
const setRes = await client.set(key, value, {
expiry: expiryVal as
Expand All @@ -8272,6 +8346,7 @@ export function runBaseTests(config: {
expect(getRes).toEqual(value);
}

// OnlyIfExist tests
for (const expiryVal of expiryCombination) {
const setRes = await client.set(key, value, {
expiry: expiryVal as
Expand All @@ -8291,18 +8366,55 @@ export function runBaseTests(config: {

expect(setRes).toBeDefined();
}

// onlyIfEqual tests
if (!cluster.checkIfServerVersionLessThan("8.1.0")) {
Maayanshani25 marked this conversation as resolved.
Show resolved Hide resolved
for (const expiryVal of expiryCombination) {
// Set the key with the initial value
await client.set(key, value);

const setRes = await client.set(key, value2, {
expiry: expiryVal as
| "keepExisting"
| {
type:
| TimeUnit.Seconds
| TimeUnit.Milliseconds
| TimeUnit.UnixSeconds
| TimeUnit.UnixMilliseconds;
count: number;
},
conditionalSet: "onlyIfEqual",
comparisonValue: value, // Ensure it matches the current key's value
});

if (setRes) {
expect(setRes).toEqual("OK"); // Should return 'OK' if the condition is met
} else {
// If condition fails, ensure value remains unchanged
const getRes = await client.get(key);
expect(getRes).toEqual(value);
}
}
}
}

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
"Set commands with options test_%p",
async (protocol) => {
await runTest(async (client: BaseClient) => {
await runTest(async (client: BaseClient, cluster) => {
await setWithExpiryOptions(client);
await setWithOnlyIfExistOptions(client);
await setWithOnlyIfNotExistOptions(client);
await setWithGetOldOptions(client);
await setWithAllOptions(client);
await testSetWithAllCombination(client);

if (!cluster.checkIfServerVersionLessThan("8.1.0")) {
await setWithOnlyIfEquals(client);
await setIfeqWithAllOptions(client);
}

await testSetWithAllCombination(client, cluster);
}, protocol);
},
config.timeout,
Expand Down
Loading