From 566f6665d68c8c4a583daa718f7749694dab58e9 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Fri, 2 Aug 2024 17:30:34 +1200 Subject: [PATCH 01/12] don't log errors that we end up raising #22557 Added new rule for this issue to the pylint_guidelines_checker file. Added corresponding tests to the test_pylint_custom_plugins file --- .../pylint_guidelines_checker.py | 38 ++++++++++++ .../tests/test_pylint_custom_plugins.py | 59 ++++++++++++++++++- 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index f79af8f887a..9f3a50ada7c 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/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 + msg = {"C4762": ( + "Do not log errors that get rasied in an exception block.", + "do-not-log-errors-that-get-raised", + "Do not log errors that get rasied in an exception block. Do not log as error, warning or info", + ), + } + + def visit_try(self, node): + """Check that errors aren't logged 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", "info", "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): @@ -2785,6 +2820,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)) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index dfdca1775bc..30c9cd01e3b 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -4829,4 +4829,61 @@ 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) \ No newline at end of file + self.checker.visit_importfrom(imd) + + +class TestDoNotLogErrorsEndUpRaising(pylint.testutils.CheckerTestCase): + + """Test that any errors raised are not logged 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.""" + # works with other logging levels too (e.g. warning, info etc.) + + 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_logging_ok_when_no_raise(self): + """Check that any exceptions can be logged at error level if 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) From 395b6d473aad62b80d8575cbb9b87da4cb36b337 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Fri, 2 Aug 2024 21:09:35 +1200 Subject: [PATCH 02/12] test_pylint_custom_plugins - added more tests Added two more tests to check for multiple exception blocks --- .../tests/test_pylint_custom_plugins.py | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 30c9cd01e3b..a0e9628ecb3 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -4887,3 +4887,44 @@ def test_unlogged_exception_block(self): ) with self.assertNoMessages(): self.checker.visit_try(exception_node) + + def test_mult_exception_blocks_seperate_raise(self): + """Check multiple exception blocks with seperate 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) From 32182a6658daf4161a4077ba7151101fab83eee8 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Mon, 5 Aug 2024 18:45:59 +1200 Subject: [PATCH 03/12] added visit_functiondef() and visit_classdef() functions to checker class --- .../pylint_guidelines_checker.py | 10 ++++++++++ .../tests/test_pylint_custom_plugins.py | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 9f3a50ada7c..ecd7b62bc38 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2721,6 +2721,16 @@ class DoNotLogErrorsEndUpRaising(BaseChecker): ), } + def visit_functiondef(self, node): + for i in node.body: + if isinstance(i, astroid.Try): + self.visit_try(i) + + def visit_classdef(self, node): + for func in node.body: + if isinstance(func, astroid.FunctionDef): + self.visit_functiondef(node) + def visit_try(self, node): """Check that errors aren't logged in exception blocks. Go through each line in the exception block and make sure it hasn't been logged if exception is raised. diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index a0e9628ecb3..5816ad12e80 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -4840,7 +4840,7 @@ class TestDoNotLogErrorsEndUpRaising(pylint.testutils.CheckerTestCase): def test_error_level_not_logged(self): """Check that any exceptions raised aren't logged at error level in the exception block.""" - # works with other logging levels too (e.g. warning, info etc.) + # works with other logging levels too (e.g. warning and info) exception_node = astroid.extract_node(''' try: From b52af5a761d2f7cc2f4e51980af924ea44e92c70 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Wed, 7 Aug 2024 20:05:17 +1200 Subject: [PATCH 04/12] Changed 'msg' to 'msgs' and removed un-needed functions --- .../pylint_guidelines_checker.py | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index ecd7b62bc38..898c1a9634c 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2714,22 +2714,12 @@ class DoNotLogErrorsEndUpRaising(BaseChecker): name = "do-not-log-errors-that-get-raised" priority = -1 - msg = {"C4762": ( + msgs = {"C4762": ( "Do not log errors that get rasied in an exception block.", "do-not-log-errors-that-get-raised", "Do not log errors that get rasied in an exception block. Do not log as error, warning or info", - ), - } - - def visit_functiondef(self, node): - for i in node.body: - if isinstance(i, astroid.Try): - self.visit_try(i) - - def visit_classdef(self, node): - for func in node.body: - if isinstance(func, astroid.FunctionDef): - self.visit_functiondef(node) + ), + } def visit_try(self, node): """Check that errors aren't logged in exception blocks. @@ -2830,8 +2820,8 @@ 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 From b475915eedc2f98408b60555c99b8dbf088fe270 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Wed, 7 Aug 2024 20:58:54 +1200 Subject: [PATCH 05/12] Made matches more specific Changed 'warning' to '.warning' etc. Help avoid any false positives e.g. 'print("error")' will now not trigger Pylint --- .../pylint_guidelines_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 898c1a9634c..2b4669a3a30 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2725,7 +2725,7 @@ def visit_try(self, node): """Check that errors aren't logged 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", "info", "error"] + matches = [".warning", ".info", ".error"] # Get the exception block - returns a list except_block = node.handlers for nod in except_block: From dc9e9756e2ed0e4fb38793cf1010ce331008d8da Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Thu, 8 Aug 2024 21:06:24 +1200 Subject: [PATCH 06/12] Update README to include rule --- .../azure-pylint-guidelines-checker/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md index 7d8449008b9..a40a93a8f14 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md +++ b/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. | \ No newline at end of file From 9103a292b87d76c0beccb7d322e25b1948aef8bf Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Sat, 10 Aug 2024 14:25:17 +1200 Subject: [PATCH 07/12] Update checker to only include 'warning' and 'error' logging levels Also added another test to test_pylint_custom_plugins.py for 'warning' level. --- .../pylint_guidelines_checker.py | 8 ++--- .../tests/test_pylint_custom_plugins.py | 33 +++++++++++++++---- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 2b4669a3a30..4c2f5ea7073 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2715,17 +2715,17 @@ class DoNotLogErrorsEndUpRaising(BaseChecker): name = "do-not-log-errors-that-get-raised" priority = -1 msgs = {"C4762": ( - "Do not log errors that get rasied in an exception block.", + "Do not log errors that get raised in an exception block.", "do-not-log-errors-that-get-raised", - "Do not log errors that get rasied in an exception block. Do not log as error, warning or info", + "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 errors aren't logged in exception blocks. + """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", ".info", ".error"] + matches = [".warning", ".error"] # Get the exception block - returns a list except_block = node.handlers for nod in except_block: diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 5816ad12e80..01df1d24a97 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -4834,14 +4834,12 @@ def test_allowed_import_else(self): class TestDoNotLogErrorsEndUpRaising(pylint.testutils.CheckerTestCase): - """Test that any errors raised are not logged in the exception block.""" + """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.""" - # works with other logging levels too (e.g. warning and info) - exception_node = astroid.extract_node(''' try: add = 1 + 2 @@ -4862,8 +4860,30 @@ def test_error_level_not_logged(self): ): 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 any exceptions can be logged at error level if exception isn't raised.""" + """Check that exceptions can be logged if the exception isn't raised.""" exception_node = astroid.extract_node(''' try: @@ -4888,8 +4908,8 @@ def test_unlogged_exception_block(self): with self.assertNoMessages(): self.checker.visit_try(exception_node) - def test_mult_exception_blocks_seperate_raise(self): - """Check multiple exception blocks with seperate raise and logging is allowed.""" + 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: @@ -4903,7 +4923,6 @@ def test_mult_exception_blocks_seperate_raise(self): 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.""" From 5f6be69004f7196c1fb9f284b29980315964a228 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Mon, 19 Aug 2024 15:21:54 +1200 Subject: [PATCH 08/12] Update to account for branches within exception blocks Also rename checker to "do-not-log-raised-errors" --- .../azure-pylint-guidelines-checker/README.md | 4 +- .../pylint_guidelines_checker.py | 65 +++-- .../tests/test_pylint_custom_plugins.py | 235 +++++++++++++++--- 3 files changed, 246 insertions(+), 58 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md index a40a93a8f14..6304b3ba651 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md +++ b/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,4 +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. | \ No newline at end of file +| do-not-log-raised-errors | Do not log errors at `error` or `warning` level when error is raised in an exception block. | pylint:disable=do-not-log-raised-errors | No Link. | \ No newline at end of file diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 4c2f5ea7073..08011784844 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2712,36 +2712,63 @@ class DoNotLogErrorsEndUpRaising(BaseChecker): """Rule to check that errors that get raised aren't logged""" - name = "do-not-log-errors-that-get-raised" + name = "do-not-log-raised-errors" 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", + "do-not-log-raised-errors", + "Do not log errors at error or warning level when error is raised in an exception block", ), } 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. + Go through exception block and branches and ensure error hasn't been logged if exception is raised. """ - matches = [".warning", ".error"] - # Get the exception block - returns a list + # Return a list of exception blocks except_block = node.handlers + # Iterate through each exception block 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, - ) + # Get the nodes in each block (e.g. nodes Expr and Raise) + exception_block_body = nod.body + self.check_for_raise(exception_block_body) + + def check_for_raise(self, node): + """ Helper function - checks for instance of 'Raise' node + Also checks 'If' and nested 'If' branches + """ + for i in node: + if isinstance(i, astroid.Raise): + self.check_for_logging(node) + # Check for any nested 'If' branches + if isinstance(i, astroid.If): + self.check_for_raise(i.body) + # Check any 'elif' and 'else' branches + elif_statements = i.orelse + while len(elif_statements) == 1: + if isinstance(elif_statements[0], astroid.If): + for x in elif_statements: + self.check_for_raise(x.body) + elif_statements = x.orelse + else: + break + # Check 'else' body for raise + self.check_for_raise(elif_statements) + + def check_for_logging(self, node): + """ Helper function - checks 'Expr' nodes to see if logging has occurred at 'warning' or 'error' + levels. Called from 'check_for_raise' function + """ + matches = [".warning", ".error"] + for j in node: + if isinstance(j, astroid.Expr): + expression = j.as_string() + if any(x in expression for x in matches): + self.add_message( + msgid=f"do-not-log-raised-errors", + node=j, + confidence=None, + ) class NoImportTypingFromTypeCheck(BaseChecker): diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 01df1d24a97..e2276ae13a9 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -4840,52 +4840,52 @@ class TestDoNotLogErrorsEndUpRaising(pylint.testutils.CheckerTestCase): 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: + try_node, expression_node = astroid.extract_node(''' + try: #@ add = 1 + 2 except Exception as e: - logger.error(str(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, + msg_id="do-not-log-raised-errors", + line=5, + node=expression_node.parent, + col_offset=4, + end_line=5, + end_col_offset=24, ) ): - self.checker.visit_try(exception_node) + self.checker.visit_try(try_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: + try_node, expression_node = astroid.extract_node(''' + try: #@ add = 1 + 2 except Exception as e: - logger.warning(str(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, + msg_id="do-not-log-raised-errors", + line=5, + node=expression_node.parent, + col_offset=4, + end_line=5, + end_col_offset=26, ) ): - self.checker.visit_try(exception_node) + self.checker.visit_try(try_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_node = astroid.extract_node(''' try: add = 1 + 2 except Exception as e: @@ -4893,12 +4893,12 @@ def test_warning_level_logging_ok_when_no_raise(self): ''' ) with self.assertNoMessages(): - self.checker.visit_try(exception_node) + self.checker.visit_try(try_node) def test_unlogged_exception_block(self): """Check that exceptions raised without logging are allowed.""" - exception_node = astroid.extract_node(''' + try_node = astroid.extract_node(''' try: add = 1 + 2 except Exception as e: @@ -4906,12 +4906,12 @@ def test_unlogged_exception_block(self): ''' ) with self.assertNoMessages(): - self.checker.visit_try(exception_node) + self.checker.visit_try(try_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_node = astroid.extract_node(''' try: add = 1 + 2 except Exception as e: @@ -4921,29 +4921,190 @@ def test_mult_exception_blocks_separate_raise(self): ''' ) with self.assertNoMessages(): - self.checker.visit_try(exception_node) + self.checker.visit_try(try_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: + try_node, expression_node = astroid.extract_node(''' + try: #@ add = 1 + 2 except Exception as e: raise except OtherException as x: - logger.error(str(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, + msg_id="do-not-log-raised-errors", + line=7, + node=expression_node.parent, + col_offset=4, + end_line=7, + end_col_offset=24, + ) + ): + self.checker.visit_try(try_node) + + def test_implicit_else_exception_logged(self): + """Check that any exceptions raised in branches aren't logged at error level.""" + try_node, expression_node = astroid.extract_node( + ''' + try: #@ + add = 1 + 2 + except Exception as e: + if e.code == "Retryable": + logging.warning(f"Retryable failure occurred: {e}, attempting to restart") + return True + elif Exception != BaseException: + logging.error(f"System shutting down due to error: {e}.") + return False + logging.error(f"Unexpected error occurred: {e}") #@ + raise SystemError("Uh oh!") from e + ''') + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=11, + node=expression_node.parent, + col_offset=4, + end_line=11, + end_col_offset=52, + ) + ): + self.checker.visit_try(try_node) + + def test_branch_exceptions_logged(self): + """Check that any exceptions raised in if branches aren't logged at error level.""" + try_node, expression_node_a, expression_node_b, expression_node_c = astroid.extract_node( + ''' + try: #@ + add = 1 + 2 + except Exception as e: + if e.code == "Retryable": + logging.warning(f"Retryable failure occurred: {e}, attempting to restart") #@ + raise SystemError("Uh oh!") from e + elif Exception != BaseException: + logging.error(f"System shutting down due to error: {e}.") #@ + raise SystemError("Uh oh!") from e + elif e.code == "Second elif branch": + logging.error(f"Second: {e}.") #@ + raise SystemError("Uh oh!") from e + logging.error(f"Unexpected error occurred: {e}") + ''') + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=6, + node=expression_node_a.parent, + col_offset=8, + end_line=6, + end_col_offset=82, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=9, + node=expression_node_b.parent, + col_offset=8, + end_line=9, + end_col_offset=65, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=12, + node=expression_node_c.parent, + col_offset=8, + end_line=12, + end_col_offset=38, + ) + ): + self.checker.visit_try(try_node) + + def test_explicit_else_branch_exception_logged(self): + """Check that any exceptions raised in else branches aren't logged at error level.""" + try_node, expression_node = astroid.extract_node( + ''' + try: #@ + add = 1 + 2 + except Exception as e: + if e.code == "Retryable": + logging.warning(f"Retryable failure occurred: {e}, attempting to restart") + return True + elif Exception != BaseException: + logging.error(f"System shutting down due to error: {e}.") + return False + else: + logging.error(f"Unexpected error occurred: {e}") #@ + raise SystemError("Uh oh!") from e + + ''') + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=12, + node=expression_node.parent, + col_offset=8, + end_line=12, + end_col_offset=56, + ) + ): + self.checker.visit_try(try_node) + + def test_extra_nested_branches_exception_logged(self): + """Check that any exceptions raised in else branches aren't logged at error level.""" + try_node, expression_node_a, expression_node_b, expression_node_c, expression_node_d = astroid.extract_node( + ''' + try: #@ + add = 1 + 2 + except Exception as e: + if e.code == "Retryable": + if e.code == "A": + logging.warning(f"A: {e}") #@ + raise SystemError("Uh oh!") from e + elif e.code == "E": + logging.warning(f"E: {e}") #@ + raise SystemError("Uh oh!") from e + else: + logging.warning(f"F: {e}") #@ + raise SystemError("Uh oh!") from e + else: + logging.error(f"Unexpected error occurred: {e}") #@ + raise SystemError("Uh oh!") from e + ''') + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=7, + node=expression_node_a.parent, + col_offset=12, + end_line=7, + end_col_offset=38, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=10, + node=expression_node_b.parent, + col_offset=12, + end_line=10, + end_col_offset=38, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=13, + node=expression_node_c.parent, + col_offset=12, + end_line=13, + end_col_offset=38, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=16, + node=expression_node_d.parent, + col_offset=8, + end_line=16, + end_col_offset=56, ) ): - self.checker.visit_try(exception_node) + self.checker.visit_try(try_node) From 666fb027c6fe55e7bc94995d2a3a65d96a7b47d7 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Tue, 20 Aug 2024 17:37:36 +1200 Subject: [PATCH 09/12] Make logging levels case insensitive Added '.lower()' to string being parsed for logging. --- .../pylint_guidelines_checker.py | 2 +- .../tests/test_pylint_custom_plugins.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 08011784844..f5441fc8f5f 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2762,7 +2762,7 @@ def check_for_logging(self, node): matches = [".warning", ".error"] for j in node: if isinstance(j, astroid.Expr): - expression = j.as_string() + expression = j.as_string().lower() if any(x in expression for x in matches): self.add_message( msgid=f"do-not-log-raised-errors", diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index e2276ae13a9..8281b25fbbb 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -4844,7 +4844,7 @@ def test_error_level_not_logged(self): try: #@ add = 1 + 2 except Exception as e: - logger.error(str(e)) #@ + logger.ERROR(str(e)) #@ raise ''' ) From 3d03db1f66bf26e757e32040be0e944a40310882 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Mon, 2 Sep 2024 12:41:09 +1200 Subject: [PATCH 10/12] Update tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py Co-authored-by: Anna Tisch --- .../pylint_guidelines_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index f5441fc8f5f..44cd8660437 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2759,7 +2759,7 @@ def check_for_logging(self, node): """ Helper function - checks 'Expr' nodes to see if logging has occurred at 'warning' or 'error' levels. Called from 'check_for_raise' function """ - matches = [".warning", ".error"] + matches = [".warning", ".error", ".exception"] for j in node: if isinstance(j, astroid.Expr): expression = j.as_string().lower() From 2ca6b859c86061b0759fae311a871009961fb494 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Mon, 2 Sep 2024 13:45:16 +1200 Subject: [PATCH 11/12] Added checking for raises in 'for' branches Also added relevant tests --- .../pylint_guidelines_checker.py | 16 +++--- .../tests/test_pylint_custom_plugins.py | 52 +++++++++++++++++++ 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 44cd8660437..35870d69bdb 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2741,19 +2741,19 @@ def check_for_raise(self, node): if isinstance(i, astroid.Raise): self.check_for_logging(node) # Check for any nested 'If' branches - if isinstance(i, astroid.If): + if isinstance(i, astroid.If) or isinstance(i, astroid.For): self.check_for_raise(i.body) - # Check any 'elif' and 'else' branches - elif_statements = i.orelse - while len(elif_statements) == 1: - if isinstance(elif_statements[0], astroid.If): - for x in elif_statements: + # Check any 'elif', 'else' or 'for' branches + orelse_statements = i.orelse + while len(orelse_statements) == 1: + if isinstance(orelse_statements[0], astroid.If): + for x in orelse_statements: self.check_for_raise(x.body) - elif_statements = x.orelse + orelse_statements = x.orelse else: break # Check 'else' body for raise - self.check_for_raise(elif_statements) + self.check_for_raise(orelse_statements) def check_for_logging(self, node): """ Helper function - checks 'Expr' nodes to see if logging has occurred at 'warning' or 'error' diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 8281b25fbbb..54922591d74 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -5108,3 +5108,55 @@ def test_extra_nested_branches_exception_logged(self): ) ): self.checker.visit_try(try_node) + + def test_for_and_nested_for_branches_exception_logged(self): + try_node, expression_node_a, expression_node_b = astroid.extract_node( + ''' + try: #@ + add = 1 + 2 + except Exception as e: + y = 3 + for x in y: + logging.error(f"F: {e}") #@ + raise SystemError("Uh oh!") from e + if e.code == "Retryable": + for z in y: + logging.error(f"F: {e}") #@ + raise SystemError("Uh oh!") from e + ''') + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=7, + node=expression_node_a.parent, + col_offset=8, + end_line=7, + end_col_offset=32, + ), + pylint.testutils.MessageTest( + msg_id="do-not-log-raised-errors", + line=11, + node=expression_node_b.parent, + col_offset=12, + end_line=11, + end_col_offset=36, + ) + ): + self.checker.visit_try(try_node) + + def test_for_and_nested_for_branches_exception_not_logged(self): + try_node, expression_node_a, expression_node_b = astroid.extract_node( + ''' + try: #@ + add = 1 + 2 + except Exception as e: + y = 3 + for x in y: + logging.error(f"F: {e}") #@ + if e.code == "Retryable": + for z in y: + logging.error(f"F: {e}") #@ + ''') + with self.assertNoMessages(): + self.checker.visit_try(try_node) + From 5ce1cf76ba2f65a32786c7825d38fcd7645ebef6 Mon Sep 17 00:00:00 2001 From: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com> Date: Tue, 3 Sep 2024 20:15:14 +1200 Subject: [PATCH 12/12] Altered 'check_for_raise' helper function Refactored function and removed 'isInstance Astroid.For' and associated tests --- .../pylint_guidelines_checker.py | 16 ++---- .../tests/test_pylint_custom_plugins.py | 51 ------------------- 2 files changed, 4 insertions(+), 63 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 35870d69bdb..c33171ebbaa 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2741,19 +2741,11 @@ def check_for_raise(self, node): if isinstance(i, astroid.Raise): self.check_for_logging(node) # Check for any nested 'If' branches - if isinstance(i, astroid.If) or isinstance(i, astroid.For): + if isinstance(i, astroid.If): self.check_for_raise(i.body) - # Check any 'elif', 'else' or 'for' branches - orelse_statements = i.orelse - while len(orelse_statements) == 1: - if isinstance(orelse_statements[0], astroid.If): - for x in orelse_statements: - self.check_for_raise(x.body) - orelse_statements = x.orelse - else: - break - # Check 'else' body for raise - self.check_for_raise(orelse_statements) + + # Check any 'elif' or 'else' branches + self.check_for_raise(i.orelse) def check_for_logging(self, node): """ Helper function - checks 'Expr' nodes to see if logging has occurred at 'warning' or 'error' diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 54922591d74..9bcafcaf3a5 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -5109,54 +5109,3 @@ def test_extra_nested_branches_exception_logged(self): ): self.checker.visit_try(try_node) - def test_for_and_nested_for_branches_exception_logged(self): - try_node, expression_node_a, expression_node_b = astroid.extract_node( - ''' - try: #@ - add = 1 + 2 - except Exception as e: - y = 3 - for x in y: - logging.error(f"F: {e}") #@ - raise SystemError("Uh oh!") from e - if e.code == "Retryable": - for z in y: - logging.error(f"F: {e}") #@ - raise SystemError("Uh oh!") from e - ''') - with self.assertAddsMessages( - pylint.testutils.MessageTest( - msg_id="do-not-log-raised-errors", - line=7, - node=expression_node_a.parent, - col_offset=8, - end_line=7, - end_col_offset=32, - ), - pylint.testutils.MessageTest( - msg_id="do-not-log-raised-errors", - line=11, - node=expression_node_b.parent, - col_offset=12, - end_line=11, - end_col_offset=36, - ) - ): - self.checker.visit_try(try_node) - - def test_for_and_nested_for_branches_exception_not_logged(self): - try_node, expression_node_a, expression_node_b = astroid.extract_node( - ''' - try: #@ - add = 1 + 2 - except Exception as e: - y = 3 - for x in y: - logging.error(f"F: {e}") #@ - if e.code == "Retryable": - for z in y: - logging.error(f"F: {e}") #@ - ''') - with self.assertNoMessages(): - self.checker.visit_try(try_node) -