Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Add cache_prefix setting (fixes #227) #680

Merged
merged 13 commits into from
Mar 12, 2016

Conversation

lavish205
Copy link
Contributor

issue #227

@lavish205
Copy link
Contributor Author

@leplatrem I'm getting cache_prefix KeyError as config.get_settings() is not returning settings defined in DEFAULT_SETTINGS

@leplatrem
Copy link
Contributor

It looks perfectly correct.

But when test are ran:

$ nosetests -s cliquet.tests.test_cache

I also get this error:

Traceback (most recent call last):
  File "/home/mathieu/Code/Mozilla/cliquet/cliquet/tests/test_cache.py", line 40, in setUp
    self.cache = self.backend.load_from_config(self._get_config())
  File "/home/mathieu/Code/Mozilla/cliquet/cliquet/cache/memory.py", line 55, in load_from_config
    prefix = settings['cache_prefix']
KeyError: 'cache_prefix'

That's because the tests setup do not use DEFAULT_SETTINGS.
See, you should add something like this on MemoryCacheTest https://github.com/mozilla-services/cliquet/blob/master/cliquet/tests/test_cache.py#L162-L165

@lavish205
Copy link
Contributor Author

@leplatrem I have fixed this KeyError by .get() method, hope this works

@leplatrem
Copy link
Contributor

hope this works.

Nope actually. You should leave ['cache_prefix'] and set its value in the tests like I pointed out.

Here is a small list of things to do before this could be merged:

  • Generalize the change to other backends (redis and postgresql)
  • Add some tests in test_cache.py to ensure that the prefix value is used to get the data associated to a key
  • Add some tests in test_cache.py to ensure that the prefix value is used to set the data associated to a key
  • Add some tests in test_cache.py to ensure that the prefix value is used to delete the data associated to a key
  • Add some tests in test_cache.py to ensure that the prefix value is used to ttl the data associated to a key
  • Add some tests in test_cache.py to ensure that the prefix value is used to expire the data associated to a key
  • Add a test to ensure that the default prefix value is empty
  • Update configuration documentation
  • Update changelog
  • Add yourself to contributors

@leplatrem leplatrem changed the title cache_prefix key added [WIP] Add cache_prefix setting (fixes #227) Mar 9, 2016
@leplatrem
Copy link
Contributor

@lavish205 you could do something like that (I didn't test it):

def test_get_uses_prefix(self):
    settings_prefix = self.settings.copy()
    settings_prefix['cache_prefix'] = 'prefix_'
    config_prefix = self._get_config(settings=settings_prefix)

    # Instantiate a cache backend with a prefix:
    backend_prefix = self.backend.load_from_config(config_prefix)

    # Set a value with a cache that has no prefix:
    self.cache.set('prefix_key', 'foo')

    # Obtain the value with the cache that has a prefix:
    obtained = backend_prefix.get('key')
    self.assertEqual(obtained, 'foo')

@lavish205
Copy link
Contributor Author

@leplatrem I think something is wrong with the code as test cases are not working as expected.

@leplatrem
Copy link
Contributor

@lavish205 yes, first you shouldn't set the prefix to 123, so both self.cache and backend_prefix have a prefix.

Then, in order to make tests pass, you should use the self.prefix in def get() in the Cache backend class.

@lavish205
Copy link
Contributor Author

@leplatrem value of backend_prefix is {'prefix': 'prefix_', '_store': {'prefix_key': 'bar'}, '_ttl': {}, so can't access prefix_key on root of dict.

backend_prefix.set('prefix_key', 'foo')

# obtain the value of cache that has prefix
obtained = backend_prefix.get('prefix_key')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be get('key') since this backend has prefix prefix_ !

@lavish205 lavish205 force-pushed the issue_227 branch 6 times, most recently from 40c3ffd to 4c7d7b9 Compare March 10, 2016 12:50
@lavish205
Copy link
Contributor Author

@leplatrem can you please check, if there is need to add more test cases?

@@ -25,20 +26,24 @@ def flush(self):
self._store = {}

def ttl(self, key):
key = self.prefix + key
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather use format here: key = "{}{}".format(self.prefix, key) or key = "%s%s" % (self.prefix, key)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why ?

@Natim
Copy link
Contributor

Natim commented Mar 10, 2016

That's a great job @lavish205 👍 Thank you for working on it :)

@lavish205
Copy link
Contributor Author

@leplatrem what to update in configuration documentation?

@Natim
Copy link
Contributor

Natim commented Mar 11, 2016

@lavish205 I believe it is just about adding the cache_prefix configuration settings as you told me on IRC.

13:52:43 <lavish_> do I need to add prefix changes here?
http://kinto.readthedocs.org/en/latest/configuration/settings.html#cache

@Natim Natim changed the title [WIP] Add cache_prefix setting (fixes #227) Add cache_prefix setting (fixes #227) Mar 11, 2016
@lavish205
Copy link
Contributor Author

@Natim what exactly I have write under "What does it do?" column ?

@Natim
Copy link
Contributor

Natim commented Mar 11, 2016

@lavish205 Something like: It adds a prefix to cache key names so that multiple kinto server can use the same cache database without disturbing each other.

@@ -10,6 +10,7 @@ This document describes changes between each past release.
**New features**

- Default console log renderer now has colours (#671)
- ``Prefix`` key option added for cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

The setting is called cache_prefix no?

@Natim
Copy link
Contributor

Natim commented Mar 12, 2016

I have added some documentation and a test to make sure cache.get added the prefix.

Thank you @lavish205 for your work on this.

Natim added a commit that referenced this pull request Mar 12, 2016
Add cache_prefix setting (fixes #227)
@Natim Natim merged commit 8cabe77 into mozilla-services:master Mar 12, 2016
@Natim Natim deleted the issue_227 branch March 12, 2016 15:27
@leplatrem
Copy link
Contributor

Congrats @lavish205 for keeping the faith and going through all this!

Thank you for getting involved with patience !

@lavish205
Copy link
Contributor Author

Thanks to both of you @leplatrem @Natim :)

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

Successfully merging this pull request may close these issues.

3 participants