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

Log warning when entities referenced in service call not found #31427

Merged
merged 5 commits into from Feb 4, 2020

Conversation

@balloob
Copy link
Member

balloob commented Feb 3, 2020

Breaking change

Service calls that reference non-existing entities will now log warning instead of silently being omitted. If you are using a template and want to select no entities, pass none instead.

The TTS integration will no longer target all media players if no entity ID passed in. Specify all instead.

Proposed change

Now that we restore entities on boot, we can be sure that entities will exist. This means that we can start raising errors print warnings when referenced entities are not found instead of silently omitting them.

This is a RFC because the impact is unknown. If we want to move on with this idea, we could start with just printing a warning instead of failing.

Tests will be added if functionality approved. Added

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

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
@balloob balloob requested a review from home-assistant/core as a code owner Feb 3, 2020
@probot-home-assistant probot-home-assistant bot added the core label Feb 3, 2020
@project-bot project-bot bot added this to Needs review in Dev Feb 3, 2020
@balloob balloob changed the title RFC: Raise entities not found error RFC: Raise error when entities not found Feb 3, 2020
found.append(entity)

if entity_ids:
raise EntitiesNotFound(entity_ids)

This comment has been minimized.

Copy link
@pvizeli

pvizeli Feb 3, 2020

Member

I would like this. But only in the case of all integration support unique_id and are restored on startup. Without this perfect world scenario, you will break automation/action/scripts after startup until everything is set up and the latest retry was successful.

@pnbruckner

This comment has been minimized.

Copy link
Contributor

pnbruckner commented Feb 3, 2020

I can tell you that this will cause a lot of breakage.

I've seen and been involved in many discussions on the forum where the solution to a tricky problem is to use a non-existing entity_id in a service call with the knowledge that that will effectively be a "NOP". (E.g., when data_template is used to select what script to run, where sometimes you don't want to call anything, in which case you can have the template return, e.g., script.none or script.nop.)

Granted, this is not documented behavior, but it's been that way so long that it has become a de facto solution.

Maybe to allow that same use case accept a new keyword for entity_id (similar to all) that would imply "no entities." Not sure if none would be good since that is a YAML keyword, and I'm sure people would often forget to quote it. (Unless, when it gets converted to Python's None, that would work, too.)

@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 3, 2020

@pnbruckner I don't understand the use case. Why would you specify an entity ID if you also have a data_template to specify it?

Edit: Oh I understand it now. You are trying to pick a script to run, but sometimes none apply and you want to do nothing.

@balloob balloob changed the title RFC: Raise error when entities not found RFC: Log warning when entities referenced in service call not found Feb 3, 2020
@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 4, 2020

Alright, so I will change it to a warning now.

@pnbruckner I think that the solution for the use case that you brought up is that people create a noop script so that they can return an existing script (that just doesn't do anything).

@Petro31

This comment has been minimized.

Copy link
Contributor

Petro31 commented Feb 4, 2020

Oof, will this affect set states for non-existing entities?

@pnbruckner

This comment has been minimized.

Copy link
Contributor

pnbruckner commented Feb 4, 2020

@balloob, yes, for scripts that is an acceptable solution and one that I've often suggested.

However, maybe I used a poor example, because I've seen the same scenario with other entities, like lights, switches, etc., when you need to determine the entity to act upon via a data_template, but sometimes it should act on no entity. And, yes, there are other ways around that, but they get rather involved because HA's native scripting language is somewhat limited. Often the simplest and cleanest solution is just to have the data_template result in a non-existing entity ID. I actually see no problem with having a second keyword like all, which would mean "no entity". Then you get the best of both worlds -- you get a warning/error when you accidentally specify a non-existing entity (which can be very helpful), and you get no warning/error when you explicitly want to specify "no entity." But, yes, that's a feature request. 馃槃

@Petro31

This comment has been minimized.

Copy link
Contributor

Petro31 commented Feb 4, 2020

I'm pretty much in agreement with @pnbruckner. Without an option like 'none' or 'no-entity' for entity_id, people would have to create many 'dummy' objects. Imagine trying to do that for media_player. Template media_player doesn't exist, so it would be impossible.

@balloob balloob requested review from pnbruckner and robbiet480 as code owners Feb 4, 2020
@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 4, 2020

I've added support for passing none to both entity_id and area_id. In both cases no selection will be made.

@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 4, 2020

@Petro31 this will not affect setting states for non-existing entities. The state machine doesn't know about entities, just states and so is not affected. This only affects picking entities in service calls.

@code-in-progress

This comment has been minimized.

Copy link

code-in-progress commented Feb 4, 2020

@balloob: Rather than forcing a warning into the logs, for those of us that know the entities don't exist, I think it'd be preferable to have a config parameter to continue to suppress the warning instead of adding more to the logs (which already generate a huge amount of I/O as it is when you have a lot of scripts with null entities).

For newcomers, I can see where this would be beneficial, but for advanced/expert users that knowingly have to create "null" entities for scripts and such, this is going to cause a lot of confusion.

Ignore as the logger: config can take care of my concern.

@UMH-Home

This comment has been minimized.

Copy link

UMH-Home commented Feb 4, 2020

I agree with code in progress to allow silent continuance via either a setting in config or a setting under the interface for advanced users

EDIT: As pointed out by Petro, we can remove items from logging as require, so amendment request revoked

@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 4, 2020

Besides using the logger, why not just set it to none if you have nothing to specify instead of null?

balloob added 2 commits Feb 4, 2020
Copy link
Contributor

pnbruckner left a comment

Looks good to me. I was surprised, though, how many files had to be changed to implement this. But after looking at the changes, I guess I shouldn't have been. Sounds like an opportunity for some refactoring someday. 馃槄

Dev automation moved this from Needs review to Reviewer approved Feb 4, 2020
@balloob

This comment has been minimized.

Copy link
Member Author

balloob commented Feb 4, 2020

Yeah, lots of places doing their own entity extraction. They won't work with area selectors or anything else that we add in the future. I added a bunch of nice new helpers in 105 that should solve this (home-assistant/developers.home-assistant#401).

@balloob balloob changed the title RFC: Log warning when entities referenced in service call not found Log warning when entities referenced in service call not found Feb 4, 2020
@balloob balloob merged commit f41623c into dev Feb 4, 2020
10 checks passed
10 checks passed
CI Build #20200204.49 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.61%)
Details
codecov/project 94.62% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Feb 4, 2020
@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Feb 4, 2020

I think that is good now. So if you change entity ids, it will not break instantly a lot of automation/service calls 馃憤

@delete-merged-branch delete-merged-branch bot deleted the entities-not-found-error branch Feb 4, 2020
@lock lock bot locked and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can鈥檛 perform that action at this time.