diff --git a/pylint_django/checkers/__init__.py b/pylint_django/checkers/__init__.py index 1a14b746..118d6300 100644 --- a/pylint_django/checkers/__init__.py +++ b/pylint_django/checkers/__init__.py @@ -4,7 +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.modelsave_forloop import ModelSaveForLoopChecker +from pylint_django.checkers.queryset_iterator import QuerysetIteratorForLoopChecker def register_checkers(linter): @@ -15,3 +18,6 @@ def register_checkers(linter): linter.register_checker(FormChecker(linter)) linter.register_checker(AuthUserChecker(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/modelfilter_forloop.py b/pylint_django/checkers/modelfilter_forloop.py new file mode 100644 index 00000000..e1d8c14d --- /dev/null +++ b/pylint_django/checkers/modelfilter_forloop.py @@ -0,0 +1,48 @@ +from astroid.nodes import Attribute, Call, Expr +from pylint import checkers, interfaces +from pylint.checkers import utils + +from pylint_django.__pkginfo__ import BASE_ID + + +class ModelFilterForLoopChecker(checkers.BaseChecker): + """ + Checks for usage of "Model.manager.filter() inside of for loops + """ + + __implements__ = interfaces.IAstroidChecker + + name = "model-filter-forloop-checker" + + msgs = { + f"R{BASE_ID}06": ( + "Consider using '__in' queries", + "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.", + ), + } + + @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 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/modelsave_forloop.py b/pylint_django/checkers/modelsave_forloop.py new file mode 100644 index 00000000..3f1859e5 --- /dev/null +++ b/pylint_django/checkers/modelsave_forloop.py @@ -0,0 +1,60 @@ +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 + + +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" + + msgs = { + f"R{BASE_ID}04": ( + "Consider using 'Model.bulk_create()'", + "consider-using-bulk-create", + "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 negatively impact " + "performance. Consider using 'Model.manager.bulk_update()' or " + "'Model.manager.bulk_create()' instead.", + ), + } + + @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 + + 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_create_save(subchild) + if isinstance(child, Call): + self._check_forloop_create_save(child) + + def _check_forloop_create_save(self, 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) diff --git a/pylint_django/checkers/queryset_iterator.py b/pylint_django/checkers/queryset_iterator.py new file mode 100644 index 00000000..1b438c25 --- /dev/null +++ b/pylint_django/checkers/queryset_iterator.py @@ -0,0 +1,43 @@ +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 + + +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" + + 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): + # 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) 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..13c8e929 --- /dev/null +++ b/pylint_django/tests/input/func_queryset_hints.py @@ -0,0 +1,41 @@ +""" +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) + + class Meta: + app_label = "test_app" + + +def for_create(): + for i in range(10): + 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)) # [consider-using-bulk-create] + + +def assigned_for_create(): + for i in range(10): + _ = 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] + + +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 new file mode 100644 index 00000000..a5a9064c --- /dev/null +++ b/pylint_django/tests/input/func_queryset_hints.txt @@ -0,0 +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: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