From a6a18c1a6f25aac006e6d3cc28c27ddd0b8a79fd Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 14 Apr 2016 15:09:59 +0100 Subject: [PATCH 1/2] This approach uses part of sqlalchemy example history_meta code adapted to recording inserts and updates. This removes need to manually create history tables. Our code still remains in control of when history records are created. --- app/dao/dao_utils.py | 48 +++++ app/dao/notifications_dao.py | 17 +- app/dao/services_dao.py | 27 +-- app/history_meta.py | 167 ++++++++++++++++++ app/models.py | 21 ++- app/service/rest.py | 1 - .../versions/0003_add_service_history.py | 56 ++++++ tests/app/conftest.py | 3 +- tests/app/dao/test_services_dao.py | 133 ++++++++++++-- tests/app/service/test_rest.py | 23 ++- 10 files changed, 442 insertions(+), 54 deletions(-) create mode 100644 app/dao/dao_utils.py create mode 100644 app/history_meta.py create mode 100644 migrations/versions/0003_add_service_history.py diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py new file mode 100644 index 000000000..810ccdebd --- /dev/null +++ b/app/dao/dao_utils.py @@ -0,0 +1,48 @@ +import datetime + +from functools import wraps + + +def create_history(obj): + history_mapper = obj.__history_mapper__ + history_model = history_mapper.class_ + history = history_model() + if obj.version: + obj.version += 1 + else: + obj.version = 1 + obj.created_at = datetime.datetime.now() + for prop in history_mapper.iterate_properties: + if obj.__mapper__.get_property(prop.key): + setattr(history, prop.key, getattr(obj, prop.key)) + history.created_by_id = obj.created_by.id + return history + + +def transactional(func): + @wraps(func) + def commit_or_rollback(*args, **kwargs): + from flask import current_app + from app import db + try: + func(*args, **kwargs) + db.session.commit() + except Exception as e: + current_app.logger.error(e) + db.session.rollback() + raise + return commit_or_rollback + + +def versioned(func): + @wraps(func) + def record_version(*args, **kwargs): + import itertools + from app import db + from app.history_meta import versioned_objects + from app.dao.dao_utils import create_history + func(*args, **kwargs) + for obj in versioned_objects(itertools.chain(db.session.new, db.session.dirty)): + history = create_history(obj) + db.session.add(history) + return record_version diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f64abfd60..60eb91eda 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -30,22 +30,7 @@ from app.clients import ( STATISTICS_REQUESTED ) -from functools import wraps - - -def transactional(func): - @wraps(func) - def commit_or_rollback(*args, **kwargs): - from flask import current_app - from app import db - try: - func(*args, **kwargs) - db.session.commit() - except Exception as e: - current_app.logger.error(e) - db.session.rollback() - raise - return commit_or_rollback +from app.dao.dao_utils import transactional def get_character_count_of_content(content, encoding='utf-8'): diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5090afebe..b7f922a6a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -1,7 +1,14 @@ +import uuid + from app import db from app.models import Service from sqlalchemy import asc +from app.dao.dao_utils import ( + transactional, + versioned +) + def dao_fetch_all_services(): return Service.query.order_by(asc(Service.created_at)).all() @@ -19,22 +26,20 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): return Service.query.filter(Service.users.any(id=user_id)).filter_by(id=service_id).one() +@transactional +@versioned def dao_create_service(service, user): - try: - from app.dao.permissions_dao import permission_dao - service.users.append(user) - permission_dao.add_default_service_permissions_for_user(user, service) - db.session.add(service) - except Exception as e: - db.session.rollback() - raise e - else: - db.session.commit() + from app.dao.permissions_dao import permission_dao + service.users.append(user) + permission_dao.add_default_service_permissions_for_user(user, service) + service.id = uuid.uuid4() # must be set now so version history model can use same id + db.session.add(service) +@transactional +@versioned def dao_update_service(service): db.session.add(service) - db.session.commit() def dao_add_user_to_service(service, user, permissions=[]): diff --git a/app/history_meta.py b/app/history_meta.py new file mode 100644 index 000000000..b87577014 --- /dev/null +++ b/app/history_meta.py @@ -0,0 +1,167 @@ +"""Versioned mixin class and other utilities. + +This is an adapted version of: + +https://bitbucket.org/zzzeek/sqlalchemy/raw/master/examples/versioned_history/history_meta.py + +It does not use the create_version function from the orginal which looks for changes to models +as we just insert a copy of a model to the history table on create or update. + +Also it does not add a created_at timestamp to the history table as we already have created_at +and updated_at timestamps. + +Lastly when to create a version is done manually in dao_utils version decorator and not via +session events. + +""" + + +from sqlalchemy.ext.declarative import declared_attr +from sqlalchemy.orm import mapper +from sqlalchemy import Table, Column, ForeignKeyConstraint, Integer +from sqlalchemy import util + + +def col_references_table(col, table): + for fk in col.foreign_keys: + if fk.references(table): + return True + return False + + +def _is_versioning_col(col): + return "version_meta" in col.info + + +def _history_mapper(local_mapper): + cls = local_mapper.class_ + + # set the "active_history" flag + # on on column-mapped attributes so that the old version + # of the info is always loaded (currently sets it on all attributes) + for prop in local_mapper.iterate_properties: + getattr(local_mapper.class_, prop.key).impl.active_history = True + + super_mapper = local_mapper.inherits + super_history_mapper = getattr(cls, '__history_mapper__', None) + + polymorphic_on = None + super_fks = [] + + def _col_copy(col): + orig = col + col = col.copy() + orig.info['history_copy'] = col + col.unique = False + col.default = col.server_default = None + return col + + properties = util.OrderedDict() + if not super_mapper or \ + local_mapper.local_table is not super_mapper.local_table: + cols = [] + version_meta = {"version_meta": True} + for column in local_mapper.local_table.c: + if _is_versioning_col(column): + continue + + col = _col_copy(column) + + if super_mapper and \ + col_references_table(column, super_mapper.local_table): + super_fks.append( + ( + col.key, + list(super_history_mapper.local_table.primary_key)[0] + ) + ) + + cols.append(col) + + if column is local_mapper.polymorphic_on: + polymorphic_on = col + + orig_prop = local_mapper.get_property_by_column(column) + # carry over column re-mappings + if len(orig_prop.columns) > 1 or \ + orig_prop.columns[0].key != orig_prop.key: + properties[orig_prop.key] = tuple( + col.info['history_copy'] for col in orig_prop.columns) + + if super_mapper: + super_fks.append( + ( + 'version', super_history_mapper.local_table.c.version + ) + ) + + # "version" stores the integer version id. This column is + # required. + cols.append( + Column( + 'version', Integer, primary_key=True, + autoincrement=False, info=version_meta)) + + if super_fks: + cols.append(ForeignKeyConstraint(*zip(*super_fks))) + + table = Table( + local_mapper.local_table.name + '_history', + local_mapper.local_table.metadata, + *cols, + schema=local_mapper.local_table.schema + ) + else: + # single table inheritance. take any additional columns that may have + # been added and add them to the history table. + for column in local_mapper.local_table.c: + if column.key not in super_history_mapper.local_table.c: + col = _col_copy(column) + super_history_mapper.local_table.append_column(col) + table = None + + if super_history_mapper: + bases = (super_history_mapper.class_,) + + if table is not None: + properties['changed'] = ( + (table.c.changed, ) + + tuple(super_history_mapper.attrs.changed.columns) + ) + + else: + bases = local_mapper.base_mapper.class_.__bases__ + versioned_cls = type.__new__(type, "%sHistory" % cls.__name__, bases, {}) + + m = mapper( + versioned_cls, + table, + inherits=super_history_mapper, + polymorphic_on=polymorphic_on, + polymorphic_identity=local_mapper.polymorphic_identity, + properties=properties + ) + cls.__history_mapper__ = m + + if not super_history_mapper: + local_mapper.local_table.append_column( + Column('version', Integer, default=1, nullable=False) + ) + local_mapper.add_property( + "version", local_mapper.local_table.c.version) + + +class Versioned(object): + @declared_attr + def __mapper_cls__(cls): + def map(cls, *arg, **kw): + mp = mapper(cls, *arg, **kw) + _history_mapper(mp) + return mp + return map + + +def versioned_objects(iter): + for obj in iter: + if hasattr(obj, '__history_mapper__'): + yield obj diff --git a/app/models.py b/app/models.py index 775c6db86..fd7a3e596 100644 --- a/app/models.py +++ b/app/models.py @@ -1,19 +1,19 @@ import uuid import datetime -from sqlalchemy import UniqueConstraint, Sequence, CheckConstraint +from sqlalchemy.dialects.postgresql import UUID -from . import db -from sqlalchemy.dialects.postgresql import ( - UUID, - ARRAY -) +from sqlalchemy import UniqueConstraint from app.encryption import ( hashpw, check_hash ) +from app import db + +from app.history_meta import Versioned + def filter_null_value_fields(obj): return dict( @@ -68,7 +68,7 @@ user_to_service = db.Table( ) -class Service(db.Model): +class Service(db.Model, Versioned): __tablename__ = 'services' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) @@ -93,6 +93,13 @@ class Service(db.Model): backref=db.backref('user_to_service', lazy='dynamic')) restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False) email_from = db.Column(db.Text, index=False, unique=True, nullable=False) + created_by = db.relationship('User') + created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) + + @classmethod + def get_history_model(cls): + history_mapper = cls.__history_mapper__ + return history_mapper.class_ class ApiKey(db.Model): diff --git a/app/service/rest.py b/app/service/rest.py index ae643ac3f..64819a7b6 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -71,7 +71,6 @@ def create_service(): user = get_model_users(data['user_id']) data.pop('user_id', None) - valid_service, errors = service_schema.load(request.get_json()) if errors: diff --git a/migrations/versions/0003_add_service_history.py b/migrations/versions/0003_add_service_history.py new file mode 100644 index 000000000..306176941 --- /dev/null +++ b/migrations/versions/0003_add_service_history.py @@ -0,0 +1,56 @@ +"""empty message + +Revision ID: 0003_add_service_history +Revises: 0002_add_content_char_count +Create Date: 2016-04-19 13:01:54.519821 + +""" + +# revision identifiers, used by Alembic. +revision = '0003_add_service_history' +down_revision = '0002_add_content_char_count' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.create_table('services_history', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('name', sa.String(length=255), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.Column('active', sa.Boolean(), nullable=False), + sa.Column('message_limit', sa.BigInteger(), nullable=False), + sa.Column('restricted', sa.Boolean(), nullable=False), + sa.Column('email_from', sa.Text(), nullable=False), + sa.Column('created_by_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('version', sa.Integer(), autoincrement=False, nullable=False), + sa.PrimaryKeyConstraint('id', 'version') + ) + op.create_index(op.f('ix_services_history_created_by_id'), 'services_history', ['created_by_id'], unique=False) + op.add_column('services', sa.Column('created_by_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.add_column('services', sa.Column('version', sa.Integer(), nullable=True)) + op.create_index(op.f('ix_services_created_by_id'), 'services', ['created_by_id'], unique=False) + op.create_foreign_key(None, 'services', 'users', ['created_by_id'], ['id']) + + op.get_bind() + op.execute('UPDATE services SET created_by_id = (SELECT user_id FROM user_to_service WHERE services.id = user_to_service.service_id LIMIT 1)') + op.execute('UPDATE services SET version = 1') + op.execute('INSERT INTO services_history SELECT * FROM services') + + op.alter_column('services', 'created_by_id', nullable=False) + op.alter_column('services', 'version', nullable=False) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint(None, 'services', type_='foreignkey') + op.drop_index(op.f('ix_services_created_by_id'), table_name='services') + op.drop_column('services', 'version') + op.drop_column('services', 'created_by_id') + op.drop_index(op.f('ix_services_history_created_by_id'), table_name='services_history') + op.drop_table('services_history') + ### end Alembic commands ### diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fcba8e9f2..f50fa13a1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -115,7 +115,8 @@ def sample_service(notify_db, 'message_limit': limit, 'active': False, 'restricted': restricted, - 'email_from': email_from + 'email_from': email_from, + 'created_by': user } service = Service.query.filter_by(name=service_name).first() if not service: diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 65ab67830..fb6d666ca 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -7,7 +7,8 @@ from app.dao.services_dao import ( dao_fetch_all_services, dao_fetch_service_by_id, dao_fetch_all_services_by_user, - dao_fetch_service_by_id_and_user + dao_fetch_service_by_id_and_user, + dao_update_service ) from app.dao.users_dao import save_model_user from app.models import Service, User @@ -17,7 +18,12 @@ from sqlalchemy.exc import IntegrityError def test_create_service(sample_user): assert Service.query.count() == 0 - service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False) + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) dao_create_service(service, sample_user) assert Service.query.count() == 1 assert Service.query.first().name == "service_name" @@ -27,8 +33,19 @@ def test_create_service(sample_user): def test_cannot_create_two_services_with_same_name(sample_user): assert Service.query.count() == 0 - service1 = Service(name="service_name", email_from="email_from1", message_limit=1000, active=True, restricted=False) - service2 = Service(name="service_name", email_from="email_from2", message_limit=1000, active=True, restricted=False) + service1 = Service(name="service_name", + email_from="email_from1", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + + service2 = Service(name="service_name", + email_from="email_from2", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: dao_create_service(service1, sample_user) dao_create_service(service2, sample_user) @@ -37,24 +54,44 @@ def test_cannot_create_two_services_with_same_name(sample_user): def test_cannot_create_two_services_with_same_email_from(sample_user): assert Service.query.count() == 0 - service1 = Service(name="service_name1", email_from="email_from", message_limit=1000, active=True, restricted=False) - service2 = Service(name="service_name2", email_from="email_from", message_limit=1000, active=True, restricted=False) + service1 = Service(name="service_name1", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + service2 = Service(name="service_name2", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: dao_create_service(service1, sample_user) dao_create_service(service2, sample_user) assert 'duplicate key value violates unique constraint "services_email_from_key"' in str(excinfo.value) -def test_cannot_create_service_with_no_user(notify_db_session): +def test_cannot_create_service_with_no_user(notify_db_session, sample_user): assert Service.query.count() == 0 - service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False) + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) with pytest.raises(FlushError) as excinfo: dao_create_service(service, None) assert "Can't flush None value found in collection Service.users" in str(excinfo.value) def test_should_add_user_to_service(sample_user): - service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False) + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) dao_create_service(service, sample_user) assert sample_user in Service.query.first().users new_user = User( @@ -69,7 +106,12 @@ def test_should_add_user_to_service(sample_user): def test_should_remove_user_from_service(sample_user): - service = Service(name="service_name", email_from="email_from", message_limit=1000, active=True, restricted=False) + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) dao_create_service(service, sample_user) new_user = User( name='Test User', @@ -174,3 +216,74 @@ def test_cannot_get_service_by_id_and_owned_by_different_user(service_factory, s with pytest.raises(NoResultFound) as e: dao_fetch_service_by_id_and_user(service2.id, sample_user.id) assert 'No row was found for one()' in str(e) + + +def test_create_service_creates_a_history_record_with_current_data(sample_user): + assert Service.query.count() == 0 + assert Service.get_history_model().query.count() == 0 + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + dao_create_service(service, sample_user) + assert Service.query.count() == 1 + assert Service.get_history_model().query.count() == 1 + + service_from_db = Service.query.first() + service_history = Service.get_history_model().query.first() + + assert service_from_db.id == service_history.id + assert service_from_db.name == service_history.name + assert service_from_db.version == 1 + assert service_from_db.version == service_history.version + assert sample_user.id == service_history.created_by_id + assert service_from_db.created_by.id == service_history.created_by_id + + +def test_update_service_creates_a_history_record_with_current_data(sample_user): + assert Service.query.count() == 0 + assert Service.get_history_model().query.count() == 0 + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + dao_create_service(service, sample_user) + + assert Service.query.count() == 1 + assert Service.query.first().version == 1 + assert Service.get_history_model().query.count() == 1 + + service.name = 'updated_service_name' + dao_update_service(service) + + assert Service.query.count() == 1 + assert Service.get_history_model().query.count() == 2 + + service_from_db = Service.query.first() + + assert service_from_db.version == 2 + + assert Service.get_history_model().query.filter_by(name='service_name').one().version == 1 + assert Service.get_history_model().query.filter_by(name='updated_service_name').one().version == 2 + + +def test_create_service_and_history_is_transactional(sample_user): + assert Service.query.count() == 0 + assert Service.get_history_model().query.count() == 0 + service = Service(name=None, + email_from="email_from", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + + with pytest.raises(IntegrityError) as excinfo: + dao_create_service(service, sample_user) + + assert 'column "name" violates not-null constraint' in str(excinfo.value) + assert Service.query.count() == 0 + assert Service.get_history_model().query.count() == 0 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 31641097a..3da4d8136 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -181,7 +181,8 @@ def test_create_service(notify_api, sample_user): 'message_limit': 1000, 'restricted': False, 'active': False, - 'email_from': 'created.service'} + 'email_from': 'created.service', + 'created_by': str(sample_user.id)} auth_header = create_authorization_header( path='/service', method='POST', @@ -212,7 +213,7 @@ def test_create_service(notify_api, sample_user): assert json_resp['data']['name'] == 'created service' -def test_should_not_create_service_with_missing_user_id_field(notify_api): +def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { @@ -220,7 +221,8 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api): 'name': 'created service', 'message_limit': 1000, 'restricted': False, - 'active': False + 'active': False, + 'created_by': str(fake_uuid) } auth_header = create_authorization_header( path='/service', @@ -250,7 +252,8 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database(no 'name': 'created service', 'message_limit': 1000, 'restricted': False, - 'active': False + 'active': False, + 'created_by': str(fake_uuid) } auth_header = create_authorization_header( path='/service', @@ -306,7 +309,8 @@ def test_should_not_create_service_with_duplicate_name(notify_api, 'message_limit': 1000, 'restricted': False, 'active': False, - 'email_from': 'sample.service2'} + 'email_from': 'sample.service2', + 'created_by': str(sample_user.id)} auth_header = create_authorization_header( path='/service', method='POST', @@ -324,7 +328,8 @@ def test_should_not_create_service_with_duplicate_name(notify_api, def test_create_service_should_throw_duplicate_key_constraint_for_existing_email_from(notify_api, notify_db, notify_db_session, - service_factory): + service_factory, + sample_user): first_service = service_factory.get('First service', email_from='first.service') with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -334,7 +339,8 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email 'message_limit': 1000, 'restricted': False, 'active': False, - 'email_from': 'first.service'} + 'email_from': 'first.service', + 'created_by': str(sample_user.id)} auth_header = create_authorization_header( path='/service', method='POST', @@ -550,7 +556,8 @@ def test_default_permissions_are_added_for_user_service(notify_api, 'message_limit': 1000, 'restricted': False, 'active': False, - 'email_from': 'created.service'} + 'email_from': 'created.service', + 'created_by': str(sample_user.id)} auth_header = create_authorization_header( path='/service', method='POST', From 8ea8bba6126850763a23d77f6ae0d24319ea1c41 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 19 Apr 2016 16:54:27 +0100 Subject: [PATCH 2/2] Fix migration downgrade --- migrations/versions/0003_add_service_history.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migrations/versions/0003_add_service_history.py b/migrations/versions/0003_add_service_history.py index 306176941..8fff86169 100644 --- a/migrations/versions/0003_add_service_history.py +++ b/migrations/versions/0003_add_service_history.py @@ -33,7 +33,7 @@ def upgrade(): op.add_column('services', sa.Column('created_by_id', postgresql.UUID(as_uuid=True), nullable=True)) op.add_column('services', sa.Column('version', sa.Integer(), nullable=True)) op.create_index(op.f('ix_services_created_by_id'), 'services', ['created_by_id'], unique=False) - op.create_foreign_key(None, 'services', 'users', ['created_by_id'], ['id']) + op.create_foreign_key('fk_services_created_by_id', 'services', 'users', ['created_by_id'], ['id']) op.get_bind() op.execute('UPDATE services SET created_by_id = (SELECT user_id FROM user_to_service WHERE services.id = user_to_service.service_id LIMIT 1)') @@ -47,7 +47,7 @@ def upgrade(): def downgrade(): ### commands auto generated by Alembic - please adjust! ### - op.drop_constraint(None, 'services', type_='foreignkey') + op.drop_constraint('fk_services_created_by_id', 'services', type_='foreignkey') op.drop_index(op.f('ix_services_created_by_id'), table_name='services') op.drop_column('services', 'version') op.drop_column('services', 'created_by_id')