Merge pull request #18 from routablehq/rou110-disallow-save-without-u…

…pdate-fields

[ROU110] - Add a new rule for disallowing .save() without update fields or comments justifying it

README.md

@@ -28,6 +28,7 @@ Here is a list of the rules supported by this Flake8 plugin:
 * `ROU107` - Inline function import is not at top of statement
 * `ROU108` - Import from model module instead of sub-packages
 * `ROU109` - Disallow rename migrations
+* `ROU110` - Disallow .save() with no update_fields
 * `ROU111` - Disallow FeatureFlag creation in code
 
 ## Testing

flake8_routable.py

@@ -34,6 +34,7 @@
 ROU107 = "ROU107 Inline function import is not at top of statement"
 ROU108 = "ROU108 Import from model module instead of sub-packages"
 ROU109 = "ROU109 Disallow rename migrations"
+ROU110 = "ROU110 Disallow .save() with no update_fields"
 ROU111 = "ROU111 Disallow FeatureFlag creation in code"
 
 
@@ -180,6 +181,7 @@ def visit(self, file_tokens: List[tokenize.TokenInfo]) -> None:
         self.lines_with_invalid_docstrings()
         self.lines_with_invalid_multi_line_strings()
         self.rename_migrations()
+        self.disallow_no_update_fields_save()
         self.disallow_feature_flag_creation()
 
     def lines_with_blank_lines_after_comments(self) -> None:
@@ -349,14 +351,51 @@ def rename_migrations(self) -> None:
                 reported.add(line_token.start[0])
                 self.errors.append((*line_token.start, ROU109))
 
+    def disallow_no_update_fields_save(self) -> None:
+        """.save() must be called with update_fields."""
+        reported = set()
+        single_line_save = re.compile(r".+(\.save\(.*)")
+        allowed_comments = [
+            "# TODO: needs fix",
+            "# file save",
+            "# form save",
+            "# ledger save",
+            "# multi-line with update_fields",
+            "# new model save",
+            "# not a model",
+            "# save extension",
+            "# serializer save",
+        ]
+
+        for line_token in self._file_tokens:
+            if line_token.start[0] in reported:
+                # There could be many tokens on a same line.
+                continue
+
+            line = line_token.line
+
+            if not single_line_save.match(line):
+                # Skip lines that don't match
+                continue
+
+            if "update_fields" in line:
+                # save, with update_fields is allowed
+                continue
+
+            if any(comment in line for comment in allowed_comments):
+                # Ignore lines with these comments, as they are valid
+                continue
+
+            reported.add(line_token.start[0])
+            self.errors.append((*line_token.start, ROU110))
+
     def disallow_feature_flag_creation(self) -> None:
         """We can not create FeatureFlags in code, they are cached on the request."""
         reported = set()
-        feature_flag_creation = re.compile(r".+(FeatureFlag\.objects\..*create)")
+        feature_flag_creation = re.compile(r"^.*?(FeatureFlag\.objects\..*create)")
         allowed_comments = [
             "# valid for legacy cross-border work",
             "# valid for management command",
-            "# valid for test usage",
         ]
 
         for line_token in self._file_tokens:

setup.cfg

@@ -1,6 +1,6 @@
 [metadata]
 name = flake8_routable
-version = 0.1.3
+version = 0.1.4
 
 [options]
 py_modules = flake8_routable

tests/test_flake8_routable.py

@@ -536,6 +536,128 @@ def test_incorrect_import_from_tests(self):
         assert errors == {"4:12: ROU109 Disallow rename migrations"}
 
 
+class TestROU110:
+    SAVE_WITH_UPDATE_FIELDS = """from app.models import Model
+instance = Model(id="123", name="test")
+instance.save(update_fields=["id", "name"])
+"""
+
+    SAVE_MULTILINE_WITH_UPDATE_FIELDS_FLAG = """from app.models import Model
+instance = Model(id="123", name="test")
+instance.save(  # multi-line with update_fields
+    update_fields=["id", "name"]
+)
+"""
+
+    SAVE_WITHOUT_UPDATE_FIELDS = """from app.models import Model
+instance = Model(id="123", name="test")
+instance.save()
+instance.save(using="default")
+"""
+
+    SAVE_WITH_COMMENT = """from app.models import Model
+instance = Model(id="123", name="test")
+instance.save()  {comment}
+"""
+
+    def test_save_with_update_fields(self):
+        errors = results(self.SAVE_WITH_UPDATE_FIELDS)
+        assert errors == set()
+
+    def test_save__multiline_with_update_fields_flag(self):
+        errors = results(self.SAVE_MULTILINE_WITH_UPDATE_FIELDS_FLAG)
+        assert errors == set()
+
+    def test_save_without_update_fields(self):
+        errors = results(self.SAVE_WITHOUT_UPDATE_FIELDS)
+        assert errors == {
+            "3:0: ROU110 Disallow .save() with no update_fields",
+            "4:0: ROU110 Disallow .save() with no update_fields",
+        }
+
+    @pytest.mark.parametrize(
+        "comment",
+        [
+            "# TODO: needs fix",
+            "# file save",
+            "# form save",
+            "# ledger save",
+            "# new model save",
+            "# not a model",
+            "# save extension",
+            "# serializer save",
+        ],
+    )
+    def test_with_comment(self, comment):
+        errors = results(self.SAVE_WITH_COMMENT.format(comment=comment))
+        assert errors == set()
+
+
+class TestROU111:
+    FEATURE_FLAG_CREATE = """from feature_config.models import FeatureFlag
+FeatureFlag.objects.create(company=company, feature_flag=flag)
+"""
+
+    FEATURE_FLAG_CREATE_MULTILINE = """from feature_config.models import FeatureFlag
+FeatureFlag.objects.create(
+    company=company, feature_flag=flag
+)
+"""
+
+    FEATURE_FLAG_GET_OR_CREATE = """from feature_config.models import FeatureFlag
+def method():
+    FeatureFlag.objects.get_or_create(company=company, feature_flag=flag)
+"""
+
+    FEATURE_FLAG_WITH_COMMENT = """from feature_config.models import FeatureFlag
+FeatureFlag.objects.create(  {comment}
+    company=company, feature_flag=flag
+)
+"""
+
+    def test_feature_flag_create(self):
+        errors = results(self.FEATURE_FLAG_CREATE)
+        assert errors == {
+            "2:0: ROU111 Disallow FeatureFlag creation in code",
+        }
+
+    def test_feature_flag_create_multiline(self):
+        errors = results(self.FEATURE_FLAG_CREATE_MULTILINE)
+        assert errors == {
+            "2:0: ROU111 Disallow FeatureFlag creation in code",
+        }
+
+    def test_feature_flag_get_or_create(self):
+        errors = results(self.FEATURE_FLAG_GET_OR_CREATE)
+        assert errors == {
+            "3:0: ROU111 Disallow FeatureFlag creation in code",
+        }
+
+    @pytest.mark.parametrize(
+        "comment",
+        [
+            "# valid for legacy cross-border work",
+            "# valid for management command",
+        ],
+    )
+    def test_with_comment(self, comment):
+        errors = results(self.FEATURE_FLAG_WITH_COMMENT.format(comment=comment))
+        assert errors == set()
+
+    @pytest.mark.parametrize(
+        "comment",
+        [
+            "# valid for something else",
+            " ",
+        ],
+    )
+    def test_with_invalid_comment(self, comment):
+        errors = results(self.FEATURE_FLAG_WITH_COMMENT.format(comment=comment))
+        assert errors == {
+            "2:0: ROU111 Disallow FeatureFlag creation in code",
+        }
+
+
 class TestVisitor:
     def test_parse_to_string_warning(self):
         visitor = Visitor()