From d6745209f2cab81efd818c75967111f6d9f25d7a Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Thu, 7 Sep 2023 16:22:41 +0000 Subject: [PATCH] django: Use .exists() instead of .count() when possible. --- tools/semgrep.yml | 24 +++++++++++++++++++ zerver/actions/invites.py | 2 +- zerver/actions/realm_domains.py | 2 +- zerver/lib/push_notifications.py | 2 +- zerver/lib/send_email.py | 2 +- zerver/management/commands/delete_realm.py | 2 +- ...237_rename_zulip_realm_to_zulipinternal.py | 2 +- zerver/models.py | 2 +- zerver/tests/test_message_edit.py | 10 ++++---- 9 files changed, 35 insertions(+), 13 deletions(-) diff --git a/tools/semgrep.yml b/tools/semgrep.yml index 6a5e12983e..00fda02b5b 100644 --- a/tools/semgrep.yml +++ b/tools/semgrep.yml @@ -209,3 +209,27 @@ rules: Annotated types containing zerver.lib.typed_endpoint.ApiParamConfig should not be nested inside Optional. Use Annotated[Optional[...], zerver.lib.typed_endpoint.ApiParamConfig(...)] instead. languages: [python] severity: ERROR + + - id: exists-instead-of-count + patterns: + - pattern-either: + - pattern: ... .count() == 0 + - pattern: | + if not ... .count(): + ... + message: 'Use "not .exists()" instead; it is more efficient' + languages: [python] + severity: ERROR + + - id: exists-instead-of-count-not-zero + patterns: + - pattern-either: + - pattern: ... .count() != 0 + - pattern: ... .count() > 0 + - pattern: ... .count() >= 1 + - pattern: | + if ... .count(): + ... + message: 'Use ".exists()" instead; it is more efficient' + languages: [python] + severity: ERROR diff --git a/zerver/actions/invites.py b/zerver/actions/invites.py index 607832703e..24a0649693 100644 --- a/zerver/actions/invites.py +++ b/zerver/actions/invites.py @@ -134,7 +134,7 @@ def too_many_recent_realm_invites(realm: Realm, num_invitees: int) -> bool: not estimated_sent["messages"] # Only after we've done the rough-estimate check, take the # time to do the exact check: - and Message.objects.filter(realm=realm, sender__is_bot=False).count() == 0 + and not Message.objects.filter(realm=realm, sender__is_bot=False).exists() ): warning_flags.append("no-messages-sent") diff --git a/zerver/actions/realm_domains.py b/zerver/actions/realm_domains.py index 1e8e6c6df2..57526dfab7 100644 --- a/zerver/actions/realm_domains.py +++ b/zerver/actions/realm_domains.py @@ -103,7 +103,7 @@ def do_remove_realm_domain( }, ) - if RealmDomain.objects.filter(realm=realm).count() == 0 and realm.emails_restricted_to_domains: + if not RealmDomain.objects.filter(realm=realm).exists() and realm.emails_restricted_to_domains: # If this was the last realm domain, we mark the realm as no # longer restricted to domain, because the feature doesn't do # anything if there are no domains, and this is probably less diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index efca9f0f8e..595c163a82 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -484,7 +484,7 @@ def send_android_push_notification( logger.warning("GCM: Got canonical ref but it already matches our ID %s!", reg_id) elif not DeviceTokenClass._default_manager.filter( token=new_reg_id, kind=DeviceTokenClass.GCM - ).count(): + ).exists(): # This case shouldn't happen; any time we get a canonical ref it should have been # previously registered in our system. # diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index 124219d78a..a6487df81a 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -454,7 +454,7 @@ def clear_scheduled_emails(user_id: int, email_type: Optional[int] = None) -> No for item in items: item.users.remove(user_id) - if item.users.all().count() == 0: + if not item.users.all().exists(): # Due to our transaction holding the row lock we have a guarantee # that the obtained COUNT is accurate, thus we can reliably use it # to decide whether to delete the ScheduledEmail row. diff --git a/zerver/management/commands/delete_realm.py b/zerver/management/commands/delete_realm.py index d8ce129b30..f7038e8c0d 100644 --- a/zerver/management/commands/delete_realm.py +++ b/zerver/management/commands/delete_realm.py @@ -39,7 +39,7 @@ realms used for testing; consider using deactivate_realm instead.""" customer = get_customer_by_realm(realm) if customer and ( customer.stripe_customer_id - or CustomerPlan.objects.filter(customer=customer).count() > 0 + or CustomerPlan.objects.filter(customer=customer).exists() ): raise CommandError("This realm has had a billing relationship associated with it!") diff --git a/zerver/migrations/0237_rename_zulip_realm_to_zulipinternal.py b/zerver/migrations/0237_rename_zulip_realm_to_zulipinternal.py index a7a02b9a38..e464edee0b 100644 --- a/zerver/migrations/0237_rename_zulip_realm_to_zulipinternal.py +++ b/zerver/migrations/0237_rename_zulip_realm_to_zulipinternal.py @@ -13,7 +13,7 @@ def rename_zulip_realm_to_zulipinternal( Realm = apps.get_model("zerver", "Realm") UserProfile = apps.get_model("zerver", "UserProfile") - if Realm.objects.count() == 0: + if not Realm.objects.exists(): # Database not yet populated, do nothing: return diff --git a/zerver/models.py b/zerver/models.py index 7fedcb89bf..621aa30298 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -3569,7 +3569,7 @@ class Attachment(AbstractAttachment): scheduled_messages = models.ManyToManyField("ScheduledMessage") def is_claimed(self) -> bool: - return self.messages.count() > 0 or self.scheduled_messages.count() > 0 + return self.messages.exists() or self.scheduled_messages.exists() def to_dict(self) -> Dict[str, Any]: return { diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 1ccea296b4..327d6e9d74 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -4639,10 +4639,9 @@ class EditMessageTest(EditMessageTestCase): ) assert ( - UserMessage.objects.filter(user_profile=cordelia, message__id=messages[2].id) + not UserMessage.objects.filter(user_profile=cordelia, message__id=messages[2].id) .extra(where=[UserMessage.where_unread()]) - .count() - == 0 + .exists() ) # Now move to a weird state and confirm we get the normal topic moved message. @@ -4712,10 +4711,9 @@ class EditMessageTest(EditMessageTestCase): ) assert ( - UserMessage.objects.filter(user_profile=cordelia, message__id=messages[4].id) + not UserMessage.objects.filter(user_profile=cordelia, message__id=messages[4].id) .extra(where=[UserMessage.where_unread()]) - .count() - == 0 + .exists() )