From 5feaf80d14912ee2c1dc924deb94b878b1fc75b5 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 09:51:21 -0800 Subject: [PATCH 01/10] Test _perm_cache --- strawberry_django/utils/query.py | 6 +++--- tests/projects/schema.py | 26 +++++++++++++++++++++++++- tests/test_permissions.py | 26 +++++++++++++++++++++++++- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/strawberry_django/utils/query.py b/strawberry_django/utils/query.py index d3ba500d..c599c285 100644 --- a/strawberry_django/utils/query.py +++ b/strawberry_django/utils/query.py @@ -124,12 +124,12 @@ def filter_for_user_q( elif len(app_labels) > 1: # pragma:nocover raise ValueError(f"Cannot mix app_labels ({app_labels!r})") + # Small optimization if the user's permissions are cached perm_cache = getattr(user, "_perm_cache", None) - if perm_cache is not None: # pragma:nocover + if perm_cache is not None: f = any if any_perm else all - user_perms: Set[str] = {p.codename for p in perm_cache} - if f(p in user_perms for p in perms_list): + if f(p in perm_cache for p in perms_list): return qs q = Q() diff --git a/tests/projects/schema.py b/tests/projects/schema.py index d2a6af89..a3bacc3c 100644 --- a/tests/projects/schema.py +++ b/tests/projects/schema.py @@ -10,13 +10,16 @@ from django.db.models import ( BooleanField, Count, + CharField, Exists, ExpressionWrapper, OuterRef, Prefetch, Q, + Subquery, + Value ) -from django.db.models.functions import Now +from django.db.models.functions import Now, Coalesce from django.db.models.query import QuerySet from strawberry import relay from strawberry.types.info import Info @@ -29,6 +32,7 @@ from strawberry_django.mutations import resolvers from strawberry_django.optimizer import DjangoOptimizerExtension from strawberry_django.permissions import ( + filter_for_user, HasPerm, HasRetvalPerm, IsAuthenticated, @@ -205,6 +209,26 @@ def milestone_name(self) -> str: def milestone_name_without_only_optimization(self) -> str: return self.milestone.name + @strawberry_django.field( + annotate={ + "_private_name": lambda info: Coalesce( + Subquery( + filter_for_user( + Issue.objects.all(), + info.context.request.user, + ["projects.view_issue"], + ) + .filter(id=OuterRef("pk")) + .values("name")[:1], + output_field=CharField(), + ), + Value(None), + ) + }, + extensions=[HasPerm(perms=["projects.view_issue"])] + ) + def private_name(self, root: Issue) -> Optional[str]: + return root._private_name # type: ignore @strawberry_django.type(Tag) class TagType(relay.Node): diff --git a/tests/test_permissions.py b/tests/test_permissions.py index a09d98e6..3c18df47 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -6,6 +6,8 @@ from strawberry.relay import to_base64 from typing_extensions import Literal, TypeAlias +from strawberry_django.optimizer import DjangoOptimizerExtension + from .projects.faker import ( GroupFactory, IssueFactory, @@ -281,6 +283,29 @@ def test_superuser_required_optional(db, gql_client: GraphQLTestClient): }, } +@pytest.mark.django_db(transaction=True) +def test_perm_cached(db, gql_client: GraphQLTestClient): + query = """ + query Issue ($id: GlobalID!) { + issuePermRequired (id: $id) { + id + privateName + } + } + """ + issue = IssueFactory.create(name="Test") + + # User with permission + user_with_perm = UserFactory.create() + user_with_perm.user_permissions.add( + Permission.objects.get(codename="view_issue"), + ) + assign_perm("view_issue", user_with_perm, issue) + with gql_client.login(user_with_perm): + if DjangoOptimizerExtension.enabled.get(): + res = gql_client.query(query, {"id": to_base64("IssueType", issue.pk)}) + assert res.data["issuePermRequired"]["privateName"] == "Test" + @pytest.mark.django_db(transaction=True) @pytest.mark.parametrize("kind", perm_kinds) @@ -349,7 +374,6 @@ def test_perm_required(db, gql_client: GraphQLTestClient, kind: PermKind): }, } - @pytest.mark.django_db(transaction=True) @pytest.mark.parametrize("kind", perm_kinds) def test_perm_required_optional(db, gql_client: GraphQLTestClient, kind: PermKind): From 19138d58a7cdc1043a6b5a436d2e9077a7bf03b6 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:04:51 -0800 Subject: [PATCH 02/10] Remove extension --- tests/projects/schema.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/projects/schema.py b/tests/projects/schema.py index a3bacc3c..f9b30e3a 100644 --- a/tests/projects/schema.py +++ b/tests/projects/schema.py @@ -9,17 +9,17 @@ from django.core.exceptions import ValidationError from django.db.models import ( BooleanField, - Count, CharField, + Count, Exists, ExpressionWrapper, OuterRef, Prefetch, Q, Subquery, - Value + Value, ) -from django.db.models.functions import Now, Coalesce +from django.db.models.functions import Coalesce, Now from django.db.models.query import QuerySet from strawberry import relay from strawberry.types.info import Info @@ -32,12 +32,12 @@ from strawberry_django.mutations import resolvers from strawberry_django.optimizer import DjangoOptimizerExtension from strawberry_django.permissions import ( - filter_for_user, HasPerm, HasRetvalPerm, IsAuthenticated, IsStaff, IsSuperuser, + filter_for_user, ) from strawberry_django.relay import ListConnectionWithTotalCount @@ -225,11 +225,11 @@ def milestone_name_without_only_optimization(self) -> str: Value(None), ) }, - extensions=[HasPerm(perms=["projects.view_issue"])] ) def private_name(self, root: Issue) -> Optional[str]: return root._private_name # type: ignore + @strawberry_django.type(Tag) class TagType(relay.Node): name: strawberry.auto From 1bb88811382ea69221a0c2653c5fcd31bd4d1f74 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:05:43 -0800 Subject: [PATCH 03/10] Touch query --- strawberry_django/utils/query.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/strawberry_django/utils/query.py b/strawberry_django/utils/query.py index c599c285..e8c19cf7 100644 --- a/strawberry_django/utils/query.py +++ b/strawberry_django/utils/query.py @@ -1,5 +1,5 @@ import functools -from typing import TYPE_CHECKING, List, Optional, Set, Type, TypeVar, cast +from typing import TYPE_CHECKING, List, Optional, Type, TypeVar, cast from django.core.exceptions import FieldDoesNotExist from django.db.models import Exists, F, Model, Q, QuerySet @@ -124,7 +124,6 @@ def filter_for_user_q( elif len(app_labels) > 1: # pragma:nocover raise ValueError(f"Cannot mix app_labels ({app_labels!r})") - # Small optimization if the user's permissions are cached perm_cache = getattr(user, "_perm_cache", None) if perm_cache is not None: From 4b72da2a7ae062cc00562eb7fdb08855fca2fad7 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:06:11 -0800 Subject: [PATCH 04/10] Touch test_permissions --- tests/test_permissions.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 3c18df47..600cd04f 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -283,6 +283,7 @@ def test_superuser_required_optional(db, gql_client: GraphQLTestClient): }, } + @pytest.mark.django_db(transaction=True) def test_perm_cached(db, gql_client: GraphQLTestClient): query = """ @@ -303,8 +304,8 @@ def test_perm_cached(db, gql_client: GraphQLTestClient): assign_perm("view_issue", user_with_perm, issue) with gql_client.login(user_with_perm): if DjangoOptimizerExtension.enabled.get(): - res = gql_client.query(query, {"id": to_base64("IssueType", issue.pk)}) - assert res.data["issuePermRequired"]["privateName"] == "Test" + res = gql_client.query(query, {"id": to_base64("IssueType", issue.pk)}) + assert res.data["issuePermRequired"]["privateName"] == "Test" @pytest.mark.django_db(transaction=True) @@ -374,6 +375,7 @@ def test_perm_required(db, gql_client: GraphQLTestClient, kind: PermKind): }, } + @pytest.mark.django_db(transaction=True) @pytest.mark.parametrize("kind", perm_kinds) def test_perm_required_optional(db, gql_client: GraphQLTestClient, kind: PermKind): From 6e5d160c916414c20460693affaf91464f0cbeea Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:14:46 -0800 Subject: [PATCH 05/10] Snapshot update --- tests/projects/snapshots/schema.gql | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/projects/snapshots/schema.gql b/tests/projects/snapshots/schema.gql index b6b6b0f7..abc62786 100644 --- a/tests/projects/snapshots/schema.gql +++ b/tests/projects/snapshots/schema.gql @@ -258,6 +258,7 @@ type IssueType implements Node { ): FavoriteTypeConnection! milestoneName: String! milestoneNameWithoutOnlyOptimization: String! + privateName: String } """A connection to a list of items.""" From cc8f331426c95c4d52cacbff3cbc2fc4b8a65304 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:28:35 -0800 Subject: [PATCH 06/10] Try to fix type errors --- tests/test_permissions.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 600cd04f..101a51c6 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -305,7 +305,18 @@ def test_perm_cached(db, gql_client: GraphQLTestClient): with gql_client.login(user_with_perm): if DjangoOptimizerExtension.enabled.get(): res = gql_client.query(query, {"id": to_base64("IssueType", issue.pk)}) - assert res.data["issuePermRequired"]["privateName"] == "Test" + + assert res.errors is None, f"GraphQL errors returned: {res.errors}" + assert res.data is not None, "Query did not return any data" + + issue_data = res.data.get("issuePermRequired") + assert ( + issue_data is not None + ), "No data for 'issuePermRequired' in the response" + private_name = issue_data.get("privateName") + assert ( + private_name == "Test" + ), f"Expected privateName to be 'Test', got '{private_name}'" @pytest.mark.django_db(transaction=True) From eeb03c8f4ae3da9aa3839f19bef6c2ec8bef48a5 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:34:25 -0800 Subject: [PATCH 07/10] Optional[Set[str]] type --- strawberry_django/utils/query.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/strawberry_django/utils/query.py b/strawberry_django/utils/query.py index e8c19cf7..d92bb5da 100644 --- a/strawberry_django/utils/query.py +++ b/strawberry_django/utils/query.py @@ -1,5 +1,5 @@ import functools -from typing import TYPE_CHECKING, List, Optional, Type, TypeVar, cast +from typing import TYPE_CHECKING, List, Optional, Set, Type, TypeVar, cast from django.core.exceptions import FieldDoesNotExist from django.db.models import Exists, F, Model, Q, QuerySet @@ -125,7 +125,7 @@ def filter_for_user_q( raise ValueError(f"Cannot mix app_labels ({app_labels!r})") # Small optimization if the user's permissions are cached - perm_cache = getattr(user, "_perm_cache", None) + perm_cache: Optional[Set[str]] = getattr(user, "_perm_cache", None) if perm_cache is not None: f = any if any_perm else all if f(p in perm_cache for p in perms_list): From 6701d157129a805bbce1a3189f53b8c0ad506a5c Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:39:36 -0800 Subject: [PATCH 08/10] Maybe this will fix types? --- tests/test_permissions.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 101a51c6..44f114be 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -305,18 +305,12 @@ def test_perm_cached(db, gql_client: GraphQLTestClient): with gql_client.login(user_with_perm): if DjangoOptimizerExtension.enabled.get(): res = gql_client.query(query, {"id": to_base64("IssueType", issue.pk)}) - - assert res.errors is None, f"GraphQL errors returned: {res.errors}" - assert res.data is not None, "Query did not return any data" - - issue_data = res.data.get("issuePermRequired") - assert ( - issue_data is not None - ), "No data for 'issuePermRequired' in the response" - private_name = issue_data.get("privateName") - assert ( - private_name == "Test" - ), f"Expected privateName to be 'Test', got '{private_name}'" + assert res.data == { + "issuePermRequired": { + "id": to_base64("IssueType", issue.pk), + "privateName": issue.name, + }, + } @pytest.mark.django_db(transaction=True) From 735f138d2dc3d35cb88444c39fc3534c32ccf872 Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 10:49:41 -0800 Subject: [PATCH 09/10] Add regression comments --- tests/test_permissions.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 44f114be..78f69d62 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -286,6 +286,20 @@ def test_superuser_required_optional(db, gql_client: GraphQLTestClient): @pytest.mark.django_db(transaction=True) def test_perm_cached(db, gql_client: GraphQLTestClient): + """Validates that the permission caching mechanism correctly stores permissions as a set of strings. + + The test targets the `_perm_cache` attribute used in `utils/query.py`. It verifies that + the attribute behaves as expected, holding a `Set[str]` that represents permission + codenames, rather than direct Permission objects. + + This test addresses a regression captured by the following error: + + ``` + user_perms: Set[str] = {p.codename for p in perm_cache} + ^^^^^^^^^^ + AttributeError: 'str' object has no attribute 'codename' + ``` + """ query = """ query Issue ($id: GlobalID!) { issuePermRequired (id: $id) { From e4b067dcfe12301d5607130b2c1bd9daa598c9ca Mon Sep 17 00:00:00 2001 From: Paul Vecchio Date: Fri, 8 Mar 2024 16:50:51 -0800 Subject: [PATCH 10/10] no coalese --- tests/projects/schema.py | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/tests/projects/schema.py b/tests/projects/schema.py index f9b30e3a..7469cdf1 100644 --- a/tests/projects/schema.py +++ b/tests/projects/schema.py @@ -9,7 +9,6 @@ from django.core.exceptions import ValidationError from django.db.models import ( BooleanField, - CharField, Count, Exists, ExpressionWrapper, @@ -17,9 +16,8 @@ Prefetch, Q, Subquery, - Value, ) -from django.db.models.functions import Coalesce, Now +from django.db.models.functions import Now from django.db.models.query import QuerySet from strawberry import relay from strawberry.types.info import Info @@ -211,19 +209,15 @@ def milestone_name_without_only_optimization(self) -> str: @strawberry_django.field( annotate={ - "_private_name": lambda info: Coalesce( - Subquery( - filter_for_user( - Issue.objects.all(), - info.context.request.user, - ["projects.view_issue"], - ) - .filter(id=OuterRef("pk")) - .values("name")[:1], - output_field=CharField(), - ), - Value(None), - ) + "_private_name": lambda info: Subquery( + filter_for_user( + Issue.objects.all(), + info.context.request.user, + ["projects.view_issue"], + ) + .filter(id=OuterRef("pk")) + .values("name")[:1], + ), }, ) def private_name(self, root: Issue) -> Optional[str]: