Skip to content

Commit

Permalink
Pylint Exception logging (#9077)
Browse files Browse the repository at this point in the history 
* add placeholders

* add new placeholder comments

* exploring AST unfinished - minor emergency had to leave

* identifying some mismatched functions

* refactor checker and tests

* fix error with non-builtins decorators

* fine tuning and testing required

* add pylint report

* add ranked listing of reports

* format report as table

* add new verbs

* update report

* update reportcounts.md

* fix formatting for reportcounts.md

* update reportcounts.md

* minimal tests added

* Base code and unit tests

More testing still to come

* Refactored class to be more specific

Also added more test cases

* Added to README

Also added one test and link for python implementation

* Update README / fix merging

* Final Refactor

* Fixed false positives

Edits to fix false positives from testing against SDKs. Added more unit tests

* not running multiple times picking up on different function types

* add TODOs

* removed code not reached

* Checks at a class level

* Looking into different connection_verify assignments

* Placeholders added back

* Checker base done

* exclue private namespaces and classes

* update reports

* Checker, Tests, & Readme done

* Update pylint_guidelines_checker.py

Fix another false positive from the SDKs

* Fix another false positive

Added corresponding test

* Check either sides of indices

Fixed another false positive

* add new prefixes

* update unit tests

* remove reports

* remove commented code

* add checker to README

* Tidy Up

* Add TODO comment re other cases to investigate

* Revert "Merge branch 'working-main' into exception-logging"

This reverts commit e2dcbb9, reversing
changes made to 7e7256b.

* Make checker more explicit

Switch "." and "name" to ".__name__"

---------

Co-authored-by: Joshua Bishop <[email protected]>
Co-authored-by: Alirose Burrell <[email protected]>
Co-authored-by: 16234397 <[email protected]>
  • Loading branch information
@JessicaBell00 @MJoshuaB @16234397
4 people authored Oct 10, 2024
1 parent 4807474 commit 88c86a6
Show file tree
Hide file tree
Showing 4 changed files with 363 additions and 8 deletions.
5 changes: 3 additions & 2 deletions tools/pylint-extensions/azure-pylint-guidelines-checker/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ In the case of a false positive, use the disable command to remove the pylint er


| 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) |
Expand Down Expand Up @@ -95,4 +95,5 @@ In the case of a false positive, use the disable command to remove the pylint er
| 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-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. |
| do-not-use-legacy-typing | Do not use legacy (&lt;Python 3.8) type hinting comments | pylint:disable=do-not-use-legacy-typing | No Link.
| do-not-import-asyncio | Do not import asyncio directly. | pylint:disable=do-not-import-asyncio | No Link. |
| do-not-import-asyncio | Do not import asyncio directly. | pylint:disable=do-not-import-asyncio | No Link. |
| do-not-log-exceptions | Do not log exceptions in levels other than debug, otherwise it can reveal sensitive information | pylint:disable=do-not-log-exceptions | [link](https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info) |
80 changes: 75 additions & 5 deletions tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2614,7 +2614,7 @@ def visit_functiondef(self, node):
try:
if node.returns.as_string() == "None":
# If there are residual comment typehints or no return value,
# we dont want to throw an error
# we don't want to throw an error
return
if node.name.startswith("delete") and node.parent.name.endswith("Client"):
if node.returns.as_string() != "None":
Expand Down Expand Up @@ -2865,10 +2865,81 @@ def visit_import(self, node):


# [Pylint] custom linter check for invalid use of @overload #3229
# [Pylint] Custom Linter check for Exception Logging #3227


class DoNotLogExceptions(BaseChecker):

"""Rule to check that exceptions aren't logged"""

name = "do-not-log-exceptions"
priority = -1
msgs = {"C4766": (
"Do not log exceptions. See Details:"
" https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info",
"do-not-log-exceptions",
"Do not log exceptions in levels other than debug, it can otherwise reveal sensitive information",
),
}

def visit_try(self, node):
"""Check that exceptions aren't logged in exception blocks.
Go through exception block and branches and ensure error hasn't been logged.
"""
# Return a list of exception blocks
except_block = node.handlers
# Iterate through each exception block
for nod in except_block:
# Get exception blocks with an exception name
if nod.name is not None:
exception_name = nod.name.name
self.check_for_logging(nod.body, exception_name)

def check_for_logging(self, node, exception_name):
""" Helper function - checks nodes to see if logging has occurred at all
levels.
"""
levels_matches = [".warning", ".error", ".info", ".debug"]
for j in node:
if isinstance(j, astroid.Expr):
expression = j.as_string().lower()
if any(x in expression for x in levels_matches) and "logger" in expression:
# Check for variables after strings
end_finder = expression.rfind("'")
delimiters = ["(", "{", "}", ")", "\"", ",", "'"]
if end_finder != -1:
expression_a = expression[end_finder + 1:]
# If there are variables after a string
if len(expression_a) > 1:
expression = expression_a
for delimiter in delimiters:
expression = " ".join(expression.split(delimiter))
expression1 = expression.split()
# Check for presence of exception name
for i in range(len(expression1)):
if exception_name == expression1[i]:
if i+1 < len(expression1):
# TODO: Investigate whether there are any other cases we don't want to raise a Pylint
# error
if ".__name__" not in expression1[i+1]:
self.add_message(
msgid=f"do-not-log-exceptions",
node=j,
confidence=None,
)
else:
self.add_message(
msgid=f"do-not-log-exceptions",
node=j,
confidence=None,
)
if isinstance(j, astroid.If):
self.check_for_logging(j.body, exception_name)
# Check any 'elif' or 'else' branches
self.check_for_logging(j.orelse, exception_name)


# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
# [Pylint] Add a check for connection_verify hardcoded settings #35355
# [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233
# [Pylint] Investigate pylint rule around missing dependency #3231


Expand Down Expand Up @@ -2907,12 +2978,11 @@ def register(linter):
linter.register_checker(NoImportTypingFromTypeCheck(linter))
linter.register_checker(DoNotUseLegacyTyping(linter))
linter.register_checker(DoNotLogErrorsEndUpRaising(linter))
linter.register_checker(DoNotLogExceptions(linter))

# [Pylint] custom linter check for invalid use of @overload #3229
# [Pylint] Custom Linter check for Exception Logging #3227
# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
# [Pylint] Add a check for connection_verify hardcoded settings #35355
# [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233
# [Pylint] Investigate pylint rule around missing dependency #3231

# disabled by default, use pylint --enable=check-docstrings if you want to use it
Expand Down
96 changes: 96 additions & 0 deletions ...lint-extensions/azure-pylint-guidelines-checker/tests/test_files/do_not_log_exceptions.py
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
from plugin import logger


# test_logging_levels_logged_str_exception
def test_logging_levels_logged_str_exception():
try: # @
add = 1 + 2
except Exception as ex:
logger.error("Error" + str(ex)) # @
logger.warning(str(ex))
logger.info(str(ex))
logger.debug(str(ex))


# test_logging_levels_logged_repr_exception
def test_logging_levels_logged_repr_exception():
try: # @
add = 1 + 2
except Exception as ex:
logger.error(repr(ex)) # @
logger.warning(repr(ex))
logger.info(repr(ex))
logger.debug(repr(ex))


# test_regular_logging_ok
def test_regular_logging_ok():
try: # @
add = 1 + 2
except Exception as ex:
logger.error("Example 1") # @
logger.warning("This is another example")
logger.info("Random logging")
logger.debug("Logging")


# test_logging_str_exception_branches
def test_logging_str_exception_branches():
try: # @
add = 1 + 2
except Exception as ex:
if ex.code == "Retryable":
logger.error(str(ex))
return True
elif Exception != BaseException:
logger.warning(repr(ex))
return False
else:
logger.info(str(ex)) # @


# test_other_logging_fails
def test_other_logging_fails():
try: # @
add = 1 + 2
except Exception as ex:
if ex.code == "Retryable":
logger.error("Something went wrong: {ex}. Try again")
return True
else:
logger.warning(ex)
return False


# test_no_logging_and_no_exception_name_ok
def test_no_logging_and_no_exception_name_ok():
try:
add = 1 + 2
except Exception as ex:
self.errors.appendleft(ex)
except Exception as ex: # pylint: disable=broad-except
_logger.warning(
"Exception occurred when instrumenting: %s.",
lib_name,
exc_info=ex,
)
except (OSError, PermissionError) as e:
logger.warning(
"Failed to read on-disk cache for component due to %s. "
"Please check if the file %s is in use or current user doesn't have the permission.",
type(e).__name__,
on_disk_cache_path.as_posix(),
)


# test_logging_without_exception_name
def test_logging_without_exception_name():
try:
add = 1 + 2
except Exception as exception:
if exception.code == "Retryable":
_LOGGER.info(
"%r returns an exception %r", self._container_id, last_exception
)
else:
module_logger.debug("Skipping file upload, reason: %s", str(e.reason))
Loading

0 comments on commit 88c86a6

Please sign in to comment.