From 5a87aa0ed7e06b6070abd5e63439a24da04c893d Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Wed, 3 Dec 2014 14:47:32 +0100 Subject: [PATCH] Improving OCC for detecting situations where the version is not valid but the changes doesn't overwrite previous ones --- taiga/projects/history/services.py | 18 ++ taiga/projects/occ/mixins.py | 41 +++- tests/integration/test_occ.py | 354 +++++++++++++++++++++-------- 3 files changed, 320 insertions(+), 93 deletions(-) diff --git a/taiga/projects/history/services.py b/taiga/projects/history/services.py index c79c7eec..f22574a2 100644 --- a/taiga/projects/history/services.py +++ b/taiga/projects/history/services.py @@ -251,6 +251,24 @@ def get_last_snapshot_for_key(key:str) -> FrozenObj: # Public api +def get_modified_fields(obj:object, last_modifications): + """ + Get the modified fields for an object through his last modifications + """ + key = make_key_from_model_object(obj) + entry_model = apps.get_model("history", "HistoryEntry") + history_entries = (entry_model.objects + .filter(key=key) + .order_by("-created_at") + .values_list("diff", flat=True) + [0:last_modifications]) + + modified_fields = [] + for history_entry in history_entries: + modified_fields += history_entry.keys() + + return modified_fields + @tx.atomic def take_snapshot(obj:object, *, comment:str="", user=None, delete:bool=False): """ diff --git a/taiga/projects/occ/mixins.py b/taiga/projects/occ/mixins.py index e4ef0244..c9bdeeac 100644 --- a/taiga/projects/occ/mixins.py +++ b/taiga/projects/occ/mixins.py @@ -19,21 +19,56 @@ from django.utils.translation import ugettext_lazy as _ from taiga.base import exceptions as exc from taiga.base.utils import db - +from taiga.projects.history.services import get_modified_fields class OCCResourceMixin(object): """ Rest Framework resource mixin for resources that need to have concurrent accesses and editions controlled. """ + def _extract_param_version(self): + param_version = self.request.DATA.get('version', None) + try: + param_version = param_version and int(param_version) + except (ValueError, TypeError): + raise exc.WrongArguments({"version": "The version must be an integer"}) + + return param_version + + def _validate_param_version(self, param_version, current_version): + if param_version is not None: + if param_version < 0: + return False + if current_version is not None and param_version > current_version: + return False + + return True + def _validate_and_update_version(self, obj): current_version = None if obj.id: current_version = type(obj).objects.model.objects.get(id=obj.id).version - param_version = self.request.DATA.get('version', None) + # Extract param version + param_version = self._extract_param_version() + if not self._validate_param_version(param_version, current_version): + raise exc.WrongArguments({"version": "The version is not valid"}) + if current_version != param_version: - raise exc.WrongArguments({"version": "The version doesn't match with the current one"}) + diff_versions = current_version - param_version + + modifying_fields = set(self.request.DATA.keys()) + if "version" in modifying_fields: + modifying_fields.remove("version") + + modified_fields = set(get_modified_fields(obj, diff_versions)) + if "version" in modifying_fields: + modified_fields.remove("version") + + both_modified = modifying_fields & modified_fields + + if both_modified: + raise exc.WrongArguments({"version": "The version doesn't match with the current one"}) if obj.id: obj.version = models.F('version') + 1 diff --git a/tests/integration/test_occ.py b/tests/integration/test_occ.py index 1f0eae79..4cd408b6 100644 --- a/tests/integration/test_occ.py +++ b/tests/integration/test_occ.py @@ -30,78 +30,6 @@ from .. import factories as f pytestmark = pytest.mark.django_db -def test_invalid_concurrent_save_for_issue(client): - user = f.UserFactory.create() - project = f.ProjectFactory.create(owner=user) - membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - issue = f.IssueFactory.create(version=10, project=project) - - client.login(user) - - mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save" - with patch(mock_path) as m: - url = reverse("issues-detail", args=(issue.id,)) - data = {"version":9} - response = client.patch(url, json.dumps(data), content_type="application/json") - assert response.status_code == 400 - -def test_valid_concurrent_save_for_issue(client): - user = f.UserFactory.create() - project = f.ProjectFactory.create(owner=user) - membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - issue = f.IssueFactory.create(version=10, project=project) - - client.login(user) - - mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save" - with patch(mock_path) as m: - url = reverse("issues-detail", args=(issue.id,)) - data = {"version": 10} - response = client.patch(url, json.dumps(data), content_type="application/json") - assert json.loads(response.content)['version'] == 11 - assert response.status_code == 200 - issue = Issue.objects.get(id=issue.id) - assert issue.version == 11 - -def test_invalid_concurrent_save_for_wiki_page(client): - user = f.UserFactory.create() - project = f.ProjectFactory.create(owner=user) - membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - wiki_page = f.WikiPageFactory.create(version=10, project=project, owner=user) - client.login(user) - - url = reverse("wiki-detail", args=(wiki_page.id,)) - data = {"version":9} - response = client.patch(url, json.dumps(data), content_type="application/json") - assert response.status_code == 400 - -def test_valid_concurrent_save_for_wiki_page(client): - user = f.UserFactory.create() - project = f.ProjectFactory.create(owner=user) - membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - wiki_page = f.WikiPageFactory.create(version=10, project=project, owner=user) - client.login(user) - - url = reverse("wiki-detail", args=(wiki_page.id,)) - data = {"version": 10} - response = client.patch(url, json.dumps(data), content_type="application/json") - assert json.loads(response.content)['version'] == 11 - assert response.status_code == 200 - wiki_page = WikiPage.objects.get(id=wiki_page.id) - assert wiki_page.version == 11 - -def test_invalid_concurrent_save_for_us(client): - user = f.UserFactory.create() - project = f.ProjectFactory.create(owner=user) - membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - userstory = f.UserStoryFactory.create(version=10, project=project) - client.login(user) - - url = reverse("userstories-detail", args=(userstory.id,)) - data = {"version":9} - response = client.patch(url, json.dumps(data), content_type="application/json") - assert response.status_code == 400 - def test_valid_us_creation(client): user = f.UserFactory.create() project = f.ProjectFactory.create(owner=user) @@ -118,48 +46,294 @@ def test_valid_us_creation(client): response = client.post(url, json.dumps(data), content_type="application/json") assert response.status_code == 201 -def test_valid_concurrent_save_for_us(client): + +def test_invalid_concurrent_save_for_issue(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("issues-list") + data = {"subject": "test", + "project": project.id, + "status": f.IssueStatusFactory.create(project=project).id, + "severity": f.SeverityFactory.create(project=project).id, + "type": f.IssueTypeFactory.create(project=project).id, + "priority": f.PriorityFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201, response.content + + issue_id = json.loads(response.content)["id"] + url = reverse("issues-detail", args=(issue_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "subject": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 400 + + +def test_valid_concurrent_save_for_issue_different_versions(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("issues-list") + data = {"subject": "test", + "project": project.id, + "status": f.IssueStatusFactory.create(project=project).id, + "severity": f.SeverityFactory.create(project=project).id, + "type": f.IssueTypeFactory.create(project=project).id, + "priority": f.PriorityFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201, response.content + + issue_id = json.loads(response.content)["id"] + url = reverse("issues-detail", args=(issue_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":2, "subject": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + +def test_valid_concurrent_save_for_issue_different_fields(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.issues.api.IssueViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("issues-list") + data = {"subject": "test", + "project": project.id, + "status": f.IssueStatusFactory.create(project=project).id, + "severity": f.SeverityFactory.create(project=project).id, + "type": f.IssueTypeFactory.create(project=project).id, + "priority": f.PriorityFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201, response.content + + issue_id = json.loads(response.content)["id"] + url = reverse("issues-detail", args=(issue_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "description": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + +def test_invalid_concurrent_save_for_wiki_page(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.wiki.api.WikiViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("wiki-list") + data = {"project": project.id, "slug": "test"} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201, response.content + + wiki_id = json.loads(response.content)["id"] + url = reverse("wiki-detail", args=(wiki_id,)) + data = {"version":1, "content": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "content": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 400 + + +def test_valid_concurrent_save_for_wiki_page_different_versions(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.wiki.api.WikiViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("wiki-list") + data = {"project": project.id, "slug": "test"} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201, response.content + + wiki_id = json.loads(response.content)["id"] + url = reverse("wiki-detail", args=(wiki_id,)) + data = {"version":1, "content": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":2, "content": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + +def test_invalid_concurrent_save_for_us(client): user = f.UserFactory.create() project = f.ProjectFactory.create(owner=user) membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) userstory = f.UserStoryFactory.create(version=10, project=project) client.login(user) - url = reverse("userstories-detail", args=(userstory.id,)) - data = {"version": 10} - response = client.patch(url, json.dumps(data), content_type="application/json") - assert json.loads(response.content)['version'] == 11 - assert response.status_code == 200 - userstory = UserStory.objects.get(id=userstory.id) - assert userstory.version == 11 + mock_path = "taiga.projects.userstories.api.UserStoryViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("userstories-list") + data = {"subject": "test", + "project": project.id, + "status": f.UserStoryStatusFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201 + + userstory_id = json.loads(response.content)["id"] + url = reverse("userstories-detail", args=(userstory_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "subject": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 400 + + +def test_valid_concurrent_save_for_us_different_versions(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.userstories.api.UserStoryViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("userstories-list") + data = {"subject": "test", + "project": project.id, + "status": f.UserStoryStatusFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201 + + userstory_id = json.loads(response.content)["id"] + url = reverse("userstories-detail", args=(userstory_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":2, "subject": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + +def test_valid_concurrent_save_for_us_different_fields(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.userstories.api.UserStoryViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("userstories-list") + data = {"subject": "test", + "project": project.id, + "status": f.UserStoryStatusFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201 + + userstory_id = json.loads(response.content)["id"] + url = reverse("userstories-detail", args=(userstory_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "description": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + def test_invalid_concurrent_save_for_task(client): user = f.UserFactory.create() project = f.ProjectFactory.create(owner=user) membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - task = f.TaskFactory.create(version=10, project=project) client.login(user) mock_path = "taiga.projects.tasks.api.TaskViewSet.pre_conditions_on_save" with patch(mock_path) as m: - url = reverse("tasks-detail", args=(task.id,)) - data = {"version":9} + url = reverse("tasks-list") + data = {"subject": "test", + "project": project.id, + "status": f.TaskStatusFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201 + + task_id = json.loads(response.content)["id"] + url = reverse("tasks-detail", args=(task_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "subject": "test 2"} response = client.patch(url, json.dumps(data), content_type="application/json") assert response.status_code == 400 -def test_valid_concurrent_save_for_task(client): + +def test_valid_concurrent_save_for_task_different_versions(client): user = f.UserFactory.create() project = f.ProjectFactory.create(owner=user) membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) - task = f.TaskFactory.create(version=10, project=project) client.login(user) mock_path = "taiga.projects.tasks.api.TaskViewSet.pre_conditions_on_save" with patch(mock_path) as m: - url = reverse("tasks-detail", args=(task.id,)) - data = {"version": 10} + url = reverse("tasks-list") + data = {"subject": "test", + "project": project.id, + "status": f.TaskStatusFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201 + + task_id = json.loads(response.content)["id"] + url = reverse("tasks-detail", args=(task_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":2, "subject": "test 2"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + +def test_valid_concurrent_save_for_task_different_fields(client): + user = f.UserFactory.create() + project = f.ProjectFactory.create(owner=user) + membership = f.MembershipFactory.create(project=project, user=user, is_owner=True) + client.login(user) + + mock_path = "taiga.projects.tasks.api.TaskViewSet.pre_conditions_on_save" + with patch(mock_path) as m: + url = reverse("tasks-list") + data = {"subject": "test", + "project": project.id, + "status": f.TaskStatusFactory.create(project=project).id} + response = client.json.post(url, json.dumps(data)) + assert response.status_code == 201 + + task_id = json.loads(response.content)["id"] + url = reverse("tasks-detail", args=(task_id,)) + data = {"version":1, "subject": "test 1"} + response = client.patch(url, json.dumps(data), content_type="application/json") + assert response.status_code == 200 + + data = {"version":1, "description": "test 2"} response = client.patch(url, json.dumps(data), content_type="application/json") - assert json.loads(response.content)['version'] == 11 assert response.status_code == 200 - task = Task.objects.get(id=task.id) - assert task.version == 11