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

Duplicated namespace in redis keys and issues when using a single store for different caches. #1142

Closed
leo-fresha opened this issue Oct 1, 2024 · 2 comments

Comments

@leo-fresha
Copy link

leo-fresha commented Oct 1, 2024

Bug description

Every newly instantiated Keyv cache sets the namespace attribute of the underlying store, which has two implications (at least when using the KeyvRedis backend):

  • the namespace is duplicated. Redis keys have names like sets:namespace:<namespace>:<namespace>:<key> instead of sets:namespace:<namespace>:<key> (I wonder if we need such a long prefix at all, vs just <prefix>:<namespace>:<key>, where <prefix> can be configured by the user when instantianing the store, but that's a different story...)
  • it's not possible to safely use the same store for multiple caches, as instantiating a new cache modifies the namespace of the underlying store, which is used to retrieve keys, thus breaking previous stored entries.

How to reproduce

Given this snippet which instantiates two Keyv caches which use the same store (which, in turn, share the same pre-existing Redis connection).

import KeyvRedis from '@keyv/redis';
import Redis from 'ioredis';
import Keyv from 'keyv';

const redisClient = new Redis(process.env.REDIS_URI);
const redisStore = new KeyvRedis(redisClient, { useRedisSets: false });

const KEY = 'jane';

const addressCache = new Keyv({
  store: redisStore,
  namespace: 'addresses',
  ttl: 24 * 60 * 60, // 1 day
});
await addressCache.set(KEY, 'Somewhere in London');

const professionCache = new Keyv({
  store: redisStore,
  namespace: 'professions',
  ttl: 10 * 24 * 60 * 60, // 10 days
});
await professionCache.set(KEY, 'Doctor');

console.log(await addressCache.get(KEY));
console.log(await professionCache.get(KEY));

await redisClient.disconnect();

Running the script you'll get

undefined
'Doctor'

Instead of the expected:

'Somewhere in London'
'Doctor'

And this is the output of redis MONITOR command while running the script:

1727813797.264412 [0 172.23.0.1:52014] "info"
1727813797.267410 [0 172.23.0.1:52014] "set" "sets:namespace:addresses:addresses:jane" "{\"value\":\"Somewhere in London\",\"expires\":1727813883685}" "PX" "86400"
1727813797.270006 [0 172.23.0.1:52014] "set" "sets:namespace:professions:professions:jane" "{\"value\":\"Doctor\",\"expires\":1727814661298}" "PX" "864000"
1727813797.271760 [0 172.23.0.1:52014] "get" "sets:namespace:professions:addresses:jane"
1727813797.275019 [0 172.23.0.1:52014] "get" "sets:namespace:professions:professions:jane"

You can see the duplicated namespace and the full key for the address cache entries changing after the professions cache is instantiated.

Suggested resolution

Keyv should not set the namespace of the underlying store at all. Store namespace (or, better, prefix) should be optional and configurable by the user at the store level. This would result in much cleaner redis keys like <store-prefix>:<key cache namespace>:<key>. It would also allow users to configure short prefixes/namespaces to save memory, which can be quite important for redis and other stores where key size is consequential.

@leo-fresha leo-fresha added the bug label Oct 1, 2024
@jaredwray
Copy link
Owner

@leo-fresha thanks for posting this and we are working on an update around this to make it better.

@jaredwray
Copy link
Owner

@leo-fresha - we are working on the rewrite now. I am going to close this issue and add it as an item to resolve on the main thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants