Added working support for private storage #16903
Conversation
|
Sorry, I removed some of your changes in #16909 to get rid of CI failures. Please add them back in this PR :) |
|
@amelchio I fixed the merge conflict but it seems that you also reverted a fix to
8000
a broken test file ( |
|
OK, I just added the file back at my end and pushed it again. It seems to work now. |
homeassistant/helpers/storage.py
Outdated
| @@ -187,7 +187,9 @@ def _write_data(self, path: str, data: Dict): | |||
| os.makedirs(os.path.dirname(path)) | |||
|
|
|||
| _LOGGER.debug('Writing data for %s', self.key) | |||
| json.save_json(path, data, self._private) | |||
| tmp_path = path + "__TEMP__" | |||
There was a problem hiding this comment.
Mark file as not visible with ., and we sh
8000
ould use a context manager to cleanup the temporary file after this.
There was a problem hiding this comment.
No need to clean up since we're using os.replace
|
@nickovs Yes, fine to handle the revert like that 👍. You can push additional commits until the PR is merged, that will also work to address review comments. |
tests/helpers/test_storage.py
Outdated
| @@ -30,6 +31,14 @@ def store(hass): | |||
| assert data == MOCK_DATA | |||
|
|
|||
|
|
|||
| async def test_overwriting(hass, store): | |||
There was a problem hiding this comment.
This actually doesn't do anything because all writes for the storage helper are mocked in tests
There was a problem hiding this comment.
Aha. Does that mean that the current storage implementation does not get tested down the filesystem level with the existing unit tests? If that's the case, is it fine to leave this out altogether?
There was a problem hiding this comment.
Correct, we try to avoid I/O as much as possible in unit tests as it's slow.
We should still test this functionality but it should test util.save_json directly, as that one is not mocked out.
tests/util/test_json.py
Outdated
| with open(fname, "w") as fh: | ||
| fh.write(TEST_BAD_SERIALIED) | ||
| with self.assertRaises(HomeAssistantError): | ||
| data = load_json(fname) |
There was a problem hiding this comment.
local variable 'data' is assigned to but never used
tests/util/test_json.py
Outdated
| save_json(fname, TEST_JSON_B) | ||
| data = load_json(fname) | ||
| self.assertEqual(data, TEST_JSON_B) | ||
|
|
tests/util/test_json.py
Outdated
| for fname in os.listdir(self.tmp_dir): | ||
| os.remove(os.path.join(self.tmp_dir, fname)) | ||
| os.rmdir(self.tmp_dir) | ||
|
|
| # Test data that can not be loaded as JSON | ||
| TEST_BAD_SERIALIED = "THIS IS NOT JSON\n" | ||
|
|
||
| class TestJSON(unittest.TestCase): |
tests/util/test_json.py
Outdated
| TEST_JSON_A = {"a":1, "B":"two"} | ||
| TEST_JSON_B = {"a":"one", "B":2} | ||
| # Test data that can not be saved as JSON (keys must be strings) | ||
| TEST_BAD_OBJECT = {("A",):1} |
tests/util/test_json.py
Outdated
|
|
||
| # Test data that can be saved as JSON | ||
| TEST_JSON_A = {"a":1, "B":"two"} | ||
| TEST_JSON_B = {"a":"one", "B":2} |
tests/util/test_json.py
Outdated
| from homeassistant.exceptions import HomeAssistantError | ||
|
|
||
| # Test data that can be saved as JSON | ||
| TEST_JSON_A = {"a":1, "B":"two"} |
tests/util/test_json.py
Outdated
| import sys | ||
| from tempfile import mkdtemp | ||
|
|
||
| from homeassistant.util.json import (SerializationError, WriteError, |
There was a problem hiding this comment.
'homeassistant.util.json.WriteError' imported but unused
tests/util/test_json.py
Outdated
| TEST_JSON_A = {"a" : 1, "B" : "two"} | ||
| TEST_JSON_B = {"a" : "one", "B" : 2} | ||
| # Test data that can not be saved as JSON (keys must be strings) | ||
| TEST_BAD_OBJECT = {("A",) : 1} |
tests/util/test_json.py
Outdated
|
|
||
| # Test data that can be saved as JSON | ||
| TEST_JSON_A = {"a" : 1, "B" : "two"} | ||
| TEST_JSON_B = {"a" : "one", "B" : 2} |
tests/util/test_json.py
Outdated
| from homeassistant.exceptions import HomeAssistantError | ||
|
|
||
| # Test data that can be saved as JSON | ||
| TEST_JSON_A = {"a" : 1, "B" : "two"} |
|
OK. I wrote a test suite for the underlying |
homeassistant/helpers/storage.py
Outdated
| json.save_json(path, data, self._private) | ||
| tmp_path = path + "__TEMP__" | ||
| json.save_json(tmp_path, data, self._private) | ||
| os.replace(tmp_path, path) |
There was a problem hiding this comment.
I think that we should move this functionality to save_json so that all calls can benefit.
I also think that we should consider cleaning up the temp path in case of an exception replacing the other file
There was a problem hiding this comment.
Fair points. I'll make it so.
Added extra clean-up code to ensure that temporary files are removed in case of errors. Updated emulated_hue unit tests to avoid errors.
|
Alright let's try again, great work! 👍 |
…ous writes Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py
…7636) Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py
…7636) Reworked tests/components/emulated_hue/test_init.py to not be dependent on the specific internal implementation of util/jsonn.py
Description:
This is a fixed version of #16878. It adds support for marking
Storeobjects objects as private to prevent data exposure.Compared to the original PR it:
utils.json.save_json()helpers.storage.Storeclass to prevent file corruption if the system crashes while writing a storage file.test/helpers/test_storage.pyto test that updating an existing storage file works correctly.Related issue (if applicable): fixes #16349
Pull request in home-assistant.io with documentation (if applicable): N/A
Example entry for
configuration.yaml(if applicable):N/A
Checklist:
tox.If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example). N/Arequirements_all.txtby runningscript/gen_requirements_all.py. N/A.coveragerc. N/AIf the code does not interact with devices: