[Storage] Fix #14054: 'NoneType' object has no attribute '__name__' by evelyn-ys · Pull Request #16993 · Azure/azure-cli

evelyn-ys · 2021-02-19T06:46:44Z

Description

Fix #14054
In previous design, if get_sdk can't find the dependency, the default behavior will return None. This will cause follow-up questions if None is not handled.
This PR exposes the real exception while getting sdk, which is ModuleNotFoundError

Testing Guide

Steps
Rename azure.multiapi.storage package(to azure.multiapi.storagev1 for example) to reproduce issue AttributeError: 'NoneType' object has no attribute '__name__' #14054
Run az storage container create -n testing
Before:

> az storage container create -n ystesting
The command failed with an unexpected error. Here is the traceback:
'NoneType' object has no attribute '__name__'
Traceback (most recent call last):
  File "D:\workspace\ysenv\lib\site-packages\knack\cli.py", line 233, in invoke
    cmd_result = self.invocation.execute(args)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 664, in execute
    raise ex
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 727, in _run_jobs_serially
    results.append(self._run_job(expanded_arg, cmd_copy))
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 719, in _run_job
    return cmd_copy.exception_handler(ex)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\__init__.py", line 338, in new_handler
    raise ex
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 698, in _run_job
    result = cmd_copy(params)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 331, in __call__
    return self.handler(*args, **kwargs)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\__init__.py", line 816, in default_command_handler
    return op(**command_args)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\operations\blob.py", line 126, in create_container
    client = blob_data_service_factory(cmd.cli_ctx, kwargs)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\_client_factory.py", line 84, in blob_data_service_factory
    location_mode=kwargs.pop('location_mode', None))
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\_client_factory.py", line 39, in generic_data_service_factory
    socket_timeout, token_credential, location_mode=location_mode)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\_client_factory.py", line 32, in get_storage_data_service_client
    location_mode=location_mode)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\client_factory.py", line 211, in get_data_service_client
    logger.debug('Getting data service client service_type=%s', service_type.__name__)
AttributeError: 'NoneType' object has no attribute '__name__'

After:

> az storage container create -n ystesting
The command failed with an unexpected error. Here is the traceback:
No module named 'azure.multiapi.storage'
Traceback (most recent call last):
  File "D:\workspace\ysenv\lib\site-packages\knack\cli.py", line 233, in invoke
    cmd_result = self.invocation.execute(args)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 664, in execute
    raise ex
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 727, in _run_jobs_serially
    results.append(self._run_job(expanded_arg, cmd_copy))
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 719, in _run_job
    return cmd_copy.exception_handler(ex)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\__init__.py", line 338, in new_handler
    raise ex
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 698, in _run_job
    result = cmd_copy(params)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\commands\__init__.py", line 331, in __call__
    return self.handler(*args, **kwargs)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\__init__.py", line 816, in default_command_handler
    return op(**command_args)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\operations\blob.py", line 126, in create_container
    client = blob_data_service_factory(cmd.cli_ctx, kwargs)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\_client_factory.py", line 76, in blob_data_service_factory
    blob_service = get_blob_service_by_type(cli_ctx, blob_type) or get_blob_service_by_type(cli_ctx, 'block')
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\sdkutil.py", line 34, in get_blob_service_by_type
    return type_to_service[blob_type](cli_ctx)
  File "d:\workspace\azure-cli\src\azure-cli\azure\cli\command_modules\storage\sdkutil.py", line 28, in <lambda>
    'block': lambda ctx: get_sdk(ctx, ResourceType.DATA_STORAGE, 'BlockBlobService', mod='blob', checked=False),
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\profiles\__init__.py", line 91, in get_sdk
    return _sdk_get_versioned_sdk(cli_ctx.cloud.profile, resource_type, *attr_args, **kwargs)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\profiles\_shared.py", line 584, in get_versioned_sdk
    loaded_obj = _get_attr(sdk_path, mod_attr_path, checked)
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\profiles\_shared.py", line 548, in _get_attr
    raise ex
  File "d:\workspace\azure-cli\src\azure-cli-core\azure\cli\core\profiles\_shared.py", line 539, in _get_attr
    op = import_module(full_mod_path)
  File "C:\Users\yishiwang\AppData\Local\Programs\Python\Python37\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
ModuleNotFoundError: No module named 'azure.multiapi.storage'

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.

yonzhan · 2021-02-19T06:54:22Z

Storage

yonzhan · 2021-02-19T06:57:31Z

@evelyn-ys Good PR description, I like it.

jiasli · 2021-02-19T07:41:03Z

src/azure-cli/azure/cli/command_modules/storage/sdkutil.py

+        'block': lambda ctx: get_sdk(ctx, ResourceType.DATA_STORAGE, 'BlockBlobService', mod='blob', checked=False),
+        'page': lambda ctx: get_sdk(ctx, ResourceType.DATA_STORAGE, 'PageBlobService', mod='blob', checked=False),
+        'append': lambda ctx: get_sdk(ctx, ResourceType.DATA_STORAGE, 'AppendBlobService', mod='blob', checked=False)


I would really prefer to make checked=False the default for get_sdk, otherwise it is not universal and 'NoneType' object has no attribute '__name__' will continue to happen in other command modules. (#15776 (comment))

I already made checked=False the default for _get_attr in #14690

azure-cli/src/azure-cli-core/azure/cli/core/profiles/_shared.py

Lines 534 to 535 in 301ba06

def _get_attr(sdk_path, mod_attr_path, checked=False):

"""If `checked` is True, None is returned in case of import failure."""

but it only applies to get_client_class which calls _get_attr without checked:

azure-cli/src/azure-cli-core/azure/cli/core/profiles/_shared.py

Lines 552 to 553 in 301ba06

def get_client_class(resource_type):

return _get_attr(resource_type.import_prefix, '#' + resource_type.client_name)

get_versioned_sdk will default checked to True via

azure-cli/src/azure-cli-core/azure/cli/core/profiles/_shared.py

Lines 573 to 574 in 301ba06

def get_versioned_sdk(api_profile, resource_type, *attr_args, **kwargs):

checked = kwargs.get('checked', True)

azure-cli/src/azure-cli-core/azure/cli/core/profiles/_shared.py

Line 585 in 301ba06

loaded_obj = _get_attr(sdk_path, mod_attr_path, checked)

Personally I totally agree with you. It's just I was afraid of making great changes to all modules. Should we discuss whether to change the behavior or not with all module owners?

We can go ahead and make the change. If CI passes, we are fine.

Python's import statement raises ImportError in case of non-existing module.

Also, checking whether the result is None is against the look before you leap practice.

jiasli · 2021-02-19T08:56:51Z

src/azure-cli-core/azure/cli/core/profiles/_shared.py


 def get_versioned_sdk(api_profile, resource_type, *attr_args, **kwargs):
-    checked = kwargs.get('checked', True)
+    checked = kwargs.get('checked', None)


This still overwrites the default checked=False of _get_attr. It is coincident that False and None have the same effect.

If the default is checked=True, it will be overwritten as None.

jiasli · 2021-02-19T10:56:36Z

src/azure-cli-core/azure/cli/core/profiles/_shared.py


 def get_versioned_sdk(api_profile, resource_type, *attr_args, **kwargs):
-    checked = kwargs.get('checked', True)
+    checked = kwargs.get('checked', None)


The current implementation of get_models is indeed a design flaw that can't distinguish

expected import failure caused by incompatible SDK profile (API version)

corrupted SDK installation

When the SDK installation is corrupted, this will trigger unclear error message like #14054 in many places:

azure-cli/src/azure-cli/azure/cli/command_modules/vm/_params.py

Line 117 in 045bbfb

c.argument('encryption_type', min_api='2019-07-01', arg_type=get_enum_type(self.get_models('EncryptionType')),

azure-cli/src/azure-cli/azure/cli/command_modules/vm/custom.py

Lines 301 to 302 in 0804c75

Disk, CreationData, DiskCreateOption, Encryption = cmd.get_models(

'Disk', 'CreationData', 'DiskCreateOption', 'Encryption')

A better pattern is to

call get_models lazily only when min_api or max_api is satisfied

error out if import failure happens

This requires large amount of effort inspecting the whole code base.

expose real exception while getting sdk

bbe3048

evelyn-ys requested review from Juliehzl, jsntcy and zhoxing-ms February 19, 2021 06:46

evelyn-ys self-assigned this Feb 19, 2021

yonzhan added this to the S183 - For Ignite milestone Feb 19, 2021

evelyn-ys mentioned this pull request Feb 19, 2021

AttributeError: 'NoneType' object has no attribute '__name__' #14054

Closed

jiasli reviewed Feb 19, 2021

View reviewed changes

evelyn-ys requested review from fengzhou-msft and houk-ms as code owners February 19, 2021 08:43

evelyn-ys changed the title ~~[Storage] Fix #14054: 'NoneType' object has no attribute '__name__'~~ [Core] Default get_sdk to raise ImportError when sdk not found Feb 19, 2021

jiasli reviewed Feb 19, 2021

View reviewed changes

evelyn-ys force-pushed the get_sdk_none branch from 9fc1d46 to bbe3048 Compare February 20, 2021 02:30

jiasli approved these changes Feb 20, 2021

View reviewed changes

evelyn-ys changed the title ~~[Core] Default get_sdk to raise ImportError when sdk not found~~ [Storage] Fix #14054: 'NoneType' object has no attribute '__name__' Feb 20, 2021

jiasli requested a review from qwordy February 20, 2021 03:32

Juliehzl approved these changes Feb 20, 2021

View reviewed changes

evelyn-ys merged commit 109f495 into Azure:dev Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage] Fix #14054: 'NoneType' object has no attribute 'name'#16993

[Storage] Fix #14054: 'NoneType' object has no attribute 'name'#16993
evelyn-ys merged 1 commit intoAzure:devfrom
evelyn-ys:get_sdk_none

evelyn-ys commented Feb 19, 2021 •

edited

Loading

Uh oh!

yonzhan commented Feb 19, 2021

Uh oh!

yonzhan commented Feb 19, 2021

Uh oh!

jiasli Feb 19, 2021 •

edited

Loading

Uh oh!

jiasli Feb 19, 2021 •

edited

Loading

Uh oh!

evelyn-ys Feb 19, 2021 •

edited

Loading

Uh oh!

jiasli Feb 19, 2021

Uh oh!

jiasli Feb 19, 2021

Uh oh!

jiasli Feb 19, 2021

Uh oh!

jiasli Feb 19, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	def _get_attr(sdk_path, mod_attr_path, checked=False):
	"""If `checked` is True, None is returned in case of import failure."""

	def get_client_class(resource_type):
	return _get_attr(resource_type.import_prefix, '#' + resource_type.client_name)

	def get_versioned_sdk(api_profile, resource_type, attr_args, *kwargs):
	checked = kwargs.get('checked', True)

	Disk, CreationData, DiskCreateOption, Encryption = cmd.get_models(
	'Disk', 'CreationData', 'DiskCreateOption', 'Encryption')

Conversation

evelyn-ys commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Feb 19, 2021

Uh oh!

yonzhan commented Feb 19, 2021

Uh oh!

jiasli Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evelyn-ys Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

jiasli Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

jiasli Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

jiasli Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

evelyn-ys commented Feb 19, 2021 •

edited

Loading

jiasli Feb 19, 2021 •

edited

Loading

jiasli Feb 19, 2021 •

edited

Loading

evelyn-ys Feb 19, 2021 •

edited

Loading

jiasli Feb 19, 2021 •

edited

Loading