From 49d60722a650d56aee2694356462d86bd640ee92 Mon Sep 17 00:00:00 2001 From: Chris Rossi Date: Fri, 21 Jun 2019 14:29:51 -0400 Subject: [PATCH] Implement cache policy. (#116) --- docs/conf.py | 1 + docs/context.rst | 7 ++ docs/index.rst | 1 + src/google/cloud/ndb/client.py | 9 ++- src/google/cloud/ndb/context.py | 121 ++++++++++++++++++++------------ src/google/cloud/ndb/key.py | 20 +++--- src/google/cloud/ndb/model.py | 5 +- tests/system/conftest.py | 4 +- tests/system/test_crud.py | 67 +++++++++++++++++- tests/unit/test_context.py | 71 ++++++++++++++++--- 10 files changed, 237 insertions(+), 69 deletions(-) create mode 100644 docs/context.rst diff --git a/docs/conf.py b/docs/conf.py index 9abab2d3..6438bc93 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -41,6 +41,7 @@ ("py:class", "google.cloud.datastore_v1.proto.entity_pb2.Entity"), ("py:class", "_datastore_query.Cursor"), ("py:meth", "_datastore_query.Cursor.urlsafe"), + ("py:class", "google.cloud.ndb.context._Context"), ("py:class", "google.cloud.ndb.metadata._BaseMetadata"), ("py:class", "google.cloud.ndb._options.ReadOptions"), ("py:class", "QueryIterator"), diff --git a/docs/context.rst b/docs/context.rst new file mode 100644 index 00000000..22135972 --- /dev/null +++ b/docs/context.rst @@ -0,0 +1,7 @@ +####### +Context +####### + +.. automodule:: google.cloud.ndb.context + :members: + :show-inheritance: diff --git a/docs/index.rst b/docs/index.rst index ba1c4416..c98c661c 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -7,6 +7,7 @@ :maxdepth: 2 client + context key model query diff --git a/src/google/cloud/ndb/client.py b/src/google/cloud/ndb/client.py index d112f06e..6e48ad65 100644 --- a/src/google/cloud/ndb/client.py +++ b/src/google/cloud/ndb/client.py @@ -92,7 +92,7 @@ def __init__(self, project=None, namespace=None, credentials=None): self.secure = not emulator @contextlib.contextmanager - def context(self): + def context(self, cache_policy=None): """Establish a context for a set of NDB calls. This method provides a context manager which establishes the runtime @@ -121,8 +121,13 @@ def context(self): In a web application, it is recommended that a single context be used per HTTP request. This can typically be accomplished in a middleware layer. + + Arguments: + cache_policy (Optional[Callable[[key.Key], bool]]): The + cache policy to use in this context. See: + :meth:`~google.cloud.ndb.context.Context.set_cache_policy`. """ - context = context_module.Context(self) + context = context_module.Context(self, cache_policy=cache_policy) with context.use(): yield context diff --git a/src/google/cloud/ndb/context.py b/src/google/cloud/ndb/context.py index 90b5dd21..756a59e0 100644 --- a/src/google/cloud/ndb/context.py +++ b/src/google/cloud/ndb/context.py @@ -21,6 +21,7 @@ from google.cloud.ndb import _datastore_api from google.cloud.ndb import _eventloop from google.cloud.ndb import exceptions +from google.cloud.ndb import model __all__ = [ @@ -32,20 +33,6 @@ ] -_ContextTuple = collections.namedtuple( - "_ContextTuple", - [ - "client", - "eventloop", - "stub", - "batches", - "commit_batches", - "transaction", - "cache", - ], -) - - class _LocalState(threading.local): """Thread local state.""" @@ -97,6 +84,41 @@ def get_and_validate(self, key): raise KeyError(key) +def _default_cache_policy(key): + """The default cache policy. + + Defers to ``_use_cache`` on the Model class for the key's kind. + + See: :meth:`~google.cloud.ndb.context.Context.set_cache_policy` + """ + flag = None + if key is not None: + modelclass = model.Model._kind_map.get(key.kind()) + if modelclass is not None: + policy = getattr(modelclass, "_use_cache", None) + if policy is not None: + if isinstance(policy, bool): + flag = policy + else: + flag = policy(key) + + return flag + + +_ContextTuple = collections.namedtuple( + "_ContextTuple", + [ + "client", + "eventloop", + "stub", + "batches", + "commit_batches", + "transaction", + "cache", + ], +) + + class _Context(_ContextTuple): """Current runtime state. @@ -106,8 +128,8 @@ class _Context(_ContextTuple): loop. A new context can be derived from an existing context using :meth:`new`. - :class:`Context` is a subclass of :class:`_Context` which provides - only publicly facing interface. The use of two classes is only to provide a + :class:`Context` is a subclass of :class:`_Context` which provides only + publicly facing interface. The use of two classes is only to provide a distinction between public and private API. Arguments: @@ -123,6 +145,7 @@ def __new__( commit_batches=None, transaction=None, cache=None, + cache_policy=None, ): if eventloop is None: eventloop = _eventloop.EventLoop() @@ -145,7 +168,7 @@ def __new__( else: cache = _Cache() - return super(_Context, cls).__new__( + context = super(_Context, cls).__new__( cls, client=client, eventloop=eventloop, @@ -156,6 +179,10 @@ def __new__( cache=cache, ) + context.set_cache_policy(cache_policy) + + return context + def new(self, **kwargs): """Create a new :class:`_Context` instance. @@ -205,9 +232,9 @@ def get_cache_policy(self): Callable: A function that accepts a :class:`~google.cloud.ndb.key.Key` instance as a single positional argument and returns a ``bool`` indicating if it - should be cached. May be :data:`None`. + should be cached. May be :data:`None`. """ - raise NotImplementedError + return self.cache_policy def get_datastore_policy(self): """Return the current context datastore policy function. @@ -238,7 +265,7 @@ def get_memcache_timeout_policy(self): Callable: A function that accepts a :class:`~google.cloud.ndb.key.Key` instance as a single positional argument and returns an ``int`` indicating the - timeout, in seconds, for the key. :data:`0` implies the default + timeout, in seconds, for the key. ``0`` implies the default timeout. May be :data:`None`. """ raise NotImplementedError @@ -252,7 +279,16 @@ def set_cache_policy(self, policy): positional argument and returns a ``bool`` indicating if it should be cached. May be :data:`None`. """ - raise NotImplementedError + if policy is None: + policy = _default_cache_policy + + elif isinstance(policy, bool): + flag = policy + + def policy(key): + return flag + + self.cache_policy = policy def set_datastore_policy(self, policy): """Set the context datastore policy function. @@ -283,7 +319,7 @@ def set_memcache_timeout_policy(self, policy): policy (Callable): A function that accepts a :class:`~google.cloud.ndb.key.Key` instance as a single positional argument and returns an ``int`` indicating the - timeout, in seconds, for the key. :data:`0` implies the default + timeout, in seconds, for the key. ``0`` implies the default timout. May be :data:`None`. """ raise NotImplementedError @@ -319,31 +355,17 @@ def in_transaction(self): """ return self.transaction is not None - @staticmethod - def default_cache_policy(key): - """Default cache policy. - - This defers to :meth:`~google.cloud.ndb.model.Model._use_cache`. - - Args: - key (google.cloud.ndb.model.key.Key): The key. - - Returns: - Union[bool, NoneType]: Whether to cache the key. - """ - raise NotImplementedError - @staticmethod def default_datastore_policy(key): """Default cache policy. - This defers to :meth:`~google.cloud.ndb.model.Model._use_datastore`. + This defers to ``Model._use_datastore``. Args: - key (google.cloud.ndb.model.key.Key): The key. + key (google.cloud.ndb.key.Key): The key. Returns: - Union[bool, NoneType]: Whether to use datastore. + Union[bool, None]: Whether to use datastore. """ raise NotImplementedError @@ -351,13 +373,13 @@ def default_datastore_policy(key): def default_memcache_policy(key): """Default memcache policy. - This defers to :meth:`~google.cloud.ndb.model.Model._use_memcache`. + This defers to ``Model._use_memcache``. Args: - key (google.cloud.ndb.model.key.Key): The key. + key (google.cloud.ndb.key.Key): The key. Returns: - Union[bool, NoneType]: Whether to cache the key. + Union[bool, None]: Whether to cache the key. """ raise NotImplementedError @@ -365,13 +387,13 @@ def default_memcache_policy(key): def default_memcache_timeout_policy(key): """Default memcache timeout policy. - This defers to :meth:`~google.cloud.ndb.model.Model._memcache_timeout`. + This defers to ``Model._memcache_timeout``. Args: - key (google.cloud.ndb.model.key.Key): The key. + key (google.cloud.ndb.key.Key): The key. Returns: - Union[int, NoneType]: Memcache timeout to use. + Union[int, None]: Memcache timeout to use. """ raise NotImplementedError @@ -416,6 +438,15 @@ def urlfetch(self, *args, **kwargs): """Fetch a resource using HTTP.""" raise NotImplementedError + def _use_cache(self, key, options): + """Return whether to use the context cache for this key.""" + flag = options.use_cache + if flag is None: + flag = self.cache_policy(key) + if flag is None: + flag = True + return flag + class ContextOptions: __slots__ = () diff --git a/src/google/cloud/ndb/key.py b/src/google/cloud/ndb/key.py index b3ccfa98..9de3fd10 100644 --- a/src/google/cloud/ndb/key.py +++ b/src/google/cloud/ndb/key.py @@ -842,12 +842,13 @@ def get_async( @tasklets.tasklet def get(): - if _options.use_cache: + context = context_module.get_context() + use_cache = context._use_cache(self, _options) + + if use_cache: try: # This result may be None, if None is cached for this key. - return context_module.get_context().cache.get_and_validate( - self - ) + return context.cache.get_and_validate(self) except KeyError: pass @@ -857,8 +858,8 @@ def get(): else: result = None - if _options.use_cache: - context_module.get_context().cache[self] = result + if use_cache: + context.cache[self] = result return result @@ -971,8 +972,11 @@ def delete_async( @tasklets.tasklet def delete(): result = yield _datastore_api.delete(self._key, _options) - if _options.use_cache: - context_module.get_context().cache[self] = None + + context = context_module.get_context() + if context._use_cache(self, _options): + context.cache[self] = None + return result future = delete() diff --git a/src/google/cloud/ndb/model.py b/src/google/cloud/ndb/model.py index f5b74e02..a33bc986 100644 --- a/src/google/cloud/ndb/model.py +++ b/src/google/cloud/ndb/model.py @@ -4751,8 +4751,9 @@ def put(self): ds_key = helpers.key_from_protobuf(key_pb) self._key = key_module.Key._from_ds_key(ds_key) - if _options.use_cache: - context_module.get_context().cache[self._key] = self + context = context_module.get_context() + if context._use_cache(self._key, _options): + context.cache[self._key] = self return self._key diff --git a/tests/system/conftest.py b/tests/system/conftest.py index 35b86f56..94691994 100644 --- a/tests/system/conftest.py +++ b/tests/system/conftest.py @@ -92,5 +92,5 @@ def namespace(): @pytest.fixture def client_context(namespace): client = ndb.Client(namespace=namespace) - with client.context(): - yield + with client.context(cache_policy=False) as the_context: + yield the_context diff --git a/tests/system/test_crud.py b/tests/system/test_crud.py index 34d306dd..d8fac370 100644 --- a/tests/system/test_crud.py +++ b/tests/system/test_crud.py @@ -15,6 +15,8 @@ """ System tests for Create, Update, Delete. (CRUD) """ +import functools +import operator import pytest @@ -23,7 +25,11 @@ from google.cloud import datastore from google.cloud import ndb -from tests.system import KIND +from tests.system import KIND, eventually + + +def _equals(n): + return functools.partial(operator.eq, n) @pytest.mark.usefixtures("client_context") @@ -44,6 +50,27 @@ class SomeKind(ndb.Model): assert entity.baz == "night" +def test_retrieve_entity_with_caching(ds_entity, client_context): + entity_id = test_utils.system.unique_resource_id() + ds_entity(KIND, entity_id, foo=42, bar="none", baz=b"night") + + class SomeKind(ndb.Model): + foo = ndb.IntegerProperty() + bar = ndb.StringProperty() + baz = ndb.StringProperty() + + client_context.set_cache_policy(None) # Use default + + key = ndb.Key(KIND, entity_id) + entity = key.get() + assert isinstance(entity, SomeKind) + assert entity.foo == 42 + assert entity.bar == "none" + assert entity.baz == "night" + + assert key.get() is entity + + @pytest.mark.usefixtures("client_context") def test_retrieve_entity_not_found(ds_entity): entity_id = test_utils.system.unique_resource_id() @@ -125,6 +152,27 @@ class SomeKind(ndb.Model): dispose_of(key._key) +def test_insert_entity_with_caching(dispose_of, client_context): + class SomeKind(ndb.Model): + foo = ndb.IntegerProperty() + bar = ndb.StringProperty() + + client_context.set_cache_policy(None) # Use default + + entity = SomeKind(foo=42, bar="none") + key = entity.put() + + with client_context.new(cache_policy=False).use(): + # Sneaky. Delete entity out from under cache so we know we're getting + # cached copy. + key.delete() + eventually(key.get, _equals(None)) + + retrieved = key.get() + assert retrieved.foo == 42 + assert retrieved.bar == "none" + + @pytest.mark.usefixtures("client_context") def test_update_entity(ds_entity): entity_id = test_utils.system.unique_resource_id() @@ -220,6 +268,23 @@ class SomeKind(ndb.Model): assert key.delete() is None +def test_delete_entity_with_caching(ds_entity, client_context): + entity_id = test_utils.system.unique_resource_id() + ds_entity(KIND, entity_id, foo=42) + + class SomeKind(ndb.Model): + foo = ndb.IntegerProperty() + + client_context.set_cache_policy(None) # Use default + + key = ndb.Key(KIND, entity_id) + assert key.get().foo == 42 + + assert key.delete() is None + assert key.get() is None + assert key.delete() is None + + @pytest.mark.usefixtures("client_context") def test_delete_entity_in_transaction(ds_entity): entity_id = test_utils.system.unique_resource_id() diff --git a/tests/unit/test_context.py b/tests/unit/test_context.py index 96c8f574..ddda5b1a 100644 --- a/tests/unit/test_context.py +++ b/tests/unit/test_context.py @@ -18,6 +18,8 @@ from google.cloud.ndb import context as context_module from google.cloud.ndb import _eventloop from google.cloud.ndb import exceptions +from google.cloud.ndb import key as key_module +from google.cloud.ndb import model import tests.unit.utils @@ -81,8 +83,9 @@ def test_flush(self): def test_get_cache_policy(self): context = self._make_one() - with pytest.raises(NotImplementedError): - context.get_cache_policy() + assert ( + context.get_cache_policy() is context_module._default_cache_policy + ) def test_get_datastore_policy(self): context = self._make_one() @@ -100,9 +103,22 @@ def test_get_memcache_timeout_policy(self): context.get_memcache_timeout_policy() def test_set_cache_policy(self): + policy = object() context = self._make_one() - with pytest.raises(NotImplementedError): - context.set_cache_policy(None) + context.set_cache_policy(policy) + assert context.get_cache_policy() is policy + + def test_set_cache_policy_to_None(self): + context = self._make_one() + context.set_cache_policy(None) + assert ( + context.get_cache_policy() is context_module._default_cache_policy + ) + + def test_set_cache_policy_with_bool(self): + context = self._make_one() + context.set_cache_policy(False) + assert context.get_cache_policy()(None) is False def test_set_datastore_policy(self): context = self._make_one() @@ -128,11 +144,6 @@ def test_in_transaction(self): context = self._make_one() assert context.in_transaction() is False - def test_default_cache_policy(self): - context = self._make_one() - with pytest.raises(NotImplementedError): - context.default_cache_policy(None) - def test_default_datastore_policy(self): context = self._make_one() with pytest.raises(NotImplementedError): @@ -220,6 +231,48 @@ def test_constructor(): context_module.TransactionOptions() +class Test_default_cache_policy: + @staticmethod + def test_key_is_None(): + assert context_module._default_cache_policy(None) is None + + @staticmethod + def test_no_model_class(): + key = mock.Mock(kind=mock.Mock(return_value="nokind"), spec=("kind",)) + assert context_module._default_cache_policy(key) is None + + @staticmethod + @pytest.mark.usefixtures("in_context") + def test_standard_model(): + class ThisKind(model.Model): + pass + + key = key_module.Key("ThisKind", 0) + assert context_module._default_cache_policy(key) is None + + @staticmethod + @pytest.mark.usefixtures("in_context") + def test_standard_model_defines_policy(): + flag = object() + + class ThisKind(model.Model): + @classmethod + def _use_cache(cls, key): + return flag + + key = key_module.Key("ThisKind", 0) + assert context_module._default_cache_policy(key) is flag + + @staticmethod + @pytest.mark.usefixtures("in_context") + def test_standard_model_defines_policy_as_bool(): + class ThisKind(model.Model): + _use_cache = False + + key = key_module.Key("ThisKind", 0) + assert context_module._default_cache_policy(key) is False + + class TestCache: @staticmethod def test_get_and_validate_valid():