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

Simplify cache namespace and key encoding logic for v1 #669

Closed
padraic-shafer opened this issue Feb 18, 2023 · 4 comments
Closed

Simplify cache namespace and key encoding logic for v1 #669

padraic-shafer opened this issue Feb 18, 2023 · 4 comments

Comments

@padraic-shafer
Copy link
Contributor

Originally posted by @Dreamsorcerer in #627 (comment)
...create a PR which reverts all the extra namespace conversions in here and does it something like this:

def __init__(self, key_builder):
    self._build_key = key_builder or lambda k, ns: "{}{}".format(ns or "", k)

def build_key(self, key, namespace=None):
    return self._build_key(key, namespace or self.namespace)

Moving the namespace logic into a single function (BaseCache.buildkey() rather than BaseCache.add(), get() ,etc.) makes a lot of sense.

This change causes tests/ut/test_base.py::TestBaseCache::test_alt_build_key_override_namespace[None-key] to fail: (AssertionError: assert 'test:key' == 'key'). This makes sense because the namespace gets overridden by self.namespace before the custom key_builder gets called. The strange part is that I think this test should have already failed before I made these changes.

I'll dig into that and the knock-on impacts to backends and decorators in a few days.

@padraic-shafer
Copy link
Contributor Author

This change causes tests/ut/test_base.py::TestBaseCache::test_alt_build_key_override_namespace[None-key] to fail: (AssertionError: assert 'test:key' == 'key'). This makes sense because the namespace gets overridden by self.namespace before the custom key_builder gets called. The strange part is that I think this test should have already failed before I made these changes.

Oh, right. That's because this test is making a bare call to build_key(), rather than going through one of the other methods (add/set/get/...). In the main branch, a custom key_builder has the ability to (optionally) override the namespace logic (including default value) of the cache. The changes in this new branch put that control back to the cache, so that the only way to override the cache's default namespace is to provide an explicit namespace when calling BaseCache.build_key().

My hunch is that this will break the ability to override the default namespace in a cache decorator that is backed by an alias cache, and have a similar effect on other classes that don't explicitly accept namespace arguments (like RedLock). Currently I have a project that relies on being able to override the namespace in this scenario.

I'll need to look into how best to resolve this.

@padraic-shafer
Copy link
Contributor Author

My hunch is that this will break the ability to override the default namespace in a cache decorator that is backed by an alias cache, and have a similar effect on other classes that don't explicitly accept namespace arguments (like RedLock). Currently I have a project that relies on being able to override the namespace in this scenario.

After preliminary review, I think it will be ok to proceed as started in this branch.

The other project I mentioned subclasses the cached decorator class to create a new alias cache on the fly with the desired custom BaseCache.build_key() override. So this works with these new changes.

Regarding the RedLock and backend implementations, even where namespace is not explicitly provided, I think it makes sense to fall back on the cache's namespace member. Otherwise it would probably be better to use a different cache object for those edge cases where a different namespace is needed.


This change causes tests/ut/test_base.py::TestBaseCache::test_alt_build_key_override_namespace[None-key] to fail: (AssertionError: assert 'test:key' == 'key').

I'm inclined to modify this test to remove this pathological behavior, where a custom build_key uses a default namespace that does not match the cache's default namespace (i.e., the value of its namespace member).


TL;DR I'm moving forward with the originally suggested changes.

@padraic-shafer padraic-shafer changed the title Simplify cache + decorator namespace logic for v1 Simplify cache namespace and key encoding logic for v1 Feb 19, 2023
@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Feb 20, 2023

Yes, I think we want to have the default namespace, so changing this behaviour seems fine. The test should still pass in the 3rd paramater, where a non-empty namespace is used.

We could also consider checking for None for the default namespace and allowing an empty namespace with "", if there's still some desired behaviour for that.

@Dreamsorcerer
Copy link
Member

#670

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

No branches or pull requests

2 participants