Skip to content

Avoid exception when loading translation file for unloaded module#15765

Closed
awarecan wants to merge 3 commits intohome-assistant:devfrom
awarecan:fix-translation-resouce-loading
Closed

Avoid exception when loading translation file for unloaded module#15765
awarecan wants to merge 3 commits intohome-assistant:devfrom
awarecan:fix-translation-resouce-loading

Conversation

@awarecan
Copy link
Copy Markdown
Contributor

@awarecan awarecan commented Jul 31, 2018

Description:

There is a unhandled exception thrown if user navigate to config/integrations page in certain condition.

2018-07-31 09:29:25 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/home/jason/ha/home-assistant/homeassistant/components/frontend/__init__.py", line 495, in send_translations
    resources = await async_get_translations(hass, msg['language'])
  File "/home/jason/ha/home-assistant/homeassistant/helpers/translation.py", line 119, in async_get_translations
    resources = await async_get_component_resources(hass, language)
  File "/home/jason/ha/home-assistant/homeassistant/helpers/translation.py", line 98, in async_get_component_resources
    hass, component, language)
  File "/home/jason/ha/home-assistant/homeassistant/helpers/translation.py", line 39, in component_translation_file
    component_path = path.dirname(module.__file__)
AttributeError: 'NoneType' object has no attribute '__file__'

In my case, I disabled nest component in configuration.yaml, but didn't remove config entry from config, so it will cause nest component not loaded correctly. I am sure there may have other reason some module cannot be loaded correctly. In such scenario, all translation will be lost, screenshot likes

image

After this PR, only nest related translation will be lost, screenshot likes

image

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@awarecan awarecan requested a review from a team as a code owner July 31, 2018 17:07
@homeassistant homeassistant added cla-signed core small-pr PRs with less than 30 lines. labels Jul 31, 2018
@ghost ghost assigned awarecan Jul 31, 2018
@ghost ghost added the in progress label Jul 31, 2018
name = component

module = get_component(hass, component)
if module is None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes no sense. get_component is not about things not being setup. It's about loading the component from file, either from your custom components dir or from built-in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, you are forcing me to fix the root cause. Got it.


Defaults to returning empty dict if file is not found.
"""
if filename is None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also not good. We should fail. This code should never be called if filename is None.

We should write code that doesn't handle "bad" inputs and tries to fix them. Raise errors instead. The moment we fix them, that function will need to keep fixing it forever, even if other things get added to the function that would allow the previously bad input. The previously bad input expects to be "changed" or else bugs in other code happen.

@awarecan
Copy link
Copy Markdown
Contributor Author

awarecan commented Aug 2, 2018

@balloob

I changed anther way to fix issue, but I am not sure I am doing right thing, the loader has pretty complex logic. Anyway, I got better result, even the nest component which has been failed during setup still get translation file loaded now.

image

try:
return hass.data[DATA_KEY][comp_or_platform] # type: ignore
comp = hass.data[DATA_KEY][comp_or_platform] # type: ignore
if comp is not None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be None?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That happens if a component is not correctly coded, we don't have such components in our code base. It's only when people are having badly written custom components, hence the warning.

@awarecan
Copy link
Copy Markdown
Contributor Author

awarecan commented Aug 2, 2018

Root cause: home-assistant/architecture#52

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 2, 2018

I think you're misunderstanding the role of homeassistant.loader. The role of it is to load the actual files and return them as Python modules. It has to check if the user has a custom component and otherwise loads the built-in component/platform. Because it's doing I/O, the resolved module will be cached. This can never be None.

If you are having an import error, it means that you probably have a custom component and something is off.

@balloob balloob mentioned this pull request Aug 2, 2018
2 tasks
@ghost ghost assigned balloob Aug 2, 2018
@awarecan
Copy link
Copy Markdown
Contributor Author

awarecan commented Aug 2, 2018

Aha, that is so tricky. Lesson learned. Closed this PR.

@awarecan awarecan closed this Aug 2, 2018
@ghost ghost removed the in progress label Aug 2, 2018
@awarecan awarecan deleted the fix-translation-resouce-loading branch August 22, 2018 12:19
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed core small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants