Code quality update for Subaru sensors (#79482)

* Use distance device class for sensors

* Change sensor name casing and unique_id

* Migrate sensor entity unique_id

* Match title-cased unique_id when migrating

* Remove unneeded regex to find '_' delimited id suffix

* Incorporate PR review comments

* Add check to prevent extra odometer entity migration

homeassistant/components/subaru/sensor.py

@@ -23,7 +23,8 @@ from homeassistant.const import (
     VOLUME_GALLONS,
     VOLUME_LITERS,
 )
-from homeassistant.core import HomeAssistant
+from homeassistant.core import HomeAssistant, callback
+from homeassistant.helpers import entity_registry as er
 from homeassistant.helpers.entity_platform import AddEntitiesCallback
 from homeassistant.helpers.update_coordinator import (
     CoordinatorEntity,
@@ -51,6 +52,7 @@ from .const import (
 
 _LOGGER = logging.getLogger(__name__)
 
+
 # Fuel consumption units
 FUEL_CONSUMPTION_LITERS_PER_HUNDRED_KILOMETERS = "L/100km"
 FUEL_CONSUMPTION_MILES_PER_GALLON = "mi/gal"
@@ -62,6 +64,7 @@ KM_PER_MI = DistanceConverter.convert(1, LENGTH_MILES, LENGTH_KILOMETERS)
 SAFETY_SENSORS = [
     SensorEntityDescription(
         key=sc.ODOMETER,
+        device_class=SensorDeviceClass.DISTANCE,
         icon="mdi:road-variant",
         name="Odometer",
         native_unit_of_measurement=LENGTH_KILOMETERS,
@@ -74,12 +77,13 @@ API_GEN_2_SENSORS = [
     SensorEntityDescription(
         key=sc.AVG_FUEL_CONSUMPTION,
         icon="mdi:leaf",
-        name="Avg Fuel Consumption",
+        name="Avg fuel consumption",
         native_unit_of_measurement=FUEL_CONSUMPTION_LITERS_PER_HUNDRED_KILOMETERS,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.DIST_TO_EMPTY,
+        device_class=SensorDeviceClass.DISTANCE,
         icon="mdi:gas-station",
         name="Range",
         native_unit_of_measurement=LENGTH_KILOMETERS,
@@ -88,42 +92,42 @@ API_GEN_2_SENSORS = [
     SensorEntityDescription(
         key=sc.TIRE_PRESSURE_FL,
         device_class=SensorDeviceClass.PRESSURE,
-        name="Tire Pressure FL",
+        name="Tire pressure FL",
         native_unit_of_measurement=PRESSURE_HPA,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.TIRE_PRESSURE_FR,
         device_class=SensorDeviceClass.PRESSURE,
-        name="Tire Pressure FR",
+        name="Tire pressure FR",
         native_unit_of_measurement=PRESSURE_HPA,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.TIRE_PRESSURE_RL,
         device_class=SensorDeviceClass.PRESSURE,
-        name="Tire Pressure RL",
+        name="Tire pressure RL",
         native_unit_of_measurement=PRESSURE_HPA,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.TIRE_PRESSURE_RR,
         device_class=SensorDeviceClass.PRESSURE,
-        name="Tire Pressure RR",
+        name="Tire pressure RR",
         native_unit_of_measurement=PRESSURE_HPA,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.EXTERNAL_TEMP,
         device_class=SensorDeviceClass.TEMPERATURE,
-        name="External Temp",
+        name="External temp",
         native_unit_of_measurement=TEMP_CELSIUS,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.BATTERY_VOLTAGE,
         device_class=SensorDeviceClass.VOLTAGE,
-        name="12V Battery Voltage",
+        name="12V battery voltage",
         native_unit_of_measurement=ELECTRIC_POTENTIAL_VOLT,
         state_class=SensorStateClass.MEASUREMENT,
     ),
@@ -133,22 +137,23 @@ API_GEN_2_SENSORS = [
 EV_SENSORS = [
     SensorEntityDescription(
         key=sc.EV_DISTANCE_TO_EMPTY,
+        device_class=SensorDeviceClass.DISTANCE,
         icon="mdi:ev-station",
-        name="EV Range",
+        name="EV range",
         native_unit_of_measurement=LENGTH_MILES,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.EV_STATE_OF_CHARGE_PERCENT,
         device_class=SensorDeviceClass.BATTERY,
-        name="EV Battery Level",
+        name="EV battery level",
         native_unit_of_measurement=PERCENTAGE,
         state_class=SensorStateClass.MEASUREMENT,
     ),
     SensorEntityDescription(
         key=sc.EV_TIME_TO_FULLY_CHARGED_UTC,
         device_class=SensorDeviceClass.TIMESTAMP,
-        name="EV Time to Full Charge",
+        name="EV time to full charge",
         state_class=SensorStateClass.MEASUREMENT,
     ),
 ]
@@ -164,6 +169,7 @@ async def async_setup_entry(
     coordinator = entry[ENTRY_COORDINATOR]
     vehicle_info = entry[ENTRY_VEHICLES]
     entities = []
+    await _async_migrate_entries(hass, config_entry)
     for info in vehicle_info.values():
         entities.extend(create_vehicle_sensors(info, coordinator))
     async_add_entities(entities)
@@ -211,7 +217,7 @@ class SubaruSensor(
         self.vin = vehicle_info[VEHICLE_VIN]
         self.entity_description = description
         self._attr_device_info = get_device_info(vehicle_info)
-        self._attr_unique_id = f"{self.vin}_{description.name}"
+        self._attr_unique_id = f"{self.vin}_{description.key}"
 
     @property
     def native_value(self) -> None | int | float:
@@ -273,3 +279,48 @@ class SubaruSensor(
         if last_update_success and self.vin not in self.coordinator.data:
             return False
         return last_update_success
+
+
+async def _async_migrate_entries(
+    hass: HomeAssistant, config_entry: ConfigEntry
+) -> None:
+    """Migrate sensor entries from HA<=2022.10 to use preferred unique_id."""
+    entity_registry = er.async_get(hass)
+
+    all_sensors = []
+    all_sensors.extend(EV_SENSORS)
+    all_sensors.extend(API_GEN_2_SENSORS)
+    all_sensors.extend(SAFETY_SENSORS)
+
+    # Old unique_id is (previously title-cased) sensor name (e.g. "VIN_Avg Fuel Consumption")
+    replacements = {str(s.name).upper(): s.key for s in all_sensors}
+
+    @callback
+    def update_unique_id(entry: er.RegistryEntry) -> dict[str, Any] | None:
+        id_split = entry.unique_id.split("_")
+        key = id_split[1].upper() if len(id_split) == 2 else None
+
+        if key not in replacements or id_split[1] == replacements[key]:
+            return None
+
+        new_unique_id = entry.unique_id.replace(id_split[1], replacements[key])
+        _LOGGER.debug(
+            "Migrating entity '%s' unique_id from '%s' to '%s'",
+            entry.entity_id,
+            entry.unique_id,
+            new_unique_id,
+        )
+        if existing_entity_id := entity_registry.async_get_entity_id(
+            entry.domain, entry.platform, new_unique_id
+        ):
+            _LOGGER.debug(
+                "Cannot migrate to unique_id '%s', already exists for '%s'",
+                new_unique_id,
+                existing_entity_id,
+            )
+            return None
+        return {
+            "new_unique_id": new_unique_id,
+        }
+
+    await er.async_migrate_entries(hass, config_entry.entry_id, update_unique_id)

tests/components/subaru/conftest.py

@@ -5,6 +5,7 @@ from unittest.mock import patch
 import pytest
 from subarulink.const import COUNTRY_USA
 
+from homeassistant import config_entries
 from homeassistant.components.homeassistant import DOMAIN as HA_DOMAIN
 from homeassistant.components.subaru.const import (
     CONF_COUNTRY,
@@ -71,6 +72,15 @@ TEST_OPTIONS = {
     CONF_UPDATE_ENABLED: True,
 }
 
+TEST_CONFIG_ENTRY = {
+    "entry_id": "1",
+    "domain": DOMAIN,
+    "title": TEST_CONFIG[CONF_USERNAME],
+    "data": TEST_CONFIG,
+    "options": TEST_OPTIONS,
+    "source": config_entries.SOURCE_USER,
+}
+
 TEST_DEVICE_NAME = "test_vehicle_2"
 TEST_ENTITY_ID = f"sensor.{TEST_DEVICE_NAME}_odometer"
 
@@ -81,26 +91,16 @@ def advance_time_to_next_fetch(hass):
     async_fire_time_changed(hass, future)
 
 
-async def setup_subaru_integration(
+async def setup_subaru_config_entry(
     hass,
-    vehicle_list=None,
+    config_entry,
-    vehicle_data=None,
+    vehicle_list=[TEST_VIN_2_EV],
-    vehicle_status=None,
+    vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
+    vehicle_status=VEHICLE_STATUS_EV,
     connect_effect=None,
     fetch_effect=None,
 ):
-    """Create Subaru entry."""
+    """Run async_setup with API mocks in place."""
-    assert await async_setup_component(hass, HA_DOMAIN, {})
-    assert await async_setup_component(hass, DOMAIN, {})
-
-    config_entry = MockConfigEntry(
-        domain=DOMAIN,
-        data=TEST_CONFIG,
-        options=TEST_OPTIONS,
-        entry_id=1,
-    )
-    config_entry.add_to_hass(hass)
-
     with patch(
         MOCK_API_CONNECT,
         return_value=connect_effect is None,
@@ -134,20 +134,22 @@ async def setup_subaru_integration(
         await hass.config_entries.async_setup(config_entry.entry_id)
         await hass.async_block_till_done()
 
+
+@pytest.fixture
+async def subaru_config_entry(hass):
+    """Create a Subaru config entry prior to setup."""
+    await async_setup_component(hass, HA_DOMAIN, {})
+    config_entry = MockConfigEntry(**TEST_CONFIG_ENTRY)
+    config_entry.add_to_hass(hass)
     return config_entry
 
 
 @pytest.fixture
-async def ev_entry(hass):
+async def ev_entry(hass, subaru_config_entry):
     """Create a Subaru entry representing an EV vehicle with full STARLINK subscription."""
-    entry = await setup_subaru_integration(
+    await setup_subaru_config_entry(hass, subaru_config_entry)
-        hass,
-        vehicle_list=[TEST_VIN_2_EV],
-        vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
-        vehicle_status=VEHICLE_STATUS_EV,
-    )
     assert DOMAIN in hass.config_entries.async_domains()
     assert len(hass.config_entries.async_entries(DOMAIN)) == 1
-    assert hass.config_entries.async_get_entry(entry.entry_id)
+    assert hass.config_entries.async_get_entry(subaru_config_entry.entry_id)
-    assert entry.state is ConfigEntryState.LOADED
+    assert subaru_config_entry.state is ConfigEntryState.LOADED
-    return entry
+    return subaru_config_entry

tests/components/subaru/test_init.py

@@ -24,7 +24,7 @@ from .conftest import (
     MOCK_API_FETCH,
     MOCK_API_UPDATE,
     TEST_ENTITY_ID,
-    setup_subaru_integration,
+    setup_subaru_config_entry,
 )
 
 
@@ -42,61 +42,70 @@ async def test_setup_ev(hass, ev_entry):
     assert check_entry.state is ConfigEntryState.LOADED
 
 
-async def test_setup_g2(hass):
+async def test_setup_g2(hass, subaru_config_entry):
     """Test setup with a G2 vehcile ."""
-    entry = await setup_subaru_integration(
+    await setup_subaru_config_entry(
         hass,
+        subaru_config_entry,
         vehicle_list=[TEST_VIN_3_G2],
         vehicle_data=VEHICLE_DATA[TEST_VIN_3_G2],
         vehicle_status=VEHICLE_STATUS_G2,
     )
-    check_entry = hass.config_entries.async_get_entry(entry.entry_id)
+    check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id)
     assert check_entry
     assert check_entry.state is ConfigEntryState.LOADED
 
 
-async def test_setup_g1(hass):
+async def test_setup_g1(hass, subaru_config_entry):
     """Test setup with a G1 vehicle."""
-    entry = await setup_subaru_integration(
+    await setup_subaru_config_entry(
-        hass, vehicle_list=[TEST_VIN_1_G1], vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1]
+        hass,
+        subaru_config_entry,
+        vehicle_list=[TEST_VIN_1_G1],
+        vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1],
     )
-    check_entry = hass.config_entries.async_get_entry(entry.entry_id)
+    check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id)
     assert check_entry
     assert check_entry.state is ConfigEntryState.LOADED
 
 
-async def test_unsuccessful_connect(hass):
+async def test_unsuccessful_connect(hass, subaru_config_entry):
     """Test unsuccessful connect due to connectivity."""
-    entry = await setup_subaru_integration(
+    await setup_subaru_config_entry(
         hass,
+        subaru_config_entry,
         connect_effect=SubaruException("Service Unavailable"),
         vehicle_list=[TEST_VIN_2_EV],
         vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
         vehicle_status=VEHICLE_STATUS_EV,
     )
-    check_entry = hass.config_entries.async_get_entry(entry.entry_id)
+    check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id)
     assert check_entry
     assert check_entry.state is ConfigEntryState.SETUP_RETRY
 
 
-async def test_invalid_credentials(hass):
+async def test_invalid_credentials(hass, subaru_config_entry):
     """Test invalid credentials."""
-    entry = await setup_subaru_integration(
+    await setup_subaru_config_entry(
         hass,
+        subaru_config_entry,
         connect_effect=InvalidCredentials("Invalid Credentials"),
         vehicle_list=[TEST_VIN_2_EV],
         vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
         vehicle_status=VEHICLE_STATUS_EV,
     )
-    check_entry = hass.config_entries.async_get_entry(entry.entry_id)
+    check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id)
     assert check_entry
     assert check_entry.state is ConfigEntryState.SETUP_ERROR
 
 
-async def test_update_skip_unsubscribed(hass):
+async def test_update_skip_unsubscribed(hass, subaru_config_entry):
     """Test update function skips vehicles without subscription."""
-    await setup_subaru_integration(
+    await setup_subaru_config_entry(
-        hass, vehicle_list=[TEST_VIN_1_G1], vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1]
+        hass,
+        subaru_config_entry,
+        vehicle_list=[TEST_VIN_1_G1],
+        vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1],
     )
 
     with patch(MOCK_API_FETCH) as mock_fetch:
@@ -126,10 +135,11 @@ async def test_update_disabled(hass, ev_entry):
         mock_update.assert_not_called()
 
 
-async def test_fetch_failed(hass):
+async def test_fetch_failed(hass, subaru_config_entry):
     """Tests when fetch fails."""
-    await setup_subaru_integration(
+    await setup_subaru_config_entry(
         hass,
+        subaru_config_entry,
         vehicle_list=[TEST_VIN_2_EV],
         vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
         vehicle_status=VEHICLE_STATUS_EV,

tests/components/subaru/test_sensor.py

@@ -1,11 +1,16 @@
 """Test Subaru sensors."""
 from unittest.mock import patch
 
+import pytest
+
+from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN
 from homeassistant.components.subaru.sensor import (
     API_GEN_2_SENSORS,
+    DOMAIN as SUBARU_DOMAIN,
     EV_SENSORS,
     SAFETY_SENSORS,
 )
+from homeassistant.helpers import entity_registry as er
 from homeassistant.util import slugify
 from homeassistant.util.unit_system import US_CUSTOMARY_SYSTEM
 
@@ -13,6 +18,7 @@ from .api_responses import (
     EXPECTED_STATE_EV_IMPERIAL,
     EXPECTED_STATE_EV_METRIC,
     EXPECTED_STATE_EV_UNAVAILABLE,
+    TEST_VIN_2_EV,
     VEHICLE_STATUS_EV,
 )
 from .conftest import (
@@ -20,6 +26,7 @@ from .conftest import (
     MOCK_API_GET_DATA,
     TEST_DEVICE_NAME,
     advance_time_to_next_fetch,
+    setup_subaru_config_entry,
 )
 
 
@@ -50,6 +57,84 @@ async def test_sensors_missing_vin_data(hass, ev_entry):
     _assert_data(hass, EXPECTED_STATE_EV_UNAVAILABLE)
 
 
+@pytest.mark.parametrize(
+    "entitydata,old_unique_id,new_unique_id",
+    [
+        (
+            {
+                "domain": SENSOR_DOMAIN,
+                "platform": SUBARU_DOMAIN,
+                "unique_id": f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}",
+            },
+            f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}",
+            f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].key}",
+        ),
+    ],
+)
+async def test_sensor_migrate_unique_ids(
+    hass, entitydata, old_unique_id, new_unique_id, subaru_config_entry
+) -> None:
+    """Test successful migration of entity unique_ids."""
+    entity_registry = er.async_get(hass)
+    entity: er.RegistryEntry = entity_registry.async_get_or_create(
+        **entitydata,
+        config_entry=subaru_config_entry,
+    )
+    assert entity.unique_id == old_unique_id
+
+    await setup_subaru_config_entry(hass, subaru_config_entry)
+
+    entity_migrated = entity_registry.async_get(entity.entity_id)
+    assert entity_migrated
+    assert entity_migrated.unique_id == new_unique_id
+
+
+@pytest.mark.parametrize(
+    "entitydata,old_unique_id,new_unique_id",
+    [
+        (
+            {
+                "domain": SENSOR_DOMAIN,
+                "platform": SUBARU_DOMAIN,
+                "unique_id": f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}",
+            },
+            f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}",
+            f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].key}",
+        )
+    ],
+)
+async def test_sensor_migrate_unique_ids_duplicate(
+    hass, entitydata, old_unique_id, new_unique_id, subaru_config_entry
+) -> None:
+    """Test unsuccessful migration of entity unique_ids due to duplicate."""
+    entity_registry = er.async_get(hass)
+    entity: er.RegistryEntry = entity_registry.async_get_or_create(
+        **entitydata,
+        config_entry=subaru_config_entry,
+    )
+    assert entity.unique_id == old_unique_id
+
+    # create existing entry with new_unique_id that conflicts with migrate
+    existing_entity = entity_registry.async_get_or_create(
+        SENSOR_DOMAIN,
+        SUBARU_DOMAIN,
+        unique_id=new_unique_id,
+        config_entry=subaru_config_entry,
+    )
+
+    await setup_subaru_config_entry(hass, subaru_config_entry)
+
+    entity_migrated = entity_registry.async_get(entity.entity_id)
+    assert entity_migrated
+    assert entity_migrated.unique_id == old_unique_id
+
+    entity_not_changed = entity_registry.async_get(existing_entity.entity_id)
+    assert entity_not_changed
+    assert entity_not_changed.unique_id == new_unique_id
+
+    assert entity_migrated != entity_not_changed
+
+
 def _assert_data(hass, expected_state):
     sensor_list = EV_SENSORS
     sensor_list.extend(API_GEN_2_SENSORS)