mirror of
https://github.com/esphome/esphome.git
synced 2026-05-26 03:07:04 +08:00
Address Copilot on PR #16296: tighter regex anchoring + same-name guard
Two follow-ups from Copilot's review. The line-scoped name rewrite's pattern was still too loose. It matched a ``name:`` substring at any position on the line, so a ``friendly_name: oldname`` line inside the ``esphome:`` block would get its ``name: oldname`` tail flipped along with the real ``name:`` line. The value side could also match a longer token as a prefix: ``[\"']?kitchen[\"']?`` would match the leading ``kitchen`` of an unrelated ``kitchen2`` value. Anchored at line start (``^``) and added an end-of-value lookahead (``(?=\s*(?:#|$))``) so only whole-token matches at the start of a child line under ``esphome:`` get swapped. The same-name refusal only fired when the new name resolved to the same path as the source. That covers the common case where the config filename matches the device name, but a config like ``weird-file.yaml`` whose ``esphome.name`` is ``kitchen`` would fall through: ``esphome rename weird-file.yaml kitchen`` then re-flashes the same hostname. Added an explicit check on the resolved ``new_name == old_name`` ahead of the path-equality gate so the "already the device's name" message matches what the user is actually seeing. Tests: three new cases pin both fixes (friendly_name substring, old_name-as-prefix, and the substitution-resolved same-name case).
This commit is contained in:
+27
-3
@@ -1779,8 +1779,16 @@ def command_rename(args: ArgsProtocol, config: ConfigType) -> int | None:
|
||||
# also clobber any other ``name:`` line whose value happens
|
||||
# to match (e.g. a sensor / output / wifi entry sharing the
|
||||
# device's hostname), silently rewriting unrelated user
|
||||
# configuration.
|
||||
name_pattern = re.compile(rf"(\s*name:\s+)[\"']?{re.escape(old_name)}[\"']?")
|
||||
# configuration. The pattern anchors:
|
||||
# - at the start of the line so ``friendly_name:``,
|
||||
# ``device_name:`` etc. don't match the trailing ``name:``
|
||||
# substring; and
|
||||
# - at the end of the value (lookahead for whitespace +
|
||||
# comment + EOL) so ``old_name`` doesn't match as a
|
||||
# prefix of a longer value (``kitchen`` vs ``kitchen2``).
|
||||
name_pattern = re.compile(
|
||||
rf"^(\s*)name:\s+[\"']?{re.escape(old_name)}[\"']?(?=\s*(?:#|$))"
|
||||
)
|
||||
out_lines: list[str] = []
|
||||
in_esphome_block = False
|
||||
for line in raw_contents.splitlines(keepends=True):
|
||||
@@ -1789,7 +1797,7 @@ def command_rename(args: ArgsProtocol, config: ConfigType) -> int | None:
|
||||
out_lines.append(line)
|
||||
continue
|
||||
if in_esphome_block:
|
||||
line = name_pattern.sub(rf'\1"{new_name}"', line, count=1)
|
||||
line = name_pattern.sub(rf'\1name: "{new_name}"', line, count=1)
|
||||
out_lines.append(line)
|
||||
new_raw = "".join(out_lines)
|
||||
else:
|
||||
@@ -1814,6 +1822,22 @@ def command_rename(args: ArgsProtocol, config: ConfigType) -> int | None:
|
||||
flags=re.MULTILINE,
|
||||
)
|
||||
|
||||
# ``new_name == old_name`` (after substitution resolution) is
|
||||
# a no-op rewrite that would still queue a pointless re-flash.
|
||||
# Catch it before the path-equality check below — covers the
|
||||
# case where the config filename doesn't match the device name
|
||||
# (e.g. ``weird-file.yaml`` whose ``esphome.name`` is
|
||||
# ``kitchen``; running ``esphome rename weird-file.yaml kitchen``
|
||||
# would otherwise just re-flash the same hostname).
|
||||
if new_name == old_name:
|
||||
print(
|
||||
color(
|
||||
AnsiFore.BOLD_RED,
|
||||
f"'{new_name}' is already the device's name.",
|
||||
)
|
||||
)
|
||||
return 1
|
||||
|
||||
new_path: Path = CORE.config_dir / (new_name + ".yaml")
|
||||
if new_path.resolve() == CORE.config_path.resolve():
|
||||
print(
|
||||
|
||||
@@ -3540,6 +3540,129 @@ esp32:
|
||||
assert "already" in captured.out.lower()
|
||||
|
||||
|
||||
def test_command_rename_does_not_touch_friendly_name_substring(
|
||||
tmp_path: Path,
|
||||
mock_run_external_process: Mock,
|
||||
) -> None:
|
||||
"""Test rename does not match the ``name:`` substring of ``friendly_name:``.
|
||||
|
||||
Without anchoring the regex at line start, the pattern
|
||||
``\\s*name:\\s+<old>`` could match the trailing ``name:``
|
||||
substring inside ``friendly_name: <old>``. The rewrite would
|
||||
flip both lines to the new name, leaving the user with a
|
||||
silently corrupted ``friendly_name``.
|
||||
"""
|
||||
config_file = tmp_path / "oldname.yaml"
|
||||
config_file.write_text("""
|
||||
esphome:
|
||||
name: oldname
|
||||
friendly_name: oldname
|
||||
|
||||
esp32:
|
||||
board: nodemcu-32s
|
||||
""")
|
||||
setup_core(tmp_path=tmp_path)
|
||||
CORE.config_path = config_file
|
||||
CORE.config = {CONF_ESPHOME: {CONF_NAME: "oldname"}}
|
||||
|
||||
args = MockArgs(name="newname", dashboard=False)
|
||||
mock_run_external_process.return_value = 0
|
||||
|
||||
result = command_rename(args, {})
|
||||
|
||||
assert result == 0
|
||||
new_file = tmp_path / "newname.yaml"
|
||||
content = new_file.read_text()
|
||||
# esphome.name swapped.
|
||||
assert 'name: "newname"' in content
|
||||
# friendly_name kept verbatim.
|
||||
assert "friendly_name: oldname" in content
|
||||
|
||||
|
||||
def test_command_rename_does_not_match_old_name_as_value_prefix(
|
||||
tmp_path: Path,
|
||||
mock_run_external_process: Mock,
|
||||
) -> None:
|
||||
"""Test rename does not match ``old_name`` as a prefix of a longer value.
|
||||
|
||||
With ``old_name = kitchen`` the value ``kitchen2`` (a sensor
|
||||
or wifi entry) would otherwise match the unanchored
|
||||
``[\\\"']?kitchen[\\\"']?`` pattern at the prefix and get
|
||||
rewritten to the new name. The end-of-value lookahead keeps
|
||||
the match restricted to whole tokens.
|
||||
"""
|
||||
config_file = tmp_path / "kitchen.yaml"
|
||||
config_file.write_text("""
|
||||
esphome:
|
||||
name: kitchen
|
||||
|
||||
esp32:
|
||||
board: nodemcu-32s
|
||||
|
||||
wifi:
|
||||
ap:
|
||||
ssid: kitchen2
|
||||
""")
|
||||
setup_core(tmp_path=tmp_path)
|
||||
CORE.config_path = config_file
|
||||
CORE.config = {CONF_ESPHOME: {CONF_NAME: "kitchen"}}
|
||||
|
||||
args = MockArgs(name="garage", dashboard=False)
|
||||
mock_run_external_process.return_value = 0
|
||||
|
||||
result = command_rename(args, {})
|
||||
|
||||
assert result == 0
|
||||
new_file = tmp_path / "garage.yaml"
|
||||
content = new_file.read_text()
|
||||
assert 'name: "garage"' in content
|
||||
# The wifi ssid value is unrelated and stays intact.
|
||||
assert "ssid: kitchen2" in content
|
||||
|
||||
|
||||
def test_command_rename_same_resolved_name_refuses(
|
||||
tmp_path: Path,
|
||||
capfd: CaptureFixture[str],
|
||||
mock_run_external_process: Mock,
|
||||
) -> None:
|
||||
"""Test rename refuses when ``new_name`` matches the resolved device name.
|
||||
|
||||
The path-equality check only catches the case where the
|
||||
config filename matches the device name. For a config whose
|
||||
filename and ``esphome.name`` differ (here ``weird-file.yaml``
|
||||
holds ``esphome.name: kitchen``), running
|
||||
``esphome rename weird-file.yaml kitchen`` would otherwise
|
||||
fall through to the rewrite + install: the YAML's name stays
|
||||
``kitchen``, the file is renamed to ``kitchen.yaml``, and the
|
||||
device gets a redundant flash. Refuse up-front so the
|
||||
"already the device's name" message matches reality.
|
||||
"""
|
||||
config_file = tmp_path / "weird-file.yaml"
|
||||
config_file.write_text("""
|
||||
esphome:
|
||||
name: kitchen
|
||||
|
||||
esp32:
|
||||
board: nodemcu-32s
|
||||
""")
|
||||
setup_core(tmp_path=tmp_path)
|
||||
CORE.config_path = config_file
|
||||
CORE.config = {CONF_ESPHOME: {CONF_NAME: "kitchen"}}
|
||||
|
||||
args = MockArgs(name="kitchen", dashboard=False)
|
||||
|
||||
result = command_rename(args, {})
|
||||
|
||||
assert result == 1
|
||||
mock_run_external_process.assert_not_called()
|
||||
# Source file untouched, no derived target written.
|
||||
assert config_file.exists()
|
||||
assert not (tmp_path / "kitchen.yaml").exists()
|
||||
|
||||
captured = capfd.readouterr()
|
||||
assert "already" in captured.out.lower()
|
||||
|
||||
|
||||
def test_command_rename_does_not_touch_lookalike_name_in_other_blocks(
|
||||
tmp_path: Path,
|
||||
mock_run_external_process: Mock,
|
||||
|
||||
Reference in New Issue
Block a user