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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log a warning when replacing existing config entry with same unique id #130567

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
12 changes: 12 additions & 0 deletions homeassistant/config_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -1481,6 +1481,18 @@ async def async_finish_flow(
result["handler"], flow.unique_id
)

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
epenet marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be clearer?
The flow should always 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

report_usage(
"creates a config entry when another entry with the same unique ID "
"exists",
core_behavior=ReportBehavior.LOG,
core_integration_behavior=ReportBehavior.LOG,
custom_integration_behavior=ReportBehavior.LOG,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shuold we start with just logging core integrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure
I would start with logging all, and we can easily adjust later if it generates too much noise

integration_domain=flow.handler,
)

# Unload the entry before setting up the new one.
# We will remove it only after the other one is set up,
# so that device customizations are not getting lost.
Expand Down
60 changes: 60 additions & 0 deletions tests/test_config_entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading