From 760bb1b88accf65a9927071a05ec7df723346e08 Mon Sep 17 00:00:00 2001 From: Penaz Date: Thu, 1 Sep 2022 21:45:50 +0200 Subject: [PATCH 01/18] Add checkers for save() and create() in for loops --- pylint_django/checkers/__init__.py | 2 + pylint_django/checkers/modelcreate_forloop.py | 42 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 pylint_django/checkers/modelcreate_forloop.py diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index 1a14b746..522326a1 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -5,6 +5,7 @@ from pylint_django.checkers.forms import FormChecker from pylint_django.checkers.json_response import JsonResponseChecker from pylint_django.checkers.models import ModelChecker +from pylint_django.checkers.modelcreate_forloop import ModelCreateForLoopChecker def register_checkers(linter): @@ -15,3 +16,4 @@ def register_checkers(linter): linter.register_checker(FormChecker(linter)) linter.register_checker(AuthUserChecker(linter)) linter.register_checker(ForeignKeyStringsChecker(linter)) + linter.register_checker(ModelCreateForLoopChecker(linter)) diff --git a/pylint_django/checkers/modelcreate_forloop.py b/pylint_django/checkers/modelcreate_forloop.py new file mode 100644 index 00000000..6e210764 --- /dev/null +++ b/pylint_django/checkers/modelcreate_forloop.py @@ -0,0 +1,42 @@ +from astroid.nodes import Call +from pylint import checkers, interfaces + +from pylint_django.__pkginfo__ import BASE_ID +from pylint_django.utils import node_is_subclass + + +class ModelCreateForLoopChecker(checkers.BaseChecker): + __implements__ = (interfaces.IAstroidChecker) + + name = "model-save-forloop-checker" + + msgs = { + f"R{BASE_ID}04": ( + "'Model.create()' in for loop", + "model-create-in-forloop", + "Using 'Model.create()' inside a for loop may impact performance. " + "Use 'Model.bulk_create()' instead." + ), + f"R{BASE_ID}05": ( + "'Model.save()' in for loop", + "model-save-in-forloop", + "Using 'Model.save()' inside a for loop may impact performance. " + "Use 'Model.bulk_update()' or 'Model.bulk_create()' instead." + ), + } + + def visit_for(self, node): + """ + Checks for a Model.create() inside of a for loop + """ + for subnode in node.body: + if not node_is_subclass(node, "django.db.models.base.Model", + ".Model"): + # We care about models only + return + for child in subnode.get_children(): + if isinstance(child, Call): + if child.func.as_string() == "create": + self.add_message(f"R{BASE_ID}01", node=child) + if child.func.as_string() == "save": + self.add_message(f"R{BASE_ID}02", node=child) From 0d042d76cfeadbb9972763951a8e76e5f522cd70 Mon Sep 17 00:00:00 2001 From: Penaz Date: Thu, 1 Sep 2022 21:46:31 +0200 Subject: [PATCH 02/18] Add Tests for create() in for loop checker --- .../tests/input/func_queryset_hints.py | 22 +++++++++++++++++++ .../tests/input/func_queryset_hints.txt | 2 ++ 2 files changed, 24 insertions(+) create mode 100644 pylint_django/tests/input/func_queryset_hints.py create mode 100644 pylint_django/tests/input/func_queryset_hints.txt diff --git a/pylint_django/tests/input/func_queryset_hints.py b/pylint_django/tests/input/func_queryset_hints.py new file mode 100644 index 00000000..5c9ffe31 --- /dev/null +++ b/pylint_django/tests/input/func_queryset_hints.py @@ -0,0 +1,22 @@ +""" +Checks that using save() or create() inside a for loop +raises complaints from PyLint +""" +# pylint: disable=missing-docstring,too-few-public-methods + +from django.db import models + + +class Book(models.Model): + name = models.CharField(max_length=100) + + +def for_create(): + for i in range(10): + Book.objects.create(name=str(i)) + + +def for_nested_if_create(): + for i in range(10): + if i % 2 == 0: + Book.objects.create(name=str(i)) diff --git a/pylint_django/tests/input/func_queryset_hints.txt b/pylint_django/tests/input/func_queryset_hints.txt new file mode 100644 index 00000000..d28f7f8f --- /dev/null +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -0,0 +1,2 @@ +model-create-in-forloop:16:22:16:40:for_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:22:26:22:44:for_create:'Model.create()' in for loop:UNDEFINED From 330d2bcfd972d47f4cc4ee4ac2b414030b4b4849 Mon Sep 17 00:00:00 2001 From: Penaz Date: Thu, 1 Sep 2022 21:49:11 +0200 Subject: [PATCH 03/18] Changed name of model save in for loop checker --- pylint_django/checkers/__init__.py | 4 ++-- .../checkers/{modelcreate_forloop.py => modelsave_forloop.py} | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename pylint_django/checkers/{modelcreate_forloop.py => modelsave_forloop.py} (96%) diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index 522326a1..3efe5d03 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -5,7 +5,7 @@ from pylint_django.checkers.forms import FormChecker from pylint_django.checkers.json_response import JsonResponseChecker from pylint_django.checkers.models import ModelChecker -from pylint_django.checkers.modelcreate_forloop import ModelCreateForLoopChecker +from pylint_django.checkers.modelsave_forloop import ModelSaveForLoopChecker def register_checkers(linter): @@ -16,4 +16,4 @@ def register_checkers(linter): linter.register_checker(FormChecker(linter)) linter.register_checker(AuthUserChecker(linter)) linter.register_checker(ForeignKeyStringsChecker(linter)) - linter.register_checker(ModelCreateForLoopChecker(linter)) + linter.register_checker(ModelSaveForLoopChecker(linter)) diff --git a/pylint_django/checkers/modelcreate_forloop.py b/pylint_django/checkers/modelsave_forloop.py similarity index 96% rename from pylint_django/checkers/modelcreate_forloop.py rename to pylint_django/checkers/modelsave_forloop.py index 6e210764..66778a64 100644 --- a/pylint_django/checkers/modelcreate_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -5,7 +5,7 @@ from pylint_django.utils import node_is_subclass -class ModelCreateForLoopChecker(checkers.BaseChecker): +class ModelSaveForLoopChecker(checkers.BaseChecker): __implements__ = (interfaces.IAstroidChecker) name = "model-save-forloop-checker" From a8d0f3651557c201ca7290931d7570e01383bbb4 Mon Sep 17 00:00:00 2001 From: Penaz Date: Thu, 1 Sep 2022 22:07:12 +0200 Subject: [PATCH 04/18] Added checker utils to modelsave_forloop --- pylint_django/checkers/modelsave_forloop.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 66778a64..37d91d4e 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -1,8 +1,10 @@ from astroid.nodes import Call from pylint import checkers, interfaces +from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID from pylint_django.utils import node_is_subclass +# from pylint_django.augmentations import is_manager_attribute class ModelSaveForLoopChecker(checkers.BaseChecker): @@ -25,6 +27,8 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): ), } + @utils.check_messages("model-save-in-forloop") + @utils.check_messages("model-create-in-forloop") def visit_for(self, node): """ Checks for a Model.create() inside of a for loop From 9fa03d6200ee46cfea7bde780ff94b38613fc8c5 Mon Sep 17 00:00:00 2001 From: Penaz Date: Thu, 1 Sep 2022 22:07:20 +0200 Subject: [PATCH 05/18] Update tests --- pylint_django/tests/input/func_queryset_hints.py | 3 +++ pylint_django/tests/input/func_queryset_hints.txt | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pylint_django/tests/input/func_queryset_hints.py b/pylint_django/tests/input/func_queryset_hints.py index 5c9ffe31..621cff54 100644 --- a/pylint_django/tests/input/func_queryset_hints.py +++ b/pylint_django/tests/input/func_queryset_hints.py @@ -10,6 +10,9 @@ class Book(models.Model): name = models.CharField(max_length=100) + class Meta: + app_label = "test_app" + def for_create(): for i in range(10): diff --git a/pylint_django/tests/input/func_queryset_hints.txt b/pylint_django/tests/input/func_queryset_hints.txt index d28f7f8f..7b679a03 100644 --- a/pylint_django/tests/input/func_queryset_hints.txt +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -1,2 +1,2 @@ -model-create-in-forloop:16:22:16:40:for_create:'Model.create()' in for loop:UNDEFINED -model-create-in-forloop:22:26:22:44:for_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:19:22:19:40:for_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:25:26:25:44:for_nested_if_create:'Model.create()' in for loop:UNDEFINED From e433e6b044c682ce0d1ed305f250bad134157076 Mon Sep 17 00:00:00 2001 From: Penaz Date: Thu, 1 Sep 2022 22:11:04 +0200 Subject: [PATCH 06/18] Fix some silly mistakes --- pylint_django/checkers/modelsave_forloop.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 37d91d4e..26652cdf 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -33,8 +33,8 @@ def visit_for(self, node): """ Checks for a Model.create() inside of a for loop """ - for subnode in node.body: - if not node_is_subclass(node, "django.db.models.base.Model", + for subnode in node.get_children(): + if not node_is_subclass(subnode, "django.db.models.base.Model", ".Model"): # We care about models only return From 2cdbcd52d677d813780d39b676a2eed1eb405dbc Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 10:28:18 +0200 Subject: [PATCH 07/18] Fix checker, may introduce false positives --- pylint_django/checkers/modelsave_forloop.py | 25 +++++++++++-------- .../tests/input/func_queryset_hints.py | 5 ++++ .../tests/input/func_queryset_hints.txt | 5 ++-- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 26652cdf..098884e4 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -1,9 +1,8 @@ -from astroid.nodes import Call +from astroid.nodes import Call, Expr from pylint import checkers, interfaces from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID -from pylint_django.utils import node_is_subclass # from pylint_django.augmentations import is_manager_attribute @@ -32,15 +31,19 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): def visit_for(self, node): """ Checks for a Model.create() inside of a for loop + + May create false positives """ - for subnode in node.get_children(): - if not node_is_subclass(subnode, "django.db.models.base.Model", - ".Model"): - # We care about models only - return + for subnode in node.body: for child in subnode.get_children(): + if isinstance(child, Expr): + for subchild in child.get_children(): + self._check_forloop_create_save(subchild) if isinstance(child, Call): - if child.func.as_string() == "create": - self.add_message(f"R{BASE_ID}01", node=child) - if child.func.as_string() == "save": - self.add_message(f"R{BASE_ID}02", node=child) + self._check_forloop_create_save(child) + + def _check_forloop_create_save(self, node): + if node.func.attrname == "create": + self.add_message(f"R{BASE_ID}04", node=node) + if node.func.attrname == "save": + self.add_message(f"R{BASE_ID}05", node=node) diff --git a/pylint_django/tests/input/func_queryset_hints.py b/pylint_django/tests/input/func_queryset_hints.py index 621cff54..15a03683 100644 --- a/pylint_django/tests/input/func_queryset_hints.py +++ b/pylint_django/tests/input/func_queryset_hints.py @@ -23,3 +23,8 @@ def for_nested_if_create(): for i in range(10): if i % 2 == 0: Book.objects.create(name=str(i)) + + +def assigned_for_create(): + for i in range(10): + _ = Book.objects.create(name=str(i)) diff --git a/pylint_django/tests/input/func_queryset_hints.txt b/pylint_django/tests/input/func_queryset_hints.txt index 7b679a03..75c46f60 100644 --- a/pylint_django/tests/input/func_queryset_hints.txt +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -1,2 +1,3 @@ -model-create-in-forloop:19:22:19:40:for_create:'Model.create()' in for loop:UNDEFINED -model-create-in-forloop:25:26:25:44:for_nested_if_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:19:8:19:40:for_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:25:12:25:44:for_nested_if_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:30:8:30:49:assigned_for_create:'Model.create()' in for loop:UNDEFINED From fbfaad458899f961db79c3e92dccc7e341f6d1e8 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 13:41:30 +0200 Subject: [PATCH 08/18] Check only calls in modelsave_forloop --- pylint_django/checkers/modelsave_forloop.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 098884e4..b830785a 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -38,7 +38,8 @@ def visit_for(self, node): for child in subnode.get_children(): if isinstance(child, Expr): for subchild in child.get_children(): - self._check_forloop_create_save(subchild) + if isinstance(subchild, Call): + self._check_forloop_create_save(subchild) if isinstance(child, Call): self._check_forloop_create_save(child) From 7806d0a53275f394164c759511104fe330b03fd3 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 13:42:00 +0200 Subject: [PATCH 09/18] Changed test output --- pylint_django/tests/input/func_queryset_hints.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint_django/tests/input/func_queryset_hints.txt b/pylint_django/tests/input/func_queryset_hints.txt index 75c46f60..9049e802 100644 --- a/pylint_django/tests/input/func_queryset_hints.txt +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -1,3 +1,3 @@ model-create-in-forloop:19:8:19:40:for_create:'Model.create()' in for loop:UNDEFINED model-create-in-forloop:25:12:25:44:for_nested_if_create:'Model.create()' in for loop:UNDEFINED -model-create-in-forloop:30:8:30:49:assigned_for_create:'Model.create()' in for loop:UNDEFINED +model-create-in-forloop:30:12:30:49:assigned_for_create:'Model.create()' in for loop:UNDEFINED From c334ca45b31e6e1137513bb267d3123ee2bbeb07 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 13:43:49 +0200 Subject: [PATCH 10/18] Change messages wording --- pylint_django/checkers/modelsave_forloop.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index b830785a..3f5ecd34 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -13,21 +13,22 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): msgs = { f"R{BASE_ID}04": ( - "'Model.create()' in for loop", - "model-create-in-forloop", + "Consider using 'Model.bulk_create()'", + "consider-using-bulk-create", "Using 'Model.create()' inside a for loop may impact performance. " - "Use 'Model.bulk_create()' instead." + "Consider using 'Model.bulk_create()' instead." ), f"R{BASE_ID}05": ( - "'Model.save()' in for loop", - "model-save-in-forloop", + "Consider using 'Model.bulk_*()", + "consider-using-bulk-create-save", "Using 'Model.save()' inside a for loop may impact performance. " - "Use 'Model.bulk_update()' or 'Model.bulk_create()' instead." + "Consider using 'Model.bulk_update()' or " + "'Model.bulk_create()' instead." ), } - @utils.check_messages("model-save-in-forloop") - @utils.check_messages("model-create-in-forloop") + @utils.check_messages("consider-using-bulk-create") + @utils.check_messages("consider-using-bulk-create-save") def visit_for(self, node): """ Checks for a Model.create() inside of a for loop From 33a4437de734adf088f2b374c378b0e37672ab14 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 14:49:02 +0200 Subject: [PATCH 11/18] Added basic checker for get() and filter() in for loops --- pylint_django/checkers/__init__.py | 3 ++ pylint_django/checkers/modelfilter_forloop.py | 41 +++++++++++++++++++ .../tests/input/func_queryset_hints.py | 13 ++++-- .../tests/input/func_queryset_hints.txt | 7 ++-- 4 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 pylint_django/checkers/modelfilter_forloop.py diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index 3efe5d03..fcce6bb5 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -6,6 +6,8 @@ from pylint_django.checkers.json_response import JsonResponseChecker from pylint_django.checkers.models import ModelChecker from pylint_django.checkers.modelsave_forloop import ModelSaveForLoopChecker +from pylint_django.checkers.modelfilter_forloop import\ + ModelFilterForLoopChecker def register_checkers(linter): @@ -17,3 +19,4 @@ def register_checkers(linter): linter.register_checker(AuthUserChecker(linter)) linter.register_checker(ForeignKeyStringsChecker(linter)) linter.register_checker(ModelSaveForLoopChecker(linter)) + linter.register_checker(ModelFilterForLoopChecker(linter)) diff --git a/pylint_django/checkers/modelfilter_forloop.py b/pylint_django/checkers/modelfilter_forloop.py new file mode 100644 index 00000000..c57432c1 --- /dev/null +++ b/pylint_django/checkers/modelfilter_forloop.py @@ -0,0 +1,41 @@ +from astroid.nodes import Call, Expr, Name, ClassDef +from pylint import checkers, interfaces +from pylint.checkers import utils + +from pylint_django.__pkginfo__ import BASE_ID + + +class ModelFilterForLoopChecker(checkers.BaseChecker): + __implements__ = (interfaces.IAstroidChecker) + + name = "model-filter-forloop-checker" + + msgs = { + f"R{BASE_ID}06": ( + "Consider using '__in' queries", + "consider-using-in-queries", + "Using 'Model.filter()' or 'Model.get() inside a for loop may " + "impact performance. Consider using a single query with as '__in' " + "filter instead, outside of the loop." + ), + } + + @utils.check_messages("consider-using-in-queries") + def visit_for(self, node): + """ + Checks for a Queryset.filter() inside of a for loop + + May create false positives + """ + for subnode in node.body: + for child in subnode.get_children(): + if isinstance(child, Expr): + for subchild in child.get_children(): + if isinstance(subchild, Call): + self._check_forloop_filter(subchild) + if isinstance(child, Call): + self._check_forloop_filter(child) + + def _check_forloop_filter(self, node): + if node.func.attrname in ("filter", "get"): + self.add_message(f"R{BASE_ID}06", node=node) diff --git a/pylint_django/tests/input/func_queryset_hints.py b/pylint_django/tests/input/func_queryset_hints.py index 15a03683..f9ef8aeb 100644 --- a/pylint_django/tests/input/func_queryset_hints.py +++ b/pylint_django/tests/input/func_queryset_hints.py @@ -8,7 +8,7 @@ class Book(models.Model): - name = models.CharField(max_length=100) + name = models.CharField(max_length=100) # [consider-using-bulk-create] class Meta: app_label = "test_app" @@ -16,15 +16,20 @@ class Meta: def for_create(): for i in range(10): - Book.objects.create(name=str(i)) + Book.objects.create(name=str(i)) # [consider-using-bulk-create] def for_nested_if_create(): for i in range(10): if i % 2 == 0: - Book.objects.create(name=str(i)) + Book.objects.create(name=str(i)) # [consider-using-bulk-create] def assigned_for_create(): for i in range(10): - _ = Book.objects.create(name=str(i)) + _ = Book.objects.create(name=str(i)) # [consider-using-bulk-create] + + +def for_filter(): + for i in range(10): + _ = Book.objects.filter(name=str(i)) # [consider-using-in-queries] diff --git a/pylint_django/tests/input/func_queryset_hints.txt b/pylint_django/tests/input/func_queryset_hints.txt index 9049e802..567ff1c9 100644 --- a/pylint_django/tests/input/func_queryset_hints.txt +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -1,3 +1,4 @@ -model-create-in-forloop:19:8:19:40:for_create:'Model.create()' in for loop:UNDEFINED -model-create-in-forloop:25:12:25:44:for_nested_if_create:'Model.create()' in for loop:UNDEFINED -model-create-in-forloop:30:12:30:49:assigned_for_create:'Model.create()' in for loop:UNDEFINED +consider-using-bulk-create:19:8:19:40:for_create:Consider using 'Model.bulk_create()':UNDEFINED +consider-using-bulk-create:25:12:25:44:for_nested_if_create:Consider using 'Model.bulk_create()':UNDEFINED +consider-using-bulk-create:30:12:30:49:assigned_for_create:Consider using 'Model.bulk_create()':UNDEFINED +consider-using-in-queries:35:12:35:49:for_filter:Consider using 'Model.bulk_*():UNDEFINED From 0570ce21706f320e9f87b63183dcc78c1119bf21 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 15:03:03 +0200 Subject: [PATCH 12/18] Added basic checker for .all() in for loops --- pylint_django/checkers/__init__.py | 5 ++- pylint_django/checkers/queryset_iterator.py | 35 +++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 pylint_django/checkers/queryset_iterator.py diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index fcce6bb5..bf6cd93f 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -5,9 +5,11 @@ from pylint_django.checkers.forms import FormChecker from pylint_django.checkers.json_response import JsonResponseChecker from pylint_django.checkers.models import ModelChecker -from pylint_django.checkers.modelsave_forloop import ModelSaveForLoopChecker from pylint_django.checkers.modelfilter_forloop import\ ModelFilterForLoopChecker +from pylint_django.checkers.modelsave_forloop import ModelSaveForLoopChecker +from pylint_django.checkers.queryset_iterator import \ + QuerysetIteratorForLoopChecker def register_checkers(linter): @@ -20,3 +22,4 @@ def register_checkers(linter): linter.register_checker(ForeignKeyStringsChecker(linter)) linter.register_checker(ModelSaveForLoopChecker(linter)) linter.register_checker(ModelFilterForLoopChecker(linter)) + linter.register_checker(QuerysetIteratorForLoopChecker(linter)) diff --git a/pylint_django/checkers/queryset_iterator.py b/pylint_django/checkers/queryset_iterator.py new file mode 100644 index 00000000..b532e66a --- /dev/null +++ b/pylint_django/checkers/queryset_iterator.py @@ -0,0 +1,35 @@ +from astroid.nodes import Call, Attribute +from pylint import checkers, interfaces +from pylint.checkers import utils + +from pylint_django.__pkginfo__ import BASE_ID +# from pylint_django.augmentations import is_manager_attribute + + +class QuerysetIteratorForLoopChecker(checkers.BaseChecker): + __implements__ = (interfaces.IAstroidChecker) + + name = "queryset-iterator-forloop-checker" + + msgs = { + f"R{BASE_ID}07": ( + "Consider using 'Queryset.iterator()'", + "consider-using-queryset-iterator", + "Using 'QuerySet.all()' may load all results in memory " + "if you need to iterate over the data only once and don't need " + "caching, 'QuerySet.iterator()' may be a more performant option." + ), + } + + @utils.check_messages("consider-using-queryset-iterator") + def visit_for(self, node): + """ + Checks for a QuerySet.all() inside of a for loop + + May create false positives + """ + for subnode in node.get_children(): + if isinstance(subnode, Call): + if isinstance(subnode.func, Attribute): + if subnode.func.attrname == "all": + self.add_message(f"R{BASE_ID}07", node=subnode) From 004793da37819e129694534679ed43c8176d9077 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 2 Sep 2022 15:03:16 +0200 Subject: [PATCH 13/18] Improved stability of .filter() checker --- pylint_django/checkers/modelfilter_forloop.py | 9 ++++++--- pylint_django/checkers/queryset_iterator.py | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pylint_django/checkers/modelfilter_forloop.py b/pylint_django/checkers/modelfilter_forloop.py index c57432c1..26875410 100644 --- a/pylint_django/checkers/modelfilter_forloop.py +++ b/pylint_django/checkers/modelfilter_forloop.py @@ -1,4 +1,4 @@ -from astroid.nodes import Call, Expr, Name, ClassDef +from astroid.nodes import Call, Expr, Attribute from pylint import checkers, interfaces from pylint.checkers import utils @@ -37,5 +37,8 @@ def visit_for(self, node): self._check_forloop_filter(child) def _check_forloop_filter(self, node): - if node.func.attrname in ("filter", "get"): - self.add_message(f"R{BASE_ID}06", node=node) + if isinstance(node.func, Attribute): + # Ensure it's an attribute, to avoid clashing with the + # filter() python builtin, may still clash with the dict .get() + if node.func.attrname in ("filter", "get"): + self.add_message(f"R{BASE_ID}06", node=node) diff --git a/pylint_django/checkers/queryset_iterator.py b/pylint_django/checkers/queryset_iterator.py index b532e66a..cdc1d076 100644 --- a/pylint_django/checkers/queryset_iterator.py +++ b/pylint_django/checkers/queryset_iterator.py @@ -31,5 +31,7 @@ def visit_for(self, node): for subnode in node.get_children(): if isinstance(subnode, Call): if isinstance(subnode.func, Attribute): + # Ensure it's an attribute, to avoid clashing with the + # all() python builtin if subnode.func.attrname == "all": self.add_message(f"R{BASE_ID}07", node=subnode) From ff0683c1d0ec2bea76422222d84bc39d84f75c21 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 9 Sep 2022 14:31:21 +0200 Subject: [PATCH 14/18] Added guard for attributes --- pylint_django/checkers/modelsave_forloop.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 3f5ecd34..8aaaa3dc 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -1,4 +1,4 @@ -from astroid.nodes import Call, Expr +from astroid.nodes import Call, Expr, Attribute from pylint import checkers, interfaces from pylint.checkers import utils @@ -45,7 +45,9 @@ def visit_for(self, node): self._check_forloop_create_save(child) def _check_forloop_create_save(self, node): - if node.func.attrname == "create": - self.add_message(f"R{BASE_ID}04", node=node) - if node.func.attrname == "save": - self.add_message(f"R{BASE_ID}05", node=node) + if isinstance(node.func, Attribute): + # Ensure node.func is an attribute to avoid crashes + if node.func.attrname == "create": + self.add_message(f"R{BASE_ID}04", node=node) + if node.func.attrname == "save": + self.add_message(f"R{BASE_ID}05", node=node) From 8a9ca96947c9b81fc3f3aae34595ad9279672c49 Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 9 Sep 2022 14:31:50 +0200 Subject: [PATCH 15/18] Fixed and added more tests --- pylint_django/tests/input/func_queryset_hints.py | 8 +++++++- pylint_django/tests/input/func_queryset_hints.txt | 5 +++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/pylint_django/tests/input/func_queryset_hints.py b/pylint_django/tests/input/func_queryset_hints.py index f9ef8aeb..13c8e929 100644 --- a/pylint_django/tests/input/func_queryset_hints.py +++ b/pylint_django/tests/input/func_queryset_hints.py @@ -8,7 +8,7 @@ class Book(models.Model): - name = models.CharField(max_length=100) # [consider-using-bulk-create] + name = models.CharField(max_length=100) class Meta: app_label = "test_app" @@ -33,3 +33,9 @@ def assigned_for_create(): def for_filter(): for i in range(10): _ = Book.objects.filter(name=str(i)) # [consider-using-in-queries] + + +def for_save(): + obj = Book(name="Test Book") + for _ in range(10): + obj.save() # [consider-using-bulk-create-save] diff --git a/pylint_django/tests/input/func_queryset_hints.txt b/pylint_django/tests/input/func_queryset_hints.txt index 567ff1c9..a5a9064c 100644 --- a/pylint_django/tests/input/func_queryset_hints.txt +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -1,4 +1,5 @@ consider-using-bulk-create:19:8:19:40:for_create:Consider using 'Model.bulk_create()':UNDEFINED consider-using-bulk-create:25:12:25:44:for_nested_if_create:Consider using 'Model.bulk_create()':UNDEFINED -consider-using-bulk-create:30:12:30:49:assigned_for_create:Consider using 'Model.bulk_create()':UNDEFINED -consider-using-in-queries:35:12:35:49:for_filter:Consider using 'Model.bulk_*():UNDEFINED +consider-using-bulk-create:30:12:30:44:assigned_for_create:Consider using 'Model.bulk_create()':UNDEFINED +consider-using-in-queries:35:12:35:44:for_filter:Consider using '__in' queries:UNDEFINED +consider-using-bulk-create-save:41:8:41:18:for_save:Consider using 'Model.bulk_*():UNDEFINED From 92134ed4a8a8585d55109ffa08acb8470a261f2b Mon Sep 17 00:00:00 2001 From: Penaz Date: Fri, 9 Sep 2022 14:36:41 +0200 Subject: [PATCH 16/18] Added docstrings --- pylint_django/checkers/modelfilter_forloop.py | 9 ++++++--- pylint_django/checkers/modelsave_forloop.py | 15 ++++++++++----- pylint_django/checkers/queryset_iterator.py | 4 ++++ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/pylint_django/checkers/modelfilter_forloop.py b/pylint_django/checkers/modelfilter_forloop.py index 26875410..496c9142 100644 --- a/pylint_django/checkers/modelfilter_forloop.py +++ b/pylint_django/checkers/modelfilter_forloop.py @@ -6,6 +6,9 @@ class ModelFilterForLoopChecker(checkers.BaseChecker): + """ + Checks for usage of "Model.manager.filter() inside of for loops + """ __implements__ = (interfaces.IAstroidChecker) name = "model-filter-forloop-checker" @@ -14,9 +17,9 @@ class ModelFilterForLoopChecker(checkers.BaseChecker): f"R{BASE_ID}06": ( "Consider using '__in' queries", "consider-using-in-queries", - "Using 'Model.filter()' or 'Model.get() inside a for loop may " - "impact performance. Consider using a single query with as '__in' " - "filter instead, outside of the loop." + "Using 'Model.manager.filter()' or 'Model.manager.get() inside a " + "for loop may negatively impact performance. Consider using a " + "single query with as '__in' filter instead, outside of the loop." ), } diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 8aaaa3dc..4304eef8 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -7,6 +7,10 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): + """ + Checks for usage of Model.manager.create() or Model.save() inside of for + loops + """ __implements__ = (interfaces.IAstroidChecker) name = "model-save-forloop-checker" @@ -15,15 +19,16 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): f"R{BASE_ID}04": ( "Consider using 'Model.bulk_create()'", "consider-using-bulk-create", - "Using 'Model.create()' inside a for loop may impact performance. " - "Consider using 'Model.bulk_create()' instead." + "Using 'Model.manager.create()' inside a for loop may negatively " + "impact performance. Consider using 'Model.manager.bulk_create()' " + "instead." ), f"R{BASE_ID}05": ( "Consider using 'Model.bulk_*()", "consider-using-bulk-create-save", - "Using 'Model.save()' inside a for loop may impact performance. " - "Consider using 'Model.bulk_update()' or " - "'Model.bulk_create()' instead." + "Using 'Model.save()' inside a for loop may negatively impact " + "performance. Consider using 'Model.manager.bulk_update()' or " + "'Model.manager.bulk_create()' instead." ), } diff --git a/pylint_django/checkers/queryset_iterator.py b/pylint_django/checkers/queryset_iterator.py index cdc1d076..74d871e6 100644 --- a/pylint_django/checkers/queryset_iterator.py +++ b/pylint_django/checkers/queryset_iterator.py @@ -7,6 +7,10 @@ class QuerysetIteratorForLoopChecker(checkers.BaseChecker): + """ + Checks for usage of "QuerySet.all()" in the head of a for loop, + eventually suggesting the usage of ".iterator()" + """ __implements__ = (interfaces.IAstroidChecker) name = "queryset-iterator-forloop-checker" From f30bcb6265fb42022e0a91daf5f0437fc8a833e6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 9 Sep 2022 12:41:19 +0000 Subject: [PATCH 17/18] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- pylint_django/checkers/__init__.py | 6 ++---- pylint_django/checkers/modelfilter_forloop.py | 7 ++++--- pylint_django/checkers/modelsave_forloop.py | 10 ++++++---- pylint_django/checkers/queryset_iterator.py | 8 +++++--- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index bf6cd93f..118d6300 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -4,12 +4,10 @@ from pylint_django.checkers.foreign_key_strings import ForeignKeyStringsChecker from pylint_django.checkers.forms import FormChecker from pylint_django.checkers.json_response import JsonResponseChecker +from pylint_django.checkers.modelfilter_forloop import ModelFilterForLoopChecker from pylint_django.checkers.models import ModelChecker -from pylint_django.checkers.modelfilter_forloop import\ - ModelFilterForLoopChecker from pylint_django.checkers.modelsave_forloop import ModelSaveForLoopChecker -from pylint_django.checkers.queryset_iterator import \ - QuerysetIteratorForLoopChecker +from pylint_django.checkers.queryset_iterator import QuerysetIteratorForLoopChecker def register_checkers(linter): diff --git a/pylint_django/checkers/modelfilter_forloop.py b/pylint_django/checkers/modelfilter_forloop.py index 496c9142..e1d8c14d 100644 --- a/pylint_django/checkers/modelfilter_forloop.py +++ b/pylint_django/checkers/modelfilter_forloop.py @@ -1,4 +1,4 @@ -from astroid.nodes import Call, Expr, Attribute +from astroid.nodes import Attribute, Call, Expr from pylint import checkers, interfaces from pylint.checkers import utils @@ -9,7 +9,8 @@ class ModelFilterForLoopChecker(checkers.BaseChecker): """ Checks for usage of "Model.manager.filter() inside of for loops """ - __implements__ = (interfaces.IAstroidChecker) + + __implements__ = interfaces.IAstroidChecker name = "model-filter-forloop-checker" @@ -19,7 +20,7 @@ class ModelFilterForLoopChecker(checkers.BaseChecker): "consider-using-in-queries", "Using 'Model.manager.filter()' or 'Model.manager.get() inside a " "for loop may negatively impact performance. Consider using a " - "single query with as '__in' filter instead, outside of the loop." + "single query with as '__in' filter instead, outside of the loop.", ), } diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 4304eef8..3f1859e5 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -1,8 +1,9 @@ -from astroid.nodes import Call, Expr, Attribute +from astroid.nodes import Attribute, Call, Expr from pylint import checkers, interfaces from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID + # from pylint_django.augmentations import is_manager_attribute @@ -11,7 +12,8 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): Checks for usage of Model.manager.create() or Model.save() inside of for loops """ - __implements__ = (interfaces.IAstroidChecker) + + __implements__ = interfaces.IAstroidChecker name = "model-save-forloop-checker" @@ -21,14 +23,14 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): "consider-using-bulk-create", "Using 'Model.manager.create()' inside a for loop may negatively " "impact performance. Consider using 'Model.manager.bulk_create()' " - "instead." + "instead.", ), f"R{BASE_ID}05": ( "Consider using 'Model.bulk_*()", "consider-using-bulk-create-save", "Using 'Model.save()' inside a for loop may negatively impact " "performance. Consider using 'Model.manager.bulk_update()' or " - "'Model.manager.bulk_create()' instead." + "'Model.manager.bulk_create()' instead.", ), } diff --git a/pylint_django/checkers/queryset_iterator.py b/pylint_django/checkers/queryset_iterator.py index 74d871e6..1b438c25 100644 --- a/pylint_django/checkers/queryset_iterator.py +++ b/pylint_django/checkers/queryset_iterator.py @@ -1,8 +1,9 @@ -from astroid.nodes import Call, Attribute +from astroid.nodes import Attribute, Call from pylint import checkers, interfaces from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID + # from pylint_django.augmentations import is_manager_attribute @@ -11,7 +12,8 @@ class QuerysetIteratorForLoopChecker(checkers.BaseChecker): Checks for usage of "QuerySet.all()" in the head of a for loop, eventually suggesting the usage of ".iterator()" """ - __implements__ = (interfaces.IAstroidChecker) + + __implements__ = interfaces.IAstroidChecker name = "queryset-iterator-forloop-checker" @@ -21,7 +23,7 @@ class QuerysetIteratorForLoopChecker(checkers.BaseChecker): "consider-using-queryset-iterator", "Using 'QuerySet.all()' may load all results in memory " "if you need to iterate over the data only once and don't need " - "caching, 'QuerySet.iterator()' may be a more performant option." + "caching, 'QuerySet.iterator()' may be a more performant option.", ), } From 0e90161aadccd39922dccbaff5eab75b26dcf997 Mon Sep 17 00:00:00 2001 From: Penaz Date: Tue, 1 Apr 2025 22:46:26 +0200 Subject: [PATCH 18/18] Update to support Pylint 3 --- pylint_django/checkers/modelfilter_forloop.py | 6 ++---- pylint_django/checkers/modelsave_forloop.py | 8 +++----- pylint_django/checkers/queryset_iterator.py | 6 ++---- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/pylint_django/checkers/modelfilter_forloop.py b/pylint_django/checkers/modelfilter_forloop.py index e1d8c14d..3ac1a4f8 100644 --- a/pylint_django/checkers/modelfilter_forloop.py +++ b/pylint_django/checkers/modelfilter_forloop.py @@ -1,5 +1,5 @@ from astroid.nodes import Attribute, Call, Expr -from pylint import checkers, interfaces +from pylint import checkers from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID @@ -10,8 +10,6 @@ class ModelFilterForLoopChecker(checkers.BaseChecker): Checks for usage of "Model.manager.filter() inside of for loops """ - __implements__ = interfaces.IAstroidChecker - name = "model-filter-forloop-checker" msgs = { @@ -24,7 +22,7 @@ class ModelFilterForLoopChecker(checkers.BaseChecker): ), } - @utils.check_messages("consider-using-in-queries") + @utils.only_required_for_messages("consider-using-in-queries") def visit_for(self, node): """ Checks for a Queryset.filter() inside of a for loop diff --git a/pylint_django/checkers/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py index 3f1859e5..4113aa0a 100644 --- a/pylint_django/checkers/modelsave_forloop.py +++ b/pylint_django/checkers/modelsave_forloop.py @@ -1,5 +1,5 @@ from astroid.nodes import Attribute, Call, Expr -from pylint import checkers, interfaces +from pylint import checkers from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID @@ -13,8 +13,6 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): loops """ - __implements__ = interfaces.IAstroidChecker - name = "model-save-forloop-checker" msgs = { @@ -34,8 +32,8 @@ class ModelSaveForLoopChecker(checkers.BaseChecker): ), } - @utils.check_messages("consider-using-bulk-create") - @utils.check_messages("consider-using-bulk-create-save") + @utils.only_required_for_messages("consider-using-bulk-create") + @utils.only_required_for_messages("consider-using-bulk-create-save") def visit_for(self, node): """ Checks for a Model.create() inside of a for loop diff --git a/pylint_django/checkers/queryset_iterator.py b/pylint_django/checkers/queryset_iterator.py index 1b438c25..043420d9 100644 --- a/pylint_django/checkers/queryset_iterator.py +++ b/pylint_django/checkers/queryset_iterator.py @@ -1,5 +1,5 @@ from astroid.nodes import Attribute, Call -from pylint import checkers, interfaces +from pylint import checkers from pylint.checkers import utils from pylint_django.__pkginfo__ import BASE_ID @@ -13,8 +13,6 @@ class QuerysetIteratorForLoopChecker(checkers.BaseChecker): eventually suggesting the usage of ".iterator()" """ - __implements__ = interfaces.IAstroidChecker - name = "queryset-iterator-forloop-checker" msgs = { @@ -27,7 +25,7 @@ class QuerysetIteratorForLoopChecker(checkers.BaseChecker): ), } - @utils.check_messages("consider-using-queryset-iterator") + @utils.only_required_for_messages("consider-using-queryset-iterator") def visit_for(self, node): """ Checks for a QuerySet.all() inside of a for loop