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 3 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
46 changes: 45 additions & 1 deletion src/drivers/redis.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { defineDriver, joinKeys } from "./utils";
import type { TransactionOptions } from "../types";
import { defineDriver, joinKeys, createError } from "./utils";
// TODO: use named import in v2
import Redis, {
Cluster,
Expand Down Expand Up @@ -32,6 +33,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 +74,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 +89,36 @@
await getRedisClient().set(p(key), value);
}
},
setItemRaw:
opts.raw === true
? async (
key: string,
value: Uint8Array,
tOptions: TransactionOptions
) => {
let valueToSave: Buffer;
if (value instanceof Uint8Array) {
if (value instanceof Buffer) {
valueToSave = value;
} else {
valueToSave = Buffer.copyBytesFrom(
value,
value.byteOffset,
value.byteLength
);
}
} else {
throw createError(DRIVER_NAME, "Expected Buffer or Uint8Array");
}

const ttl = tOptions?.ttl ?? opts.ttl;
if (ttl) {
await getRedisClient().set(p(key), valueToSave, "EX", ttl);

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

View check run for this annotation

Codecov / codecov/patch

src/drivers/redis.ts#L116

Added line #L116 was not covered by tests
} else {
await getRedisClient().set(p(key), valueToSave);
}
}
: undefined,
async removeItem(key) {
await getRedisClient().del(p(key));
},
Expand Down
72 changes: 71 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,79 @@ 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();
});

it("expects binary data to be sent to setItemRaw", async () => {
expect(() =>
ctx.storage.setItemRaw("s4:a", "Hello, world!")
).rejects.toThrow("Expected Buffer or Uint8Array");
expect(() => ctx.storage.setItemRaw("s5:a", 100)).rejects.toThrow(
"Expected Buffer or Uint8Array"
);
expect(() =>
ctx.storage.setItemRaw("s5:a", { foo: "bar" })
).rejects.toThrow("Expected Buffer or Uint8Array");
});
},
});
});
Loading