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

Node: added COPY command #2024

Merged
merged 12 commits into from
Jul 29, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
* Node: Added FUNCTION DELETE command ([#1990](https:/valkey-io/valkey-glide/pull/1990))
* Node: Added FUNCTION FLUSH command ([#1984](https:/valkey-io/valkey-glide/pull/1984))
* Node: Added FCALL and FCALL_RO commands ([#2011](https:/valkey-io/valkey-glide/pull/2011))
* Node: Added COPY command ([#2024](https:/valkey-io/valkey-glide/pull/2024))
* Node: Added ZMPOP command ([#1994](https:/valkey-io/valkey-glide/pull/1994))
* Node: Added ZINCRBY command ([#2009](https:/valkey-io/valkey-glide/pull/2009))
* Node: Added BZMPOP command ([#2018](https:/valkey-io/valkey-glide/pull/2018))
Expand Down
37 changes: 34 additions & 3 deletions node/src/BaseClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ import * as net from "net";
import { Buffer, BufferWriter, Reader, Writer } from "protobufjs";
import {
AggregationType,
BitmapIndexType,
BitOffsetOptions,
BitmapIndexType,
BitwiseOperation,
ExpireOptions,
GeoAddOptions,
GeospatialData,
GeoUnit,
GeospatialData,
InsertPosition,
KeyWeight,
LPosOptions,
Expand All @@ -35,10 +35,11 @@ import {
ZAddOptions,
createBLPop,
createBRPop,
createBZMPop,
createBitCount,
createBitOp,
createBZMPop,
createBitPos,
createCopy,
createDecr,
createDecrBy,
createDel,
Expand Down Expand Up @@ -3494,6 +3495,36 @@ export class BaseClient {
return this.createWritePromise(createObjectRefcount(key));
}

/**
* Copies the value stored at the `source` to the `destination` key if the `destination` key does not
* yet exist.
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
*
* See https://valkey.io/commands/copy/ for more details.
*
* since Valkey version 6.2.0.
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
*
* @remarks When in cluster mode, `source` and `destination` must map to the same hash slot.
* @param source - The key to the source value.
* @param destination - The key where the value should be copied to.
* @param replace - If the destination key should be removed before copying the value to it.
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
* @returns `true` if `source` was copied, `false` if the `source` was not copied.
*
* @example
* ```typescript
* const result = await client.copy("set1", "set2", true);
* console.log(result); // Output: true - "set1" was copied to "set2".
* ```
*/
public async copy(
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
source: string,
destination: string,
replace?: boolean,
): Promise<boolean> {
return this.createWritePromise(
createCopy(source, destination, undefined, replace),
);
}

/**
* Invokes a previously loaded function.
*
Expand Down
23 changes: 23 additions & 0 deletions node/src/Commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1982,6 +1982,29 @@ export function createFlushDB(mode?: FlushMode): command_request.Command {
}
}

/**
*
* @internal
*/
export function createCopy(
source: string,
destination: string,
destinationDB?: number,
replace?: boolean,
): command_request.Command {
let args: string[] = [source, destination];

if (destinationDB) {
args = args.concat("DB", destinationDB.toString());
}
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved

if (replace) {
args.push("REPLACE");
}

return createCommand(RequestType.Copy, args);
}

/**
* Optional arguments to LPOS command.
*
Expand Down
32 changes: 32 additions & 0 deletions node/src/GlideClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
createConfigResetStat,
createConfigRewrite,
createConfigSet,
createCopy,
createCustomCommand,
createDBSize,
createEcho,
Expand Down Expand Up @@ -371,6 +372,37 @@ export class GlideClient extends BaseClient {
return this.createWritePromise(createTime());
}

/**
* Copies the value stored at the `source` to the `destination` key on `destinationDB`. When `replace`
* is true, removes the `destination` key first if it already exists, otherwise performs no action.
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
*
* See https://valkey.io/commands/copy/ for more details.
*
* since Valkey version 6.2.0.
*
* @param source - The key to the source value.
* @param destination - The key where the value should be copied to.
* @param destinationDB - The alternative logical database index for the destination key.
* @param replace - If the destination key should be removed before copying the value to it.
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
* @returns `true` if `source` was copied, `false` if the `source` was not copied.
*
* @example
* ```typescript
* const result = await client.copy("set1", "set2", 1, false);
* console.log(result); // Output: true - "set1" was copied to "set2".
* ```
*/
public async copyDB(
aaron-congo marked this conversation as resolved.
Show resolved Hide resolved
source: string,
destination: string,
destinationDB?: number,
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
replace?: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't remember what we decided about having multiple options, which forces the user to supply undefined for destinationDB if they want to supply replace but not destinationDB. Did we decide to use an options type in these scenarios?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since destinationDB is only visible to one of the mode, I prefer to keep the design simple and not using options type.

yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
): Promise<boolean> {
return this.createWritePromise(
createCopy(source, destination, destinationDB, replace),
);
}

/**
* Displays a piece of generative computer art and the server version.
*
Expand Down
27 changes: 27 additions & 0 deletions node/src/Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
createConfigResetStat,
createConfigRewrite,
createConfigSet,
createCopy,
createCustomCommand,
createDBSize,
createDecr,
Expand Down Expand Up @@ -1951,6 +1952,32 @@ export class BaseTransaction<T extends BaseTransaction<T>> {
return this.addAndReturn(createObjectRefcount(key));
}

/**
* Copies the value stored at the `source` to the `destination` key on `destinationDB`. When `replace`
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
* is true, removes the `destination` key first if it already exists, otherwise performs no action.
*
* See https://valkey.io/commands/copy/ for more details.
*
* since Valkey version 6.2.0.
*
* @param source - The key to the source value.
* @param destination - The key where the value should be copied to.
* @param destinationDB - The alternative logical database index for the destination key.
* @param replace - If the destination key should be removed before copying the value to it.
*
* Command Response - `true` if `source` was copied, `false` if the `source` was not copied.
*/
public copy(
source: string,
destination: string,
destinationDB?: number, // ychen - should I remove this since copy with DB doesn't work under cluster mode
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
replace?: boolean,
): T {
return this.addAndReturn(
createCopy(source, destination, destinationDB, replace),
);
}

/**
* Displays a piece of generative computer art and the server version.
*
Expand Down
86 changes: 86 additions & 0 deletions node/tests/RedisClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,92 @@ describe("GlideClient", () => {
TIMEOUT,
);

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
"copy with DB test_%p",
async (protocol) => {
if (cluster.checkIfServerVersionLessThan("6.2.0")) return;

const client = await GlideClient.createClient(
getClientConfigurationOption(cluster.getAddresses(), protocol),
);

const source = `{key}-${uuidv4()}`;
const destination = `{key}-${uuidv4()}`;
const value1 = uuidv4();
const value2 = uuidv4();
const index0 = 0;
const index1 = 1;
const index2 = 2;

try {
// neither key exists
checkSimple(
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
await client.copyDB(source, destination, index1, false),
).toEqual(false);

// source exists, destination does not
expect(await client.set(source, value1)).toEqual("OK");
checkSimple(
await client.copyDB(source, destination, index1, false),
).toEqual(true);
expect(await client.select(index1)).toEqual("OK");
checkSimple(await client.get(destination)).toEqual(value1);

// new value for source key
expect(await client.select(index0)).toEqual("OK");
expect(await client.set(source, value2)).toEqual("OK");

// no REPLACE, copying to existing key on DB 0 & 1, non-existing key on DB 2
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
checkSimple(
await client.copyDB(source, destination, index1, false),
).toEqual(false);
checkSimple(
await client.copyDB(source, destination, index2, false),
).toEqual(true);

// new value only gets copied to DB 2
expect(await client.select(index1)).toEqual("OK");
checkSimple(await client.get(destination)).toEqual(value1);
expect(await client.select(index2)).toEqual("OK");
checkSimple(await client.get(destination)).toEqual(value2);

// both exists, with REPLACE, when value isn't the same, source always get copied to
// destination
expect(await client.select(index0)).toEqual("OK");
checkSimple(
await client.copyDB(source, destination, index1, true),
).toEqual(true);
expect(await client.select(index1)).toEqual("OK");
checkSimple(await client.get(destination)).toEqual(value2);
} finally {
// switching back to db 0
await client.select(index0);
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
}

// transaction tests
try {
const transaction = new Transaction();
transaction.select(index1);
transaction.set(source, value1);
transaction.copy(source, destination, index1, true);
transaction.get(destination);
const results = await client.exec(transaction);

if (results != null) {
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
checkSimple(results.length).toEqual(4);
checkSimple(results[2]).toEqual(true);
checkSimple(results[3]).toEqual(value1);
}
} finally {
// switching back to db 0
await client.select(index0);
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
}

client.close();
},
TIMEOUT,
);

yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
"function load test_%p",
async (protocol) => {
Expand Down
50 changes: 49 additions & 1 deletion node/tests/SharedTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ import {
FlushMode,
GeoUnit,
GeospatialData,
ListDirection,
GlideClient,
GlideClusterClient,
InfoOptions,
InsertPosition,
ListDirection,
ProtocolVersion,
RequestError,
ScoreFilter,
Expand Down Expand Up @@ -4482,6 +4482,54 @@ export function runBaseTests<Context>(config: {
config.timeout,
);

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
"copy test_%p",
async (protocol) => {
await runTest(async (client: BaseClient, cluster: RedisCluster) => {
if (cluster.checkIfServerVersionLessThan("6.2.0")) return;

const source = `{key}-${uuidv4()}`;
const destination = `{key}-${uuidv4()}`;
const value1 = uuidv4();
const value2 = uuidv4();

// neither key exists
checkSimple(
yipin-chen marked this conversation as resolved.
Show resolved Hide resolved
await client.copy(source, destination, false),
).toEqual(false);
checkSimple(await client.copy(source, destination)).toEqual(
false,
);

// source exists, destination does not
expect(await client.set(source, value1)).toEqual("OK");
checkSimple(
await client.copy(source, destination, false),
).toEqual(true);
checkSimple(await client.get(destination)).toEqual(value1);

// new value for source key
expect(await client.set(source, value2)).toEqual("OK");

// both exists, no REPLACE
checkSimple(await client.copy(source, destination)).toEqual(
false,
);
checkSimple(
await client.copy(source, destination, false),
).toEqual(false);
checkSimple(await client.get(destination)).toEqual(value1);

// both exists, with REPLACE
checkSimple(
await client.copy(source, destination, true),
).toEqual(true);
checkSimple(await client.get(destination)).toEqual(value2);
}, protocol);
},
config.timeout,
);

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
`flushall test_%p`,
async (protocol) => {
Expand Down
8 changes: 7 additions & 1 deletion node/tests/TestUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
import { beforeAll, expect } from "@jest/globals";
import { exec } from "child_process";
import parseArgs from "minimist";
import { v4 as uuidv4 } from "uuid";
import { gte } from "semver";
import { v4 as uuidv4 } from "uuid";
import {
BaseClient,
BaseClientConfiguration,
Expand Down Expand Up @@ -717,6 +717,12 @@ export async function transactionTest(
'xtrim(key9, { method: "minid", threshold: "0-2", exact: true }',
1,
]);

if (gte("6.2.0", version)) {
baseTransaction.copy(key9, key10, undefined, true);
responseData.push(["copy(key9, key10, undefined, true)", true]);
}

baseTransaction.rename(key9, key10);
responseData.push(["rename(key9, key10)", "OK"]);
baseTransaction.exists([key10]);
Expand Down
Loading