Skip to content

Commit

Permalink
Node binding: keep strings as binary data (#1563)
Browse files Browse the repository at this point in the history 
* Node binding: string values returning from Rust core, are preserved as binary data
Node: tests are fixed to handle binary strings returned from Rust core

* Fixed linter issues

* More linter fixes

* When converting Set into Array, sort the items

* stlye fixes
  • Loading branch information
@eifrah-aws
eifrah-aws committed Jun 16, 2024
1 parent 4ef40e1 commit 7df2498
Show file tree
Hide file tree
Showing 7 changed files with 418 additions and 287 deletions.
9 changes: 4 additions & 5 deletions node/rust-client/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,7 @@ fn redis_value_to_js(val: Value, js_env: Env) -> Result<JsUnknown> {
.map(|val| val.into_unknown()),
Value::Okay => js_env.create_string("OK").map(|val| val.into_unknown()),
Value::Int(num) => js_env.create_int64(num).map(|val| val.into_unknown()),
Value::BulkString(data) => {
let str = to_js_result(std::str::from_utf8(data.as_ref()))?;
js_env.create_string(str).map(|val| val.into_unknown())
}
Value::BulkString(data) => Ok(js_env.create_buffer_with_data(data)?.into_unknown()),
Value::Array(array) => {
let mut js_array_view = js_env.create_array_with_length(array.len())?;
for (index, item) in array.into_iter().enumerate() {
Expand Down Expand Up @@ -224,7 +221,9 @@ fn redis_value_to_js(val: Value, js_env: Env) -> Result<JsUnknown> {
}
}

#[napi(ts_return_type = "null | string | number | {} | Boolean | BigInt | Set<any> | any[]")]
#[napi(
ts_return_type = "null | string | Uint8Array | number | {} | Boolean | BigInt | Set<any> | any[]"
)]
pub fn value_from_split_pointer(js_env: Env, high_bits: u32, low_bits: u32) -> Result<JsUnknown> {
let mut bytes = [0_u8; 8];
(&mut bytes[..4])
Expand Down
4 changes: 2 additions & 2 deletions node/src/BaseClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,8 @@ export class BaseClient {
* ```
*/
public set(
key: string,
value: string,
key: string | Uint8Array,
value: string | Uint8Array,
options?: SetOptions,
): Promise<"OK" | string | null> {
return this.createWritePromise(createSet(key, value, options));
Expand Down
20 changes: 13 additions & 7 deletions node/src/Commands.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { redis_request } from "./ProtobufMessage";

import RequestType = redis_request.RequestType;

function isLargeCommand(args: string[]) {
function isLargeCommand(args: BulkString[]) {
let lenSum = 0;

for (const arg of args) {
Expand All @@ -23,14 +23,20 @@ function isLargeCommand(args: string[]) {
return false;
}

type BulkString = string | Uint8Array;

/**
* Convert a string array into Uint8Array[]
*/
function toBuffersArray(args: string[]) {
function toBuffersArray(args: BulkString[]) {
const argsBytes: Uint8Array[] = [];

for (const str of args) {
argsBytes.push(Buffer.from(str));
for (const arg of args) {
if (typeof arg == "string") {
argsBytes.push(Buffer.from(arg));
} else {
argsBytes.push(arg);
}
}

return argsBytes;
Expand All @@ -56,7 +62,7 @@ export function parseInfoResponse(response: string): Record<string, string> {

function createCommand(
requestType: redis_request.RequestType,
args: string[],
args: BulkString[],
): redis_request.Command {
const singleCommand = redis_request.Command.create({
requestType,
Expand Down Expand Up @@ -137,8 +143,8 @@ export type SetOptions = {
* @internal
*/
export function createSet(
key: string,
value: string,
key: BulkString,
value: BulkString,
options?: SetOptions,
): redis_request.Command {
const args = [key, value];
Expand Down
24 changes: 15 additions & 9 deletions node/tests/RedisClient.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import {
parseCommandLineArgs,
parseEndpoints,
transactionTest,
intoString,
checkSimple,
} from "./TestUtilities";

/* eslint-disable @typescript-eslint/no-var-requires */
Expand Down Expand Up @@ -108,9 +110,13 @@ describe("RedisClient", () => {
getClientConfigurationOption(cluster.getAddresses(), protocol),
);
const result = await client.info();
expect(result).toEqual(expect.stringContaining("# Server"));
expect(result).toEqual(expect.stringContaining("# Replication"));
expect(result).toEqual(
expect(intoString(result)).toEqual(
expect.stringContaining("# Server"),
);
expect(intoString(result)).toEqual(
expect.stringContaining("# Replication"),
);
expect(intoString(result)).toEqual(
expect.not.stringContaining("# Latencystats"),
);
},
Expand All @@ -123,20 +129,20 @@ describe("RedisClient", () => {
getClientConfigurationOption(cluster.getAddresses(), protocol),
);
let selectResult = await client.select(0);
expect(selectResult).toEqual("OK");
checkSimple(selectResult).toEqual("OK");

const key = uuidv4();
const value = uuidv4();
const result = await client.set(key, value);
expect(result).toEqual("OK");
checkSimple(result).toEqual("OK");

selectResult = await client.select(1);
expect(selectResult).toEqual("OK");
checkSimple(selectResult).toEqual("OK");
expect(await client.get(key)).toEqual(null);

selectResult = await client.select(0);
expect(selectResult).toEqual("OK");
expect(await client.get(key)).toEqual(value);
checkSimple(selectResult).toEqual("OK");
checkSimple(await client.get(key)).toEqual(value);
},
);

Expand All @@ -151,7 +157,7 @@ describe("RedisClient", () => {
transaction.select(0);
const result = await client.exec(transaction);
expectedRes.push("OK");
expect(result).toEqual(expectedRes);
expect(intoString(result)).toEqual(intoString(expectedRes));
},
);

Expand Down
85 changes: 47 additions & 38 deletions node/tests/RedisClusterClient.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import {
parseCommandLineArgs,
parseEndpoints,
transactionTest,
intoString,
intoArray,
} from "./TestUtilities";
type Context = {
client: RedisClusterClient;
Expand Down Expand Up @@ -92,24 +94,20 @@ describe("RedisClusterClient", () => {
const info_server = getFirstResult(
await client.info([InfoOptions.Server]),
);
expect(info_server).toEqual(expect.stringContaining("# Server"));

const result = (await client.info([
InfoOptions.Replication,
])) as Record<string, string>;
const clusterNodes = await client.customCommand([
"CLUSTER",
"NODES",
]);
expect(
(clusterNodes as string)?.split("master").length - 1,
).toEqual(Object.keys(result).length);
Object.values(result).every((item) => {
expect(item).toEqual(expect.stringContaining("# Replication"));
expect(item).toEqual(
expect.not.stringContaining("# Errorstats"),
);
});
expect(intoString(info_server)).toEqual(
expect.stringContaining("# Server"),
);

const infoReplicationValues = Object.values(
await client.info([InfoOptions.Replication]),
);

const replicationInfo = intoArray(infoReplicationValues);

for (const item of replicationInfo) {
expect(item).toContain("role:master");
expect(item).toContain("# Replication");
}
},
TIMEOUT,
);
Expand All @@ -124,9 +122,12 @@ describe("RedisClusterClient", () => {
[InfoOptions.Server],
"randomNode",
);
expect(typeof result).toEqual("string");
expect(result).toEqual(expect.stringContaining("# Server"));
expect(result).toEqual(expect.not.stringContaining("# Errorstats"));
expect(intoString(result)).toEqual(
expect.stringContaining("# Server"),
);
expect(intoString(result)).toEqual(
expect.not.stringContaining("# Errorstats"),
);
},
TIMEOUT,
);
Expand All @@ -148,10 +149,12 @@ describe("RedisClusterClient", () => {
getClientConfigurationOption(cluster.getAddresses(), protocol),
);
const result = cleanResult(
(await client.customCommand(
["cluster", "nodes"],
"randomNode",
)) as string,
intoString(
await client.customCommand(
["cluster", "nodes"],
"randomNode",
),
),
);

// check that routing without explicit port works
Expand All @@ -162,10 +165,12 @@ describe("RedisClusterClient", () => {
}

const secondResult = cleanResult(
(await client.customCommand(["cluster", "nodes"], {
type: "routeByAddress",
host,
})) as string,
intoString(
await client.customCommand(["cluster", "nodes"], {
type: "routeByAddress",
host,
}),
),
);

expect(result).toEqual(secondResult);
Expand All @@ -174,11 +179,13 @@ describe("RedisClusterClient", () => {

// check that routing with explicit port works
const thirdResult = cleanResult(
(await client.customCommand(["cluster", "nodes"], {
type: "routeByAddress",
host: host2,
port: Number(port),
})) as string,
intoString(
await client.customCommand(["cluster", "nodes"], {
type: "routeByAddress",
host: host2,
port: Number(port),
}),
),
);

expect(result).toEqual(thirdResult);
Expand Down Expand Up @@ -212,7 +219,9 @@ describe("RedisClusterClient", () => {
transaction.configSet({ timeout: "1000" });
transaction.configGet(["timeout"]);
const result = await client.exec(transaction);
expect(result).toEqual(["OK", { timeout: "1000" }]);
expect(intoString(result)).toEqual(
intoString(["OK", { timeout: "1000" }]),
);
},
TIMEOUT,
);
Expand All @@ -226,7 +235,7 @@ describe("RedisClusterClient", () => {
const transaction = new ClusterTransaction();
const expectedRes = await transactionTest(transaction);
const result = await client.exec(transaction);
expect(result).toEqual(expectedRes);
expect(intoString(result)).toEqual(intoString(expectedRes));
},
TIMEOUT,
);
Expand Down Expand Up @@ -267,8 +276,8 @@ describe("RedisClusterClient", () => {
const echoDict = await client.echo(message, "allNodes");

expect(typeof echoDict).toBe("object");
expect(Object.values(echoDict)).toEqual(
expect.arrayContaining([message]),
expect(intoArray(echoDict)).toEqual(
expect.arrayContaining(intoArray([message])),
);
},
TIMEOUT,
Expand Down
Loading

0 comments on commit 7df2498

Please sign in to comment.