Protect state.as_dict from mutation by balloob · Pull Request #65693 · home-assistant/core

balloob · 2022-02-04T17:34:23Z

Breaking change

Proposed change

State objects cache their as_dict output. @olle69 noticed that the Tuya diagnostics was mutating this, impacting the cache.

This PR makes it immutable and fixes Tuya. Homekit Controller also mutated the dict, but first passed it through redact_data which makes a copy.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-02-04T17:34:29Z

Hey there @tuya, @zlinoliver, @METISU, @frenck, mind taking a look at this pull request as it has been labeled with an integration (tuya) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

bdraco · 2022-02-04T18:16:05Z

Will do some performance testing

bdraco · 2022-02-04T18:17:16Z

Hmm

Not starting up

2022-02-04 18:15:23 ERROR (MainThread) [homeassistant.components.websocket_api.messages] Unable to serialize to JSON. Bad data found at $.result[0](State: person.j_nick_koston)={'entity_id': 'person.j_nick_koston', 'state': 'not_home', 'attributes': .....'context': mappingproxy({'id': '7f293c20e52d8cf128043ac9d695e37f',

bdraco · 2022-02-04T18:18:52Z

The websocket can't serialize the states with the mappingproxytype

bdraco

The frontend can't connect (setup) with this

balloob · 2022-02-04T18:26:25Z

minor issue 🤣 I will be very surprised if the tests pass (they don't).

Jc2k · 2022-02-04T18:27:11Z

homeassistant/components/homekit_controller/diagnostics.py

        state_dict = None
        if state:
-            state_dict = async_redact_data(state.as_dict(), REDACTED_STATE)
+            state_dict = cast(dict, async_redact_data(state.as_dict(), REDACTED_STATE))


I think the typing changes in cc6b0cc are wrong. async_redact_data is returning a dict, not a MappingProxyType, so the cast here is papering over a bug in async_redact_data IMO.

async_redact_data will convert any type inheriting from Mapping to a dict. It's typed to have input = output type.

Let me see if I can fix that.

Initial attempt gives me an error.

@overload def async_redact_data(data: Mapping, to_redact: Iterable[Any]) -> dict: ... @overload def async_redact_data(data: T, to_redact: Iterable[Any]) -> T: ...

homeassistant/components/diagnostics/util.py:15: error: Overloaded function signatures 1 and 2 overlap with incompatible return types [misc]
Found 1 error in 1 file (checked 2 source files)

ignored that error ec868f3 🤷

balloob · 2022-02-04T18:46:34Z

Argh so this is not a trivial issue to fix because the JSONEncoder in Python is hard coded to check isinstance(obj, dict). The only way to go seems to be to subclass dict such that it looks like a dict but blows up when mutated. That would probably silence typing 😬

balloob · 2022-02-04T19:00:11Z

Pulling out the Tuya change into a new PR for the hot fix as this change will be a bit more elaborate.

balloob · 2022-02-04T19:02:14Z

Tuya PR #65708

bdraco · 2022-02-04T19:23:00Z

That looks like it will be more performant as well 👍

Testing now....

bdraco · 2022-02-04T19:25:59Z

So far so good.. 👍

bdraco · 2022-02-04T19:33:54Z

Functional and performance testing looks good. Its effectively imperceptible from a performance standpoint.

bdraco

LGTM sans the typing issues

marvin-w

KNX looks good!

balloob · 2022-02-04T19:50:19Z

I suggest we adopt ReadOnlyDict in future PRs in all places where we use MappingProxy

Protect state.as_dict from mutation

cb3d52e

balloob added this to the 2022.2.2 milestone Feb 4, 2022

balloob requested review from a team and frenck as code owners February 4, 2022 17:34

probot-home-assistant bot added bugfix core integration: tuya small-pr PRs with less than 30 lines. labels Feb 4, 2022

homeassistant added the cla-signed label Feb 4, 2022

frenck approved these changes Feb 4, 2022

View reviewed changes

balloob mentioned this pull request Feb 4, 2022

Add diagnostics support for Asuswrt #65605

Merged

22 tasks

Fix type

6cb6951

balloob requested review from Jc2k and bdraco as code owners February 4, 2022 17:59

bdraco requested changes Feb 4, 2022

View reviewed changes

Jc2k reviewed Feb 4, 2022

View reviewed changes

balloob added 2 commits February 4, 2022 10:49

Fix react_data typing

ec868f3

Also redact mappings stored in dicts

ccf1896

balloob removed this from the 2022.2.2 milestone Feb 4, 2022

Revert tuya

6b2d532

balloob added 2 commits February 4, 2022 11:16

Add ReadOnlyDict

9d05e04

Add test

45e18fb

improve core test

4e9037b

bdraco approved these changes Feb 4, 2022

View reviewed changes

Cleanup types

f5a8a3b

balloob requested review from OttoWinter, epenet, farmio, jesserockz and marvin-w as code owners February 4, 2022 19:35

marvin-w reviewed Feb 4, 2022

View reviewed changes

Shelly types

6d3d600

balloob requested review from bieniu, chemelli74 and thecode as code owners February 4, 2022 19:48

Fix restore cache mocking

89fcca9

balloob merged commit 5da923c into dev Feb 4, 2022

balloob deleted the protect-state-as-dict-mutation branch February 4, 2022 22:45

github-actions bot locked and limited conversation to collaborators Feb 5, 2022

Uh oh!

Conversation

balloob commented Feb 4, 2022

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant bot commented Feb 4, 2022

Uh oh!

bdraco commented Feb 4, 2022

Uh oh!

bdraco commented Feb 4, 2022

Uh oh!

bdraco commented Feb 4, 2022

Uh oh!

bdraco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jc2k Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

balloob Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

balloob Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

balloob Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Feb 4, 2022

Uh oh!

balloob commented Feb 4, 2022

Uh oh!

balloob commented Feb 4, 2022

Uh oh!

bdraco commented Feb 4, 2022

Uh oh!

bdraco commented Feb 4, 2022

Uh oh!

bdraco commented Feb 4, 2022

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

marvin-w left a comment

Choose a reason for hiding this comment

Uh oh!

balloob commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bdraco left a comment •

edited

Loading

balloob commented Feb 4, 2022 •

edited

Loading

balloob Feb 4, 2022 •

edited

Loading