ref(rules): Fix typing for rule_snooze & rule #80306

Christinarlong · 2024-11-06T10:50:20Z

Trying a new thing where we try and do all (read: most) of the typing upfront and then move the modules to their new home.

issue ref #73858

codecov · 2024-11-06T11:27:16Z

Codecov Report

Attention: Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/sentry/api/endpoints/rule_snooze.py	97.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #80306       +/-   ##
===========================================
+ Coverage   59.79%   78.13%   +18.33%     
===========================================
  Files        7200     7191        -9     
  Lines      318458   318047      -411     
  Branches    43902    43834       -68     
===========================================
+ Hits       190419   248500    +58081     
+ Misses     123251    63183    -60068     
- Partials     4788     6364     +1576

Christinarlong · 2024-11-06T10:52:30Z

src/sentry/api/endpoints/rule_snooze.py

@@ -76,14 +77,15 @@ def serialize(self, obj, attrs, user, **kwargs):
 @region_silo_endpoint
 class BaseRuleSnoozeEndpoint(ProjectEndpoint):
    permission_classes = (ProjectAlertRulePermission,)
-    rule_model = type[Rule] | type[AlertRule]
+    rule_model: type[AlertRule | Rule]


Moved back from Generic to union typehint, since the Model bound was giving causing mypy to freak out over what T could be. E.g in lines 87-89/89-91 where we access BaseManager[AlertRule] to get fetch_for_project but mypy says BaseManager[T] doesn't have access to that func.....

might be useful to have an # abstract comment on this just so it's clear

btw the original code was a type alias so I'm surprised this worked at all!

Christinarlong · 2024-11-06T11:08:39Z

src/sentry/api/serializers/rest_framework/rule.py

@@ -64,10 +69,15 @@ def to_internal_value(self, data):
            # XXX(epurkhiser): Very hacky, but we really just want validation
            # errors that are more specific, not just 'this wasn't filled out',
            # give a more generic error for those.
-            first_error = next(iter(form.errors.values()))[0]


Interestingly, by using .values() on ErrorDict the list of ValidationError(<message>) turns into a list of strings. mypy doesn't pick up on that (django-stubs doesn't say so either) and assumes first_error is a str | ValidationError. The below way feels a bit more "stable", since I'm not sure if that transformation of ValidationError -> str via .values() is totally intentional.

so the : str here shouldn't do anything (and shouldn't be there!) -- and actually it looks like the typechecker is correct

form.errors is a ErrorsDict which is a dict[str, ErrorsList] and ErrorsList is a UserList[ValidationError | _StrOrPromise]

yesssss, though the .values() transformation makes the result a UserList[_StrOrPromise] in reality, so I guess we could just type hint the variable to that.

I would just remove the annotation personally

Christinarlong · 2024-11-06T18:50:29Z

src/sentry/api/serializers/rest_framework/rule.py

@@ -168,31 +177,6 @@ def validate_conditions(self, conditions):
    def validate(self, attrs):
        return super().validate(validate_actions(attrs))

-    def save(self, rule):


No code ran/referenced this ._.

I think this gets called from rest_framework maybe?

'o' is .save() run implicitly when people instantiate the serializer ? I couldn't find any explicit calls for .save(), and this method doesn't implement any of the create() or update(). So, we should be safe ?

DRF won't call save() automatically, it would be a method call we're making. However, I wasn't able to find any callsites for this method either.

I am not sure -- I thought it was magically invoked by forms somehow -- could probably trace when this function was introduced and see how it was used in that PR and then from there figure out if that code path is still used

:0) good thinking, looks like this (vintage) function used to be used in the rules endpoints, but now we use dataclasses (see: ProjectRuleCreator & ProjectRuleUpdater) that do the same actions instead

asottile-sentry · 2024-11-06T18:55:19Z

src/sentry/api/serializers/rest_framework/rule.py

@@ -168,31 +177,6 @@ def validate_conditions(self, conditions):
    def validate(self, attrs):
        return super().validate(validate_actions(attrs))

-    def save(self, rule):


I think this gets called from rest_framework maybe?

asottile-sentry · 2024-11-06T18:58:21Z

src/sentry/api/serializers/rest_framework/rule.py

@@ -64,10 +69,15 @@ def to_internal_value(self, data):
            # XXX(epurkhiser): Very hacky, but we really just want validation
            # errors that are more specific, not just 'this wasn't filled out',
            # give a more generic error for those.
-            first_error = next(iter(form.errors.values()))[0]


so the : str here shouldn't do anything (and shouldn't be there!) -- and actually it looks like the typechecker is correct

form.errors is a ErrorsDict which is a dict[str, ErrorsList] and ErrorsList is a UserList[ValidationError | _StrOrPromise]

asottile-sentry · 2024-11-06T18:59:34Z

src/sentry/api/endpoints/rule_snooze.py

+            # Due to mypy requiring keyword arguments to not be kwargs, we need to check the type of rule and query accordingly
+            # In RuleSnooze, rule and alert_rule are mutually exclusive (cannot both be not null)


the better fix is to give kwargs a TypedDict and leave the code as is

same above too!

nvm 🥲 it turns out a union of Literals in a variable key for a typed dict isn't supported by mypy currently (python/mypy#7752)

class RuleField(TypedDict, total=False): rule: Rule alert_rule: AlertRule kwargs: RuleField = {self.rule_field: rule}

really starting to think this base class is a maintenance problem and trying to be too cute -- and that these are actually two disparate things trying to be shoved into a BaseRuleSnoozeEndpoint shaped box

yeahhhhh like we could delegate the RuleSnooze call to the child classes, and keep the rest of the logic in a helper in the base case, since the main conflict is that the base class doesn't know if it wants rule or alert_rule in the db call.

yeah or a few composable functions would probably work too!

Wait, can you elaborate more on what composable functions means in this context ? The google tells me composable functions when we chain functions together/when functions take in other functions but I'm not sure how that would help in this case :thonk

maybe won't need to compose -- but can probably make two separate functions for create_rule_snooze and delete_rule_snooze and then call them with the appropriate arguments in the two "subclasses" here -- basically undo the inheritance and instead have free functions which accomplish the same thing (but are less reliant on weird conventions of self to shoehorn in the right arguments).

Pushing the differences in the subclasses into methods/functions could work. We have similar patterns for the OrganizationUnsubscribe endpoints and AvatarMixin.

asottile-sentry · 2024-11-06T19:00:13Z

src/sentry/api/endpoints/rule_snooze.py

@@ -104,20 +106,25 @@ def post(self, request: Request, project: Project, rule: Rule | AlertRule) -> Re

        if not can_edit_alert_rule(project.organization, request):
            raise PermissionDenied(
-                detail="Requesting user cannot mute this rule.", code=status.HTTP_403_FORBIDDEN
+                detail="Requesting user cannot mute this rule.", code=str(status.HTTP_403_FORBIDDEN)


the str(403) seems suspicious -- what was this fixing? do we even need code= (surely a PermissionDenied is always going to be a 403 right?)

yeah, good pt. the 403 comes included in the PermissionDenied exception class already

asottile-sentry · 2024-11-06T19:01:03Z

src/sentry/api/endpoints/rule_snooze.py

@@ -76,14 +77,15 @@ def serialize(self, obj, attrs, user, **kwargs):
 @region_silo_endpoint
 class BaseRuleSnoozeEndpoint(ProjectEndpoint):
    permission_classes = (ProjectAlertRulePermission,)
-    rule_model = type[Rule] | type[AlertRule]
+    rule_model: type[AlertRule | Rule]


might be useful to have an # abstract comment on this just so it's clear

btw the original code was a type alias so I'm surprised this worked at all!

markstory · 2024-11-06T19:58:21Z

src/sentry/api/endpoints/rule_snooze.py

@@ -169,11 +181,13 @@ def delete(self, request: Request, project: Project, rule: Rule | AlertRule) ->
            shared_snooze.delete()
            deletion_type = "everyone"

-        # next check if there is a mute for me that I can remove


It would be good to retain this comment.

markstory · 2024-11-06T19:58:50Z

src/sentry/api/endpoints/rule_snooze.py

@@ -197,7 +211,8 @@ def delete(self, request: Request, project: Project, rule: Rule | AlertRule) ->
        # didn't find a match but there is a shared snooze
        if shared_snooze:
            raise PermissionDenied(
-                detail="Requesting user cannot unmute this rule.", code=status.HTTP_403_FORBIDDEN
+                detail="Requesting user cannot unmute this rule.",
+                code=str(status.HTTP_403_FORBIDDEN),


This code parameter can likely be removed.

markstory · 2024-11-06T20:03:57Z

src/sentry/api/serializers/rest_framework/rule.py

@@ -168,31 +177,6 @@ def validate_conditions(self, conditions):
    def validate(self, attrs):
        return super().validate(validate_actions(attrs))

-    def save(self, rule):


DRF won't call save() automatically, it would be a method call we're making. However, I wasn't able to find any callsites for this method either.

…lass

asottile-sentry

Christinarlong added 2 commits November 5, 2024 15:37

fix mypy errors in rule modules

caf2e01

ahhhhhh all the tpying

7ce90d3

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 6, 2024

vercel bot deployed to Preview November 6, 2024 10:52 View deployment

Christinarlong added 2 commits November 6, 2024 10:31

nits

8887dea

remove unused function

f83c410

vercel bot deployed to Preview November 6, 2024 18:46 View deployment

Christinarlong commented Nov 6, 2024

View reviewed changes

Christinarlong requested a review from markstory November 6, 2024 18:49

Christinarlong marked this pull request as ready for review November 6, 2024 18:49

Christinarlong requested a review from a team as a code owner November 6, 2024 18:49

Christinarlong commented Nov 6, 2024

View reviewed changes

asottile-sentry reviewed Nov 6, 2024

View reviewed changes

markstory reviewed Nov 6, 2024

View reviewed changes

Christinarlong added 3 commits November 7, 2024 07:43

typing annotations & remove code

a2f3474

delegating to child endpoints

e22d2bb

holy moyl delegate everything

2507812

vercel bot deployed to Preview November 8, 2024 20:50 View deployment

Christinarlong added 3 commits November 8, 2024 13:12

Merge branch 'master' into christinarlong/fix-types-for-rules

fd14271

for now switch up the generic

b684856

im blind, silo endpoint decorators not compatible with base generic c…

7dd9791

…lass

vercel bot deployed to Preview November 9, 2024 01:42 View deployment

asottile-sentry approved these changes Nov 11, 2024

View reviewed changes

Christinarlong merged commit c946d7b into master Nov 11, 2024
50 checks passed

Christinarlong deleted the christinarlong/fix-types-for-rules branch November 11, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(rules): Fix typing for rule_snooze & rule #80306

ref(rules): Fix typing for rule_snooze & rule #80306

Christinarlong commented Nov 6, 2024

codecov bot commented Nov 6, 2024 •

edited

Loading

Christinarlong Nov 6, 2024 •

edited

Loading

asottile-sentry Nov 6, 2024

Christinarlong Nov 6, 2024

asottile-sentry Nov 6, 2024

Christinarlong Nov 6, 2024

asottile-sentry Nov 6, 2024

Christinarlong Nov 6, 2024

asottile-sentry Nov 6, 2024

Christinarlong Nov 6, 2024

markstory Nov 6, 2024

asottile-sentry Nov 6, 2024

Christinarlong Nov 7, 2024

asottile-sentry Nov 6, 2024

asottile-sentry Nov 6, 2024

asottile-sentry Nov 6, 2024

asottile-sentry Nov 6, 2024

Christinarlong Nov 7, 2024

asottile-sentry Nov 7, 2024

Christinarlong Nov 7, 2024 •

edited

Loading

asottile-sentry Nov 7, 2024

Christinarlong Nov 7, 2024

asottile-sentry Nov 7, 2024

markstory Nov 8, 2024

asottile-sentry Nov 6, 2024

Christinarlong Nov 6, 2024

asottile-sentry Nov 6, 2024

markstory Nov 6, 2024

markstory Nov 6, 2024

markstory Nov 6, 2024

asottile-sentry left a comment

		# Due to mypy requiring keyword arguments to not be kwargs, we need to check the type of rule and query accordingly
		# In RuleSnooze, rule and alert_rule are mutually exclusive (cannot both be not null)

ref(rules): Fix typing for rule_snooze & rule #80306

ref(rules): Fix typing for rule_snooze & rule #80306

Conversation

Christinarlong commented Nov 6, 2024

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

Christinarlong Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Christinarlong Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asottile-sentry left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2024 •

edited

Loading

Christinarlong Nov 6, 2024 •

edited

Loading

Christinarlong Nov 7, 2024 •

edited

Loading