Fix WUnderground duplicate entity ids by OttoWinter · Pull Request #13285 · home-assistant/core

OttoWinter · 2018-03-17T11:25:38Z

Description:

With #12346, when using multiple wunderground stations duplicate entity ids would manually be generated, thus breaking the platform. This PR fixes that by manually keeping track of our own generated entity ids.

Also changed some type hints to allow passing in set()s to the entity_id methods, but code-wise nothing has changed there.

Related issue (if applicable): fixes #12852

Example entry for `configuration.yaml` (if applicable):

sensor:
  - platform: wunderground
    api_key: !secret wunderground_api_key
    monitored_conditions:
      - weather
      - weather_1d_metric
      - weather_1n_metric

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.

MartinHjelmare

Looks good!

NovapaX · 2018-03-17T11:40:05Z

Shouldn't the unique_id not be also based on the weather station id then?
What if the sensors will get initialized in another order then the first time, will they get a different unique-id then the first time?

OttoWinter · 2018-03-17T11:47:17Z

This platform doesn't have any unique_id support and can't really get any unique_id support either (unless you want to change hundreds of entities every time you change the lat/long of the platform).

But concerning entity ids it is true that they're not "stable", that was also the previous behavior though. Thankfully we have an entity_namespace: option, I will put it in this PR.

OttoWinter · 2018-03-17T11:56:00Z

I now implemented the "entity_namespace" option. If you specify

sensor:
  - platform: wunderground
    api_key: !secret wunderground_api_key
    entity_namespace: hi
    monitored_conditions:
      - weather
      - weather_1d_metric
      - weather_1n_metric

The sensors will be called sensor.hi_weather and so on instead of sensor.pws_weather.

(Compared to 0.62 this would technically be a breaking change, since in that version the namespaced sensors would be called sensor.hi_pws_weather, but compared to 0.63+ it's not; I personally like being able to hide the ugly pws...)

NovapaX · 2018-03-17T12:10:50Z

Oh... lol.. I completely misread this 🤦‍♂️
Good job!

balloob · 2018-03-17T18:11:33Z

                  'Latitude and longitude must exist together'): cv.longitude,
    vol.Required(CONF_MONITORED_CONDITIONS):
        vol.All(cv.ensure_list, vol.Length(min=1), [vol.In(SENSOR_TYPES)]),
+    vol.Optional(CONF_ENTITY_NAMESPACE,


This is wrong. Entity namespace is implemented on the entity component level: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity_component.py#L138

Yes, but we're generating our own entity ids here. So the namespace is never used in the entity component level - the entity namespace code is never executed in the entity component. we have to do it on our own here :/ (There might be another way though.)

balloob · 2018-03-17T18:13:48Z


+DEFAULT_ENTITY_NAMESPACE = 'pws'
+
 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({


You are literally extending a platform schema here that includes entity namespace.

As far as I saw, there was no ENTITY_NAMESPACE in the original platform schema:

https://github.com/home-assistant/home-assistant/blob/4d3743f3f79a1127208ce885ae501991ec3e704e/homeassistant/helpers/config_validation.py#L493-L496

Hmm, there should be 🤔

balloob · 2018-03-17T18:15:05Z

        self.rest.request_feature(SENSOR_TYPES[condition].feature)
+        current_ids = set(hass.states.async_entity_ids(sensor.DOMAIN))
+        current_ids |= hass.data[ADDED_ENTITY_IDS_KEY]
        self.entity_id = async_generate_entity_id(


The real problem here in this component is that this entity is taking care of it's own entity ids. That is not the preferred approach. And guess what, because of this hack, we now need to add another hack to support entity namespace? Seems like a red flag to me…

It is a hack - this whole platform is very hackish :/

I just don't see any other way we can do custom names + constant "stable" entity ids at the same time. I mean we could take a look at disabling custom names to clean up the platform. Or we could even at some point take a look at transitioning this to the weather component.

If you want I can revert this. I will also make sure to wait for more feedback before merging these sorts of PRs that are a bit of a hack.

I think that the real solution would be to do nothing with entity ids and just give it a unique id. Then people can rename it themselves.

For now let's keep it in, I guess? I don't really care that strongly.

balloob · 2018-03-17T18:16:19Z

        self._entity_picture = None
        self._unit_of_measurement = self._cfg_expand("unit_of_measurement")
        self.rest.request_feature(SENSOR_TYPES[condition].feature)
+        current_ids = set(hass.states.async_entity_ids(sensor.DOMAIN))


Why would you generate this in every constructor call? Just do it once and pass it into the constructor

Hmm, true... that's inefficient indeed.

(on a side note: all entities (from all entity platforms) do not cache this: https://github.com/home-assistant/home-assistant/blob/4d3743f3f79a1127208ce885ae501991ec3e704e/homeassistant/helpers/entity_platform.py#L239-L240)

Fix WUnderground duplicate entity ids

4999a9b

OttoWinter requested a review from a team as a code owner March 17, 2018 11:25

homeassistant added cla-signed core platform: sensor.wunderground labels Mar 17, 2018

MartinHjelmare approved these changes Mar 17, 2018

View reviewed changes

Entity Namespace

d2a64f2

homeassistant added the cla-signed label Mar 17, 2018

MartinHjelmare approved these changes Mar 17, 2018

View reviewed changes

OttoWinter merged commit 3442b67 into home-assistant:dev Mar 17, 2018

OttoWinter deleted the wunderground-duplicate-entity-id branch March 17, 2018 12:16

balloob reviewed Mar 17, 2018

View reviewed changes

OttoWinter mentioned this pull request Mar 18, 2018

WUnderground unique ids #13311

Merged

3 tasks

balloob mentioned this pull request Mar 30, 2018

0.66.0 #13554

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: wunderground and removed platform: sensor.wunderground labels Mar 21, 2019


		DEFAULT_ENTITY_NAMESPACE = 'pws'

		PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({

Uh oh!

Conversation

OttoWinter commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

NovapaX commented Mar 17, 2018

Uh oh!

OttoWinter commented Mar 17, 2018

Uh oh!

OttoWinter commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NovapaX commented Mar 17, 2018

Uh oh!

balloob Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

OttoWinter Mar 17, 2018 • 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!

balloob Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

OttoWinter Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

balloob Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

balloob Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

OttoWinter Mar 17, 2018 • 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!

balloob Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

balloob Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

OttoWinter Mar 17, 2018 • 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!

5 participants

OttoWinter commented Mar 17, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

OttoWinter commented Mar 17, 2018 •

edited

Loading

OttoWinter Mar 17, 2018 •

edited

Loading

OttoWinter Mar 17, 2018 •

edited

Loading

OttoWinter Mar 17, 2018 •

edited

Loading