From d8f92237a4c25cba2cef36fd4e2c444fc91b41d2 Mon Sep 17 00:00:00 2001 From: Maxim Zhiltsov Date: Mon, 20 Jan 2025 14:44:48 +0200 Subject: [PATCH 1/5] Remove extra request in project serializer --- cvat/apps/engine/serializers.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cvat/apps/engine/serializers.py b/cvat/apps/engine/serializers.py index 6c760b42ba65..5335a2c8743e 100644 --- a/cvat/apps/engine/serializers.py +++ b/cvat/apps/engine/serializers.py @@ -2465,10 +2465,14 @@ class Meta: def to_representation(self, instance): response = super().to_representation(instance) - task_subsets = {task.subset for task in instance.tasks.all()} - task_subsets.discard('') + + task_subsets = {task.subset for task in instance.tasks.all() if task.subset} + task_dimension = next( + (task.dimension for task in instance.tasks.all() if task.dimension), + None + ) response['task_subsets'] = list(task_subsets) - response['dimension'] = getattr(instance.tasks.first(), 'dimension', None) + response['dimension'] = task_dimension return response class ProjectWriteSerializer(serializers.ModelSerializer): From c382cc4c68273e65f5b4badb01e1e61a7d458d6e Mon Sep 17 00:00:00 2001 From: Maxim Zhiltsov Date: Mon, 20 Jan 2025 17:51:57 +0200 Subject: [PATCH 2/5] Optimize inefficient related object id access in IAM checks --- cvat/apps/engine/models.py | 6 +- cvat/apps/engine/permissions.py | 174 ++++++++++++----------- cvat/apps/organizations/permissions.py | 12 +- cvat/apps/quality_control/permissions.py | 28 ++-- cvat/apps/webhooks/permissions.py | 18 ++- 5 files changed, 125 insertions(+), 113 deletions(-) diff --git a/cvat/apps/engine/models.py b/cvat/apps/engine/models.py index e012c75bef1c..fd8606d36053 100644 --- a/cvat/apps/engine/models.py +++ b/cvat/apps/engine/models.py @@ -1315,8 +1315,10 @@ class AnnotationGuide(TimestampedModel): is_public = models.BooleanField(default=False) @property - def target(self): - return self.project or self.task + def target(self) -> Task | Project: + target = self.project or self.task + assert target # one of the fields must be set + return target @property def organization_id(self): diff --git a/cvat/apps/engine/permissions.py b/cvat/apps/engine/permissions.py index a180410142cd..793ae503bafc 100644 --- a/cvat/apps/engine/permissions.py +++ b/cvat/apps/engine/permissions.py @@ -22,7 +22,7 @@ ) from cvat.apps.organizations.models import Organization -from .models import AnnotationGuide, CloudStorage, Issue, Job, Label, Project, Task +from .models import AnnotationGuide, CloudStorage, Comment, Issue, Job, Label, Project, Task, User def _get_key(d: dict[str, Any], key_path: Union[str, Sequence[str]]) -> Optional[Any]: @@ -75,6 +75,8 @@ def get_resource(self): return None class UserPermission(OpenPolicyAgentPermission): + obj: Optional[User] + class Scopes(StrEnum): LIST = 'list' VIEW = 'view' @@ -134,6 +136,8 @@ def get_resource(self): return data class CloudStoragePermission(OpenPolicyAgentPermission): + obj: Optional[CloudStorage] + class Scopes(StrEnum): LIST = 'list' LIST_CONTENT = 'list:content' @@ -196,15 +200,17 @@ def get_resource(self): elif self.obj: data = { 'id': self.obj.id, - 'owner': { 'id': getattr(self.obj.owner, 'id', None) }, + 'owner': { 'id': self.obj.owner_id }, 'organization': { - 'id': self.obj.organization.id - } if self.obj.organization else None + 'id': self.obj.organization_id + } if self.obj.organization_id else None } return data class ProjectPermission(OpenPolicyAgentPermission): + obj: Optional[Project] + class Scopes(StrEnum): LIST = 'list' CREATE = 'create' @@ -345,11 +351,9 @@ def get_resource(self): if self.obj: data = { "id": self.obj.id, - "owner": { "id": getattr(self.obj.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.assignee, 'id', None) }, - 'organization': { - "id": getattr(self.obj.organization, 'id', None) - } + "owner": { "id": self.obj.owner_id }, + "assignee": { "id": self.obj.assignee_id }, + 'organization': { "id": self.obj.organization_id }, } elif self.scope in [__class__.Scopes.CREATE, __class__.Scopes.IMPORT_BACKUP]: data = { @@ -357,7 +361,7 @@ def get_resource(self): "owner": { "id": self.user_id }, "assignee": { "id": self.assignee_id, - } if getattr(self, 'assignee_id', None) else None, + } if self.assignee_id else None, 'organization': { "id": self.org_id, } if self.org_id else None, @@ -366,6 +370,8 @@ def get_resource(self): return data class TaskPermission(OpenPolicyAgentPermission): + obj: Optional[Task] + class Scopes(StrEnum): LIST = 'list' CREATE = 'create' @@ -456,7 +462,7 @@ def create(cls, request, view, obj, iam_context): def create_scope_view(cls, request, task: Union[int, Task], iam_context=None): if isinstance(task, int): try: - task = Task.objects.get(id=task) + task = Task.objects.select_related("organization").get(id=task) except Task.DoesNotExist as ex: raise ValidationError(str(ex)) @@ -558,17 +564,13 @@ def get_resource(self): if self.obj: data = { "id": self.obj.id, - "owner": { "id": getattr(self.obj.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.assignee, 'id', None) }, - 'organization': { - "id": getattr(self.obj.organization, 'id', None) - }, + "owner": { "id": self.obj.owner_id }, + "assignee": { "id": self.obj.assignee_id }, + 'organization': { "id": self.obj.organization_id }, "project": { - "owner": { "id": getattr(self.obj.project.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.project.assignee, 'id', None) }, - 'organization': { - "id": getattr(self.obj.project.organization, 'id', None) - }, + "owner": { "id": self.obj.project.owner_id }, + "assignee": { "id": self.obj.project.assignee_id }, + 'organization': { "id": self.obj.project.organization_id }, } if self.obj.project else None } elif self.scope in [ @@ -593,11 +595,11 @@ def get_resource(self): "id": self.org_id }, "project": { - "owner": { "id": getattr(project.owner, 'id', None) }, - "assignee": { "id": getattr(project.assignee, 'id', None) }, + "owner": { "id": project.owner_id }, + "assignee": { "id": project.assignee_id }, 'organization': { - "id": getattr(project.organization, 'id', None), - } if project.organization is not None else None, + "id": project.organization_id, + } if project.organization_id else None, } if project is not None else None, } @@ -605,6 +607,7 @@ def get_resource(self): class JobPermission(OpenPolicyAgentPermission): task_id: Optional[int] + obj: Optional[Job] class Scopes(StrEnum): CREATE = 'create' @@ -750,24 +753,23 @@ def get_scopes(request, view, obj): def get_resource(self): data = None if self.obj: + organization_id = None if self.obj.segment.task.project: - organization = self.obj.segment.task.project.organization + organization_id = self.obj.segment.task.project.organization_id else: - organization = self.obj.segment.task.organization + organization_id = self.obj.segment.task.organization_id data = { "id": self.obj.id, - "assignee": { "id": getattr(self.obj.assignee, 'id', None) }, - 'organization': { - "id": getattr(organization, 'id', None) - }, + "assignee": { "id": self.obj.assignee_id }, + 'organization': { "id": organization_id }, "task": { - "owner": { "id": getattr(self.obj.segment.task.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.segment.task.assignee, 'id', None) } + "owner": { "id": self.obj.segment.task.owner_id }, + "assignee": { "id": self.obj.segment.task.assignee_id } }, "project": { - "owner": { "id": getattr(self.obj.segment.task.project.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.segment.task.project.assignee, 'id', None) } + "owner": { "id": self.obj.segment.task.project.owner_id }, + "assignee": { "id": self.obj.segment.task.project.assignee_id } } if self.obj.segment.task.project else None } elif self.scope == __class__.Scopes.CREATE: @@ -775,28 +777,29 @@ def get_resource(self): raise ValidationError("task_id is not specified") task = Task.objects.get(id=self.task_id) + organization_id = None if task.project: - organization = task.project.organization + organization_id = task.project.organization_id else: - organization = task.organization + organization_id = task.organization_id data = { - 'organization': { - "id": getattr(organization, 'id', None) - }, + 'organization': { "id": organization_id }, "task": { - "owner": { "id": getattr(task.owner, 'id', None) }, - "assignee": { "id": getattr(task.assignee, 'id', None) } + "owner": { "id": task.owner_id }, + "assignee": { "id": task.assignee_id } }, "project": { - "owner": { "id": getattr(task.project.owner, 'id', None) }, - "assignee": { "id": getattr(task.project.assignee, 'id', None) } + "owner": { "id": task.project.owner_id }, + "assignee": { "id": task.project.assignee_id } } if task.project else None } return data class CommentPermission(OpenPolicyAgentPermission): + obj: Optional[Comment] + class Scopes(StrEnum): LIST = 'list' CREATE = 'create' @@ -834,30 +837,29 @@ def get_scopes(request, view, obj): def get_resource(self): data = None def get_common_data(db_issue): + organization_id = None if db_issue.job.segment.task.project: - organization = db_issue.job.segment.task.project.organization + organization_id = db_issue.job.segment.task.project.organization_id else: - organization = db_issue.job.segment.task.organization + organization_id = db_issue.job.segment.task.organization_id data = { "project": { - "owner": { "id": getattr(db_issue.job.segment.task.project.owner, 'id', None) }, - "assignee": { "id": getattr(db_issue.job.segment.task.project.assignee, 'id', None) } + "owner": { "id": db_issue.job.segment.task.project.owner_id }, + "assignee": { "id": db_issue.job.segment.task.project.assignee_id } } if db_issue.job.segment.task.project else None, "task": { - "owner": { "id": getattr(db_issue.job.segment.task.owner, 'id', None) }, - "assignee": { "id": getattr(db_issue.job.segment.task.assignee, 'id', None) } + "owner": { "id": db_issue.job.segment.task.owner_id}, + "assignee": { "id": db_issue.job.segment.task.assignee_id } }, "job": { - "assignee": { "id": getattr(db_issue.job.assignee, 'id', None) } + "assignee": { "id": db_issue.job.assignee_id } }, "issue": { - "owner": { "id": getattr(db_issue.owner, 'id', None) }, - "assignee": { "id": getattr(db_issue.assignee, 'id', None) } + "owner": { "id": db_issue.owner_id}, + "assignee": { "id": db_issue.assignee_id } }, - 'organization': { - "id": getattr(organization, 'id', None) - } + 'organization': { "id": organization_id } } return data @@ -882,6 +884,8 @@ def get_common_data(db_issue): return data class IssuePermission(OpenPolicyAgentPermission): + obj: Optional[Issue] + class Scopes(StrEnum): LIST = 'list' CREATE = 'create' @@ -926,25 +930,26 @@ def get_scopes(request, view, obj): def get_resource(self): data = None def get_common_data(db_job): + organization_id = None if db_job.segment.task.project: - organization = db_job.segment.task.project.organization + organization_id = db_job.segment.task.project.organization_id else: - organization = db_job.segment.task.organization + organization_id = db_job.segment.task.organization_id data = { "project": { - "owner": { "id": getattr(db_job.segment.task.project.owner, 'id', None) }, - "assignee": { "id": getattr(db_job.segment.task.project.assignee, 'id', None) } + "owner": { "id": db_job.segment.task.project.owner_id }, + "assignee": { "id": db_job.segment.task.project.assignee_id } } if db_job.segment.task.project else None, "task": { - "owner": { "id": getattr(db_job.segment.task.owner, 'id', None) }, - "assignee": { "id": getattr(db_job.segment.task.assignee, 'id', None) } + "owner": { "id": db_job.segment.task.owner_id }, + "assignee": { "id": db_job.segment.task.assignee_id } }, "job": { - "assignee": { "id": getattr(db_job.assignee, 'id', None) } + "assignee": { "id": db_job.assignee_id } }, 'organization': { - "id": getattr(organization, 'id', None) + "id": organization_id } } @@ -955,8 +960,8 @@ def get_common_data(db_job): data = get_common_data(db_job) data.update({ "id": self.obj.id, - "owner": { "id": getattr(self.obj.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.assignee, 'id', None) } + "owner": { "id": self.obj.owner_id }, + "assignee": { "id": self.obj.assignee_id } }) elif self.scope.startswith(__class__.Scopes.CREATE): job_id = self.job_id @@ -1051,29 +1056,30 @@ def get_resource(self): data = None if self.obj: + organization_id = None if self.obj.project: - organization = self.obj.project.organization + organization_id = self.obj.project.organization_id else: - organization = self.obj.task.organization + organization_id = self.obj.task.organization_id data = { "id": self.obj.id, - 'organization': { - "id": getattr(organization, 'id', None) - }, + 'organization': { "id": organization_id }, "task": { - "owner": { "id": getattr(self.obj.task.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.task.assignee, 'id', None) } + "owner": { "id": self.obj.task.owner_id }, + "assignee": { "id": self.obj.task.assignee_id } } if self.obj.task else None, "project": { - "owner": { "id": getattr(self.obj.project.owner, 'id', None) }, - "assignee": { "id": getattr(self.obj.project.assignee, 'id', None) } + "owner": { "id": self.obj.project.owner_id }, + "assignee": { "id": self.obj.project.assignee_id } } if self.obj.project else None, } return data class AnnotationGuidePermission(OpenPolicyAgentPermission): + obj: Optional[AnnotationGuide] + class Scopes(StrEnum): VIEW = 'view' UPDATE = 'update' @@ -1112,16 +1118,15 @@ def get_scopes(request, view, obj): def get_resource(self): data = {} if self.obj: - db_target = getattr(self.obj, 'target', {}) - db_organization = getattr(db_target, 'organization', {}) + db_target = self.obj.target data.update({ 'id': self.obj.id, 'target': { - 'owner': { 'id': getattr(getattr(db_target, 'owner', {}), 'id', None) }, - 'assignee': { 'id': getattr(getattr(db_target, 'assignee', {}), 'id', None) }, + 'owner': { 'id': db_target.owner_id }, + 'assignee': { 'id': db_target.assignee_id }, 'is_job_staff': db_target.is_job_staff(self.user_id), }, - 'organization': { 'id': getattr(db_organization, 'id', None) } + 'organization': { 'id': self.obj.organization_id } }) elif self.scope == __class__.Scopes.CREATE: db_target = None @@ -1135,13 +1140,14 @@ def get_resource(self): db_target = Task.objects.get(id=self.task_id) except Task.DoesNotExist as ex: raise ValidationError(str(ex)) - db_organization = getattr(db_target, 'organization', {}) + + organization_id = getattr(db_target, 'organization_id', None) data.update({ 'target': { - 'owner': { 'id': db_target.owner.id }, - 'assignee': { 'id': getattr(db_target.assignee, 'id', None) } + 'owner': { 'id': getattr(db_target, "owner_id", None) }, + 'assignee': { 'id': getattr(db_target, "assignee_id", None) }, }, - 'organization': { 'id': getattr(db_organization, 'id', None) } + 'organization': { 'id': organization_id } }) return data diff --git a/cvat/apps/organizations/permissions.py b/cvat/apps/organizations/permissions.py index 1e18cf5e20c5..9c1ac67c6abc 100644 --- a/cvat/apps/organizations/permissions.py +++ b/cvat/apps/organizations/permissions.py @@ -50,7 +50,7 @@ def get_resource(self): membership = Membership.objects.filter(organization=self.obj, user=self.user_id).first() return { "id": self.obj.id, - "owner": {"id": getattr(self.obj.owner, "id", None)}, + "owner": {"id": self.obj.owner_id}, "user": {"role": membership.role if membership else None}, } elif self.scope.startswith(__class__.Scopes.CREATE.value): @@ -108,10 +108,10 @@ def get_resource(self): data = None if self.obj: data = { - "owner": {"id": getattr(self.obj.owner, "id", None)}, - "invitee": {"id": getattr(self.obj.membership.user, "id", None)}, + "owner": {"id": self.obj.owner_id}, + "invitee": {"id": self.obj.membership.user_id}, "role": self.obj.membership.role, - "organization": {"id": self.obj.membership.organization.id}, + "organization": {"id": self.obj.membership.organization_id}, } elif self.scope.startswith(__class__.Scopes.CREATE.value): data = { @@ -181,8 +181,8 @@ def get_resource(self): return { "role": self.obj.role, "is_active": self.obj.is_active, - "user": {"id": self.obj.user.id}, - "organization": {"id": self.obj.organization.id}, + "user": {"id": self.obj.user_id}, + "organization": {"id": self.obj.organization_id}, } else: return None diff --git a/cvat/apps/quality_control/permissions.py b/cvat/apps/quality_control/permissions.py index d6d1a6bf1797..035a5263db3b 100644 --- a/cvat/apps/quality_control/permissions.py +++ b/cvat/apps/quality_control/permissions.py @@ -139,25 +139,25 @@ def get_resource(self): if task and task.project: project = task.project - organization = project.organization + organization_id = project.organization_id else: - organization = getattr(task, "organization", None) + organization_id = task.organization_id data = { "id": obj_id, - "organization": {"id": getattr(organization, "id", None)}, + "organization": {"id": organization_id}, "task": ( { - "owner": {"id": getattr(task.owner, "id", None)}, - "assignee": {"id": getattr(task.assignee, "id", None)}, + "owner": {"id": task.owner_id}, + "assignee": {"id": task.assignee_id}, } if task else None ), "project": ( { - "owner": {"id": getattr(project.owner, "id", None)}, - "assignee": {"id": getattr(project.assignee, "id", None)}, + "owner": {"id": project.owner_id}, + "assignee": {"id": project.assignee_id}, } if project else None @@ -279,25 +279,25 @@ def get_resource(self): if self.obj: task = self.obj.task if task.project: - organization = task.project.organization + organization_id = task.project.organization_id else: - organization = task.organization + organization_id = task.organization_id data = { "id": self.obj.id, - "organization": {"id": getattr(organization, "id", None)}, + "organization": {"id": organization_id}, "task": ( { - "owner": {"id": getattr(task.owner, "id", None)}, - "assignee": {"id": getattr(task.assignee, "id", None)}, + "owner": {"id": task.owner_id}, + "assignee": {"id": task.assignee_id}, } if task else None ), "project": ( { - "owner": {"id": getattr(task.project.owner, "id", None)}, - "assignee": {"id": getattr(task.project.assignee, "id", None)}, + "owner": {"id": task.project.owner_id}, + "assignee": {"id": task.project.assignee_id}, } if task.project else None diff --git a/cvat/apps/webhooks/permissions.py b/cvat/apps/webhooks/permissions.py index 3ce72bd350a4..83aefcb172de 100644 --- a/cvat/apps/webhooks/permissions.py +++ b/cvat/apps/webhooks/permissions.py @@ -3,6 +3,8 @@ # # SPDX-License-Identifier: MIT +from typing import Optional + from django.conf import settings from rest_framework.exceptions import ValidationError @@ -10,10 +12,12 @@ from cvat.apps.engine.permissions import ProjectPermission, UserPermission from cvat.apps.iam.permissions import OpenPolicyAgentPermission, StrEnum -from .models import WebhookTypeChoice +from .models import Webhook, WebhookTypeChoice class WebhookPermission(OpenPolicyAgentPermission): + obj: Optional[Webhook] + class Scopes(StrEnum): CREATE = "create" CREATE_IN_PROJECT = "create@project" @@ -81,12 +85,12 @@ def get_resource(self): if self.obj: data = { "id": self.obj.id, - "owner": {"id": getattr(self.obj.owner, "id", None)}, - "organization": {"id": getattr(self.obj.organization, "id", None)}, + "owner": {"id": self.obj.owner_id}, + "organization": {"id": self.obj.organization_id}, "project": None, } - if self.obj.type == "project" and getattr(self.obj, "project", None): - data["project"] = {"owner": {"id": getattr(self.obj.project.owner, "id", None)}} + if self.obj.type == "project" and self.obj.project_id: + data["project"] = {"owner": {"id": self.obj.project.owner_id}} elif self.scope in [ __class__.Scopes.CREATE, __class__.Scopes.CREATE_IN_PROJECT, @@ -108,9 +112,9 @@ def get_resource(self): { "owner": ( { - "id": project.owner.id, + "id": project.owner_id, } - if project.owner + if project.owner_id else None ), } From 2864da11bd0cd85aae9210a9e05f34e8ce9cb0d2 Mon Sep 17 00:00:00 2001 From: Maxim Zhiltsov Date: Tue, 21 Jan 2025 18:18:29 +0200 Subject: [PATCH 3/5] Remove extra change --- cvat/apps/engine/permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cvat/apps/engine/permissions.py b/cvat/apps/engine/permissions.py index 793ae503bafc..a79a159ae088 100644 --- a/cvat/apps/engine/permissions.py +++ b/cvat/apps/engine/permissions.py @@ -462,7 +462,7 @@ def create(cls, request, view, obj, iam_context): def create_scope_view(cls, request, task: Union[int, Task], iam_context=None): if isinstance(task, int): try: - task = Task.objects.select_related("organization").get(id=task) + task = Task.objects.get(id=task) except Task.DoesNotExist as ex: raise ValidationError(str(ex)) From dd0375e0bf0e75cb11bddb1279f2449c78f16459 Mon Sep 17 00:00:00 2001 From: Maxim Zhiltsov Date: Tue, 21 Jan 2025 18:19:22 +0200 Subject: [PATCH 4/5] Remove extra assignment --- cvat/apps/engine/permissions.py | 1 - 1 file changed, 1 deletion(-) diff --git a/cvat/apps/engine/permissions.py b/cvat/apps/engine/permissions.py index a79a159ae088..fe6d70603ca3 100644 --- a/cvat/apps/engine/permissions.py +++ b/cvat/apps/engine/permissions.py @@ -753,7 +753,6 @@ def get_scopes(request, view, obj): def get_resource(self): data = None if self.obj: - organization_id = None if self.obj.segment.task.project: organization_id = self.obj.segment.task.project.organization_id else: From 0bebade83995147267cd96cd0a0ea669fbb65042 Mon Sep 17 00:00:00 2001 From: Maxim Zhiltsov Date: Wed, 22 Jan 2025 13:02:41 +0200 Subject: [PATCH 5/5] Remove extra code --- cvat/apps/engine/permissions.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cvat/apps/engine/permissions.py b/cvat/apps/engine/permissions.py index fe6d70603ca3..0a81751284c3 100644 --- a/cvat/apps/engine/permissions.py +++ b/cvat/apps/engine/permissions.py @@ -776,7 +776,6 @@ def get_resource(self): raise ValidationError("task_id is not specified") task = Task.objects.get(id=self.task_id) - organization_id = None if task.project: organization_id = task.project.organization_id else: @@ -836,7 +835,6 @@ def get_scopes(request, view, obj): def get_resource(self): data = None def get_common_data(db_issue): - organization_id = None if db_issue.job.segment.task.project: organization_id = db_issue.job.segment.task.project.organization_id else: @@ -929,7 +927,6 @@ def get_scopes(request, view, obj): def get_resource(self): data = None def get_common_data(db_job): - organization_id = None if db_job.segment.task.project: organization_id = db_job.segment.task.project.organization_id else: @@ -1055,7 +1052,6 @@ def get_resource(self): data = None if self.obj: - organization_id = None if self.obj.project: organization_id = self.obj.project.organization_id else: