From e66f2dbe59f55a1380820b327c5f0c894367e584 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Wed, 13 Nov 2024 21:50:19 +0000 Subject: [PATCH 01/13] Log a warning when replacing existing config entry with same unique id --- homeassistant/config_entries.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index ade4cd855cac2..077e8d8a1749f 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -35,6 +35,7 @@ CONF_NAME, EVENT_HOMEASSISTANT_STARTED, EVENT_HOMEASSISTANT_STOP, + PATCH_VERSION, Platform, ) from .core import ( @@ -1486,6 +1487,16 @@ async def async_finish_flow( result["handler"], flow.unique_id ) + if existing_entry is not None and PATCH_VERSION == "0.dev0": + # We temporarily restrict this to `dev` to get a feel for how many + # integrations this concerns + report_usage( + "creates a config entry when another entry with the same unique ID " + "exists, causing the old entry to be removed and replaced when it " + "should most likely update the previous entry and abort the flow", + core_behavior=ReportBehavior.LOG, + ) + # Unload the entry before setting up the new one. if existing_entry is not None and existing_entry.state.recoverable: await self.config_entries.async_unload(existing_entry.entry_id) From d182b703d263a4560bb3fa4371dc7af3ab0903e8 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Wed, 13 Nov 2024 22:03:58 +0000 Subject: [PATCH 02/13] Exclude mobile_app --- homeassistant/config_entries.py | 1 + 1 file changed, 1 insertion(+) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 077e8d8a1749f..5e92817e1c139 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1495,6 +1495,7 @@ async def async_finish_flow( "exists, causing the old entry to be removed and replaced when it " "should most likely update the previous entry and abort the flow", core_behavior=ReportBehavior.LOG, + exclude_integrations={"mobile_app"}, ) # Unload the entry before setting up the new one. From 885e32d75dd75e39b6721167aa827575b6dc11de Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Wed, 13 Nov 2024 22:06:03 +0000 Subject: [PATCH 03/13] Ignore custom integrations --- homeassistant/config_entries.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 5e92817e1c139..41fa18af34424 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1495,6 +1495,8 @@ async def async_finish_flow( "exists, causing the old entry to be removed and replaced when it " "should most likely update the previous entry and abort the flow", core_behavior=ReportBehavior.LOG, + core_integration_behavior=ReportBehavior.LOG, + custom_integration_behavior=ReportBehavior.IGNORE, exclude_integrations={"mobile_app"}, ) From abd5f77429d99ab36cada688ef6cce9189d75d55 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Fri, 15 Nov 2024 10:56:51 +0100 Subject: [PATCH 04/13] Apply suggestions from code review --- homeassistant/config_entries.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 41fa18af34424..d66b2d14c60da 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1487,9 +1487,9 @@ async def async_finish_flow( result["handler"], flow.unique_id ) - if existing_entry is not None and PATCH_VERSION == "0.dev0": - # We temporarily restrict this to `dev` to get a feel for how many - # integrations this concerns + if existing_entry is not None: + # `mobile_app` does this on purpose + # if too many integrations are impacted, we may need to reconsider report_usage( "creates a config entry when another entry with the same unique ID " "exists, causing the old entry to be removed and replaced when it " From 44de2aa57e91e90708d581b0edf84b2557e9c7e5 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:12:28 +0100 Subject: [PATCH 05/13] Apply suggestions from code review --- homeassistant/config_entries.py | 1 - 1 file changed, 1 deletion(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index d66b2d14c60da..7ac963c944f2b 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -35,7 +35,6 @@ CONF_NAME, EVENT_HOMEASSISTANT_STARTED, EVENT_HOMEASSISTANT_STOP, - PATCH_VERSION, Platform, ) from .core import ( From 0dbdf3b163cfd2ab8cb9909f0c791827118415bb Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 21 Nov 2024 16:39:59 +0100 Subject: [PATCH 06/13] Update config_entries.py --- homeassistant/config_entries.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 7ac963c944f2b..c7b694699d803 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1486,9 +1486,8 @@ async def async_finish_flow( result["handler"], flow.unique_id ) - if existing_entry is not None: + if existing_entry is not None and self.handler != "mobile_app": # `mobile_app` does this on purpose - # if too many integrations are impacted, we may need to reconsider report_usage( "creates a config entry when another entry with the same unique ID " "exists, causing the old entry to be removed and replaced when it " @@ -1496,7 +1495,7 @@ async def async_finish_flow( core_behavior=ReportBehavior.LOG, core_integration_behavior=ReportBehavior.LOG, custom_integration_behavior=ReportBehavior.IGNORE, - exclude_integrations={"mobile_app"}, + integration_domain=self.handler, ) # Unload the entry before setting up the new one. From 60831790970f47d014343f3f1ac338a33d6fa6c3 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 21 Nov 2024 15:58:38 +0000 Subject: [PATCH 07/13] Fix handler --- homeassistant/config_entries.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index c7b694699d803..3df27f8cdc755 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1486,7 +1486,7 @@ async def async_finish_flow( result["handler"], flow.unique_id ) - if existing_entry is not None and self.handler != "mobile_app": + if existing_entry is not None and flow.handler != "mobile_app": # `mobile_app` does this on purpose report_usage( "creates a config entry when another entry with the same unique ID " @@ -1495,7 +1495,7 @@ async def async_finish_flow( core_behavior=ReportBehavior.LOG, core_integration_behavior=ReportBehavior.LOG, custom_integration_behavior=ReportBehavior.IGNORE, - integration_domain=self.handler, + integration_domain=flow.handler, ) # Unload the entry before setting up the new one. From c6434f90df349172d32cfdf10322febe4c1d1564 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 21 Nov 2024 16:32:52 +0000 Subject: [PATCH 08/13] Adjust and add tests --- homeassistant/config_entries.py | 6 ++-- tests/test_config_entries.py | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 3df27f8cdc755..463796e356e69 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1487,11 +1487,11 @@ async def async_finish_flow( ) if existing_entry is not None and flow.handler != "mobile_app": - # `mobile_app` does this on purpose + # This causes the old entry to be removed and replaced when it + # should most likely update the previous entry and abort the flow report_usage( "creates a config entry when another entry with the same unique ID " - "exists, causing the old entry to be removed and replaced when it " - "should most likely update the previous entry and abort the flow", + "exists", core_behavior=ReportBehavior.LOG, core_integration_behavior=ReportBehavior.LOG, custom_integration_behavior=ReportBehavior.IGNORE, diff --git a/tests/test_config_entries.py b/tests/test_config_entries.py index aba85a35349d3..a962d1c708e38 100644 --- a/tests/test_config_entries.py +++ b/tests/test_config_entries.py @@ -7616,3 +7616,63 @@ async def test_add_description_placeholder_automatically_not_overwrites( result = await hass.config_entries.flow.async_configure(flows[0]["flow_id"], None) assert result["type"] == FlowResultType.FORM assert result["description_placeholders"] == {"name": "Custom title"} + + +@pytest.mark.parametrize( + ("domain", "expected_log"), + [ + ("some_integration", True), + ("mobile_app", False), + ], +) +async def test_create_entry_existing_unique_id( + hass: HomeAssistant, + domain: str, + expected_log: bool, + caplog: pytest.LogCaptureFixture, +) -> None: + """Test to highlight unexpected behavior on create_entry.""" + entry = MockConfigEntry( + title="From config flow", + domain=domain, + entry_id="01J915Q6T9F6G5V0QJX6HBC94T", + data={"host": "any", "port": 123}, + unique_id="mock-unique-id", + ) + entry.add_to_hass(hass) + + assert len(hass.config_entries.async_entries(domain)) == 1 + + mock_setup_entry = AsyncMock(return_value=True) + + mock_integration(hass, MockModule(domain, async_setup_entry=mock_setup_entry)) + mock_platform(hass, f"{domain}.config_flow", None) + + class TestFlow(config_entries.ConfigFlow): + """Test flow.""" + + VERSION = 1 + + async def async_step_user(self, user_input=None): + """Test user step.""" + await self.async_set_unique_id("mock-unique-id") + return self.async_create_entry(title="mock-title", data={}) + + with ( + mock_config_flow(domain, TestFlow), + patch.object(frame, "_REPORTED_INTEGRATIONS", set()), + ): + result = await hass.config_entries.flow.async_init( + domain, context={"source": config_entries.SOURCE_USER} + ) + await hass.async_block_till_done() + assert result["type"] is FlowResultType.CREATE_ENTRY + + assert len(hass.config_entries.async_entries(domain)) == 1 + + log_text = ( + f"Detected that integration '{domain}' creates a config entry " + "when another entry with the same unique ID exists. Please " + "create a bug report at https:" + ) + assert (log_text in caplog.text) == expected_log From fe94c8b50c97fd331d205facbc54be0a5c748575 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 21 Nov 2024 18:01:43 +0100 Subject: [PATCH 09/13] Apply suggestions from code review --- homeassistant/config_entries.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 463796e356e69..cdb1d0e10b26b 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1494,7 +1494,7 @@ async def async_finish_flow( "exists", core_behavior=ReportBehavior.LOG, core_integration_behavior=ReportBehavior.LOG, - custom_integration_behavior=ReportBehavior.IGNORE, + custom_integration_behavior=ReportBehavior.LOG, integration_domain=flow.handler, ) From 79b5ada475dd1c4ca517090909d0a0590330f980 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 21 Nov 2024 18:03:27 +0100 Subject: [PATCH 10/13] Apply suggestions from code review --- homeassistant/config_entries.py | 1 + 1 file changed, 1 insertion(+) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index cdb1d0e10b26b..38474543a627c 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1489,6 +1489,7 @@ async def async_finish_flow( if existing_entry is not None and flow.handler != "mobile_app": # This causes the old entry to be removed and replaced when it # should most likely update the previous entry and abort the flow + # see https://developers.home-assistant.io/blog/2024/11/22/config-flow-unique-id/ report_usage( "creates a config entry when another entry with the same unique ID " "exists", From eb49e163510d589e3397e57100c9cf7d72d9dd2c Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 21 Nov 2024 19:18:25 +0000 Subject: [PATCH 11/13] Update comment --- homeassistant/config_entries.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 38474543a627c..c532638d45bad 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1487,8 +1487,12 @@ async def async_finish_flow( ) if existing_entry is not None and flow.handler != "mobile_app": - # This causes the old entry to be removed and replaced when it - # should most likely update the previous entry and abort the flow + # This causes the old entry to be removed and replaced, when the flow + # should instead be aborted. + # In case of manual flows, integrations should implement options, reauth, + # reconfigure to allow the user to change settings. + # In case of non user visible flows, the integration should optionally + # update the existing entry before aborting. # see https://developers.home-assistant.io/blog/2024/11/22/config-flow-unique-id/ report_usage( "creates a config entry when another entry with the same unique ID " From 5a26bb73f303346377043cbf3c6ef3f53d6f18c1 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 19 Dec 2024 08:42:26 +0100 Subject: [PATCH 12/13] Update config_entries.py --- homeassistant/config_entries.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index c532638d45bad..d057545008d57 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1493,7 +1493,7 @@ async def async_finish_flow( # reconfigure to allow the user to change settings. # In case of non user visible flows, the integration should optionally # update the existing entry before aborting. - # see https://developers.home-assistant.io/blog/2024/11/22/config-flow-unique-id/ + # see https://developers.home-assistant.io/blog/2024/12/19/config-flow-unique-id/ report_usage( "creates a config entry when another entry with the same unique ID " "exists", From ce4000edc9d982fb1553944b6e57954909916d94 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Thu, 16 Jan 2025 18:01:59 +0100 Subject: [PATCH 13/13] Apply suggestions from code review --- homeassistant/config_entries.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index d057545008d57..04bc3904a99b4 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1493,7 +1493,7 @@ async def async_finish_flow( # reconfigure to allow the user to change settings. # In case of non user visible flows, the integration should optionally # update the existing entry before aborting. - # see https://developers.home-assistant.io/blog/2024/12/19/config-flow-unique-id/ + # see https://developers.home-assistant.io/blog/2025/01/16/config-flow-unique-id/ report_usage( "creates a config entry when another entry with the same unique ID " "exists",