From e0672f4c4df74636f46ecd656ca4a52b11a37ece Mon Sep 17 00:00:00 2001 From: Waldemar Hummer Date: Sun, 12 Jun 2016 19:51:55 +1000 Subject: [PATCH] add optimistic locking for dashboard editing --- rd_ui/app/scripts/controllers/dashboard.js | 4 +- .../directives/dashboard_directives.js | 15 ++++- redash/handlers/access.py | 4 +- redash/handlers/dashboards.py | 59 ++++++++++++++++++- redash/handlers/queries.py | 4 +- redash/models.py | 2 +- tests/handlers/test_queries.py | 1 + 7 files changed, 78 insertions(+), 11 deletions(-) diff --git a/rd_ui/app/scripts/controllers/dashboard.js b/rd_ui/app/scripts/controllers/dashboard.js index dbb3a5aa..28d4d9b0 100644 --- a/rd_ui/app/scripts/controllers/dashboard.js +++ b/rd_ui/app/scripts/controllers/dashboard.js @@ -9,7 +9,7 @@ }); }; - var DashboardCtrl = function($scope, Events, Widget, $routeParams, $location, $http, $timeout, $q, $modal, Dashboard, User) { + var DashboardCtrl = function($scope, Events, Widget, $routeParams, $location, $http, $timeout, $q, $modal, Dashboard) { $scope.refreshEnabled = false; $scope.isFullscreen = false; $scope.refreshRate = 60; @@ -271,7 +271,7 @@ }; angular.module('redash.controllers') - .controller('DashboardCtrl', ['$scope', 'Events', 'Widget', '$routeParams', '$location', '$http', '$timeout', '$q', '$modal', 'Dashboard', 'User', DashboardCtrl]) + .controller('DashboardCtrl', ['$scope', 'Events', 'Widget', '$routeParams', '$location', '$http', '$timeout', '$q', '$modal', 'Dashboard', DashboardCtrl]) .controller('PublicDashboardCtrl', ['$scope', 'Events', 'Widget', '$routeParams', '$location', '$http', '$timeout', '$q', 'Dashboard', PublicDashboardCtrl]) .controller('WidgetCtrl', ['$scope', '$location', 'Events', 'Query', '$modal', WidgetCtrl]) diff --git a/rd_ui/app/scripts/directives/dashboard_directives.js b/rd_ui/app/scripts/directives/dashboard_directives.js index 3217d5e0..51cccd55 100644 --- a/rd_ui/app/scripts/directives/dashboard_directives.js +++ b/rd_ui/app/scripts/directives/dashboard_directives.js @@ -3,8 +3,8 @@ var directives = angular.module('redash.directives'); - directives.directive('editDashboardForm', ['Events', '$http', '$location', '$timeout', 'Dashboard', - function(Events, $http, $location, $timeout, Dashboard) { + directives.directive('editDashboardForm', ['Events', '$http', '$location', '$timeout', 'Dashboard', 'growl', + function(Events, $http, $location, $timeout, Dashboard, growl) { return { restrict: 'E', scope: { @@ -81,10 +81,19 @@ $scope.dashboard.layout = layout; layout = JSON.stringify(layout); - Dashboard.save({slug: $scope.dashboard.id, name: $scope.dashboard.name, layout: layout}, function(dashboard) { + Dashboard.save({slug: $scope.dashboard.id, name: $scope.dashboard.name, + latest_version: $scope.dashboard.latest_version, layout: layout}, function(dashboard) { $scope.dashboard = dashboard; $scope.saveInProgress = false; $(element).modal('hide'); + }, function(error) { + $scope.saveInProgress = false; + if(error.status == 403) { + growl.addErrorMessage("Unable to save dashboard: Permission denied."); + } else if(error.status == 409) { + growl.addErrorMessage('It seems like the dashboard has been modified by another user. ' + + 'Please copy/backup your changes and reload this page.', {ttl: -1}); + } }); Events.record(currentUser, 'edit', 'dashboard', $scope.dashboard.id); } else { diff --git a/redash/handlers/access.py b/redash/handlers/access.py index 926c11e3..9d444e8b 100644 --- a/redash/handlers/access.py +++ b/redash/handlers/access.py @@ -38,7 +38,7 @@ class AccessGrantResource(BaseResource): .where(AccessPermission.access_type == access_type) if permissions.count() > 0: - return + return {'result': 'already_granted'} perm = AccessPermission() perm.object_type = object_type @@ -47,6 +47,7 @@ class AccessGrantResource(BaseResource): perm.grantor = self.current_user perm.grantee = grantee perm.save() + return {'result': 'permission_added'} class AccessRevokeResource(BaseResource): @@ -73,3 +74,4 @@ class AccessAttemptResource(BaseResource): if access: return {'result': 'access_granted'} abort(403) + return False diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index 26ce573f..4944cfbf 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -1,4 +1,7 @@ +import logging + from flask import request, url_for +from flask_restful import abort from funcy import distinct, take from itertools import chain @@ -8,6 +11,19 @@ from redash.permissions import require_permission, require_admin_or_owner from redash.handlers.base import BaseResource, get_object_or_404 +def _save_change(user, dashboard_id, old_dashboard, new_dashboard, change_type): + change = models.Change() + change.object_id = dashboard_id + change.object_type = models.Dashboard.__name__ + change.change_type = change_type + change.user = user + change.change = { + "before": old_dashboard, + "after": new_dashboard + } + change.save() + return change + class RecentDashboardsResource(BaseResource): @require_permission('list_dashboards') def get(self): @@ -25,6 +41,11 @@ class DashboardListResource(BaseResource): def get(self): dashboards = [d.to_dict() for d in models.Dashboard.all(self.current_org, self.current_user.groups, self.current_user)] + for dashboard in dashboards: + last_change = models.Change.get_latest(object_id=dashboard['id'], object_type=models.Dashboard.__name__) + if last_change: + dashboard['latest_version'] = last_change.id + return dashboards @require_permission('create_dashboard') @@ -35,7 +56,14 @@ class DashboardListResource(BaseResource): user=self.current_user, layout='[]') dashboard.save() - return dashboard.to_dict() + + # create a new Changes record to keep track of the changes + new_dashboard = {'name': dashboard.name, 'layout': dashboard.layout} + new_change = _save_change(self.current_user, dashboard.id, None, new_dashboard, change_type=models.Change.TYPE_CREATE) + + result = dashboard.to_dict() + result['latest_version'] = new_change.id + return result class DashboardResource(BaseResource): @@ -49,6 +77,10 @@ class DashboardResource(BaseResource): response['public_url'] = url_for('redash.public_dashboard', token=api_key.api_key, org_slug=self.current_org.slug, _external=True) response['api_key'] = api_key.api_key + last_change = models.Change.get_latest(object_id=dashboard.id, object_type=models.Dashboard.__name__) + if last_change: + response['latest_version'] = last_change.id + return response @require_permission('edit_dashboard') @@ -56,11 +88,34 @@ class DashboardResource(BaseResource): dashboard_properties = request.get_json(force=True) # TODO: either convert all requests to use slugs or ids dashboard = models.Dashboard.get_by_id_and_org(dashboard_slug, self.current_org) + + # check access permissions + if self.current_user.id != dashboard.user.id: + if not self.current_user.has_access( + access_type=models.AccessPermission.ACCESS_TYPE_MODIFY, + object_id=dashboard.id, + object_type=models.Dashboard.__name__): + abort(403) + + # Optimistic locking: figure out which user made the last + # change to this dashboard, and bail out if necessary + last_change = models.Change.get_latest(object_id=dashboard.id, object_type=models.Dashboard.__name__) + if last_change and 'latest_version' in dashboard_properties: + if last_change.id > dashboard_properties['latest_version']: + abort(409) # HTTP 'Conflict' status code + + old_dashboard = {'name': dashboard.name, 'layout': dashboard.layout} dashboard.layout = dashboard_properties['layout'] dashboard.name = dashboard_properties['name'] dashboard.save() - return dashboard.to_dict(with_widgets=True, user=self.current_user) + # create a new Changes record to keep track of the changes + new_dashboard = {'name': dashboard.name, 'layout': dashboard.layout} + new_change = _save_change(self.current_user, dashboard.id, old_dashboard, new_dashboard, change_type=models.Change.TYPE_MODIFY) + + result = dashboard.to_dict(with_widgets=True, user=self.current_user) + result['latest_version'] = new_change.id + return result @require_permission('edit_dashboard') def delete(self, dashboard_slug): diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index f75adbfb..76ce1aac 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -25,7 +25,7 @@ def format_sql_query(org_slug=None): return sqlparse.format(query, reindent=True, keyword_case='upper') -def _save_change(user, query, old_query, new_query, change_type=models.Change.TYPE_UPDATE): +def _save_change(user, query, old_query, new_query, change_type=models.Change.TYPE_MODIFY): if 'data_source' in new_query: new_query['data_source_id'] = new_query.pop('data_source') for field in ['data_source_id', 'user', 'last_modified_by', 'org']: @@ -141,7 +141,7 @@ class QueryResource(BaseResource): # Optimistic locking: figure out which user made the last # change to this query, and bail out if necessary last_change = models.Change.get_latest(object_id=query.id, object_type=models.Query.__name__) - if last_change: + if last_change and 'latest_version' in query_def: if last_change.id > query_def['latest_version']: abort(409) # HTTP 'Conflict' status code diff --git a/redash/models.py b/redash/models.py index 4a617618..094ed6d7 100644 --- a/redash/models.py +++ b/redash/models.py @@ -819,7 +819,7 @@ class Change(BaseModel): created_at = DateTimeTZField(default=datetime.datetime.now) TYPE_CREATE = 'create' - TYPE_UPDATE = 'update' + TYPE_MODIFY = 'modify' class Meta: db_table = 'changes' diff --git a/tests/handlers/test_queries.py b/tests/handlers/test_queries.py index e2998853..54923ac2 100644 --- a/tests/handlers/test_queries.py +++ b/tests/handlers/test_queries.py @@ -77,6 +77,7 @@ class QueryAPITest(BaseTestCase, AuthenticationTestMixin): rv = self.make_request('get', '/api/queries/{}'.format(query.id), user=self.factory.create_admin()) self.assertEquals(rv.status_code, 200) + class QueryRefreshTest(BaseTestCase): def setUp(self): super(QueryRefreshTest, self).setUp()