Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debug more information when splitting entity ID failed #49944

Closed
wants to merge 1 commit into from

Conversation

frenck
Copy link
Member

@frenck frenck commented May 1, 2021

Proposed change

Small change to a core method, that adds more information as soon as an issue occurs with splitting entity ID's.

Issues from 2 reporters during the beta of 2021.5:

Logger: homeassistant.core
Source: core.py:117
First occurred: 18:07:07 (8 occurrences)
Last logged: 21:22:02

Error in event filter
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/core.py", line 717, in async_fire
    if not event_filter(event):
  File "/usr/src/homeassistant/homeassistant/components/recorder/__init__.py", line 367, in _async_event_filter
    return bool(entity_id is None or self.entity_filter(entity_id))
  File "/usr/src/homeassistant/homeassistant/helpers/entityfilter.py", line 169, in entity_filter_2
    domain = split_entity_id(entity_id)[0]
  File "/usr/src/homeassistant/homeassistant/core.py", line 117, in split_entity_id
    return entity_id.split(".", 1)
AttributeError: 'list' object has no attribute 'split'

It would help if we knew the contents that it is trying to split to determine the source of the issue.
This PR adjusts the handling, to raise an HomeAssistant error with the entity_id it actually tries to split.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant probot-home-assistant bot added code-quality core small-pr PRs with less than 30 lines. labels May 1, 2021
@project-bot project-bot bot added this to Needs review in Dev May 1, 2021
@frenck frenck added this to the 2021.5.0 milestone May 1, 2021
@hmmbob
Copy link
Contributor

hmmbob commented May 1, 2021

Patched it in my running container to see if we can pinpoint the issue before next beta. Thanks Frenck!

Dev automation moved this from Needs review to Reviewer approved May 1, 2021
try:
return entity_id.split(".", 1)
except AttributeError as err:
raise HomeAssistantError("Unexpected entity ID format", entity_id) from err
Copy link
Member

Choose a reason for hiding this comment

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

Did you expect to include the entity ID in the message? I don't see how the second arg is used?

Also did you verify every place where this is used to make sure no one was catching attribute error?

Copy link
Contributor

@hmmbob hmmbob May 1, 2021

Choose a reason for hiding this comment

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

FWIW, it is adding the entity_ids. Patched my running container and got this error after a long wait:

homeassistant.exceptions.HomeAssistantError: ('Unexpected entity ID format', ['switch.grijze_lamp', 'light.window_level', 'switch.staande_lamp', 'switch.zoutlamp', 'climate.woonkamer'])

Full trace in the linked issue.

edit: it's google_assitant, I was able to reproduce. See issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you expect to include the entity ID in the message?

Yes, see, above. In example linked by @hmmbob

Also did you verify every place where this is used to make sure no one was catching attribute error?

No, but the PR can be closed as the goal of this PR was to find the issue... which has been found. It is caused by this PR:

Send only a single event per incoming Google command
#49449

Copy link
Member

Choose a reason for hiding this comment

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

TIL we can just pass tuples to exceptions 馃槅

@balloob balloob closed this May 2, 2021
Dev automation moved this from Reviewer approved to Cancelled May 2, 2021
@frenck frenck deleted the frenck-2021-1340 branch May 2, 2021 07:00
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

5 participants