-
Notifications
You must be signed in to change notification settings - Fork 3
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
Deprecate clock_tick context manager #39
base: master
Are you sure you want to change the base?
Changes from 4 commits
5ed28fb
f2c2229
6512d60
3df510d
9afe85b
d7e52ad
c97443e
de145e4
e06df10
44eb508
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,18 +9,71 @@ | |
import sqlalchemy as sa | ||
import sqlalchemy.dialects.postgresql as sap | ||
import sqlalchemy.orm as orm | ||
import sqlalchemy.orm.base as base | ||
import sqlalchemy.orm.attributes as attributes | ||
import psycopg2.extras as psql_extras | ||
|
||
from temporal_sqlalchemy import nine | ||
from temporal_sqlalchemy.metadata import get_session_metadata | ||
|
||
_ClockSet = collections.namedtuple('_ClockSet', ('effective', 'vclock')) | ||
_PersistentClockPair = collections.namedtuple('_PersistentClockPairs', | ||
('effective', 'vclock')) | ||
|
||
T_PROPS = typing.TypeVar( | ||
'T_PROP', orm.RelationshipProperty, orm.ColumnProperty) | ||
|
||
|
||
class ActivityState: | ||
def __set__(self, instance, value): | ||
assert instance.temporal_options.activity_cls, "Make this better Joey" | ||
# TODO should not be able to change activity once changes have been made to temporal properties | ||
setattr(instance, '__temporal_current_activity', value) | ||
|
||
if value: | ||
current_clock = instance.current_clock | ||
current_clock.activity = value | ||
|
||
def __get__(self, instance, owner): | ||
if not instance: | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to use A minimal example: import types
ns = types.SimpleNamespace(activity=1)
# This works
ActivityState.reset_activity(ns, None)
class B:
astate = ActivityState()
# This crashes
B.astate.reset_activity(ns, None) |
||
|
||
return getattr(instance, '__temporal_current_activity', None) | ||
|
||
@staticmethod | ||
def reset_activity(target, attr): | ||
target.activity = None | ||
|
||
@staticmethod | ||
def activity_required(target, value, oldvalue, initiator): | ||
if not target.activity and oldvalue is not base.NEVER_SET: | ||
raise ValueError("activity required") | ||
|
||
|
||
class ClockState: | ||
def __set__(self, instance, value: 'EntityClock'): | ||
setattr(instance, '__temporal_current_tick', value) | ||
if value: | ||
instance.clock.append(value) | ||
|
||
def __get__(self, instance, owner): | ||
if not instance: | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same idea here about returning |
||
|
||
vclock = getattr(instance, 'vclock') or 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using vclock = getattr(instance, 'vclock', 0) |
||
if not getattr(instance, '__temporal_current_tick', None): | ||
new_version = vclock + 1 | ||
instance.vclock = new_version | ||
clock_tick = instance.temporal_options.clock_model(tick=new_version) | ||
setattr(instance, '__temporal_current_tick', clock_tick) | ||
instance.clock.append(clock_tick) | ||
|
||
return getattr(instance, '__temporal_current_tick') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same note here about using However, be warned: attributes starting with double underscores may not behave as you expect. Python has a concept of "private variables" that may result in an From the docs:
Because it's required to occur inside the class definition I think this will be okay, but I thought I'd point it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think in this case it's okay, but it's a good call out |
||
|
||
@staticmethod | ||
def reset_tick(target, attr): | ||
if target: | ||
target.current_clock = None | ||
|
||
|
||
class EntityClock(object): | ||
id = sa.Column(sap.UUID(as_uuid=True), default=uuid.uuid4, primary_key=True) | ||
tick = sa.Column(sa.Integer, nullable=False) | ||
|
@@ -50,11 +103,13 @@ def __init__( | |
temporal_props: typing.Iterable[T_PROPS], | ||
clock_model: nine.Type[EntityClock], | ||
activity_cls: nine.Type[TemporalActivityMixin] = None): | ||
|
||
self.history_models = history_models | ||
self.temporal_props = temporal_props | ||
|
||
self.clock_model = clock_model | ||
self.activity_cls = activity_cls | ||
self.model = None | ||
|
||
@property | ||
def clock_table(self): | ||
|
@@ -73,7 +128,7 @@ def history_tables(self): | |
@staticmethod | ||
def make_clock(effective_lower: dt.datetime, | ||
vclock_lower: int, | ||
**kwargs) -> _ClockSet: | ||
**kwargs) -> _PersistentClockPair: | ||
"""construct a clock set tuple""" | ||
effective_upper = kwargs.get('effective_upper', None) | ||
vclock_upper = kwargs.get('vclock_upper', None) | ||
|
@@ -82,25 +137,14 @@ def make_clock(effective_lower: dt.datetime, | |
effective_lower, effective_upper) | ||
vclock = psql_extras.NumericRange(vclock_lower, vclock_upper) | ||
|
||
return _ClockSet(effective, vclock) | ||
return _PersistentClockPair(effective, vclock) | ||
|
||
def record_history(self, | ||
clocked: 'Clocked', | ||
session: orm.Session, | ||
timestamp: dt.datetime): | ||
"""record all history for a given clocked object""" | ||
state = attributes.instance_state(clocked) | ||
vclock_history = attributes.get_history(clocked, 'vclock') | ||
try: | ||
new_tick = state.dict['vclock'] | ||
except KeyError: | ||
# TODO understand why this is necessary | ||
new_tick = getattr(clocked, 'vclock') | ||
|
||
is_strict_mode = get_session_metadata(session).get('strict_mode', False) | ||
is_vclock_unchanged = vclock_history.unchanged and new_tick == vclock_history.unchanged[0] | ||
|
||
new_clock = self.make_clock(timestamp, new_tick) | ||
attr = {'entity': clocked} | ||
|
||
for prop, cls in self.history_models.items(): | ||
|
@@ -111,16 +155,14 @@ def record_history(self, | |
|
||
if isinstance(prop, orm.RelationshipProperty): | ||
changes = attributes.get_history( | ||
clocked, prop.key, | ||
clocked, | ||
prop.key, | ||
passive=attributes.PASSIVE_NO_INITIALIZE) | ||
else: | ||
changes = attributes.get_history(clocked, prop.key) | ||
|
||
if changes.added: | ||
if is_strict_mode: | ||
assert not is_vclock_unchanged, \ | ||
'flush() has triggered for a changed temporalized property outside of a clock tick' | ||
|
||
new_clock = self.make_clock(timestamp, clocked.current_clock.tick) | ||
# Cap previous history row if exists | ||
if sa.inspect(clocked).identity is not None: | ||
# but only if it already exists!! | ||
|
@@ -184,6 +226,10 @@ class Clocked(object): | |
first_tick = None # type: EntityClock | ||
latest_tick = None # type: EntityClock | ||
|
||
# temporal descriptors | ||
current_clock = None # type: ClockState | ||
activity = None # type: typing.Optional[ActivityState] | ||
|
||
@property | ||
def date_created(self): | ||
return self.first_tick.timestamp | ||
|
@@ -194,22 +240,13 @@ def date_modified(self): | |
|
||
@contextlib.contextmanager | ||
def clock_tick(self, activity: TemporalActivityMixin = None): | ||
warnings.warn("clock_tick is going away in 0.5.0", | ||
PendingDeprecationWarning) | ||
"""Increments vclock by 1 with changes scoped to the session""" | ||
if self.temporal_options.activity_cls is not None and activity is None: | ||
raise ValueError("activity is missing on edit") from None | ||
|
||
session = orm.object_session(self) | ||
with session.no_autoflush: | ||
yield self | ||
|
||
if session.is_modified(self): | ||
self.vclock += 1 | ||
warnings.warn("clock_tick is deprecated, assign an activity directly", | ||
DeprecationWarning) | ||
if self.temporal_options.activity_cls: | ||
if not activity: | ||
raise ValueError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we put an error message here please? It'll make debugging easier. I think the original error message might suffice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 should probably be: raise ValueError("activity is missing on edit") from None |
||
self.activity = activity | ||
|
||
new_clock_tick = self.temporal_options.clock_model( | ||
entity=self, tick=self.vclock) | ||
if activity is not None: | ||
new_clock_tick.activity = activity | ||
yield self | ||
|
||
session.add(new_clock_tick) | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left it in so it looks less confusing to me, but I'm strange that way 😆 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ | |
from temporal_sqlalchemy import nine, util | ||
from temporal_sqlalchemy.bases import ( | ||
T_PROPS, | ||
ClockState, | ||
ActivityState, | ||
Clocked, | ||
TemporalOption, | ||
TemporalActivityMixin, | ||
|
@@ -78,6 +80,10 @@ def temporal_map(*track, mapper: orm.Mapper, activity_class=None, schema=None): | |
backref_name = '%s_clock' % entity_table.name | ||
clock_properties['activity'] = \ | ||
orm.relationship(lambda: activity_class, backref=backref_name) | ||
cls.activity = ActivityState() | ||
event.listen(cls, 'expire', ActivityState.reset_activity) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NicoleZuckerman this the |
||
for prop in tracked_props: | ||
event.listen(prop, 'set', ActivityState.activity_required) | ||
|
||
clock_model = build_clock_class(cls.__name__, | ||
entity_table.metadata, | ||
|
@@ -94,24 +100,19 @@ def temporal_map(*track, mapper: orm.Mapper, activity_class=None, schema=None): | |
clock_model=clock_model, | ||
activity_cls=activity_class | ||
) | ||
|
||
cls.current_clock = ClockState() | ||
event.listen(cls, 'expire', ClockState.reset_tick) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NicoleZuckerman this is the expire for the |
||
event.listen(cls, 'init', init_clock) | ||
|
||
|
||
def init_clock(obj: Clocked, args, kwargs): | ||
kwargs.setdefault('vclock', 1) | ||
initial_tick = obj.temporal_options.clock_model( | ||
tick=kwargs['vclock'], | ||
entity=obj, | ||
) | ||
obj.current_clock = obj.temporal_options.clock_model(tick=kwargs['vclock']) | ||
|
||
if obj.temporal_options.activity_cls and 'activity' not in kwargs: | ||
raise ValueError( | ||
"%r missing keyword argument: activity" % obj.__class__) | ||
|
||
if 'activity' in kwargs: | ||
initial_tick.activity = kwargs.pop('activity') | ||
|
||
materialize_defaults(obj, kwargs) | ||
|
||
|
||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this error message to something a bit more helpful? 🙂 Perhaps:
(That's assuming that my interpretation of this assertion is correct.)