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

feat(redis): opt-in raw support #561

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
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
3 changes: 3 additions & 0 deletions docs/2.drivers/redis.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import redisDriver from "unstorage/drivers/redis";

const storage = createStorage({
driver: redisDriver({
raw: true,
base: "unstorage",
host: 'HOSTNAME',
tls: true as any,
Expand All @@ -46,6 +47,7 @@ Usage with Redis cluster (e.g. AWS ElastiCache or Azure Redis Cache):
```js
const storage = createStorage({
driver: redisDriver({
raw: true,
base: "{unstorage}",
cluster: [
{
Expand All @@ -70,6 +72,7 @@ const storage = createStorage({
- `cluster`: List of redis nodes to use for cluster mode. Takes precedence over `url` and `host` options.
- `clusterOptions`: Options to use for cluster mode.
- `ttl`: Default TTL for all items in **seconds**.
- `raw`: If enabled, `getItemRaw` and `setItemRaw` will use binary data instead of base64 encoded strings. (this option will be enabled by default in the next major version.)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest something like binary/useBinary here. I think it makes the effect a bit more clear because it changes how raw data is stored, but does not enable/disable the ability to save raw data. Or it could be inverted to base64 so in the future the default value can be false.

Copy link
Member

@pi0 pi0 Jan 3, 2025

Choose a reason for hiding this comment

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

I agree that in general binary could be more describing than raw. In the context of unstorage, we call binary-compatible feature raw which was the reason I was thinking to use this.

Considering it is for short-term solution, are you happy we go with raw?

(Also check fb2977a, I have extended normalization)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extending normalization to additional variants of TypedArray makes sense, but it should only allow for binary data and continue to throw when other data is passed. Otherwise it would lead to some unexpected behavior as you save an object but get a Buffer back.

That's also why I think the different naming is helpful. binary means that setItemRaw accepts binary and getItemRaw returns binary. Whereas the existing raw behavior accepts binary and other things.

Copy link
Member

@pi0 pi0 Jan 3, 2025

Choose a reason for hiding this comment

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

This is consistent with generic unstorage.*raw* interface behavior -- happy to change it in the future (so ALL calls to setItemRaw will be restricted to accept binary only not objects) but normally, users should use the same key either for raw or non-raw purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm would it be possible to add an additional option in that case? Either on the driver or the method? What I'm looking for is a way to save binary data and ensure it is binary and saved as binary and when getting, that I get a Buffer in return.

My assumption is that is how getItem(key, { type: 'bytes'})/setItem(key, value, { type: 'bytes'}) would work in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yes exactly for type: 'bytes' we can get strict.

Currently, if someone passes a string to setItemRaw, getItemRaw gives a Buffer representation of Utf8 bytes which isn't that odd ,is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what I find a bit unexpected 😅

Do you think there would be an issue if we added type: 'bytes' as a method option here? It would be just for the redis driver at the moment, but done in a forward compatible manner.

Or is it better to use something else and save that wording for later


See [ioredis](https://github.com/luin/ioredis/blob/master/API.md#new-redisport-host-options) for all available options.

Expand Down
62 changes: 62 additions & 0 deletions src/drivers/redis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@
* Default TTL for all items in seconds.
*/
ttl?: number;

/**
* If enabled, `getItemRaw` and `setItemRaw` will use binary data instead of base64 encoded strings.
* This option will be enabled by default in the next major version.
*/
raw?: boolean;
}

const DRIVER_NAME = "redis";
Expand Down Expand Up @@ -67,6 +73,13 @@
const value = await getRedisClient().get(p(key));
return value ?? null;
},
getItemRaw:
opts.raw === true
? async (key: string) => {
const value = await getRedisClient().getBuffer(p(key));
return value ?? null;
}
: undefined,
async setItem(key, value, tOptions) {
const ttl = tOptions?.ttl ?? opts.ttl;
if (ttl) {
Expand All @@ -75,6 +88,18 @@
await getRedisClient().set(p(key), value);
}
},
setItemRaw:
opts.raw === true
? async (key, value, tOptions) => {
const _value = normalizeValue(value);
const ttl = tOptions?.ttl ?? opts.ttl;
if (ttl) {
await getRedisClient().set(p(key), _value, "EX", ttl);

Check warning on line 97 in src/drivers/redis.ts

View check run for this annotation

Codecov / codecov/patch

src/drivers/redis.ts#L97

Added line #L97 was not covered by tests
} else {
await getRedisClient().set(p(key), _value);
}
}
: undefined,
async removeItem(key) {
await getRedisClient().del(p(key));
},
Expand All @@ -96,3 +121,40 @@
},
};
});

function normalizeValue(value: unknown): Buffer | string | number {
const type = typeof value;
if (type === "string" || type === "number") {
return value as string | number;
}

Check warning on line 129 in src/drivers/redis.ts

View check run for this annotation

Codecov / codecov/patch

src/drivers/redis.ts#L128-L129

Added lines #L128 - L129 were not covered by tests
if (Buffer.isBuffer(value)) {
return value;
}
if (isTypedArray(value)) {
if (Buffer.copyBytesFrom) {
return Buffer.copyBytesFrom(value, value.byteOffset, value.byteLength);
} else {
return Buffer.from(value.buffer, value.byteOffset, value.byteLength);
}

Check warning on line 138 in src/drivers/redis.ts

View check run for this annotation

Codecov / codecov/patch

src/drivers/redis.ts#L137-L138

Added lines #L137 - L138 were not covered by tests
}
if (value instanceof ArrayBuffer) {
return Buffer.from(value);
}
return JSON.stringify(value);
}

Check warning on line 144 in src/drivers/redis.ts

View check run for this annotation

Codecov / codecov/patch

src/drivers/redis.ts#L140-L144

Added lines #L140 - L144 were not covered by tests

function isTypedArray(value: unknown): value is TypedArray {
return (
value instanceof Int8Array ||
value instanceof Uint8Array ||
value instanceof Uint8ClampedArray ||
value instanceof Int16Array ||
value instanceof Uint16Array ||
value instanceof Int32Array ||
value instanceof Uint32Array ||
value instanceof Float32Array ||
value instanceof Float64Array ||
value instanceof BigInt64Array ||
value instanceof BigUint64Array

Check warning on line 158 in src/drivers/redis.ts

View check run for this annotation

Codecov / codecov/patch

src/drivers/redis.ts#L158

Added line #L158 was not covered by tests
);
}
60 changes: 59 additions & 1 deletion test/drivers/redis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { testDriver } from "./utils";

vi.mock("ioredis", () => ioredis);

describe("drivers: redis", () => {
describe("drivers: redis (raw: false)", () => {
const driver = redisDriver({
base: "test:",
url: "ioredis://localhost:6379/0",
Expand All @@ -32,9 +32,67 @@ describe("drivers: redis", () => {
await client.disconnect();
});

it("saves raw data as a base64 string", async () => {
const helloBuffer = Buffer.from("Hello, world!", "utf8");
const byteArray = new Uint8Array(4);
byteArray[0] = 2;
byteArray[1] = 0;
byteArray[2] = 2;
byteArray[3] = 5;

await ctx.storage.setItemRaw("s4:a", helloBuffer);
await ctx.storage.setItemRaw("s5:a", byteArray);

const client = new ioredis.default("ioredis://localhost:6379/0");

const bufferValue = await client.get("test:s4:a");
expect(bufferValue).toEqual("base64:SGVsbG8sIHdvcmxkIQ==");

const byteArrayValue = await client.get("test:s5:a");
expect(byteArrayValue).toEqual("base64:AgACBQ==");

await client.disconnect();
});

it("exposes instance", () => {
expect(driver.getInstance?.()).toBeInstanceOf(ioredis.default);
});
},
});
});

describe("drivers: redis (raw: true)", () => {
const binaryDriver = redisDriver({
base: "test:",
url: "ioredis://localhost:6379/0",
lazyConnect: false,
raw: true,
});

testDriver({
driver: binaryDriver,
additionalTests(ctx) {
it("saves raw data as binary", async () => {
const helloBuffer = Buffer.from("Hello, world!", "utf8");
const byteArray = new Uint8Array(4);
byteArray[0] = 2;
byteArray[1] = 0;
byteArray[2] = 2;
byteArray[3] = 5;

await ctx.storage.setItemRaw("s4:a", helloBuffer);
await ctx.storage.setItemRaw("s5:a", byteArray);

const client = new ioredis.default("ioredis://localhost:6379/0");

const bufferValue = await client.getBuffer("test:s4:a");
expect(bufferValue).toEqual(helloBuffer);

const byteArrayValue = await client.getBuffer("test:s5:a");
expect(byteArrayValue).toEqual(Buffer.from([2, 0, 2, 5]));

await client.disconnect();
});
},
});
});
Loading