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

Ambiguous behavior of clear method with no namespace in Redis adapter #1222

Closed
mahdavipanah opened this issue Dec 2, 2024 · 3 comments
Closed
Labels

Comments

@mahdavipanah
Copy link
Contributor

Description
When reading through the test cases for the Redis adapter I came across this test case (on this line) that says:

test('should clear with no namespace but not the namespace ones', async () => {
    const keyvRedis = new KeyvRedis();
    const client = await keyvRedis.getClient() as RedisClientType;
    await client.flushDb();
    keyvRedis.namespace = 'ns1';
    await keyvRedis.set('foo91', 'bar');
    keyvRedis.namespace = undefined;
    await keyvRedis.set('foo912', 'bar2');
    await keyvRedis.set('foo913', 'bar3');
    await keyvRedis.clear();
    keyvRedis.namespace = 'ns1';
    const value = await keyvRedis.get('foo91');
    expect(value).toBe('bar');
    await keyvRedis.disconnect();
});

which matches the implementation meaning that if the namespace is not set the clear method will not remove any keys with the keyPrefixSeparator in it. This looks very surprising as it's not mentioned anywhere in the README. Actually, there is this description in the README that to me indicates a totally different behavior, which is that the clear method will remove all keys if no namespace is set:

clear - Clear all keys. If the namespace is set it will only clear keys with that namespace.

Proposal
Options:

  1. Change the behavior of the clear method so it removes all keys (basically a FLUSHDB) when no namespace is set
  2. Update the README file so it makes this behavior very clear

I am in favor of option 1 for these reasons:

  1. It is more intuitive to think that when no namespace is set, all the defined keys in the DB are in the space of the Keyv instance
  2. It makes the clear method useful in certain production situations, but currently due to its logic it's very risky to use it in production
@jaredwray
Copy link
Owner

@mahdavipanah - the issue that we have is the lack of native namespace support in redis without using something like Redis Sets which causes major performance issues. I see both scenarios that we should support:

  • if you are using multiple namespaces but then switch to undefined it should only clear the keys that are not appended with a namespace -- this is currently supported but the documentation needs to be updated.
  • if you want to do a complete clear (flushDb()) by setting the namespace to undefined then we should add in a option such as flushDbOnClear which you can set it and the namespace is undefined it will do a full clear.

Thoughts?

@mahdavipanah
Copy link
Contributor Author

  1. Updating the documentation is a good idea. I think it's also important to add a note to the documentation warning the reader not to use the key prefix separator in the key because it can mess up the clear and iterator logic. Or maybe we can even add logic to check this in the adapter, although I'm not sure.
  2. I think we should add an option noNamespaceAffectsAll that can be used in both clear and iterator methods.

Let me know what you think.

@jaredwray
Copy link
Owner

@mahdavipanah - I am going to update the document and then have scheduled time to add in the option in the future. Will close this now since the docs are updated.

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

No branches or pull requests

2 participants