Don't log errors that we end up raising #22557

tools/pylint-extensions/azure-pylint-guidelines-checker/README.md

@@ -53,7 +53,7 @@ In the case of a false positive, use the disable command to remove the pylint er
 ## Rules List
 
 | Pylint checker name                                | How to fix this                                                                                                                                                      | How to disable this rule                                            | Link to python guideline                                                                                |
-| -------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- |
+| -------------------------------------------------- |----------------------------------------------------------------------------------------------------------------------------------------------------------------------| ------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- |
 | client-method-should-not-use-static-method         | Use module level functions instead.                                                                                                                                  | # pylint:disable=client-method-should-not-use-static-method         | [link](https://azure.github.io/azure-sdk/python_implementation.html#method-signatures)                  |
 | missing-client-constructor-parameter-credential    | Add a credential parameter to the client constructor. Do not use plural form "credentials".                                                                          | # pylint:disable=missing-client-constructor-parameter-credential    | [link](https://azure.github.io/azure-sdk/python_design.html#client-configuration)                       |
 | missing-client-constructor-parameter-kwargs        | Add a \*\*kwargs parameter to the client constructor.                                                                                                                | # pylint:disable=missing-client-constructor-parameter-kwargs        | [link](https://azure.github.io/azure-sdk/python_design.html#client-configuration)                       |
@@ -92,3 +92,4 @@ In the case of a false positive, use the disable command to remove the pylint er
 | docstring-keyword-should-match-keyword-only        | Docstring keyword arguments and keyword-only method arguments should match.                                                                                          | pylint:disable=docstring-keyword-should-match-keyword-only          | [link](https://azure.github.io/azure-sdk/python_documentation.html#docstrings)                          |
 | docstring-type-do-not-use-class                    | Docstring type is formatted incorrectly. Do not use `:class` in docstring type.                                                                                      | pylint:disable=docstring-type-do-not-use-class                      | [link](https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html)                            |
 | no-typing-import-in-type-check                     | Do not import typing under TYPE_CHECKING.                                                                                                                            | pylint:disable=no-typing-import-in-type-check                       | No Link.                                                                                                |
+| do-not-log-errors-that-get-raised                  | Do not log errors that get rasied in an exception block.                                                                                                             | pylint:disable=do-not-log-errors-that-get-raised                    | No Link.                                                                                                |

tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py

@@ -2708,6 +2708,41 @@ def visit_importfrom(self, node):
                     )
 
 
+class DoNotLogErrorsEndUpRaising(BaseChecker):
+
+    """Rule to check that errors that get raised aren't logged"""
+
+    name = "do-not-log-errors-that-get-raised"
+    priority = -1
+    msgs = {"C4762": (
+            "Do not log errors that get raised in an exception block.",
+            "do-not-log-errors-that-get-raised",
+            "Do not log errors that get raised in an exception block. Do not log at error or warning levels",
+            ),
+            }
+
+    def visit_try(self, node):
+        """Check that raised errors aren't logged at 'error' or 'warning' levels in exception blocks.
+           Go through each line in the exception block and make sure it hasn't been logged if exception is raised.
+        """
+        matches = [".warning", ".error"]
+        # Get the exception block - returns a list
+        except_block = node.handlers
+        for nod in except_block:
+            # Split into list of strings
+            split = nod.as_string().splitlines()
+            for i in split:
+                # Search to check if 'raise' is in the exception block
+                raise_found = i.find("raise")
+                if raise_found != -1:
+                    for j in split:
+                        if any(x in j for x in matches):
+                            self.add_message(
+                            msgid=f"do-not-log-errors-that-get-raised",
+                            node=node,
+                            confidence=None,
+                            )
+
 
 class NoImportTypingFromTypeCheck(BaseChecker):
 
@@ -2786,6 +2821,9 @@ def register(linter):
     # disabled by default, use pylint --enable=check-docstrings if you want to use it
     linter.register_checker(CheckDocstringParameters(linter))
 
+
+    linter.register_checker(DoNotLogErrorsEndUpRaising(linter))
+
     # Rules are disabled until false positive rate improved
     # linter.register_checker(CheckForPolicyUse(linter))
     # linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))

tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py

@@ -4829,4 +4829,121 @@ def test_allowed_import_else(self):
             self.checker.visit_importfrom(ima)
             self.checker.visit_import(imb)
             self.checker.visit_import(imc)
-            self.checker.visit_importfrom(imd)
+            self.checker.visit_importfrom(imd)
+
+
+class TestDoNotLogErrorsEndUpRaising(pylint.testutils.CheckerTestCase):
+
+    """Test that any errors raised are not logged at 'error' or 'warning' levels in the exception block."""
+
+    CHECKER_CLASS = checker.DoNotLogErrorsEndUpRaising
+
+    def test_error_level_not_logged(self):
+        """Check that any exceptions raised aren't logged at error level in the exception block."""
+        exception_node = astroid.extract_node('''
+        try:
+            add = 1 + 2
+        except Exception as e:
+            logger.error(str(e))
+            raise
+        '''
+                                              )
+        with self.assertAddsMessages(
+            pylint.testutils.MessageTest(
+                msg_id="do-not-log-errors-that-get-raised",
+                line=2,
+                node=exception_node,
+                col_offset=0,
+                end_line=6,
+                end_col_offset=9,
+            )
+        ):
+            self.checker.visit_try(exception_node)
+
+    def test_warning_level_not_logged(self):
+        """Check that any exceptions raised aren't logged at warning level in the exception block."""
+        exception_node = astroid.extract_node('''
+        try:
+            add = 1 + 2
+        except Exception as e:
+            logger.warning(str(e))
+            raise
+        '''
+                                              )
+        with self.assertAddsMessages(
+            pylint.testutils.MessageTest(
+                msg_id="do-not-log-errors-that-get-raised",
+                line=2,
+                node=exception_node,
+                col_offset=0,
+                end_line=6,
+                end_col_offset=9,
+            )
+        ):
+            self.checker.visit_try(exception_node)
+
+    def test_warning_level_logging_ok_when_no_raise(self):
+        """Check that exceptions can be logged if the exception isn't raised."""
+
+        exception_node = astroid.extract_node('''
+        try:
+            add = 1 + 2
+        except Exception as e:
+            logger.warning(str(e))
+        '''
+                                              )
+        with self.assertNoMessages():
+            self.checker.visit_try(exception_node)
+
+    def test_unlogged_exception_block(self):
+        """Check that exceptions raised without logging are allowed."""
+
+        exception_node = astroid.extract_node('''
+        try:
+            add = 1 + 2
+        except Exception as e:
+            raise
+        '''
+                                              )
+        with self.assertNoMessages():
+            self.checker.visit_try(exception_node)
+
+    def test_mult_exception_blocks_separate_raise(self):
+        """Check multiple exception blocks with separate raise and logging is allowed."""
+
+        exception_node = astroid.extract_node('''
+        try:
+            add = 1 + 2
+        except Exception as e:
+            raise
+        except OtherException as x:
+            logger.error(str(x))
+        '''
+                                              )
+        with self.assertNoMessages():
+            self.checker.visit_try(exception_node)
+
+    def test_mult_exception_blocks_with_raise(self):
+        """Check that multiple exception blocks with raise and logging is not allowed."""
+
+        exception_node = astroid.extract_node('''
+        try:
+            add = 1 + 2
+        except Exception as e:
+            raise
+        except OtherException as x:
+            logger.error(str(x))
+            raise
+        '''
+                                              )
+        with self.assertAddsMessages(
+                pylint.testutils.MessageTest(
+                    msg_id="do-not-log-errors-that-get-raised",
+                    line=2,
+                    node=exception_node,
+                    col_offset=0,
+                    end_line=8,
+                    end_col_offset=9,
+                )
+        ):
+            self.checker.visit_try(exception_node)