From 6d4bc4c987964292c74b6b8d9ce8086e49734683 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Wed, 1 Apr 2020 21:09:38 +0300 Subject: [PATCH 01/21] Change tracking --- redash/models/__init__.py | 3 +- redash/models/changes.py | 133 +++++++++++++++++++++++++++----------- 2 files changed, 96 insertions(+), 40 deletions(-) diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 68cce99955..158d2dd979 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -42,7 +42,7 @@ from redash.models.parameterized_query import ParameterizedQuery from .base import db, gfk_type, Column, GFKBase, SearchBaseQuery -from .changes import ChangeTrackingMixin, Change # noqa +from .changes import ChangeTrackingMixin, Change, track_changes # noqa from .mixins import BelongsToOrgMixin, TimestampMixin from .organizations import Organization from .types import ( @@ -451,6 +451,7 @@ def should_schedule_next( "schedule", "schedule_failures", ) +@track_changes("query_text") class Query(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) version = Column(db.Integer, default=1) diff --git a/redash/models/changes.py b/redash/models/changes.py index a670703514..4812836108 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -1,5 +1,8 @@ from sqlalchemy.inspection import inspect from sqlalchemy_utils.models import generic_repr +from sqlalchemy.orm import object_session +from sqlalchemy.orm.session import Session +from sqlalchemy import event from .base import GFKBase, db, Column from .types import PseudoJSON @@ -47,47 +50,99 @@ def last_change(cls, obj): class ChangeTrackingMixin(object): - skipped_fields = ("id", "created_at", "updated_at", "version") - _clean_values = None - - def __init__(self, *a, **kw): - super(ChangeTrackingMixin, self).__init__(*a, **kw) - self.record_changes(self.user) - - def prep_cleanvalues(self): - self.__dict__["_clean_values"] = {} - for attr in inspect(self.__class__).column_attrs: - col, = attr.columns - # 'query' is col name but not attr name - self._clean_values[col.name] = None - - def __setattr__(self, key, value): - if self._clean_values is None: - self.prep_cleanvalues() - for attr in inspect(self.__class__).column_attrs: - col, = attr.columns - previous = getattr(self, attr.key, None) - self._clean_values[col.name] = previous - - super(ChangeTrackingMixin, self).__setattr__(key, value) - def record_changes(self, changed_by): - db.session.add(self) - db.session.flush() - changes = {} - for attr in inspect(self.__class__).column_attrs: - col, = attr.columns - if attr.key not in self.skipped_fields: - changes[col.name] = { - "previous": self._clean_values[col.name], - "current": getattr(self, attr.key), - } - - db.session.add( + pass + + # skipped_fields = ("id", "created_at", "updated_at", "version") + # _clean_values = None + # + # def __init__(self, *a, **kw): + # super(ChangeTrackingMixin, self).__init__(*a, **kw) + # self.record_changes(self.user) + # + # def prep_cleanvalues(self): + # self.__dict__["_clean_values"] = {} + # for attr in inspect(self.__class__).column_attrs: + # col, = attr.columns + # # 'query' is col name but not attr name + # self._clean_values[col.name] = None + # + # def __setattr__(self, key, value): + # if self._clean_values is None: + # self.prep_cleanvalues() + # for attr in inspect(self.__class__).column_attrs: + # col, = attr.columns + # previous = getattr(self, attr.key, None) + # self._clean_values[col.name] = previous + # + # super(ChangeTrackingMixin, self).__setattr__(key, value) + # + # def record_changes(self, changed_by): + # db.session.add(self) + # db.session.flush() + # changes = {} + # for attr in inspect(self.__class__).column_attrs: + # col, = attr.columns + # if attr.key not in self.skipped_fields: + # changes[col.name] = { + # "previous": self._clean_values[col.name], + # "current": getattr(self, attr.key), + # } + # + # db.session.add( + # Change( + # object=self, + # object_version=self.version, + # user=changed_by, + # change=changes, + # ) + # ) + + +@event.listens_for(Session, 'before_flush') +def my_before_flush(session, flush_context, instances): + changed_objects = session.info.get("__change_tracking__", set()) + for obj in changed_objects: + changes = getattr(obj, "__changes__") + # TODO: use inspect(self.__class__).column_attrs to map attributes to fields + session.add( Change( - object=self, - object_version=self.version, - user=changed_by, + object=obj, + object_version=obj.version, + user=obj.user, # ??? change=changes, ) ) + obj.__changes__ = {} + session.info["__change_tracking__"] = set() + + +def track_changes(*attrs): + def decorator(cls): + class ChangeTracking(cls): + def __setattr__(self, key, value): + previous = None + if key in attrs: + previous = getattr(self, key) + + super(ChangeTracking, self).__setattr__(key, value) + + if key in attrs: + if not hasattr(self, "__changes__"): + self.__changes__ = {} + change = self.__changes__.get(key, { + "previous": previous, + "current": None, + }) + change["current"] = getattr(self, key) + self.__changes__[key] = change + + # make object available in `before_flush` hook + session = object_session(self) + changed_objects = session.info.get("__change_tracking__", set()) + changed_objects.add(self) + session.info["__change_tracking__"] = changed_objects + + return ChangeTracking + + return decorator From dcf2a55fe4b99d545f217dc3251c010d005627af Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Thu, 2 Apr 2020 21:20:00 +0300 Subject: [PATCH 02/21] Change tracking --- redash/handlers/dashboards.py | 1 - redash/handlers/queries.py | 2 +- redash/models/__init__.py | 24 +++++-- redash/models/changes.py | 130 ++++++++++++++-------------------- 4 files changed, 70 insertions(+), 87 deletions(-) diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index c5fceab7c1..fceba570f4 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -240,7 +240,6 @@ def delete(self, dashboard_slug): dashboard_slug, self.current_org ) dashboard.is_archived = True - dashboard.record_changes(changed_by=self.current_user) models.db.session.add(dashboard) d = serialize_dashboard(dashboard, with_widgets=True, user=self.current_user) models.db.session.commit() diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index 129f7e3e6d..a7948819e4 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -416,7 +416,7 @@ def delete(self, query_id): models.Query.get_by_id_and_org, query_id, self.current_org ) require_admin_or_owner(query.user_id) - query.archive(self.current_user) + query.archive() models.db.session.commit() diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 158d2dd979..a64301e618 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -42,7 +42,7 @@ from redash.models.parameterized_query import ParameterizedQuery from .base import db, gfk_type, Column, GFKBase, SearchBaseQuery -from .changes import ChangeTrackingMixin, Change, track_changes # noqa +from .changes import Change, track_changes # noqa from .mixins import BelongsToOrgMixin, TimestampMixin from .organizations import Organization from .types import ( @@ -451,8 +451,20 @@ def should_schedule_next( "schedule", "schedule_failures", ) -@track_changes("query_text") -class Query(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model): +@track_changes( + attributes=[ + "name", + "description", + "query_text", + "is_archived", + "is_draft", + "api_key", + "options", + "schedule", + "tags", + ] +) +class Query(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) version = Column(db.Integer, default=1) org_id = Column(db.Integer, db.ForeignKey("organizations.id")) @@ -501,7 +513,7 @@ class Query(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model): def __str__(self): return str(self.id) - def archive(self, user=None): + def archive(self): db.session.add(self) self.is_archived = True self.schedule = None @@ -513,8 +525,6 @@ def archive(self, user=None): for a in self.alerts: db.session.delete(a) - if user: - self.record_changes(user) def regenerate_api_key(self): self.api_key = generate_token(40) @@ -1056,7 +1066,7 @@ def generate_slug(ctx): @generic_repr( "id", "name", "slug", "user_id", "org_id", "version", "is_archived", "is_draft" ) -class Dashboard(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model): +class Dashboard(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) version = Column(db.Integer) org_id = Column(db.Integer, db.ForeignKey("organizations.id")) diff --git a/redash/models/changes.py b/redash/models/changes.py index 4812836108..e37dc01a12 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -1,12 +1,13 @@ from sqlalchemy.inspection import inspect from sqlalchemy_utils.models import generic_repr -from sqlalchemy.orm import object_session from sqlalchemy.orm.session import Session from sqlalchemy import event from .base import GFKBase, db, Column from .types import PseudoJSON +import logging + @generic_repr("id", "object_type", "object_id", "created_at") class Change(GFKBase, db.Model): @@ -49,99 +50,72 @@ def last_change(cls, obj): ) -class ChangeTrackingMixin(object): - def record_changes(self, changed_by): - pass - - # skipped_fields = ("id", "created_at", "updated_at", "version") - # _clean_values = None - # - # def __init__(self, *a, **kw): - # super(ChangeTrackingMixin, self).__init__(*a, **kw) - # self.record_changes(self.user) - # - # def prep_cleanvalues(self): - # self.__dict__["_clean_values"] = {} - # for attr in inspect(self.__class__).column_attrs: - # col, = attr.columns - # # 'query' is col name but not attr name - # self._clean_values[col.name] = None - # - # def __setattr__(self, key, value): - # if self._clean_values is None: - # self.prep_cleanvalues() - # for attr in inspect(self.__class__).column_attrs: - # col, = attr.columns - # previous = getattr(self, attr.key, None) - # self._clean_values[col.name] = previous - # - # super(ChangeTrackingMixin, self).__setattr__(key, value) - # - # def record_changes(self, changed_by): - # db.session.add(self) - # db.session.flush() - # changes = {} - # for attr in inspect(self.__class__).column_attrs: - # col, = attr.columns - # if attr.key not in self.skipped_fields: - # changes[col.name] = { - # "previous": self._clean_values[col.name], - # "current": getattr(self, attr.key), - # } - # - # db.session.add( - # Change( - # object=self, - # object_version=self.version, - # user=changed_by, - # change=changes, - # ) - # ) - - -@event.listens_for(Session, 'before_flush') -def my_before_flush(session, flush_context, instances): - changed_objects = session.info.get("__change_tracking__", set()) - for obj in changed_objects: - changes = getattr(obj, "__changes__") - # TODO: use inspect(self.__class__).column_attrs to map attributes to fields +def get_object_changes(obj, reset=True): + result = {} + changes = getattr(obj, "__changes__", None) + + if changes: + columns = {} + for attr in inspect(obj.__class__).column_attrs: + col, = attr.columns + columns[attr.key] = col.name + + for key, change in changes.items(): + if change["current"] != change["previous"]: + col = columns.get(key, key) + result[col] = change + + if reset: + changes.clear() + + return result + + +def record_object_changes(session, obj, action): + changes = get_object_changes(obj) + if changes: session.add( Change( object=obj, object_version=obj.version, - user=obj.user, # ??? - change=changes, + # user=changed_by, + user_id=1, # TODO: current user - ? + change={ + "type": action, + "changes": changes, + }, ) ) - obj.__changes__ = {} - session.info["__change_tracking__"] = set() -def track_changes(*attrs): +@event.listens_for(Session, 'after_flush') +def handle_before_flush(session, flush_context): + # It's safe to insert new records, but do not read anything from DB here! + for obj in session.new: + record_object_changes(session, obj, "created") + for obj in session.dirty: + record_object_changes(session, obj, "modified") + for obj in session.deleted: + record_object_changes(session, obj, "deleted") + + +def track_changes(attributes): + attributes = set(attributes) - {"id", "created_at", "updated_at", "version"} + def decorator(cls): class ChangeTracking(cls): - def __setattr__(self, key, value): - previous = None - if key in attrs: - previous = getattr(self, key) + __changes__ = {} - super(ChangeTracking, self).__setattr__(key, value) - - if key in attrs: - if not hasattr(self, "__changes__"): - self.__changes__ = {} + def __setattr__(self, key, value): + if key in attributes: change = self.__changes__.get(key, { - "previous": previous, + "previous": getattr(self, key), "current": None, }) - change["current"] = getattr(self, key) + change["current"] = value self.__changes__[key] = change - # make object available in `before_flush` hook - session = object_session(self) - changed_objects = session.info.get("__change_tracking__", set()) - changed_objects.add(self) - session.info["__change_tracking__"] = changed_objects + super(ChangeTracking, self).__setattr__(key, value) return ChangeTracking From 1d440fa8e205aa3501cce59a2b1926262f2fd8af Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Fri, 3 Apr 2020 16:44:06 +0300 Subject: [PATCH 03/21] Change tracking: query and dashboard attributes --- redash/handlers/dashboards.py | 13 +++++--- redash/handlers/queries.py | 9 ++++++ redash/models/__init__.py | 1 + redash/models/changes.py | 61 +++++++++++++++++------------------ 4 files changed, 47 insertions(+), 37 deletions(-) diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index fceba570f4..95911f000e 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -107,6 +107,10 @@ def post(self): ) models.db.session.add(dashboard) models.db.session.commit() + + dashboard.record_changes(self.current_user, models.Change.Type.Created) + models.db.session.commit() + return serialize_dashboard(dashboard) @@ -210,9 +214,9 @@ def post(self, dashboard_slug): updates["changed_by"] = self.current_user - self.update_model(dashboard, updates) - models.db.session.add(dashboard) try: + self.update_model(dashboard, updates) + dashboard.record_changes(self.current_user) models.db.session.commit() except StaleDataError: abort(409) @@ -240,15 +244,14 @@ def delete(self, dashboard_slug): dashboard_slug, self.current_org ) dashboard.is_archived = True - models.db.session.add(dashboard) - d = serialize_dashboard(dashboard, with_widgets=True, user=self.current_user) + dashboard.record_changes(self.current_user) models.db.session.commit() self.record_event( {"action": "archive", "object_id": dashboard.id, "object_type": "dashboard"} ) - return d + return serialize_dashboard(dashboard, with_widgets=True, user=self.current_user) class PublicDashboardResource(BaseResource): diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index a7948819e4..0e283861d9 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -259,6 +259,9 @@ def post(self): models.db.session.add(query) models.db.session.commit() + query.record_changes(self.current_user, models.Change.Type.Created) + models.db.session.commit() + self.record_event( {"action": "create", "object_id": query.id, "object_type": "query"} ) @@ -376,6 +379,7 @@ def post(self, query_id): try: self.update_model(query, query_def) + query.record_changes(self.current_user) models.db.session.commit() except StaleDataError: abort(409) @@ -417,6 +421,7 @@ def delete(self, query_id): ) require_admin_or_owner(query.user_id) query.archive() + query.record_changes(self.current_user) models.db.session.commit() @@ -428,6 +433,7 @@ def post(self, query_id): ) require_admin_or_owner(query.user_id) query.regenerate_api_key() + query.record_changes(self.current_user) models.db.session.commit() self.record_event( @@ -459,6 +465,9 @@ def post(self, query_id): forked_query = query.fork(self.current_user) models.db.session.commit() + forked_query.record_changes(self.current_user, models.Change.Type.Created) + models.db.session.commit() + self.record_event( {"action": "fork", "object_id": query_id, "object_type": "query"} ) diff --git a/redash/models/__init__.py b/redash/models/__init__.py index a64301e618..36f7b63410 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -1066,6 +1066,7 @@ def generate_slug(ctx): @generic_repr( "id", "name", "slug", "user_id", "org_id", "version", "is_archived", "is_draft" ) +@track_changes(attributes=["name", "is_archived", "is_draft", "tags", "dashboard_filters_enabled"]) class Dashboard(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) version = Column(db.Integer) diff --git a/redash/models/changes.py b/redash/models/changes.py index e37dc01a12..bf0ed5d206 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -1,16 +1,20 @@ from sqlalchemy.inspection import inspect from sqlalchemy_utils.models import generic_repr -from sqlalchemy.orm.session import Session -from sqlalchemy import event +from sqlalchemy.orm import object_session + +from enum import Enum from .base import GFKBase, db, Column from .types import PseudoJSON -import logging - @generic_repr("id", "object_type", "object_id", "created_at") class Change(GFKBase, db.Model): + class Type(str, Enum): # `str` to make it json-serializable + Created = "created" + Modified = "modified" + Deleted = "deleted" + id = Column(db.Integer, primary_key=True) # 'object' defined in GFKBase object_version = Column(db.Integer, default=0) @@ -71,34 +75,6 @@ def get_object_changes(obj, reset=True): return result -def record_object_changes(session, obj, action): - changes = get_object_changes(obj) - if changes: - session.add( - Change( - object=obj, - object_version=obj.version, - # user=changed_by, - user_id=1, # TODO: current user - ? - change={ - "type": action, - "changes": changes, - }, - ) - ) - - -@event.listens_for(Session, 'after_flush') -def handle_before_flush(session, flush_context): - # It's safe to insert new records, but do not read anything from DB here! - for obj in session.new: - record_object_changes(session, obj, "created") - for obj in session.dirty: - record_object_changes(session, obj, "modified") - for obj in session.deleted: - record_object_changes(session, obj, "deleted") - - def track_changes(attributes): attributes = set(attributes) - {"id", "created_at", "updated_at", "version"} @@ -117,6 +93,27 @@ def __setattr__(self, key, value): super(ChangeTracking, self).__setattr__(key, value) + def record_changes(self, changed_by, change_type=Change.Type.Modified): + session = object_session(self) + if not session: + return + + changes = get_object_changes(self) + if not changes: + return + + session.add( + Change( + object=self, + object_version=self.version, + user=changed_by, + change={ + "type": change_type, + "changes": changes, + }, + ) + ) + return ChangeTracking return decorator From 49feef70841492e608239febfd165407507169c7 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Fri, 3 Apr 2020 17:35:35 +0300 Subject: [PATCH 04/21] Change tracking: visualization and widget attributes --- redash/handlers/visualizations.py | 10 ++++++++-- redash/handlers/widgets.py | 4 ++++ redash/models/__init__.py | 6 ++++-- redash/models/changes.py | 5 +++-- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/redash/handlers/visualizations.py b/redash/handlers/visualizations.py index 1621ea50cd..e2f42a58b5 100644 --- a/redash/handlers/visualizations.py +++ b/redash/handlers/visualizations.py @@ -23,6 +23,10 @@ def post(self): vis = models.Visualization(**kwargs) models.db.session.add(vis) models.db.session.commit() + + vis.record_changes(self.current_user, models.Change.Type.Created) + models.db.session.commit() + return serialize_visualization(vis, with_query=False) @@ -42,9 +46,10 @@ def post(self, visualization_id): kwargs.pop("query_id", None) self.update_model(vis, kwargs) - d = serialize_visualization(vis, with_query=False) + vis.record_changes(self.current_user) models.db.session.commit() - return d + + return serialize_visualization(vis, with_query=False) @require_permission("edit_query") def delete(self, visualization_id): @@ -59,5 +64,6 @@ def delete(self, visualization_id): "object_type": "Visualization", } ) + vis.record_changes(self.current_user, models.Change.Type.Deleted) models.db.session.delete(vis) models.db.session.commit() diff --git a/redash/handlers/widgets.py b/redash/handlers/widgets.py index 6907943405..491bc8a309 100644 --- a/redash/handlers/widgets.py +++ b/redash/handlers/widgets.py @@ -50,7 +50,9 @@ def post(self): models.db.session.add(widget) models.db.session.commit() + widget.record_changes(self.current_user, models.Change.Type.Created) models.db.session.commit() + return serialize_widget(widget) @@ -70,6 +72,7 @@ def post(self, widget_id): widget_properties = request.get_json(force=True) widget.text = widget_properties["text"] widget.options = json_dumps(widget_properties["options"]) + widget.record_changes(self.current_user) models.db.session.commit() return serialize_widget(widget) @@ -85,5 +88,6 @@ def delete(self, widget_id): self.record_event( {"action": "delete", "object_id": widget_id, "object_type": "widget"} ) + widget.record_changes(self.current_user, models.Change.Type.Deleted) models.db.session.delete(widget) models.db.session.commit() diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 36f7b63410..53fdeb1969 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -1173,6 +1173,7 @@ def lowercase_name(cls): @generic_repr("id", "name", "type", "query_id") +@track_changes(attributes=["type", "name", "description", "options"]) class Visualization(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) type = Column(db.String(100)) @@ -1190,7 +1191,7 @@ def __str__(self): @classmethod def get_by_id_and_org(cls, object_id, org): - return super(Visualization, cls).get_by_id_and_org(object_id, org, Query) + return super().get_by_id_and_org(object_id, org, Query) def copy(self): return { @@ -1202,6 +1203,7 @@ def copy(self): @generic_repr("id", "visualization_id", "dashboard_id") +@track_changes(attributes=["text", "visualization_id", "options"]) class Widget(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) visualization_id = Column( @@ -1222,7 +1224,7 @@ def __str__(self): @classmethod def get_by_id_and_org(cls, object_id, org): - return super(Widget, cls).get_by_id_and_org(object_id, org, Dashboard) + return super().get_by_id_and_org(object_id, org, Dashboard) @generic_repr( diff --git a/redash/models/changes.py b/redash/models/changes.py index bf0ed5d206..3485e01f20 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -99,13 +99,14 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): return changes = get_object_changes(self) - if not changes: + # for `created` and `deleted` log even empty changes set + if not changes and (change_type == Change.Type.Modified): return session.add( Change( object=self, - object_version=self.version, + object_version=getattr(self, "version", None), user=changed_by, change={ "type": change_type, From b2adadcc33248d1370178558a09a1097ed74d03a Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Fri, 3 Apr 2020 21:16:18 +0300 Subject: [PATCH 05/21] Log changes to nested objects (visualizations, widgets) related to their parents (query, dashboard) --- redash/models/__init__.py | 10 ++++++++-- redash/models/changes.py | 28 +++++++++++++++++----------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 53fdeb1969..4932a34d32 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -1173,7 +1173,10 @@ def lowercase_name(cls): @generic_repr("id", "name", "type", "query_id") -@track_changes(attributes=["type", "name", "description", "options"]) +@track_changes( + parent_attr="query_rel", + attributes=["type", "name", "description", "options"] +) class Visualization(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) type = Column(db.String(100)) @@ -1203,7 +1206,10 @@ def copy(self): @generic_repr("id", "visualization_id", "dashboard_id") -@track_changes(attributes=["text", "visualization_id", "options"]) +@track_changes( + parent_attr="dashboard", + attributes=["text", "visualization_id", "options"] +) class Widget(TimestampMixin, BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) visualization_id = Column( diff --git a/redash/models/changes.py b/redash/models/changes.py index 3485e01f20..ea61c7dfd2 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -75,7 +75,7 @@ def get_object_changes(obj, reset=True): return result -def track_changes(attributes): +def track_changes(attributes, parent_attr=None): attributes = set(attributes) - {"id", "created_at", "updated_at", "version"} def decorator(cls): @@ -103,18 +103,24 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): if not changes and (change_type == Change.Type.Modified): return - session.add( - Change( - object=self, - object_version=getattr(self, "version", None), - user=changed_by, - change={ - "type": change_type, - "changes": changes, - }, - ) + changes = Change( + object=self, + object_version=getattr(self, "version", None), + user=changed_by, + change={ + "object_type": self.__table__.name, + "object_id": self.id, + "change_type": change_type, + "changes": changes, + }, ) + if parent_attr: + changes.object = getattr(self, parent_attr, self) + changes.object_version = getattr(changes.object, "version", None), + + session.add(changes) + return ChangeTracking return decorator From 3b4bc15bfa6ef8d259789f4860fdb5116b44f617 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 6 Apr 2020 16:25:06 +0300 Subject: [PATCH 06/21] Migraton to clean changes table --- migrations/versions/4952e040e9dd_.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 migrations/versions/4952e040e9dd_.py diff --git a/migrations/versions/4952e040e9dd_.py b/migrations/versions/4952e040e9dd_.py new file mode 100644 index 0000000000..854addfc20 --- /dev/null +++ b/migrations/versions/4952e040e9dd_.py @@ -0,0 +1,24 @@ +"""clear_changes_table + +Revision ID: 4952e040e9dd +Revises: e5c7a4e2df4d +Create Date: 2020-04-06 13:22:15.256635 + +""" +from alembic import op + + +# revision identifiers, used by Alembic. +revision = '4952e040e9dd' +down_revision = 'e5c7a4e2df4d' +branch_labels = None +depends_on = None + + +def upgrade(): + connection = op.get_bind() + connection.execute("DELETE FROM changes") + + +def downgrade(): + pass From 15d3af32f3fcc0ce55a808cbb1c3948817d49877 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 6 Apr 2020 19:46:59 +0300 Subject: [PATCH 07/21] Replace inheritance with patching to avoid SQLAlchemy issues --- redash/models/changes.py | 100 ++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 44 deletions(-) diff --git a/redash/models/changes.py b/redash/models/changes.py index ea61c7dfd2..04faee7929 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -54,7 +54,7 @@ def last_change(cls, obj): ) -def get_object_changes(obj, reset=True): +def _get_object_changes(obj, reset=True): result = {} changes = getattr(obj, "__changes__", None) @@ -75,52 +75,64 @@ def get_object_changes(obj, reset=True): return result +def _patch_setattr_method(cls, attributes): + original_setattr = cls.__setattr__ + + def new_setattr(self, key, value): + if key in attributes: + if not hasattr(self, "__changes__"): + self.__changes__ = {} + change = self.__changes__.get(key, { + "previous": getattr(self, key), + "current": None, + }) + change["current"] = value + self.__changes__[key] = change + + original_setattr(self, key, value) + + cls.__setattr__ = new_setattr + + +def _patch_record_changes_method(cls, parent_attr): + def record_changes(self, changed_by, change_type=Change.Type.Modified): + session = object_session(self) + if not session: + return + + changes = _get_object_changes(self) + # for `created` and `deleted` log even empty changes set + if not changes and (change_type == Change.Type.Modified): + return + + changes = Change( + object=self, + object_version=getattr(self, "version", None), + user=changed_by, + change={ + "object_type": self.__table__.name, + "object_id": self.id, + "change_type": change_type, + "changes": changes, + }, + ) + + if parent_attr: + changes.object = getattr(self, parent_attr, self) + changes.object_version = getattr(changes.object, "version", None), + + session.add(changes) + + cls.record_changes = record_changes + + def track_changes(attributes, parent_attr=None): attributes = set(attributes) - {"id", "created_at", "updated_at", "version"} + # monkey-patch class because inheritance will break SQLAlchemy def decorator(cls): - class ChangeTracking(cls): - __changes__ = {} - - def __setattr__(self, key, value): - if key in attributes: - change = self.__changes__.get(key, { - "previous": getattr(self, key), - "current": None, - }) - change["current"] = value - self.__changes__[key] = change - - super(ChangeTracking, self).__setattr__(key, value) - - def record_changes(self, changed_by, change_type=Change.Type.Modified): - session = object_session(self) - if not session: - return - - changes = get_object_changes(self) - # for `created` and `deleted` log even empty changes set - if not changes and (change_type == Change.Type.Modified): - return - - changes = Change( - object=self, - object_version=getattr(self, "version", None), - user=changed_by, - change={ - "object_type": self.__table__.name, - "object_id": self.id, - "change_type": change_type, - "changes": changes, - }, - ) - - if parent_attr: - changes.object = getattr(self, parent_attr, self) - changes.object_version = getattr(changes.object, "version", None), - - session.add(changes) - - return ChangeTracking + _patch_setattr_method(cls, attributes) + _patch_record_changes_method(cls, parent_attr) + return cls return decorator From 527b73a8f8017c2f4fde61258370d097e18e3335 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Tue, 7 Apr 2020 20:26:54 +0300 Subject: [PATCH 08/21] Temp --- tests/models/test_changes.py | 51 +++++++++++++++--------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/tests/models/test_changes.py b/tests/models/test_changes.py index e9847b0a0a..47e7af832e 100644 --- a/tests/models/test_changes.py +++ b/tests/models/test_changes.py @@ -1,6 +1,6 @@ from tests import BaseTestCase -from redash.models import db, Query, Change, ChangeTrackingMixin +from redash.models import db, Query, Change def create_object(factory): @@ -12,18 +12,12 @@ def create_object(factory): data_source=factory.data_source, org=factory.org, ) + db.session.add(obj) + db.session.commit() return obj -class TestChangesProperty(BaseTestCase): - def test_returns_initial_state(self): - obj = create_object(self.factory) - - for change in Change.query.filter(Change.object == obj): - self.assertIsNone(change.change["previous"]) - - class TestLogChange(BaseTestCase): def obj(self): obj = Query( @@ -37,25 +31,36 @@ def obj(self): return obj - def test_properly_logs_first_creation(self): + def test_logs_creation(self): obj = create_object(self.factory) - obj.record_changes(changed_by=self.factory.user) + obj.record_changes(self.factory.user, Change.Type.Created) + db.session.commit() change = Change.last_change(obj) self.assertIsNotNone(change) - self.assertEqual(change.object_version, 1) + self.assertEqual(change.change["change_type"], Change.Type.Created) + + def test_logs_modification(self): + obj = create_object(self.factory) + obj.record_changes(self.factory.user) + db.session.commit() + change = Change.last_change(obj) + + self.assertIsNotNone(change) + self.assertEqual(change.change["change_type"], Change.Type.Modified) - def test_skips_unnecessary_fields(self): + def _test_skips_unnecessary_fields(self): obj = create_object(self.factory) obj.record_changes(changed_by=self.factory.user) + db.session.commit() change = Change.last_change(obj) self.assertIsNotNone(change) self.assertEqual(change.object_version, 1) - for field in ChangeTrackingMixin.skipped_fields: - self.assertNotIn(field, change.change) + # for field in ChangeTrackingMixin.skipped_fields: + # self.assertNotIn(field, change.change) - def test_properly_log_modification(self): + def _test_properly_log_modification(self): obj = create_object(self.factory) obj.record_changes(changed_by=self.factory.user) obj.name = "Query 2" @@ -71,17 +76,3 @@ def test_properly_log_modification(self): self.assertEqual(change.object_version, obj.version) self.assertIn("name", change.change) self.assertIn("description", change.change) - - def test_logs_create_method(self): - q = Query( - name="Query", - description="", - query_text="", - user=self.factory.user, - data_source=self.factory.data_source, - org=self.factory.org, - ) - change = Change.last_change(q) - - self.assertIsNotNone(change) - self.assertEqual(q.user, change.user) From 69ee572be78d638378335c78744ff3d7bc53111d Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 13 Apr 2020 12:37:27 +0300 Subject: [PATCH 09/21] Update migration: add index, drop unused column, add downgrade script --- migrations/versions/4952e040e9dd_.py | 24 ------------- .../4952e040e9dd_clear_changes_table.py | 34 +++++++++++++++++++ redash/models/changes.py | 6 +--- 3 files changed, 35 insertions(+), 29 deletions(-) delete mode 100644 migrations/versions/4952e040e9dd_.py create mode 100644 migrations/versions/4952e040e9dd_clear_changes_table.py diff --git a/migrations/versions/4952e040e9dd_.py b/migrations/versions/4952e040e9dd_.py deleted file mode 100644 index 854addfc20..0000000000 --- a/migrations/versions/4952e040e9dd_.py +++ /dev/null @@ -1,24 +0,0 @@ -"""clear_changes_table - -Revision ID: 4952e040e9dd -Revises: e5c7a4e2df4d -Create Date: 2020-04-06 13:22:15.256635 - -""" -from alembic import op - - -# revision identifiers, used by Alembic. -revision = '4952e040e9dd' -down_revision = 'e5c7a4e2df4d' -branch_labels = None -depends_on = None - - -def upgrade(): - connection = op.get_bind() - connection.execute("DELETE FROM changes") - - -def downgrade(): - pass diff --git a/migrations/versions/4952e040e9dd_clear_changes_table.py b/migrations/versions/4952e040e9dd_clear_changes_table.py new file mode 100644 index 0000000000..ad5b4f565a --- /dev/null +++ b/migrations/versions/4952e040e9dd_clear_changes_table.py @@ -0,0 +1,34 @@ +"""clear_changes_table + +Revision ID: 4952e040e9dd +Revises: e5c7a4e2df4d +Create Date: 2020-04-06 13:22:15.256635 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '4952e040e9dd' +down_revision = 'e5c7a4e2df4d' +branch_labels = None +depends_on = None + + +def upgrade(): + op.execute("DELETE FROM changes") + op.drop_column("changes", "object_version") + op.create_index( + "ix_changes_object_ref", + "changes", + ["object_type", "object_id"], + unique=False, + ) + + +def downgrade(): + op.add_column("changes", sa.Column("object_version", sa.Integer(), nullable=True, default=0)) + op.execute("UPDATE changes SET object_version = 0") + op.alter_column("changes", "object_version", nullable=False) + op.drop_index("ix_changes_object_ref") diff --git a/redash/models/changes.py b/redash/models/changes.py index 04faee7929..dfed126520 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -17,7 +17,6 @@ class Type(str, Enum): # `str` to make it json-serializable id = Column(db.Integer, primary_key=True) # 'object' defined in GFKBase - object_version = Column(db.Integer, default=0) user_id = Column(db.Integer, db.ForeignKey("users.id")) user = db.relationship("User", backref="changes") change = Column(PseudoJSON) @@ -31,7 +30,6 @@ def to_dict(self, full=True): "object_id": self.object_id, "object_type": self.object_type, "change_type": self.change_type, - "object_version": self.object_version, "change": self.change, "created_at": self.created_at, } @@ -49,7 +47,7 @@ def last_change(cls, obj): cls.query.filter( cls.object_id == obj.id, cls.object_type == obj.__class__.__tablename__ ) - .order_by(cls.object_version.desc()) + .order_by(cls.id.desc()) .first() ) @@ -107,7 +105,6 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): changes = Change( object=self, - object_version=getattr(self, "version", None), user=changed_by, change={ "object_type": self.__table__.name, @@ -119,7 +116,6 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): if parent_attr: changes.object = getattr(self, parent_attr, self) - changes.object_version = getattr(changes.object, "version", None), session.add(changes) From 1a8b46e0cb75237c4a9656daa930f91ff6207c61 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 13 Apr 2020 13:13:50 +0300 Subject: [PATCH 10/21] Ensure that all fields are logged when creating a new object --- redash/models/changes.py | 53 +++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/redash/models/changes.py b/redash/models/changes.py index dfed126520..5664a89ab0 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -7,6 +7,23 @@ from .base import GFKBase, db, Column from .types import PseudoJSON +# Structure of Change object +# user_id +# reference to a user that modified object +# created_at +# time when modification was made +# object_type/object_id +# reference to modified object (e.g. query, dashboard) +# changes.object_type/changes.object_id +# reference to a real target. When there are relation between entities, object_type/object_id contains +# a reference to "parent", and changes.object_type/changes.object_id contains a reference to "child". +# For example, when modifying a visualization, changes.object_type/changes.object_id will be the reference +# to that visualization, and object_type/object_id will be the reference to a query for this visualization. +# changes.changes +# a dictionary where keys are names of modified fields and values are tuples with field values. +# Tuple may contain one value if field wasn't changes, or two values - previous and new one. In both cases +# tuple is used to avoid situations when field contains an JSON array and may be wrongly interpreted. + @generic_repr("id", "object_type", "object_id", "created_at") class Change(GFKBase, db.Model): @@ -65,11 +82,31 @@ def _get_object_changes(obj, reset=True): for key, change in changes.items(): if change["current"] != change["previous"]: col = columns.get(key, key) - result[col] = change + result[col] = (change["previous"], change["current"]) if reset: changes.clear() + return result if result else None + + +def _collect_all_attributes(obj, attributes, reset=True): + result = {} + + columns = {} + for attr in inspect(obj.__class__).column_attrs: + col, = attr.columns + columns[attr.key] = col.name + + for key in attributes: + col = columns.get(key, key) + value = getattr(obj, key, None) + result[col] = (value,) + + changes = getattr(obj, "__changes__", None) + if changes and reset: + changes.clear() + return result @@ -92,15 +129,19 @@ def new_setattr(self, key, value): cls.__setattr__ = new_setattr -def _patch_record_changes_method(cls, parent_attr): +def _patch_record_changes_method(cls, attributes, parent_attr): def record_changes(self, changed_by, change_type=Change.Type.Modified): session = object_session(self) if not session: return - changes = _get_object_changes(self) - # for `created` and `deleted` log even empty changes set - if not changes and (change_type == Change.Type.Modified): + changes = {} + if change_type == Change.Type.Created: + changes = _collect_all_attributes(self, attributes) + if change_type == Change.Type.Modified: + changes = _get_object_changes(self) + + if changes is None: return changes = Change( @@ -128,7 +169,7 @@ def track_changes(attributes, parent_attr=None): # monkey-patch class because inheritance will break SQLAlchemy def decorator(cls): _patch_setattr_method(cls, attributes) - _patch_record_changes_method(cls, parent_attr) + _patch_record_changes_method(cls, attributes, parent_attr) return cls return decorator From baac67aca00a578b2cf1d3744916d6e3cf4a4394 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 13 Apr 2020 13:41:17 +0300 Subject: [PATCH 11/21] Log changes to API keys; optimize a way we access to a parent object (stop loading it from DB) --- redash/handlers/dashboards.py | 5 ++++- redash/models/__init__.py | 8 ++++++-- redash/models/changes.py | 26 ++++++++++++++++++-------- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index 95911f000e..e3490c4ebc 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -285,7 +285,9 @@ def post(self, dashboard_id): dashboard = models.Dashboard.get_by_id_and_org(dashboard_id, self.current_org) require_admin_or_owner(dashboard.user_id) api_key = models.ApiKey.create_for_object(dashboard, self.current_user) - models.db.session.flush() + models.db.session.commit() + + api_key.record_changes(self.current_user, models.Change.Type.Created) models.db.session.commit() public_url = url_for( @@ -318,6 +320,7 @@ def delete(self, dashboard_id): if api_key: api_key.active = False models.db.session.add(api_key) + api_key.record_changes(self.current_user) models.db.session.commit() self.record_event( diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 4932a34d32..9924f9f3fb 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -1174,7 +1174,7 @@ def lowercase_name(cls): @generic_repr("id", "name", "type", "query_id") @track_changes( - parent_attr="query_rel", + parent=(Query, "query_id"), attributes=["type", "name", "description", "options"] ) class Visualization(TimestampMixin, BelongsToOrgMixin, db.Model): @@ -1207,7 +1207,7 @@ def copy(self): @generic_repr("id", "visualization_id", "dashboard_id") @track_changes( - parent_attr="dashboard", + parent=(Dashboard, "dashboard_id"), attributes=["text", "visualization_id", "options"] ) class Widget(TimestampMixin, BelongsToOrgMixin, db.Model): @@ -1295,6 +1295,10 @@ def record(cls, event): @generic_repr("id", "created_by_id", "org_id", "active") +@track_changes( + parent=("object_type", "object_id"), + attributes=["api_key", "created_by_id", "active"] +) class ApiKey(TimestampMixin, GFKBase, db.Model): id = Column(db.Integer, primary_key=True) org_id = Column(db.Integer, db.ForeignKey("organizations.id")) diff --git a/redash/models/changes.py b/redash/models/changes.py index 5664a89ab0..830b5802cc 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -2,6 +2,7 @@ from sqlalchemy_utils.models import generic_repr from sqlalchemy.orm import object_session +from inspect import isclass from enum import Enum from .base import GFKBase, db, Column @@ -129,7 +130,7 @@ def new_setattr(self, key, value): cls.__setattr__ = new_setattr -def _patch_record_changes_method(cls, attributes, parent_attr): +def _patch_record_changes_method(cls, attributes, parent): def record_changes(self, changed_by, change_type=Change.Type.Modified): session = object_session(self) if not session: @@ -144,32 +145,41 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): if changes is None: return + self_type = self.__table__.name + self_id = self.id + changes = Change( - object=self, + object_type=self_type, + object_id=self_id, user=changed_by, change={ - "object_type": self.__table__.name, - "object_id": self.id, + "object_type": self_type, + "object_id": self_id, "change_type": change_type, "changes": changes, }, ) - if parent_attr: - changes.object = getattr(self, parent_attr, self) + if parent: + parent_type, parent_id = parent + if isclass(parent_type): # SQLAlchemy model + changes.object_type = parent_type.__table__.name + else: + changes.object_type = getattr(self, parent_type, self_type) + changes.object_id = getattr(self, parent_id, self_id) session.add(changes) cls.record_changes = record_changes -def track_changes(attributes, parent_attr=None): +def track_changes(attributes, parent=None): attributes = set(attributes) - {"id", "created_at", "updated_at", "version"} # monkey-patch class because inheritance will break SQLAlchemy def decorator(cls): _patch_setattr_method(cls, attributes) - _patch_record_changes_method(cls, attributes, parent_attr) + _patch_record_changes_method(cls, attributes, parent) return cls return decorator From 26b6bff89809c05217e2ee117e4e2099927b5069 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 13 Apr 2020 13:45:24 +0300 Subject: [PATCH 12/21] When creating/forking query - log changes to its visualzations --- redash/handlers/queries.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index 0e283861d9..416f718712 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -260,6 +260,8 @@ def post(self): models.db.session.commit() query.record_changes(self.current_user, models.Change.Type.Created) + for vis in query.visualizations: + vis.record_changes(self.current_user, models.Change.Type.Created) models.db.session.commit() self.record_event( @@ -466,6 +468,8 @@ def post(self, query_id): models.db.session.commit() forked_query.record_changes(self.current_user, models.Change.Type.Created) + for vis in forked_query.visualizations: + vis.record_changes(self.current_user, models.Change.Type.Created) models.db.session.commit() self.record_event( From 505e90c2770ef6b67059a29e3855d3a0596cc3a7 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 13 Apr 2020 15:36:15 +0300 Subject: [PATCH 13/21] Add org_id column to changes table --- ...table.py => 4952e040e9dd_update_changes_table.py} | 4 +++- redash/models/changes.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) rename migrations/versions/{4952e040e9dd_clear_changes_table.py => 4952e040e9dd_update_changes_table.py} (84%) diff --git a/migrations/versions/4952e040e9dd_clear_changes_table.py b/migrations/versions/4952e040e9dd_update_changes_table.py similarity index 84% rename from migrations/versions/4952e040e9dd_clear_changes_table.py rename to migrations/versions/4952e040e9dd_update_changes_table.py index ad5b4f565a..7cbfa987d2 100644 --- a/migrations/versions/4952e040e9dd_clear_changes_table.py +++ b/migrations/versions/4952e040e9dd_update_changes_table.py @@ -1,4 +1,4 @@ -"""clear_changes_table +"""update_changes_table Revision ID: 4952e040e9dd Revises: e5c7a4e2df4d @@ -19,6 +19,7 @@ def upgrade(): op.execute("DELETE FROM changes") op.drop_column("changes", "object_version") + op.add_column("changes", sa.Column("org_id", sa.Integer(), nullable=False)) op.create_index( "ix_changes_object_ref", "changes", @@ -28,6 +29,7 @@ def upgrade(): def downgrade(): + op.drop_column("changes", "org_id") op.add_column("changes", sa.Column("object_version", sa.Integer(), nullable=True, default=0)) op.execute("UPDATE changes SET object_version = 0") op.alter_column("changes", "object_version", nullable=False) diff --git a/redash/models/changes.py b/redash/models/changes.py index 830b5802cc..a5f41592be 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -35,6 +35,8 @@ class Type(str, Enum): # `str` to make it json-serializable id = Column(db.Integer, primary_key=True) # 'object' defined in GFKBase + org_id = Column(db.Integer, db.ForeignKey("organizations.id")) + org = db.relationship("Organization") user_id = Column(db.Integer, db.ForeignKey("users.id")) user = db.relationship("User", backref="changes") change = Column(PseudoJSON) @@ -47,7 +49,6 @@ def to_dict(self, full=True): "id": self.id, "object_id": self.object_id, "object_type": self.object_type, - "change_type": self.change_type, "change": self.change, "created_at": self.created_at, } @@ -59,6 +60,7 @@ def to_dict(self, full=True): return d + # TODO: remove this method @classmethod def last_change(cls, obj): return ( @@ -69,6 +71,13 @@ def last_change(cls, obj): .first() ) + @classmethod + def get_by_object(cls, target): + return cls.query.filter( + cls.object_type == target.__table__.name, + cls.object_id == target.id, + ).order_by(cls.id.desc()) + def _get_object_changes(obj, reset=True): result = {} @@ -151,6 +160,7 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): changes = Change( object_type=self_type, object_id=self_id, + org=changed_by.org, user=changed_by, change={ "object_type": self_type, From a36fbe4318bbb3a2e1ff643f34b16a93128721f5 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Tue, 14 Apr 2020 19:30:17 +0300 Subject: [PATCH 14/21] Add tests for all models that use change tracking --- redash/models/changes.py | 12 ++- tests/models/test_changes.py | 204 +++++++++++++++++++++++------------ 2 files changed, 146 insertions(+), 70 deletions(-) diff --git a/redash/models/changes.py b/redash/models/changes.py index a5f41592be..5384f2e09a 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -128,7 +128,7 @@ def new_setattr(self, key, value): if not hasattr(self, "__changes__"): self.__changes__ = {} change = self.__changes__.get(key, { - "previous": getattr(self, key), + "previous": getattr(self, key, None), "current": None, }) change["current"] = value @@ -139,6 +139,15 @@ def new_setattr(self, key, value): cls.__setattr__ = new_setattr +def _patch_clear_detected_changes_method(cls): + def clear_detected_changes(self): + changes = getattr(self, "__changes__", None) + if changes: + changes.clear() + + cls.clear_detected_changes = clear_detected_changes + + def _patch_record_changes_method(cls, attributes, parent): def record_changes(self, changed_by, change_type=Change.Type.Modified): session = object_session(self) @@ -190,6 +199,7 @@ def track_changes(attributes, parent=None): def decorator(cls): _patch_setattr_method(cls, attributes) _patch_record_changes_method(cls, attributes, parent) + _patch_clear_detected_changes_method(cls) return cls return decorator diff --git a/tests/models/test_changes.py b/tests/models/test_changes.py index 47e7af832e..7451ec81fd 100644 --- a/tests/models/test_changes.py +++ b/tests/models/test_changes.py @@ -1,78 +1,144 @@ from tests import BaseTestCase -from redash.models import db, Query, Change - - -def create_object(factory): - obj = Query( - name="Query", - description="", - query_text="SELECT 1", - user=factory.user, - data_source=factory.data_source, - org=factory.org, - ) - db.session.add(obj) - db.session.commit() - - return obj - - -class TestLogChange(BaseTestCase): - def obj(self): - obj = Query( - name="Query", - description="", - query_text="SELECT 1", - user=self.factory.user, - data_source=self.factory.data_source, - org=self.factory.org, - ) - - return obj - - def test_logs_creation(self): - obj = create_object(self.factory) - obj.record_changes(self.factory.user, Change.Type.Created) +from redash.models import db, Change + + +class BaseChangeTestCase(BaseTestCase): + def _update_object(self, obj, new_values): + result = {} + for key, new_value in new_values.items(): + old_value = getattr(obj, key, None) + setattr(obj, key, new_value) + if old_value == new_value: + result[key] = [new_value] + else: + result[key] = [old_value, new_value] + return result + + def _record_changes(self, obj, change_type=Change.Type.Modified): + obj.record_changes(self.factory.user, change_type) db.session.commit() - change = Change.last_change(obj) + db.session.flush() - self.assertIsNotNone(change) - self.assertEqual(change.change["change_type"], Change.Type.Created) + def _get_changes_by_object(self, obj, expected_count=None): + changes = list(Change.get_by_object(obj)) + if expected_count is not None: + self.assertEquals(len(changes), expected_count, "Expected %i change(s)" % expected_count) + return changes - def test_logs_modification(self): - obj = create_object(self.factory) - obj.record_changes(self.factory.user) - db.session.commit() - change = Change.last_change(obj) + def _assert_change_ownership(self, change, target, parent): + if parent: + self.assertEquals(change.object_type, parent.__table__.name, "Parent reference mismatch") + self.assertEquals(change.object_id, parent.id, "Parent reference mismatch") - self.assertIsNotNone(change) - self.assertEqual(change.change["change_type"], Change.Type.Modified) + if target: + self.assertEquals(change.change["object_type"], target.__table__.name, "Target reference mismatch") + self.assertEquals(change.change["object_id"], target.id, "Target reference mismatch") - def _test_skips_unnecessary_fields(self): - obj = create_object(self.factory) - obj.record_changes(changed_by=self.factory.user) - db.session.commit() - change = Change.last_change(obj) - - self.assertIsNotNone(change) - self.assertEqual(change.object_version, 1) - # for field in ChangeTrackingMixin.skipped_fields: - # self.assertNotIn(field, change.change) - - def _test_properly_log_modification(self): - obj = create_object(self.factory) - obj.record_changes(changed_by=self.factory.user) - obj.name = "Query 2" - obj.description = "description" - db.session.flush() - obj.record_changes(changed_by=self.factory.user) + def _test_creation(self, target, parent): + self._record_changes(target, Change.Type.Created) + + changes = self._get_changes_by_object(parent, expected_count=1) + change = changes[0] + self._assert_change_ownership(change, target, parent) + + self.assertTrue(change.change["changes"], "There should be some changes") + + def _test_modification(self, target, parent, new_values): + target.clear_detected_changes() + + updates = self._update_object(target, new_values) + self._record_changes(target, Change.Type.Modified) + + changes = self._get_changes_by_object(parent, expected_count=1) + change = changes[0] + self._assert_change_ownership(change, target, parent) + + self.assertDictEqual(change.change["changes"], updates) + + def _test_deletion(self, target, parent): + target.clear_detected_changes() + + self._record_changes(target, Change.Type.Deleted) + + changes = self._get_changes_by_object(parent, expected_count=1) + change = changes[0] + self._assert_change_ownership(change, target, parent) + + self.assertDictEqual(change.change["changes"], {}) + + +class TestQueryChanges(BaseChangeTestCase): + def test_creation(self): + query = self.factory.create_query() + self._test_creation(query, query) + + def test_modification(self): + query = self.factory.create_query() + self._test_modification(query, query, {"name": "New Name"}) + + def test_deletion(self): + query = self.factory.create_query() + self._test_deletion(query, query) + + +class TestVisualizationChanges(BaseChangeTestCase): + def test_creation(self): + visualization = self.factory.create_visualization() + self._test_creation(visualization, visualization.query_rel) + + def test_modification(self): + visualization = self.factory.create_visualization() + self._test_modification(visualization, visualization.query_rel, {"name": "New Name"}) + + def test_deletion(self): + visualization = self.factory.create_visualization() + self._test_deletion(visualization, visualization.query_rel) + + +class TestDashboardChanges(BaseChangeTestCase): + def test_creation(self): + dashboard = self.factory.create_dashboard() + self._test_creation(dashboard, dashboard) + + def test_modification(self): + dashboard = self.factory.create_dashboard() + self._test_modification(dashboard, dashboard, {"name": "New Name"}) + + def test_deletion(self): + dashboard = self.factory.create_dashboard() + self._test_deletion(dashboard, dashboard) + + +class TestWidgetChanges(BaseChangeTestCase): + def test_creation(self): + widget = self.factory.create_widget() + self._test_creation(widget, widget.dashboard) + + def test_modification(self): + widget = self.factory.create_widget() + self._test_modification(widget, widget.dashboard, { + "visualization_id": None, + "text": "Text of widget" + }) + + def test_deletion(self): + widget = self.factory.create_widget() + self._test_deletion(widget, widget.dashboard) + + +class TestApiKeyChanges(BaseChangeTestCase): + def test_creation(self): + dashboard = self.factory.create_dashboard() + api_key = self.factory.create_api_key(object=dashboard, active=True) + self._test_creation(api_key, dashboard) - change = Change.last_change(obj) + def test_modification(self): + dashboard = self.factory.create_dashboard() + api_key = self.factory.create_api_key(object=dashboard, active=True) + self._test_modification(api_key, dashboard, {"active": False}) - self.assertIsNotNone(change) - # TODO: https://github.com/getredash/redash/issues/1550 - # self.assertEqual(change.object_version, 2) - self.assertEqual(change.object_version, obj.version) - self.assertIn("name", change.change) - self.assertIn("description", change.change) + def test_deletion(self): + dashboard = self.factory.create_dashboard() + api_key = self.factory.create_api_key(object=dashboard, active=True) + self._test_deletion(api_key, dashboard) From 8e61afec6559d2f555d009c90bd163f13d00826f Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Tue, 14 Apr 2020 19:38:09 +0300 Subject: [PATCH 15/21] Add some comments to tests --- tests/models/test_changes.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/models/test_changes.py b/tests/models/test_changes.py index 7451ec81fd..897fa4e113 100644 --- a/tests/models/test_changes.py +++ b/tests/models/test_changes.py @@ -4,6 +4,7 @@ class BaseChangeTestCase(BaseTestCase): + # updates an object and returns changes in the same form as they're stored in DB def _update_object(self, obj, new_values): result = {} for key, new_value in new_values.items(): @@ -18,7 +19,6 @@ def _update_object(self, obj, new_values): def _record_changes(self, obj, change_type=Change.Type.Modified): obj.record_changes(self.factory.user, change_type) db.session.commit() - db.session.flush() def _get_changes_by_object(self, obj, expected_count=None): changes = list(Change.get_by_object(obj)) @@ -54,6 +54,7 @@ def _test_modification(self, target, parent, new_values): change = changes[0] self._assert_change_ownership(change, target, parent) + # this will also check if only modified fields were stored to DB self.assertDictEqual(change.change["changes"], updates) def _test_deletion(self, target, parent): @@ -65,6 +66,7 @@ def _test_deletion(self, target, parent): change = changes[0] self._assert_change_ownership(change, target, parent) + # when deleting an object, no changes are logged, just a fact of deletion self.assertDictEqual(change.change["changes"], {}) From 587e7559173f40105d7c96cbb0746018107ae06b Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Wed, 15 Apr 2020 13:44:13 +0300 Subject: [PATCH 16/21] API endpoints to access changes (basic implementation) --- redash/handlers/api.py | 13 ++++++++ redash/handlers/changes.py | 60 ++++++++++++++++++++++++++++++++++ redash/models/changes.py | 20 ++++-------- redash/serializers/__init__.py | 15 +++++++++ 4 files changed, 95 insertions(+), 13 deletions(-) create mode 100644 redash/handlers/changes.py diff --git a/redash/handlers/api.py b/redash/handlers/api.py index ef479ef3c3..ca49523a1b 100644 --- a/redash/handlers/api.py +++ b/redash/handlers/api.py @@ -83,6 +83,12 @@ VisualizationResource, ) from redash.handlers.widgets import WidgetListResource, WidgetResource +from redash.handlers.changes import ( + ChangesListResource, + QueryChangesListResource, + DashboardChangesListResource, + AlertChangesListResource, +) from redash.utils import json_dumps @@ -313,3 +319,10 @@ def json_representation(data, code, headers=None): api.add_org_resource( OrganizationSettings, "/api/settings/organization", endpoint="organization_settings" ) + +api.add_org_resource(ChangesListResource, "/api/changes", endpoint="organization_changes") +api.add_org_resource(QueryChangesListResource, "/api/queries//changes", endpoint="query_changes") +api.add_org_resource( + DashboardChangesListResource, "/api/dashboards//changes", endpoint="dashboard_changes" +) +api.add_org_resource(AlertChangesListResource, "/api/alerts//changes", endpoint="alert_changes") diff --git a/redash/handlers/changes.py b/redash/handlers/changes.py new file mode 100644 index 0000000000..679acec4c1 --- /dev/null +++ b/redash/handlers/changes.py @@ -0,0 +1,60 @@ +from flask import jsonify, request, url_for +from redash.handlers.base import BaseResource, paginate, get_object_or_404 +from redash.models import Change, Query, Dashboard, Alert +from redash.serializers import serialize_change + + +class BaseChangesListResource(BaseResource): + def _prepare_response(self, results_set): + """ + :qparam number page_size: Number of changes to return per page + :qparam number page: Page number to retrieve + + Responds with an array of `Change` objects. + """ + + page = request.args.get("page", 1, type=int) + page_size = request.args.get("page_size", 25, type=int) + + # TODO: order by - ? + # TODO: `serialize_change` may need access to nested objects - need to ensure that all are loaded at once + + response = paginate( + results_set, + page=page, + page_size=page_size, + serializer=serialize_change, + with_stats=True, + with_last_modified_by=False, + ) + + self.record_event({"action": "list", "object_type": "change"}) + + return response + + +class ChangesListResource(BaseChangesListResource): + def get(self): + changes_set = Change.all_changes(self.current_org) + return self._prepare_response(changes_set) + + +class QueryChangesListResource(BaseChangesListResource): + def get(self, query_id): + query = get_object_or_404(Query.get_by_id_and_org, query_id, self.current_org) + changes_set = Change.get_by_object(query, self.current_org) + return self._prepare_response(changes_set) + + +class DashboardChangesListResource(BaseChangesListResource): + def get(self, dashboard_id): + dashboard = get_object_or_404(Dashboard.get_by_id_and_org, dashboard_id, self.current_org) + changes_set = Change.get_by_object(dashboard, self.current_org) + return self._prepare_response(changes_set) + + +class AlertChangesListResource(BaseChangesListResource): + def get(self, alert_id): + alert = get_object_or_404(Alert.get_by_id_and_org, alert_id, self.current_org) + changes_set = Change.get_by_object(alert, self.current_org) + return self._prepare_response(changes_set) diff --git a/redash/models/changes.py b/redash/models/changes.py index 5384f2e09a..70877c1fce 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -36,7 +36,7 @@ class Type(str, Enum): # `str` to make it json-serializable id = Column(db.Integer, primary_key=True) # 'object' defined in GFKBase org_id = Column(db.Integer, db.ForeignKey("organizations.id")) - org = db.relationship("Organization") + org = db.relationship("Organization", backref="changes") user_id = Column(db.Integer, db.ForeignKey("users.id")) user = db.relationship("User", backref="changes") change = Column(PseudoJSON) @@ -60,22 +60,16 @@ def to_dict(self, full=True): return d - # TODO: remove this method @classmethod - def last_change(cls, obj): - return ( - cls.query.filter( - cls.object_id == obj.id, cls.object_type == obj.__class__.__tablename__ - ) - .order_by(cls.id.desc()) - .first() - ) + def all_changes(cls, org): + return cls.query.filter(cls.org == org).order_by(cls.id.desc()) @classmethod - def get_by_object(cls, target): + def get_by_object(cls, target_object, org): return cls.query.filter( - cls.object_type == target.__table__.name, - cls.object_id == target.id, + cls.org == org, + cls.object_type == target_object.__table__.name, + cls.object_id == target_object.id, ).order_by(cls.id.desc()) diff --git a/redash/serializers/__init__.py b/redash/serializers/__init__.py index 992d7d6b56..c89081e7f9 100644 --- a/redash/serializers/__init__.py +++ b/redash/serializers/__init__.py @@ -305,3 +305,18 @@ def serialize_job(job): "query_result_id": query_result_id, } } + + +def serialize_change(change): + return { + "created_at": change.created_at, + # TODO: return an object (and target?) as a sub-structure, but carefully - they may be removed + "object_type": change.object_type, + "object_id": change.object_id, + "target_type": change.change["object_type"], + "target_id": change.change["object_id"], + "change_type": change.change["change_type"], + "changes": change.change["changes"], + # TODO: user may be deleted - need to handle this + "user": change.user.to_dict(), + } From b74beb0c103b2e0e408477016fb78b04724df3df Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Wed, 15 Apr 2020 19:05:57 +0300 Subject: [PATCH 17/21] Track changes to Alerts and Alert Destinations --- redash/models/__init__.py | 6 +++++ redash/models/changes.py | 12 ++++++++-- tests/models/test_changes.py | 43 +++++++++++++++++++++++++++++++++++- 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 9924f9f3fb..2868867a0d 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -951,6 +951,7 @@ def next_state(op, value, threshold): @generic_repr( "id", "name", "query_id", "user_id", "state", "last_triggered_at", "rearm" ) +@track_changes(attributes=("name", "query_id", "options", "rearm")) class Alert(TimestampMixin, BelongsToOrgMixin, db.Model): UNKNOWN_STATE = "unknown" OK_STATE = "ok" @@ -1334,6 +1335,7 @@ def create_for_object(cls, object, user): @generic_repr("id", "name", "type", "user_id", "org_id", "created_at") +@track_changes(attributes=("name", "type", "options")) class NotificationDestination(BelongsToOrgMixin, db.Model): id = Column(db.Integer, primary_key=True) org_id = Column(db.Integer, db.ForeignKey("organizations.id")) @@ -1391,6 +1393,10 @@ def notify(self, alert, query, user, new_state, app, host): @generic_repr("id", "user_id", "destination_id", "alert_id") +@track_changes( + parent=(Alert, "alert_id"), + attributes=("destination_id",) +) class AlertSubscription(TimestampMixin, db.Model): id = Column(db.Integer, primary_key=True) user_id = Column(db.Integer, db.ForeignKey("users.id")) diff --git a/redash/models/changes.py b/redash/models/changes.py index 70877c1fce..470b805014 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -5,6 +5,8 @@ from inspect import isclass from enum import Enum +from redash.utils.configuration import ConfigurationContainer + from .base import GFKBase, db, Column from .types import PseudoJSON @@ -73,6 +75,12 @@ def get_by_object(cls, target_object, org): ).order_by(cls.id.desc()) +def _to_json(value): + if isinstance(value, ConfigurationContainer): + return value.to_json() + return value + + def _get_object_changes(obj, reset=True): result = {} changes = getattr(obj, "__changes__", None) @@ -86,7 +94,7 @@ def _get_object_changes(obj, reset=True): for key, change in changes.items(): if change["current"] != change["previous"]: col = columns.get(key, key) - result[col] = (change["previous"], change["current"]) + result[col] = (_to_json(change["previous"]), _to_json(change["current"])) if reset: changes.clear() @@ -105,7 +113,7 @@ def _collect_all_attributes(obj, attributes, reset=True): for key in attributes: col = columns.get(key, key) value = getattr(obj, key, None) - result[col] = (value,) + result[col] = (_to_json(value),) changes = getattr(obj, "__changes__", None) if changes and reset: diff --git a/tests/models/test_changes.py b/tests/models/test_changes.py index 897fa4e113..2e17d17ec1 100644 --- a/tests/models/test_changes.py +++ b/tests/models/test_changes.py @@ -21,7 +21,7 @@ def _record_changes(self, obj, change_type=Change.Type.Modified): db.session.commit() def _get_changes_by_object(self, obj, expected_count=None): - changes = list(Change.get_by_object(obj)) + changes = list(Change.get_by_object(obj, self.factory.org)) if expected_count is not None: self.assertEquals(len(changes), expected_count, "Expected %i change(s)" % expected_count) return changes @@ -144,3 +144,44 @@ def test_deletion(self): dashboard = self.factory.create_dashboard() api_key = self.factory.create_api_key(object=dashboard, active=True) self._test_deletion(api_key, dashboard) + + +class TestNotificationDestinationChanges(BaseChangeTestCase): + def test_creation(self): + destination = self.factory.create_destination() + self._test_creation(destination, destination) + + def test_modification(self): + destination = self.factory.create_destination() + self._test_modification(destination, destination, {"name": "New Name"}) + + def test_deletion(self): + destination = self.factory.create_destination() + self._test_deletion(destination, destination) + + +class TestAlertChanges(BaseChangeTestCase): + def test_creation(self): + alert = self.factory.create_alert() + self._test_creation(alert, alert) + + def test_modification(self): + alert = self.factory.create_alert() + self._test_modification(alert, alert, {"name": "New Name"}) + + def test_deletion(self): + alert = self.factory.create_alert() + self._test_deletion(alert, alert) + + +# Alert subscriptions can only be added and removed, so not testing for modifications +class TestAlertSubscriptionChanges(BaseChangeTestCase): + def test_creation(self): + alert = self.factory.create_alert() + subscription = self.factory.create_alert_subscription(alert=alert) + self._test_creation(subscription, alert) + + def test_deletion(self): + alert = self.factory.create_alert() + subscription = self.factory.create_alert_subscription(alert=alert) + self._test_deletion(subscription, alert) From 3592ca56ce76186c5c469ba2129927f0b4e4220e Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Wed, 15 Apr 2020 20:48:47 +0300 Subject: [PATCH 18/21] Log changes to Alerts and Alert Destinations --- redash/handlers/alerts.py | 15 +++++++++++---- redash/handlers/destinations.py | 4 ++++ redash/models/changes.py | 30 +++++++++++++++++++++++++++--- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/redash/handlers/alerts.py b/redash/handlers/alerts.py index 7929df9c05..c05a07a87e 100644 --- a/redash/handlers/alerts.py +++ b/redash/handlers/alerts.py @@ -1,5 +1,3 @@ -import time - from flask import request from funcy import project @@ -12,7 +10,6 @@ require_permission, view_only, ) -from redash.utils import json_dumps class AlertResource(BaseResource): @@ -35,6 +32,7 @@ def post(self, alert_id): require_admin_or_owner(alert.user.id) self.update_model(alert, params) + alert.record_changes(self.current_user, models.Change.Type.Modified) models.db.session.commit() self.record_event( @@ -48,6 +46,7 @@ def delete(self, alert_id): models.Alert.get_by_id_and_org, alert_id, self.current_org ) require_admin_or_owner(alert.user_id) + alert.record_changes(self.current_user, models.Change.Type.Deleted) models.db.session.delete(alert) models.db.session.commit() @@ -60,6 +59,7 @@ def post(self, alert_id): require_admin_or_owner(alert.user.id) alert.options["muted"] = True + alert.record_changes(self.current_user, models.Change.Type.Modified) models.db.session.commit() self.record_event( @@ -73,6 +73,7 @@ def delete(self, alert_id): require_admin_or_owner(alert.user.id) alert.options["muted"] = False + alert.record_changes(self.current_user, models.Change.Type.Modified) models.db.session.commit() self.record_event( @@ -97,7 +98,9 @@ def post(self): ) models.db.session.add(alert) - models.db.session.flush() + models.db.session.commit() + + alert.record_changes(self.current_user, models.Change.Type.Created) models.db.session.commit() self.record_event( @@ -133,6 +136,9 @@ def post(self, alert_id): models.db.session.add(subscription) models.db.session.commit() + subscription.record_changes(self.current_user, models.Change.Type.Created) + models.db.session.commit() + self.record_event( { "action": "subscribe", @@ -158,6 +164,7 @@ class AlertSubscriptionResource(BaseResource): def delete(self, alert_id, subscriber_id): subscription = models.AlertSubscription.query.get_or_404(subscriber_id) require_admin_or_owner(subscription.user.id) + subscription.record_changes(self.current_user, models.Change.Type.Deleted) models.db.session.delete(subscription) models.db.session.commit() diff --git a/redash/handlers/destinations.py b/redash/handlers/destinations.py index e935ed84bb..bb337a46b9 100644 --- a/redash/handlers/destinations.py +++ b/redash/handlers/destinations.py @@ -51,6 +51,7 @@ def post(self, destination_id): destination.options.set_schema(schema) destination.options.update(req["options"]) models.db.session.add(destination) + destination.record_changes(self.current_user, models.Change.Type.Modified) models.db.session.commit() except ValidationError: abort(400) @@ -71,6 +72,7 @@ def delete(self, destination_id): destination = models.NotificationDestination.get_by_id_and_org( destination_id, self.current_org ) + destination.record_changes(self.current_user, models.Change.Type.Deleted) models.db.session.delete(destination) models.db.session.commit() @@ -131,6 +133,8 @@ def post(self): try: models.db.session.add(destination) models.db.session.commit() + destination.record_changes(self.current_user, models.Change.Type.Created) + models.db.session.commit() except IntegrityError as e: if "name" in str(e): abort( diff --git a/redash/models/changes.py b/redash/models/changes.py index 470b805014..684c79efb7 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -1,6 +1,7 @@ from sqlalchemy.inspection import inspect from sqlalchemy_utils.models import generic_repr from sqlalchemy.orm import object_session +from sqlalchemy.ext.mutable import MutableBase from inspect import isclass from enum import Enum @@ -24,8 +25,9 @@ # to that visualization, and object_type/object_id will be the reference to a query for this visualization. # changes.changes # a dictionary where keys are names of modified fields and values are tuples with field values. -# Tuple may contain one value if field wasn't changes, or two values - previous and new one. In both cases -# tuple is used to avoid situations when field contains an JSON array and may be wrongly interpreted. +# Tuple may contain one value if field wasn't changes OR if value was mutated (MutableDict/List/Set), +# or two values - previous and new one. In both cases tuple is used to avoid situations when field +# contains an JSON array and may be wrongly interpreted. @generic_repr("id", "object_type", "object_id", "created_at") @@ -77,10 +79,28 @@ def get_by_object(cls, target_object, org): def _to_json(value): if isinstance(value, ConfigurationContainer): - return value.to_json() + return value.to_dict() return value +def _detect_mutations(obj, attributes): + if not hasattr(obj, "__changes__"): + obj.__changes__ = {} + + state = inspect(obj) + for key in attributes: + value = getattr(obj, key, None) + if isinstance(value, MutableBase): + attr = getattr(state.attrs, key, None) + if attr: + attr.load_history() + if attr.history and attr.history.has_changes(): + obj.__changes__[key] = { + "previous": value, + "current": value, + } + + def _get_object_changes(obj, reset=True): result = {} changes = getattr(obj, "__changes__", None) @@ -95,6 +115,9 @@ def _get_object_changes(obj, reset=True): if change["current"] != change["previous"]: col = columns.get(key, key) result[col] = (_to_json(change["previous"]), _to_json(change["current"])) + elif isinstance(change["current"], MutableBase): + col = columns.get(key, key) + result[col] = (_to_json(change["current"]),) if reset: changes.clear() @@ -160,6 +183,7 @@ def record_changes(self, changed_by, change_type=Change.Type.Modified): if change_type == Change.Type.Created: changes = _collect_all_attributes(self, attributes) if change_type == Change.Type.Modified: + _detect_mutations(self, attributes) changes = _get_object_changes(self) if changes is None: From 3f94434b4d3e75b8a12eb9ef527e728fbda35db8 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Fri, 17 Apr 2020 12:36:10 +0300 Subject: [PATCH 19/21] Add API endpoint for Alert Destination changes; refine API code --- redash/handlers/api.py | 4 ++++ redash/handlers/changes.py | 24 ++++++++++++++---------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/redash/handlers/api.py b/redash/handlers/api.py index ca49523a1b..264722b4ee 100644 --- a/redash/handlers/api.py +++ b/redash/handlers/api.py @@ -88,6 +88,7 @@ QueryChangesListResource, DashboardChangesListResource, AlertChangesListResource, + DestinationChangesListResource, ) from redash.utils import json_dumps @@ -326,3 +327,6 @@ def json_representation(data, code, headers=None): DashboardChangesListResource, "/api/dashboards//changes", endpoint="dashboard_changes" ) api.add_org_resource(AlertChangesListResource, "/api/alerts//changes", endpoint="alert_changes") +api.add_org_resource( + DestinationChangesListResource, "/api/destinations//changes", endpoint="destination_changes" +) diff --git a/redash/handlers/changes.py b/redash/handlers/changes.py index 679acec4c1..db81256a0c 100644 --- a/redash/handlers/changes.py +++ b/redash/handlers/changes.py @@ -1,6 +1,6 @@ from flask import jsonify, request, url_for from redash.handlers.base import BaseResource, paginate, get_object_or_404 -from redash.models import Change, Query, Dashboard, Alert +from redash.models import Change, Query, Dashboard, Alert, NotificationDestination from redash.serializers import serialize_change @@ -32,6 +32,11 @@ def _prepare_response(self, results_set): return response + def _get_by_object(self, fn, *args, **kwargs): + obj = get_object_or_404(fn, *args, **kwargs) + changes_set = Change.get_by_object(obj, self.current_org) + return self._prepare_response(changes_set) + class ChangesListResource(BaseChangesListResource): def get(self): @@ -41,20 +46,19 @@ def get(self): class QueryChangesListResource(BaseChangesListResource): def get(self, query_id): - query = get_object_or_404(Query.get_by_id_and_org, query_id, self.current_org) - changes_set = Change.get_by_object(query, self.current_org) - return self._prepare_response(changes_set) + return self._get_by_object(Query.get_by_id_and_org, query_id, self.current_org) class DashboardChangesListResource(BaseChangesListResource): def get(self, dashboard_id): - dashboard = get_object_or_404(Dashboard.get_by_id_and_org, dashboard_id, self.current_org) - changes_set = Change.get_by_object(dashboard, self.current_org) - return self._prepare_response(changes_set) + return self._get_by_object(Dashboard.get_by_id_and_org, dashboard_id, self.current_org) class AlertChangesListResource(BaseChangesListResource): def get(self, alert_id): - alert = get_object_or_404(Alert.get_by_id_and_org, alert_id, self.current_org) - changes_set = Change.get_by_object(alert, self.current_org) - return self._prepare_response(changes_set) + return self._get_by_object(Alert.get_by_id_and_org, alert_id, self.current_org) + + +class DestinationChangesListResource(BaseChangesListResource): + def get(self, destination_id): + return self._get_by_object(NotificationDestination.get_by_id_and_org, destination_id, self.current_org) From 995fa3303703dd4a9fe0e413ea2a84dadd0f8804 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 20 Apr 2020 17:48:28 +0300 Subject: [PATCH 20/21] Changes API: order of items --- redash/handlers/changes.py | 33 +++++++++++++++++++++++++++------ redash/models/changes.py | 23 ++++++++++++++--------- redash/serializers/__init__.py | 15 --------------- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/redash/handlers/changes.py b/redash/handlers/changes.py index db81256a0c..3d46ecd9f5 100644 --- a/redash/handlers/changes.py +++ b/redash/handlers/changes.py @@ -1,7 +1,28 @@ from flask import jsonify, request, url_for -from redash.handlers.base import BaseResource, paginate, get_object_or_404 +from funcy import partial +from redash.handlers.base import ( + BaseResource, + paginate, + order_results as _order_results, + get_object_or_404 +) from redash.models import Change, Query, Dashboard, Alert, NotificationDestination -from redash.serializers import serialize_change + + +order_map = { + "created_at": "created_at", + "-created_at": "-created_at", + "object_type": "object_type", + "-object_type": "-object_type", + "object_id": "object_id", + "-object_id": "-object_id", + "user_id": "user_id", + "-user_id": "-user_id", +} + +order_results = partial( + _order_results, default_order="-created_at", allowed_orders=order_map +) class BaseChangesListResource(BaseResource): @@ -9,6 +30,7 @@ def _prepare_response(self, results_set): """ :qparam number page_size: Number of changes to return per page :qparam number page: Page number to retrieve + :qparam number order: Name of column to order by Responds with an array of `Change` objects. """ @@ -16,14 +38,13 @@ def _prepare_response(self, results_set): page = request.args.get("page", 1, type=int) page_size = request.args.get("page_size", 25, type=int) - # TODO: order by - ? - # TODO: `serialize_change` may need access to nested objects - need to ensure that all are loaded at once + ordered_results = order_results(results_set) response = paginate( - results_set, + ordered_results, page=page, page_size=page_size, - serializer=serialize_change, + serializer=lambda c: c.to_dict(), with_stats=True, with_last_modified_by=False, ) diff --git a/redash/models/changes.py b/redash/models/changes.py index 684c79efb7..ea9146ec2f 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -48,21 +48,26 @@ class Type(str, Enum): # `str` to make it json-serializable __tablename__ = "changes" - def to_dict(self, full=True): - d = { + def to_dict(self, extended=True): + result = { "id": self.id, - "object_id": self.object_id, - "object_type": self.object_type, - "change": self.change, "created_at": self.created_at, + # TODO: return an object (and target?) as a sub-structure, but carefully - they may be removed + "object_type": self.object_type, + "object_id": self.object_id, + "target_type": self.change["object_type"], + "target_id": self.change["object_id"], + "change_type": self.change["change_type"], + "changes": self.change["changes"], } - if full: - d["user"] = self.user.to_dict() + # TODO: user may be deleted - need to handle this + if extended: + result["user"] = self.user.to_dict() else: - d["user_id"] = self.user_id + result["user_id"] = self.user_id - return d + return result @classmethod def all_changes(cls, org): diff --git a/redash/serializers/__init__.py b/redash/serializers/__init__.py index c89081e7f9..992d7d6b56 100644 --- a/redash/serializers/__init__.py +++ b/redash/serializers/__init__.py @@ -305,18 +305,3 @@ def serialize_job(job): "query_result_id": query_result_id, } } - - -def serialize_change(change): - return { - "created_at": change.created_at, - # TODO: return an object (and target?) as a sub-structure, but carefully - they may be removed - "object_type": change.object_type, - "object_id": change.object_id, - "target_type": change.change["object_type"], - "target_id": change.change["object_id"], - "change_type": change.change["change_type"], - "changes": change.change["changes"], - # TODO: user may be deleted - need to handle this - "user": change.user.to_dict(), - } From 49ff502409e795ebbef8c7325bd51d46f26b03c2 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 20 Apr 2020 18:57:03 +0300 Subject: [PATCH 21/21] Add some tests for API endpoints --- tests/handlers/test_changes.py | 80 ++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 tests/handlers/test_changes.py diff --git a/tests/handlers/test_changes.py b/tests/handlers/test_changes.py new file mode 100644 index 0000000000..2d3ec30902 --- /dev/null +++ b/tests/handlers/test_changes.py @@ -0,0 +1,80 @@ +from tests import BaseTestCase +from redash.models import db, Change + + +class TestChangesGet(BaseTestCase): + def setUp(self): + super().setUp() + + query = self.factory.create_query() + query.record_changes(self.factory.user, Change.Type.Created) + query.query_text = "SELECT 'test'" + query.record_changes(self.factory.user, Change.Type.Modified) + query.name = "Test Query" + query.record_changes(self.factory.user, Change.Type.Modified) + db.session.commit() + + widget = self.factory.create_widget() + dashboard = widget.dashboard + dashboard.record_changes(self.factory.user, Change.Type.Created) + widget.record_changes(self.factory.user, Change.Type.Created) + dashboard.name = "Test Dashboard" + dashboard.record_changes(self.factory.user, Change.Type.Modified) + widget.visualization_id = None + widget.text = "Text of widget" + widget.record_changes(self.factory.user, Change.Type.Modified) + db.session.commit() + + self._objects = (query, dashboard, widget) + + def _assert_changes_structure(self, changes, expected_count=None): + if expected_count is not None: + self.assertEquals(len(changes), expected_count, "Expected %i items" % expected_count) + expected_keys = { + "created_at", "object_type", "object_id", "target_type", "target_id", "change_type", "changes", + } + for c in changes: + keys = set(c.keys()) + self.assertTrue(expected_keys.issubset(keys), "Change structure mismatch") + self.assertIsInstance(c["changes"], dict, "Change structure mismatch") + + def _assert_changes_ownership(self, changes, refs): + for c in changes: + ref = (c["object_type"], c["object_id"], c["target_type"], c["target_id"]) + self.assertTrue(ref in refs, "Change does not belong to any of specified objects") + + def test_get_all_changes(self): + query, dashboard, widget = self._objects + rv = self.make_request("get", "/api/changes") + self.assertEqual(rv.status_code, 200) + + changes = rv.json["results"] + self._assert_changes_structure(changes, expected_count=7) + self._assert_changes_ownership(changes, { + (query.__table__.name, query.id, query.__table__.name, query.id), + (dashboard.__table__.name, dashboard.id, dashboard.__table__.name, dashboard.id), + (dashboard.__table__.name, dashboard.id, widget.__table__.name, widget.id), + }) + + def test_get_all_changes_for_object(self): + query, dashboard, widget = self._objects + rv = self.make_request("get", "/api/queries/{0}/changes".format(query.id)) + self.assertEqual(rv.status_code, 200) + + changes = rv.json["results"] + self._assert_changes_structure(changes, expected_count=3) + self._assert_changes_ownership(changes, { + (query.__table__.name, query.id, query.__table__.name, query.id), + }) + + def test_get_all_changes_for_object_with_relations(self): + query, dashboard, widget = self._objects + rv = self.make_request("get", "/api/dashboards/{0}/changes".format(dashboard.id)) + self.assertEqual(rv.status_code, 200) + + changes = rv.json["results"] + self._assert_changes_structure(changes, expected_count=4) + self._assert_changes_ownership(changes, { + (dashboard.__table__.name, dashboard.id, dashboard.__table__.name, dashboard.id), + (dashboard.__table__.name, dashboard.id, widget.__table__.name, widget.id), + })