Restore Google/Alexa extra significant change checks (#46335)

homeassistant/components/alexa/state_report.py

@@ -8,7 +8,7 @@ import aiohttp
 import async_timeout
 
 from homeassistant.const import HTTP_ACCEPTED, MATCH_ALL, STATE_ON
-from homeassistant.core import State
+from homeassistant.core import HomeAssistant, State, callback
 from homeassistant.helpers.significant_change import create_checker
 import homeassistant.util.dt as dt_util
 
@@ -28,7 +28,20 @@ async def async_enable_proactive_mode(hass, smart_home_config):
     # Validate we can get access token.
     await smart_home_config.async_get_access_token()
 
-    checker = await create_checker(hass, DOMAIN)
+    @callback
+    def extra_significant_check(
+        hass: HomeAssistant,
+        old_state: str,
+        old_attrs: dict,
+        old_extra_arg: dict,
+        new_state: str,
+        new_attrs: dict,
+        new_extra_arg: dict,
+    ):
+        """Check if the serialized data has changed."""
+        return old_extra_arg is not None and old_extra_arg != new_extra_arg
+
+    checker = await create_checker(hass, DOMAIN, extra_significant_check)
 
     async def async_entity_state_listener(
         changed_entity: str,
@@ -70,15 +83,22 @@ async def async_enable_proactive_mode(hass, smart_home_config):
         if not should_report and not should_doorbell:
             return
 
-        if not checker.async_is_significant_change(new_state):
-            return
-
         if should_doorbell:
             should_report = False
 
+        if should_report:
+            alexa_properties = list(alexa_changed_entity.serialize_properties())
+        else:
+            alexa_properties = None
+
+        if not checker.async_is_significant_change(
+            new_state, extra_arg=alexa_properties
+        ):
+            return
+
         if should_report:
             await async_send_changereport_message(
-                hass, smart_home_config, alexa_changed_entity
+                hass, smart_home_config, alexa_changed_entity, alexa_properties
             )
 
         elif should_doorbell:
@@ -92,7 +112,7 @@ async def async_enable_proactive_mode(hass, smart_home_config):
 
 
 async def async_send_changereport_message(
-    hass, config, alexa_entity, *, invalidate_access_token=True
+    hass, config, alexa_entity, alexa_properties, *, invalidate_access_token=True
 ):
     """Send a ChangeReport message for an Alexa entity.
 
@@ -107,7 +127,7 @@ async def async_send_changereport_message(
     payload = {
         API_CHANGE: {
             "cause": {"type": Cause.APP_INTERACTION},
-            "properties": list(alexa_entity.serialize_properties()),
+            "properties": alexa_properties,
         }
     }
 
@@ -146,7 +166,7 @@ async def async_send_changereport_message(
     ):
         config.async_invalidate_access_token()
         return await async_send_changereport_message(
-            hass, config, alexa_entity, invalidate_access_token=False
+            hass, config, alexa_entity, alexa_properties, invalidate_access_token=False
         )
 
     _LOGGER.error(

homeassistant/components/google_assistant/report_state.py

@@ -38,42 +38,59 @@ def async_enable_report_state(hass: HomeAssistant, google_config: AbstractConfig
         if not entity.is_supported():
             return
 
-        if not checker.async_is_significant_change(new_state):
-            return
-
         try:
             entity_data = entity.query_serialize()
         except SmartHomeError as err:
             _LOGGER.debug("Not reporting state for %s: %s", changed_entity, err.code)
             return
 
+        if not checker.async_is_significant_change(new_state, extra_arg=entity_data):
+            return
+
         _LOGGER.debug("Reporting state for %s: %s", changed_entity, entity_data)
 
         await google_config.async_report_state_all(
             {"devices": {"states": {changed_entity: entity_data}}}
         )
 
+    @callback
+    def extra_significant_check(
+        hass: HomeAssistant,
+        old_state: str,
+        old_attrs: dict,
+        old_extra_arg: dict,
+        new_state: str,
+        new_attrs: dict,
+        new_extra_arg: dict,
+    ):
+        """Check if the serialized data has changed."""
+        return old_extra_arg != new_extra_arg
+
     async def inital_report(_now):
         """Report initially all states."""
         nonlocal unsub, checker
         entities = {}
 
-        checker = await create_checker(hass, DOMAIN)
+        checker = await create_checker(hass, DOMAIN, extra_significant_check)
 
         for entity in async_get_entities(hass, google_config):
             if not entity.should_expose():
                 continue
 
-            # Tell our significant change checker that we're reporting
-            # So it knows with subsequent changes what was already reported.
-            if not checker.async_is_significant_change(entity.state):
-                continue
-
             try:
-                entities[entity.entity_id] = entity.query_serialize()
+                entity_data = entity.query_serialize()
             except SmartHomeError:
                 continue
 
+            # Tell our significant change checker that we're reporting
+            # So it knows with subsequent changes what was already reported.
+            if not checker.async_is_significant_change(
+                entity.state, extra_arg=entity_data
+            ):
+                continue
+
+            entities[entity.entity_id] = entity_data
+
         if not entities:
             return
 

homeassistant/helpers/significant_change.py

@@ -27,7 +27,7 @@ The following cases will never be passed to your function:
 - state adding/removing
 """
 from types import MappingProxyType
-from typing import Any, Callable, Dict, Optional, Union
+from typing import Any, Callable, Dict, Optional, Tuple, Union
 
 from homeassistant.const import STATE_UNAVAILABLE, STATE_UNKNOWN
 from homeassistant.core import HomeAssistant, State, callback
@@ -47,13 +47,28 @@ CheckTypeFunc = Callable[
     Optional[bool],
 ]
 
+ExtraCheckTypeFunc = Callable[
+    [
+        HomeAssistant,
+        str,
+        Union[dict, MappingProxyType],
+        Any,
+        str,
+        Union[dict, MappingProxyType],
+        Any,
+    ],
+    Optional[bool],
+]
+
 
 async def create_checker(
-    hass: HomeAssistant, _domain: str
+    hass: HomeAssistant,
+    _domain: str,
+    extra_significant_check: Optional[ExtraCheckTypeFunc] = None,
 ) -> "SignificantlyChangedChecker":
     """Create a significantly changed checker for a domain."""
     await _initialize(hass)
-    return SignificantlyChangedChecker(hass)
+    return SignificantlyChangedChecker(hass, extra_significant_check)
 
 
 # Marked as singleton so multiple calls all wait for same output.
@@ -105,34 +120,46 @@ class SignificantlyChangedChecker:
     Will always compare the entity to the last entity that was considered significant.
     """
 
-    def __init__(self, hass: HomeAssistant) -> None:
+    def __init__(
+        self,
+        hass: HomeAssistant,
+        extra_significant_check: Optional[ExtraCheckTypeFunc] = None,
+    ) -> None:
         """Test if an entity has significantly changed."""
         self.hass = hass
-        self.last_approved_entities: Dict[str, State] = {}
+        self.last_approved_entities: Dict[str, Tuple[State, Any]] = {}
+        self.extra_significant_check = extra_significant_check
 
     @callback
-    def async_is_significant_change(self, new_state: State) -> bool:
-        """Return if this was a significant change."""
-        old_state: Optional[State] = self.last_approved_entities.get(
+    def async_is_significant_change(
+        self, new_state: State, *, extra_arg: Optional[Any] = None
+    ) -> bool:
+        """Return if this was a significant change.
+
+        Extra kwargs are passed to the extra significant checker.
+        """
+        old_data: Optional[Tuple[State, Any]] = self.last_approved_entities.get(
             new_state.entity_id
         )
 
         # First state change is always ok to report
-        if old_state is None:
-            self.last_approved_entities[new_state.entity_id] = new_state
+        if old_data is None:
+            self.last_approved_entities[new_state.entity_id] = (new_state, extra_arg)
             return True
 
+        old_state, old_extra_arg = old_data
+
         # Handle state unknown or unavailable
         if new_state.state in (STATE_UNKNOWN, STATE_UNAVAILABLE):
             if new_state.state == old_state.state:
                 return False
 
-            self.last_approved_entities[new_state.entity_id] = new_state
+            self.last_approved_entities[new_state.entity_id] = (new_state, extra_arg)
             return True
 
         # If last state was unknown/unavailable, also significant.
         if old_state.state in (STATE_UNKNOWN, STATE_UNAVAILABLE):
-            self.last_approved_entities[new_state.entity_id] = new_state
+            self.last_approved_entities[new_state.entity_id] = (new_state, extra_arg)
             return True
 
         functions: Optional[Dict[str, CheckTypeFunc]] = self.hass.data.get(
@@ -144,23 +171,36 @@ class SignificantlyChangedChecker:
 
         check_significantly_changed = functions.get(new_state.domain)
 
-        # No platform available means always true.
-        if check_significantly_changed is None:
-            self.last_approved_entities[new_state.entity_id] = new_state
-            return True
+        if check_significantly_changed is not None:
+            result = check_significantly_changed(
+                self.hass,
+                old_state.state,
+                old_state.attributes,
+                new_state.state,
+                new_state.attributes,
+            )
 
-        result = check_significantly_changed(
-            self.hass,
-            old_state.state,
-            old_state.attributes,
-            new_state.state,
-            new_state.attributes,
-        )
+            if result is False:
+                return False
 
-        if result is False:
-            return False
+        if self.extra_significant_check is not None:
+            result = self.extra_significant_check(
+                self.hass,
+                old_state.state,
+                old_state.attributes,
+                old_extra_arg,
+                new_state.state,
+                new_state.attributes,
+                extra_arg,
+            )
+
+            if result is False:
+                return False
 
         # Result is either True or None.
         # None means the function doesn't know. For now assume it's True
-        self.last_approved_entities[new_state.entity_id] = new_state
+        self.last_approved_entities[new_state.entity_id] = (
+            new_state,
+            extra_arg,
+        )
         return True

tests/components/alexa/test_state_report.py

@@ -178,6 +178,7 @@ async def test_doorbell_event(hass, aioclient_mock):
 
 async def test_proactive_mode_filter_states(hass, aioclient_mock):
     """Test all the cases that filter states."""
+    aioclient_mock.post(TEST_URL, text="", status=202)
     await state_report.async_enable_proactive_mode(hass, DEFAULT_CONFIG)
 
     # First state should report
@@ -186,7 +187,8 @@ async def test_proactive_mode_filter_states(hass, aioclient_mock):
         "on",
         {"friendly_name": "Test Contact Sensor", "device_class": "door"},
     )
-    assert len(aioclient_mock.mock_calls) == 0
+    await hass.async_block_till_done()
+    assert len(aioclient_mock.mock_calls) == 1
 
     aioclient_mock.clear_requests()
 
@@ -238,3 +240,24 @@ async def test_proactive_mode_filter_states(hass, aioclient_mock):
     await hass.async_block_till_done()
     await hass.async_block_till_done()
     assert len(aioclient_mock.mock_calls) == 0
+
+    # If serializes to same properties, it should not report
+    aioclient_mock.post(TEST_URL, text="", status=202)
+    with patch(
+        "homeassistant.components.alexa.entities.AlexaEntity.serialize_properties",
+        return_value=[{"same": "info"}],
+    ):
+        hass.states.async_set(
+            "binary_sensor.same_serialize",
+            "off",
+            {"friendly_name": "Test Contact Sensor", "device_class": "door"},
+        )
+        await hass.async_block_till_done()
+        hass.states.async_set(
+            "binary_sensor.same_serialize",
+            "off",
+            {"friendly_name": "Test Contact Sensor", "device_class": "door"},
+        )
+
+        await hass.async_block_till_done()
+    assert len(aioclient_mock.mock_calls) == 1

tests/components/google_assistant/test_report_state.py

@@ -46,6 +46,24 @@ async def test_report_state(hass, caplog, legacy_patchable_time):
         "devices": {"states": {"light.kitchen": {"on": True, "online": True}}}
     }
 
+    # Test that if serialize returns same value, we don't send
+    with patch(
+        "homeassistant.components.google_assistant.report_state.GoogleEntity.query_serialize",
+        return_value={"same": "info"},
+    ), patch.object(BASIC_CONFIG, "async_report_state_all", AsyncMock()) as mock_report:
+        # New state, so reported
+        hass.states.async_set("light.double_report", "on")
+        await hass.async_block_till_done()
+
+        # Changed, but serialize is same, so filtered out by extra check
+        hass.states.async_set("light.double_report", "off")
+        await hass.async_block_till_done()
+
+        assert len(mock_report.mock_calls) == 1
+        assert mock_report.mock_calls[0][1][0] == {
+            "devices": {"states": {"light.double_report": {"same": "info"}}}
+        }
+
     # Test that only significant state changes are reported
     with patch.object(
         BASIC_CONFIG, "async_report_state_all", AsyncMock()

tests/helpers/test_significant_change.py

@@ -5,7 +5,6 @@ from homeassistant.components.sensor import DEVICE_CLASS_BATTERY
 from homeassistant.const import ATTR_DEVICE_CLASS, STATE_UNAVAILABLE, STATE_UNKNOWN
 from homeassistant.core import State
 from homeassistant.helpers import significant_change
-from homeassistant.setup import async_setup_component
 
 
 @pytest.fixture(name="checker")
@@ -26,8 +25,6 @@ async def checker_fixture(hass):
 
 async def test_signicant_change(hass, checker):
     """Test initialize helper works."""
-    assert await async_setup_component(hass, "sensor", {})
-
     ent_id = "test_domain.test_entity"
     attrs = {ATTR_DEVICE_CLASS: DEVICE_CLASS_BATTERY}
 
@@ -48,3 +45,30 @@ async def test_signicant_change(hass, checker):
     # State turned unavailable
     assert checker.async_is_significant_change(State(ent_id, "100", attrs))
     assert checker.async_is_significant_change(State(ent_id, STATE_UNAVAILABLE, attrs))
+
+
+async def test_significant_change_extra(hass, checker):
+    """Test extra significant checker works."""
+    ent_id = "test_domain.test_entity"
+    attrs = {ATTR_DEVICE_CLASS: DEVICE_CLASS_BATTERY}
+
+    assert checker.async_is_significant_change(State(ent_id, "100", attrs), extra_arg=1)
+    assert checker.async_is_significant_change(State(ent_id, "200", attrs), extra_arg=1)
+
+    # Reset the last significiant change to 100 to repeat test but with
+    # extra checker installed.
+    assert checker.async_is_significant_change(State(ent_id, "100", attrs), extra_arg=1)
+
+    def extra_significant_check(
+        hass, old_state, old_attrs, old_extra_arg, new_state, new_attrs, new_extra_arg
+    ):
+        return old_extra_arg != new_extra_arg
+
+    checker.extra_significant_check = extra_significant_check
+
+    # This is normally a significant change (100 -> 200), but the extra arg check marks it
+    # as insignificant.
+    assert not checker.async_is_significant_change(
+        State(ent_id, "200", attrs), extra_arg=1
+    )
+    assert checker.async_is_significant_change(State(ent_id, "200", attrs), extra_arg=2)