Skip to content
Merged
4 changes: 3 additions & 1 deletion homeassistant/helpers/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mark file as not visible with ., and we should use a context manager to cleanup the temporary file after this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to clean up since we're using os.replace

json.save_json(tmp_path, data, self._private)
os.replace(tmp_path, path)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair points. I'll make it so.


async def _async_migrate_func(self, old_version, old_data):
"""Migrate to the new version."""
Expand Down
6 changes: 5 additions & 1 deletion homeassistant/util/json.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from typing import Union, List, Dict

import json
import os
from os import O_WRONLY, O_CREAT, O_TRUNC

from homeassistant.exceptions import HomeAssistantError

Expand Down Expand Up @@ -46,7 +48,9 @@ def save_json(filename: str, data: Union[List, Dict],
"""
try:
json_data = json.dumps(data, sort_keys=True, indent=4)
with open(filename, 'w', encoding='utf-8') as fdesc:
mode = 0o600 if private else 0o644
with open(os.open(filename, O_WRONLY | O_CREAT | O_TRUNC, mode),
'w', encoding='utf-8') as fdesc:
fdesc.write(json_data)
except TypeError as error:
_LOGGER.exception('Failed to serialize to JSON: %s',
Expand Down
97 changes: 50 additions & 47 deletions tests/components/emulated_hue/test_init.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,25 @@ def test_config_google_home_entity_id_to_number():
handle = mop()

with patch('homeassistant.util.json.open', mop, create=True):
number = conf.entity_id_to_number('light.test')
assert number == '2'
assert handle.write.call_count == 1
assert json.loads(handle.write.mock_calls[0][1][0]) == {
'1': 'light.test2',
'2': 'light.test',
}
with patch('homeassistant.util.json.os.open', return_value=0):
number = conf.entity_id_to_number('light.test')
assert number == '2'
assert handle.write.call_count == 1
assert json.loads(handle.write.mock_calls[0][1][0]) == {
'1': 'light.test2',
'2': 'light.test',
}

number = conf.entity_id_to_number('light.test')
assert number == '2'
assert handle.write.call_count == 1
number = conf.entity_id_to_number('light.test')
assert number == '2'
assert handle.write.call_count == 1

number = conf.entity_id_to_number('light.test2')
assert number == '1'
assert handle.write.call_count == 1
number = conf.entity_id_to_number('light.test2')
assert number == '1'
assert handle.write.call_count == 1

entity_id = conf.number_to_entity_id('1')
assert entity_id == 'light.test2'
entity_id = conf.number_to_entity_id('1')
assert entity_id == 'light.test2'


def test_config_google_home_entity_id_to_number_altered():
Expand All @@ -46,24 +47,25 @@ def test_config_google_home_entity_id_to_number_altered():
handle = mop()

with patch('homeassistant.util.json.open', mop, create=True):
number = conf.entity_id_to_number('light.test')
assert number == '22'
assert handle.write.call_count == 1
assert json.loads(handle.write.mock_calls[0][1][0]) == {
'21': 'light.test2',
'22': 'light.test',
}
with patch('homeassistant.util.json.os.open', return_value=0):
number = conf.entity_id_to_number('light.test')
assert number == '22'
assert handle.write.call_count == 1
assert json.loads(handle.write.mock_calls[0][1][0]) == {
'21': 'light.test2',
'22': 'light.test',
}

number = conf.entity_id_to_number('light.test')
assert number == '22'
assert handle.write.call_count == 1
number = conf.entity_id_to_number('light.test')
assert number == '22'
assert handle.write.call_count == 1

number = conf.entity_id_to_number('light.test2')
assert number == '21'
assert handle.write.call_count == 1
number = conf.entity_id_to_number('light.test2')
assert number == '21'
assert handle.write.call_count == 1

entity_id = conf.number_to_entity_id('21')
assert entity_id == 'light.test2'
entity_id = conf.number_to_entity_id('21')
assert entity_id == 'light.test2'


def test_config_google_home_entity_id_to_number_empty():
Expand All @@ -76,23 +78,24 @@ def test_config_google_home_entity_id_to_number_empty():
handle = mop()

with patch('homeassistant.util.json.open', mop, create=True):
number = conf.entity_id_to_number('light.test')
assert number == '1'
assert handle.write.call_count == 1
assert json.loads(handle.write.mock_calls[0][1][0]) == {
'1': 'light.test',
}

number = conf.entity_id_to_number('light.test')
assert number == '1'
assert handle.write.call_count == 1

number = conf.entity_id_to_number('light.test2')
assert number == '2'
assert handle.write.call_count == 2

entity_id = conf.number_to_entity_id('2')
assert entity_id == 'light.test2'
with patch('homeassistant.util.json.os.open', return_value=0):
number = conf.entity_id_to_number('light.test')
assert number == '1'
assert handle.write.call_count == 1
assert json.loads(handle.write.mock_calls[0][1][0]) == {
'1': 'light.test',
}

number = conf.entity_id_to_number('light.test')
assert number == '1'
assert handle.write.call_count == 1

number = conf.entity_id_to_number('light.test2')
assert number == '2'
assert handle.write.call_count == 2

entity_id = conf.number_to_entity_id('2')
assert entity_id == 'light.test2'


def test_config_alexa_entity_id_to_number():
Expand Down
9 changes: 9 additions & 0 deletions tests/helpers/test_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
MOCK_VERSION = 1
MOCK_KEY = 'storage-test'
MOCK_DATA = {'hello': 'world'}
MOCK_DATA2 = {'goodbye': 'cruel world'}


@pytest.fixture
Expand All @@ -30,6 +31,14 @@ async def test_loading(hass, store):
assert data == MOCK_DATA


async def test_overwriting(hass, store):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This actually doesn't do anything because all writes for the storage helper are mocked in tests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

"""Test we can save, overwrite and reload data."""
await store.async_save(MOCK_DATA)
await store.async_save(MOCK_DATA2)
data = await store.async_load()
assert data == MOCK_DATA2


async def test_loading_non_existing(hass, store):
"""Test we can save and load data."""
with patch('homeassistant.util.json.open', side_effect=FileNotFoundError):
Expand Down