From e0ce459c96fde6190844528546a4536c9b6a5fdb Mon Sep 17 00:00:00 2001 From: Andrei Date: Sat, 29 Jul 2017 17:58:26 -0700 Subject: [PATCH] [bugfix] fix behavior of cached_property This commit is a fairly large chunk of code that fixes a previously annoying and nasty bug causing cached_properties to not be cleared. Alongside this bug fix I took the opportunity to refactor the entire methdology behind cached properties, and bind them more strictly to the behavior of models. Prior to this commit, cached_properties where not be properly reset upon a model's `update` method being called due to some invalid code. Along with this issue, the actual behavior of cached properties landed in a weird in-between/gray area where they had to support both non-model and model use cases. The new version of cached_property is strictly for use with models, and another version `simple_cached_property` was added to support other use cases. The new cached_property simply builds an actual `property` within the metaclass. This is fairly efficient, and also reduces the surface area of behavior. When writing this I messed around with a few ideas, including allowing the user of `cached_property` to define a linkage between the property and fields that should invalidate its cached value, but this was both messy and introduced massive cognitive load on understand when a cached_property should be cleared. Although this version is slighty less efficient, I'm very much in favor of it vs the alternatives I tried. --- disco/bot/command.py | 12 +++++----- disco/types/base.py | 47 ++++++++++++++++++++++++++++++++++------ disco/types/channel.py | 4 ++-- disco/types/guild.py | 4 ++-- disco/types/message.py | 3 +-- disco/types/voice.py | 3 +-- disco/types/webhook.py | 3 +-- disco/util/functional.py | 31 ++++++++++---------------- disco/util/snowflake.py | 9 ++++---- tests/types/base.py | 31 ++++++++++++++++++++++++++ tests/util/functional.py | 21 ++++++++++++++++++ tests/utils.py | 10 +++++++++ 12 files changed, 132 insertions(+), 46 deletions(-) create mode 100644 tests/types/base.py create mode 100644 tests/util/functional.py diff --git a/disco/bot/command.py b/disco/bot/command.py index a301360..d17a8fb 100644 --- a/disco/bot/command.py +++ b/disco/bot/command.py @@ -6,7 +6,7 @@ from holster.enum import Enum from six import integer_types from disco.bot.parser import ArgumentSet, ArgumentError -from disco.util.functional import cached_property +from disco.util.functional import simple_cached_property ARGS_REGEX = '(?: ((?:\n|.)*)$|$)' ARGS_UNGROUPED_REGEX = '(?: (?:\n|.)*$|$)' @@ -75,28 +75,28 @@ class CommandEvent(object): return src - @cached_property + @simple_cached_property def member(self): """ Guild member (if relevant) for the user that created the message. """ return self.guild.get_member(self.author) - @property + @simple_cached_property def channel(self): """ Channel the message was created in. """ return self.msg.channel - @property + @simple_cached_property def guild(self): """ Guild (if relevant) the message was created in. """ return self.msg.guild - @property + @simple_cached_property def author(self): """ Author of the message. @@ -243,7 +243,7 @@ class Command(object): raise TypeError('Cannot resolve mention: {}'.format(raw)) return _f - @cached_property + @simple_cached_property def compiled_regex(self): """ A compiled version of this command's regex. diff --git a/disco/types/base.py b/disco/types/base.py index 04679ff..fb15cff 100644 --- a/disco/types/base.py +++ b/disco/types/base.py @@ -8,7 +8,6 @@ from datetime import datetime as real_datetime from disco.util.chains import Chainable from disco.util.hashmap import HashMap -from disco.util.functional import CachedSlotProperty DATETIME_FORMATS = [ '%Y-%m-%dT%H:%M:%S.%f', @@ -33,6 +32,18 @@ class Unset(object): UNSET = Unset() +def cached_property(method): + method._cached_property = set() + return method + + +def strict_cached_property(*args): + def _cached_property(method): + method._cached_property = set(args) + return method + return _cached_property + + class ConversionError(Exception): def __init__(self, field, raw, e): super(ConversionError, self).__init__( @@ -236,29 +247,51 @@ Model = None SlottedModel = None +def _get_cached_property(name, func): + def _getattr(self): + try: + return getattr(self, '_' + name) + except AttributeError: + value = func(self) + setattr(self, '_' + name, value) + return value + + def _setattr(self, value): + setattr(self, '_' + name) + + def _delattr(self): + delattr(self, '_' + name) + + prop = property(_getattr, _setattr, _delattr) + return prop + + class ModelMeta(type): def __new__(mcs, name, parents, dct): fields = {} + slots = set() for parent in parents: if Model and issubclass(parent, Model) and parent != Model: fields.update(parent._fields) for k, v in six.iteritems(dct): + if hasattr(v, '_cached_property'): + dct[k] = _get_cached_property(k, v) + slots.add('_' + k) + if not isinstance(v, Field): continue v.name = k fields[k] = v + slots.add(k) if SlottedModel and any(map(lambda k: issubclass(k, SlottedModel), parents)): - bases = set(v.stored_name for v in six.itervalues(dct) if isinstance(v, CachedSlotProperty)) - - if '__slots__' in dct: - dct['__slots__'] = tuple(set(dct['__slots__']) | set(fields.keys()) | bases) - else: - dct['__slots__'] = tuple(fields.keys()) + tuple(bases) + # Merge our set of field slots with any other slots from the mro + dct['__slots__'] = tuple(set(dct.get('__slots__', [])) | slots) + # Remove all fields from the dict dct = {k: v for k, v in six.iteritems(dct) if k not in dct['__slots__']} else: dct = {k: v for k, v in six.iteritems(dct) if k not in fields} diff --git a/disco/types/channel.py b/disco/types/channel.py index bd51c1a..c0c8e2a 100644 --- a/disco/types/channel.py +++ b/disco/types/channel.py @@ -5,9 +5,9 @@ from six.moves import map from holster.enum import Enum from disco.util.snowflake import to_snowflake -from disco.util.functional import cached_property, one_or_many, chunks +from disco.util.functional import one_or_many, chunks from disco.types.user import User -from disco.types.base import SlottedModel, Field, AutoDictField, snowflake, enum, text +from disco.types.base import SlottedModel, Field, AutoDictField, snowflake, enum, text, cached_property from disco.types.permissions import Permissions, Permissible, PermissionValue diff --git a/disco/types/guild.py b/disco/types/guild.py index fe2b4a3..bacb2e5 100644 --- a/disco/types/guild.py +++ b/disco/types/guild.py @@ -6,9 +6,9 @@ from disco.gateway.packets import OPCode from disco.api.http import APIException from disco.util.paginator import Paginator from disco.util.snowflake import to_snowflake -from disco.util.functional import cached_property from disco.types.base import ( - SlottedModel, Field, ListField, AutoDictField, DictField, snowflake, text, enum, datetime + SlottedModel, Field, ListField, AutoDictField, DictField, snowflake, text, enum, datetime, + cached_property ) from disco.types.user import User from disco.types.voice import VoiceState diff --git a/disco/types/message.py b/disco/types/message.py index 54e8f16..deb03b2 100644 --- a/disco/types/message.py +++ b/disco/types/message.py @@ -8,11 +8,10 @@ from holster.enum import Enum from disco.types.base import ( SlottedModel, Field, ListField, AutoDictField, snowflake, text, - datetime, enum + datetime, enum, cached_property ) from disco.util.paginator import Paginator from disco.util.snowflake import to_snowflake -from disco.util.functional import cached_property from disco.types.user import User diff --git a/disco/types/voice.py b/disco/types/voice.py index 3d7cb32..bb7c055 100644 --- a/disco/types/voice.py +++ b/disco/types/voice.py @@ -1,5 +1,4 @@ -from disco.types.base import SlottedModel, Field, snowflake -from disco.util.functional import cached_property +from disco.types.base import SlottedModel, Field, snowflake, cached_property class VoiceState(SlottedModel): diff --git a/disco/types/webhook.py b/disco/types/webhook.py index b8930c7..fa73aac 100644 --- a/disco/types/webhook.py +++ b/disco/types/webhook.py @@ -1,8 +1,7 @@ import re -from disco.types.base import SlottedModel, Field, snowflake +from disco.types.base import SlottedModel, Field, snowflake, cached_property from disco.types.user import User -from disco.util.functional import cached_property WEBHOOK_URL_RE = re.compile(r'\/api\/webhooks\/(\d+)\/(.[^/]+)') diff --git a/disco/util/functional.py b/disco/util/functional.py index d52d4e9..d2e0273 100644 --- a/disco/util/functional.py +++ b/disco/util/functional.py @@ -48,28 +48,21 @@ def one_or_many(f): return _f -class CachedSlotProperty(object): - __slots__ = ['stored_name', 'function', '__doc__'] - - def __init__(self, name, function): - self.stored_name = '_' + name - self.function = function - self.__doc__ = getattr(function, '__doc__') - - def set(self, value): - setattr(self.stored_name, value) - - def __get__(self, instance, owner): - if instance is None: - return self +def simple_cached_property(method): + key = '_{}'.format(method.__name__) + def _getattr(inst): try: - return getattr(instance, self.stored_name) + return getattr(inst, key) except AttributeError: - value = self.function(instance) - setattr(instance, self.stored_name, value) + value = method(inst) + setattr(inst, key, value) return value + def _setattr(inst, value): + setattr(inst, key, value) + + def _delattr(inst): + delattr(inst, key) -def cached_property(f): - return CachedSlotProperty(f.__name__, f) + return property(_getattr, _setattr, _delattr) diff --git a/disco/util/snowflake.py b/disco/util/snowflake.py index ad453a7..c0d24bb 100644 --- a/disco/util/snowflake.py +++ b/disco/util/snowflake.py @@ -26,10 +26,11 @@ def from_datetime(date): def from_timestamp(ts): - if six.PY3: - return int(ts * 1000.0 - DISCORD_EPOCH) << 22 - else: - return long(ts * 1000.0 - DISCORD_EPOCH) << 22 + return from_timestamp_ms(ts * 1000.0) + + +def from_timestamp_ms(ts): + return int(ts - DISCORD_EPOCH) << 22 def to_snowflake(i): diff --git a/tests/types/base.py b/tests/types/base.py new file mode 100644 index 0000000..5da7ea1 --- /dev/null +++ b/tests/types/base.py @@ -0,0 +1,31 @@ +import pytest + +from disco.types.base import Model, Field, cached_property + + +@pytest.fixture +def model(): + class TestModel(Model): + a = Field(int) + b = Field(int) + + @cached_property + def value(self): + return self.a + self.b + + return TestModel + + +def test_cached_property(model): + inst = model(a=1, b=3) + assert inst.value == 4 + + inst.a = 2 + assert inst.value == 4 + + +def test_cached_property_clear_on_update(model): + inst = model(a=1, b=3) + assert inst.value == 4 + inst.update(model(a=2, b=3)) + assert inst.value == 5 diff --git a/tests/util/functional.py b/tests/util/functional.py new file mode 100644 index 0000000..9971972 --- /dev/null +++ b/tests/util/functional.py @@ -0,0 +1,21 @@ +from disco.util.functional import simple_cached_property + + +def test_simple_cached_property(): + class Test(object): + def __init__(self, a, b): + self.a = a + self.b = b + + @simple_cached_property + def value(self): + return self.a + self.b + + inst = Test(1, 1) + assert inst.value == 2 + + inst.a = 4 + assert inst.value == 2 + + del inst.value + assert inst.value == 5 diff --git a/tests/utils.py b/tests/utils.py index 36fde6f..9fee58f 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,4 +1,8 @@ +import time +import random + from disco.api.client import APIClient as _APIClient +from disco.util.snowflake import from_timestamp_ms class CallContainer(object): @@ -13,3 +17,9 @@ class APIClient(_APIClient): def __init__(self): self.client = None self.http = CallContainer() + + +def random_snowflake(): + return from_timestamp_ms( + (time.time() * 1000.0) + random.randint(1, 9999) + )