From 7d52f3b7b711fe35277e3c70fd26ede057f9c635 Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Wed, 10 Aug 2022 11:20:45 -0700 Subject: [PATCH 1/7] block transport imports --- .../pylint_guidelines_checker.py | 2 +- .../tests/test_pylint_custom_plugins.py | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py index b372190540a..83a6ea41366 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py @@ -1932,8 +1932,8 @@ class NonCoreNetworkImport(BaseChecker): "This import is only allowed in azure.core.pipeline.transport.", ), } - BLOCKED_MODULES = ["aiohttp", "requests", "trio"] AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport" + BLOCKED_MODULES = ["aiohttp", "requests", "trio", "azure.core.pipeline.transport"] def visit_import(self, node): """Check that we dont have blocked imports.""" diff --git a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 1a4b5c691eb..451156d8f6b 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -3037,6 +3037,30 @@ def test_disallowed_imports(self): ): self.checker.visit_import(import_node) + import_node = astroid.extract_node("import azure.core.pipeline.transport as transport") + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="networking-import-outside-azure-core-transport", + line=1, + node=import_node, + col_offset=0, + ) + ): + self.checker.visit_import(import_node) + + import_node = astroid.extract_node("import azure.core.pipeline.transport.AioHttpTransport") + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="networking-import-outside-azure-core-transport", + line=1, + node=import_node, + col_offset=0, + ) + ): + self.checker.visit_import(import_node) + + + # blocked import from outside of core. importfrom_node = astroid.extract_node("from aiohttp import get") with self.assertAddsMessages( @@ -3049,6 +3073,18 @@ def test_disallowed_imports(self): ): self.checker.visit_importfrom(importfrom_node) + importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import RequestsTransport") + with self.assertAddsMessages( + pylint.testutils.MessageTest( + msg_id="networking-import-outside-azure-core-transport", + line=1, + node=importfrom_node, + col_offset=0, + ) + ): + self.checker.visit_importfrom(importfrom_node) + + def test_allowed_imports(self): """Check that allowed imports don't raise warnings.""" @@ -3073,3 +3109,4 @@ def test_allowed_imports(self): importfrom_node.root().name = "azure.core.pipeline.transport._private_module" with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) + From 039b6eba72ce7f2fb26be55b78a787285bd74d8b Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Thu, 11 Aug 2022 10:44:25 -0700 Subject: [PATCH 2/7] use separate rule --- .../pylint_guidelines_checker.py | 29 +++++++- .../tests/test_pylint_custom_plugins.py | 69 ++++++++++--------- 2 files changed, 65 insertions(+), 33 deletions(-) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py index 83a6ea41366..ac44ebfe325 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py @@ -1933,7 +1933,7 @@ class NonCoreNetworkImport(BaseChecker): ), } AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport" - BLOCKED_MODULES = ["aiohttp", "requests", "trio", "azure.core.pipeline.transport"] + BLOCKED_MODULES = ["aiohttp", "requests", "trio"] def visit_import(self, node): """Check that we dont have blocked imports.""" @@ -1956,6 +1956,33 @@ def _check_import(self, name, node): msgid=f"networking-import-outside-azure-core-transport", node=node, confidence=None ) +class NonAbstractTransportImport(BaseChecker): + name = "non-abstract-transport-import" + priority = -1 + msgs = { + "C4750": ( + "Only import abstract transports.", + "non-abstract-transport-import", + "Only import abstract transports. Let core or end-user decide which transport to use.", + ), + } + AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport" + ABSTRACT_CLASSES = {"HttpTransport", "HttpRequest", "HttpResponse", "AsyncHttpTransport", "AsyncHttpResponse"} + + + def visit_importfrom(self, node): + """Check that we aren't import from a blocked package.""" + if node.root().name.startswith(self.AZURE_CORE_TRANSPORT_NAME): + return + if node.modname == self.AZURE_CORE_TRANSPORT_NAME: + for name, _ in node.names: + if name not in self.ABSTRACT_CLASSES: + self.add_message( + msgid=f"non-abstract-transport-import", + node=node, + confidence=None, + ) + # if a linter is registered in this function then it will be checked with pylint def register(linter): diff --git a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 451156d8f6b..5000ddba8c8 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -3037,46 +3037,53 @@ def test_disallowed_imports(self): ): self.checker.visit_import(import_node) - import_node = astroid.extract_node("import azure.core.pipeline.transport as transport") + # blocked import from outside of core. + importfrom_node = astroid.extract_node("from aiohttp import get") with self.assertAddsMessages( pylint.testutils.MessageTest( msg_id="networking-import-outside-azure-core-transport", line=1, - node=import_node, + node=importfrom_node, col_offset=0, ) ): - self.checker.visit_import(import_node) + self.checker.visit_importfrom(importfrom_node) - import_node = astroid.extract_node("import azure.core.pipeline.transport.AioHttpTransport") - with self.assertAddsMessages( - pylint.testutils.MessageTest( - msg_id="networking-import-outside-azure-core-transport", - line=1, - node=import_node, - col_offset=0, - ) - ): + def test_allowed_imports(self): + """Check that allowed imports don't raise warnings.""" + # import not in the blocked list. + import_node = astroid.extract_node("import math") + with self.assertNoMessages(): self.checker.visit_import(import_node) + # from import not in the blocked list. + importfrom_node = astroid.extract_node("from azure.core.pipeline import Pipeline") + with self.assertNoMessages(): + self.checker.visit_importfrom(importfrom_node) + # blocked import, but in core. + import_node = astroid.extract_node("import requests") + import_node.root().name = "azure.core.pipeline.transport" + with self.assertNoMessages(): + self.checker.visit_import(import_node) - # blocked import from outside of core. - importfrom_node = astroid.extract_node("from aiohttp import get") - with self.assertAddsMessages( - pylint.testutils.MessageTest( - msg_id="networking-import-outside-azure-core-transport", - line=1, - node=importfrom_node, - col_offset=0, - ) - ): + # blocked from import, but in core. + importfrom_node = astroid.extract_node("from requests.exceptions import HttpException") + importfrom_node.root().name = "azure.core.pipeline.transport._private_module" + with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) + +class TestCheckNonAbstractTransportImport(pylint.testutils.CheckerTestCase): + """Test that we are blocking disallowed imports and allowing allowed imports.""" + CHECKER_CLASS = checker.NonAbstractTransportImport + + def test_disallowed_imports(self): + """Check that illegal imports raise warnings""" importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import RequestsTransport") with self.assertAddsMessages( pylint.testutils.MessageTest( - msg_id="networking-import-outside-azure-core-transport", + msg_id="non-abstract-transport-import", line=1, node=importfrom_node, col_offset=0, @@ -3084,28 +3091,26 @@ def test_disallowed_imports(self): ): self.checker.visit_importfrom(importfrom_node) - - def test_allowed_imports(self): """Check that allowed imports don't raise warnings.""" # import not in the blocked list. - import_node = astroid.extract_node("import math") + importfrom_node = astroid.extract_node("from math import PI") with self.assertNoMessages(): - self.checker.visit_import(import_node) + self.checker.visit_importfrom(importfrom_node) # from import not in the blocked list. importfrom_node = astroid.extract_node("from azure.core.pipeline import Pipeline") with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) - # blocked import, but in core. - import_node = astroid.extract_node("import requests") - import_node.root().name = "azure.core.pipeline.transport" + # blocked from import, but in core. + importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import HttpTransport, HttpRequest, HttpResponse, AsyncHttpTransport, AsyncHttpResponse") with self.assertNoMessages(): - self.checker.visit_import(import_node) + self.checker.visit_importfrom(importfrom_node) # blocked from import, but in core. - importfrom_node = astroid.extract_node("from requests.exceptions import HttpException") + importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import RequestsTransport, AioHttpTransport, AioHttpTransportResponse" + ) importfrom_node.root().name = "azure.core.pipeline.transport._private_module" with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) From 50a90cbe62efc3b183081a051e494a3f5d21957b Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Thu, 11 Aug 2022 11:01:36 -0700 Subject: [PATCH 3/7] register linter --- .../pylint-guidelines-checker/pylint_guidelines_checker.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py index ac44ebfe325..d6850e29309 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py @@ -2006,6 +2006,7 @@ def register(linter): linter.register_checker(CheckEnum(linter)) linter.register_checker(NonCoreNetworkImport(linter)) linter.register_checker(ClientListMethodsUseCorePaging(linter)) + linter.register_checker(NonAbstractTransportImport(linter)) From 9697033c45ee6e4d88f827c87f82cbaf98383899 Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Thu, 11 Aug 2022 15:40:38 -0700 Subject: [PATCH 4/7] styling --- .../pylint-guidelines-checker/pylint_guidelines_checker.py | 7 ++++--- .../tests/test_pylint_custom_plugins.py | 3 +-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py index d6850e29309..fa5473d4e00 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py @@ -1932,8 +1932,8 @@ class NonCoreNetworkImport(BaseChecker): "This import is only allowed in azure.core.pipeline.transport.", ), } - AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport" BLOCKED_MODULES = ["aiohttp", "requests", "trio"] + AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport" def visit_import(self, node): """Check that we dont have blocked imports.""" @@ -1957,6 +1957,9 @@ def _check_import(self, name, node): ) class NonAbstractTransportImport(BaseChecker): + """Rule to check that we aren't import transports outside of `azure.core.pipeline.transport`. + Transport selection should be up to `azure.core` or the end-user, not individual SDKs. + """ name = "non-abstract-transport-import" priority = -1 msgs = { @@ -1969,7 +1972,6 @@ class NonAbstractTransportImport(BaseChecker): AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport" ABSTRACT_CLASSES = {"HttpTransport", "HttpRequest", "HttpResponse", "AsyncHttpTransport", "AsyncHttpResponse"} - def visit_importfrom(self, node): """Check that we aren't import from a blocked package.""" if node.root().name.startswith(self.AZURE_CORE_TRANSPORT_NAME): @@ -1982,7 +1984,6 @@ def visit_importfrom(self, node): node=node, confidence=None, ) - # if a linter is registered in this function then it will be checked with pylint def register(linter): diff --git a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 5000ddba8c8..06a6a643e83 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -3109,8 +3109,7 @@ def test_allowed_imports(self): self.checker.visit_importfrom(importfrom_node) # blocked from import, but in core. - importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import RequestsTransport, AioHttpTransport, AioHttpTransportResponse" - ) + importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import RequestsTransport, AioHttpTransport, AioHttpTransportResponse") importfrom_node.root().name = "azure.core.pipeline.transport._private_module" with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) From f552d382f99b5622ba4b1d82c78ed995991969ad Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Thu, 11 Aug 2022 15:44:58 -0700 Subject: [PATCH 5/7] fix comments --- .../tests/test_pylint_custom_plugins.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index 06a6a643e83..06dc3c7115a 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -3049,6 +3049,7 @@ def test_disallowed_imports(self): ): self.checker.visit_importfrom(importfrom_node) + def test_allowed_imports(self): """Check that allowed imports don't raise warnings.""" # import not in the blocked list. @@ -3103,12 +3104,12 @@ def test_allowed_imports(self): with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) - # blocked from import, but in core. + # Import abstract classes importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import HttpTransport, HttpRequest, HttpResponse, AsyncHttpTransport, AsyncHttpResponse") with self.assertNoMessages(): self.checker.visit_importfrom(importfrom_node) - # blocked from import, but in core. + # Import non-abstract classes, but from in `azure.core.pipeline.transport`. importfrom_node = astroid.extract_node("from azure.core.pipeline.transport import RequestsTransport, AioHttpTransport, AioHttpTransportResponse") importfrom_node.root().name = "azure.core.pipeline.transport._private_module" with self.assertNoMessages(): From f387b39a03f58f152f42d73305b90ecae4b1ef66 Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Thu, 11 Aug 2022 15:54:22 -0700 Subject: [PATCH 6/7] readme --- tools/pylint-extensions/pylint-guidelines-checker/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/README.md b/tools/pylint-extensions/pylint-guidelines-checker/README.md index f78a489ecfd..97f47d7aa6a 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/README.md +++ b/tools/pylint-extensions/pylint-guidelines-checker/README.md @@ -74,4 +74,6 @@ In the case of a false positive, use the disable command to remove the pylint er | client-accepts-api-version-keyword | Ensure that the client constructor accepts a keyword-only api_version argument. | # pylint:disable=client-accepts-api-version-keyword | [link](https://azure.github.io/azure-sdk/python_design.html#specifying-the-service-version) | | enum-must-be-uppercase | The enum name must be all uppercase. | # pylint:disable=enum-must-be-uppercase | [link](https://azure.github.io/azure-sdk/python_design.html#enumerations) | | enum-must-inherit-case-insensitive-enum-meta | The enum should inherit from CaseInsensitiveEnumMeta. | # pylint:disable=enum-must-inherit-case-insensitive-enum-meta | [link](https://azure.github.io/azure-sdk/python_implementation.html#extensible-enumerations) | -| networking-import-outside-azure-core-transport | This import is only allowed in azure.core.pipeline.transport. | # pylint:disable=networking-import-outside-azure-core-transport | [link](https://github.com/Azure/azure-sdk-for-python/issues/24989) | \ No newline at end of file +| networking-import-outside-azure-core-transport | This import is only allowed in azure.core.pipeline.transport. | # pylint:disable=networking-import-outside-azure-core-transport | [link](https://github.com/Azure/azure-sdk-for-python/issues/24989) | +| non-abstract-transport-import | Only import abstract transports. Let core or end-user decide which transport to use. | # pylint:disable=non-abstract-transport-import | [link](https://github.com/Azure/azure-sdk-for-python/issues/25533) | + From 10042b8f81cf84ccef8bc816e363c17c80908a4d Mon Sep 17 00:00:00 2001 From: Steven Jin Date: Fri, 12 Aug 2022 14:04:39 -0700 Subject: [PATCH 7/7] typos --- .../pylint-guidelines-checker/pylint_guidelines_checker.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py index fa5473d4e00..ee793a48c23 100644 --- a/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/pylint-guidelines-checker/pylint_guidelines_checker.py @@ -1943,7 +1943,7 @@ def visit_import(self, node): self._check_import(import_, node) def visit_importfrom(self, node): - """Check that we aren't import from a blocked package.""" + """Check that we aren't importing from a blocked package.""" if node.root().name.startswith(self.AZURE_CORE_TRANSPORT_NAME): return self._check_import(node.modname, node) @@ -1957,7 +1957,7 @@ def _check_import(self, name, node): ) class NonAbstractTransportImport(BaseChecker): - """Rule to check that we aren't import transports outside of `azure.core.pipeline.transport`. + """Rule to check that we aren't importing transports outside of `azure.core.pipeline.transport`. Transport selection should be up to `azure.core` or the end-user, not individual SDKs. """ name = "non-abstract-transport-import" @@ -1973,7 +1973,7 @@ class NonAbstractTransportImport(BaseChecker): ABSTRACT_CLASSES = {"HttpTransport", "HttpRequest", "HttpResponse", "AsyncHttpTransport", "AsyncHttpResponse"} def visit_importfrom(self, node): - """Check that we aren't import from a blocked package.""" + """Check that we aren't importing from a blocked package.""" if node.root().name.startswith(self.AZURE_CORE_TRANSPORT_NAME): return if node.modname == self.AZURE_CORE_TRANSPORT_NAME: