Make rest sensor and binary sensor more efficient (#14484)

* create binary sensor even if initial update fails

* fixed broken test assertion

* fixed broken test assertion

* avoid fetching resource twice - manually in the setup_platform and then through add_devices

* raising PlatformNotReady instead of creating the sensor if the initial rest call fails; throttling the update to avoid fetching the same resource again immediately after setting up sensor

* rolled back throttling of the rest update call; can still avoid updating the binary sensor's rest resoure twice; fixed tests

* typo

homeassistant/components/binary_sensor/rest.py

@@ -18,6 +18,7 @@ from homeassistant.const import (
     CONF_HEADERS, CONF_AUTHENTICATION, HTTP_BASIC_AUTHENTICATION,
     HTTP_DIGEST_AUTHENTICATION, CONF_DEVICE_CLASS)
 import homeassistant.helpers.config_validation as cv
+from homeassistant.exceptions import PlatformNotReady
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -66,13 +67,13 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
 
     rest = RestData(method, resource, auth, headers, payload, verify_ssl)
     rest.update()
-
     if rest.data is None:
-        _LOGGER.error("Unable to fetch REST data from %s", resource)
-        return False
+        raise PlatformNotReady
 
+    # No need to update the sensor now because it will determine its state
+    # based in the rest resource that has just been retrieved.
     add_entities([RestBinarySensor(
-        hass, rest, name, device_class, value_template)], True)
+        hass, rest, name, device_class, value_template)])
 
 
 class RestBinarySensor(BinarySensorDevice):

homeassistant/components/sensor/rest.py

@@ -18,6 +18,7 @@ from homeassistant.const import (
     CONF_UNIT_OF_MEASUREMENT, CONF_USERNAME,
     CONF_VALUE_TEMPLATE, CONF_VERIFY_SSL,
     HTTP_BASIC_AUTHENTICATION, HTTP_DIGEST_AUTHENTICATION, STATE_UNKNOWN)
+from homeassistant.exceptions import PlatformNotReady
 from homeassistant.helpers.entity import Entity
 import homeassistant.helpers.config_validation as cv
 
@@ -76,7 +77,11 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
         auth = None
     rest = RestData(method, resource, auth, headers, payload, verify_ssl)
     rest.update()
+    if rest.data is None:
+        raise PlatformNotReady
 
+    # Must update the sensor now (including fetching the rest resource) to
+    # ensure it's updating its state.
     add_entities([RestSensor(
         hass, rest, name, unit, value_template, json_attrs, force_update
     )], True)
@@ -170,6 +175,7 @@ class RestData:
 
     def update(self):
         """Get the latest data from REST service with provided method."""
+        _LOGGER.debug("Updating from %s", self._request.url)
         try:
             with requests.Session() as sess:
                 response = sess.send(

tests/components/binary_sensor/test_rest.py

@@ -1,11 +1,13 @@
 """The tests for the REST binary sensor platform."""
 import unittest
+from pytest import raises
 from unittest.mock import patch, Mock
 
 import requests
 from requests.exceptions import Timeout, MissingSchema
 import requests_mock
 
+from homeassistant.exceptions import PlatformNotReady
 from homeassistant.setup import setup_component
 import homeassistant.components.binary_sensor as binary_sensor
 import homeassistant.components.binary_sensor.rest as rest
@@ -18,9 +20,18 @@ from tests.common import get_test_home_assistant, assert_setup_component
 class TestRestBinarySensorSetup(unittest.TestCase):
     """Tests for setting up the REST binary sensor platform."""
 
+    DEVICES = []
+
+    def add_devices(self, devices, update_before_add=False):
+        """Mock add devices."""
+        for device in devices:
+            self.DEVICES.append(device)
+
     def setUp(self):
         """Set up things to be run when tests are started."""
         self.hass = get_test_home_assistant()
+        # Reset for this test.
+        self.DEVICES = []
 
     def tearDown(self):
         """Stop everything that was started."""
@@ -45,76 +56,80 @@ class TestRestBinarySensorSetup(unittest.TestCase):
            side_effect=requests.exceptions.ConnectionError())
     def test_setup_failed_connect(self, mock_req):
         """Test setup when connection error occurs."""
-        self.assertFalse(rest.setup_platform(self.hass, {
-            'platform': 'rest',
-            'resource': 'http://localhost',
-        }, lambda devices, update=True: None))
+        with raises(PlatformNotReady):
+            rest.setup_platform(self.hass, {
+                'platform': 'rest',
+                'resource': 'http://localhost',
+            }, self.add_devices, None)
+        self.assertEqual(len(self.DEVICES), 0)
 
     @patch('requests.Session.send', side_effect=Timeout())
     def test_setup_timeout(self, mock_req):
         """Test setup when connection timeout occurs."""
-        self.assertFalse(rest.setup_platform(self.hass, {
-            'platform': 'rest',
-            'resource': 'http://localhost',
-        }, lambda devices, update=True: None))
+        with raises(PlatformNotReady):
+            rest.setup_platform(self.hass, {
+                'platform': 'rest',
+                'resource': 'http://localhost',
+            }, self.add_devices, None)
+        self.assertEqual(len(self.DEVICES), 0)
 
     @requests_mock.Mocker()
     def test_setup_minimum(self, mock_req):
         """Test setup with minimum configuration."""
         mock_req.get('http://localhost', status_code=200)
-        self.assertTrue(setup_component(self.hass, 'binary_sensor', {
-            'binary_sensor': {
-                'platform': 'rest',
-                'resource': 'http://localhost'
-            }
-        }))
-        self.assertEqual(2, mock_req.call_count)
-        assert_setup_component(1, 'switch')
+        with assert_setup_component(1, 'binary_sensor'):
+            self.assertTrue(setup_component(self.hass, 'binary_sensor', {
+                'binary_sensor': {
+                    'platform': 'rest',
+                    'resource': 'http://localhost'
+                }
+            }))
+        self.assertEqual(1, mock_req.call_count)
 
     @requests_mock.Mocker()
     def test_setup_get(self, mock_req):
         """Test setup with valid configuration."""
         mock_req.get('http://localhost', status_code=200)
-        self.assertTrue(setup_component(self.hass, 'binary_sensor', {
-            'binary_sensor': {
-                'platform': 'rest',
-                'resource': 'http://localhost',
-                'method': 'GET',
-                'value_template': '{{ value_json.key }}',
-                'name': 'foo',
-                'unit_of_measurement': 'MB',
-                'verify_ssl': 'true',
-                'authentication': 'basic',
-                'username': 'my username',
-                'password': 'my password',
-                'headers': {'Accept': 'application/json'}
-            }
-        }))
-        self.assertEqual(2, mock_req.call_count)
-        assert_setup_component(1, 'binary_sensor')
+        with assert_setup_component(1, 'binary_sensor'):
+            self.assertTrue(setup_component(self.hass, 'binary_sensor', {
+                'binary_sensor': {
+                    'platform': 'rest',
+                    'resource': 'http://localhost',
+                    'method': 'GET',
+                    'value_template': '{{ value_json.key }}',
+                    'name': 'foo',
+                    'unit_of_measurement': 'MB',
+                    'verify_ssl': 'true',
+                    'authentication': 'basic',
+                    'username': 'my username',
+                    'password': 'my password',
+                    'headers': {'Accept': 'application/json'}
+                }
+            }))
+        self.assertEqual(1, mock_req.call_count)
 
     @requests_mock.Mocker()
     def test_setup_post(self, mock_req):
         """Test setup with valid configuration."""
         mock_req.post('http://localhost', status_code=200)
-        self.assertTrue(setup_component(self.hass, 'binary_sensor', {
-            'binary_sensor': {
-                'platform': 'rest',
-                'resource': 'http://localhost',
-                'method': 'POST',
-                'value_template': '{{ value_json.key }}',
-                'payload': '{ "device": "toaster"}',
-                'name': 'foo',
-                'unit_of_measurement': 'MB',
-                'verify_ssl': 'true',
-                'authentication': 'basic',
-                'username': 'my username',
-                'password': 'my password',
-                'headers': {'Accept': 'application/json'}
-            }
-        }))
-        self.assertEqual(2, mock_req.call_count)
-        assert_setup_component(1, 'binary_sensor')
+        with assert_setup_component(1, 'binary_sensor'):
+            self.assertTrue(setup_component(self.hass, 'binary_sensor', {
+                'binary_sensor': {
+                    'platform': 'rest',
+                    'resource': 'http://localhost',
+                    'method': 'POST',
+                    'value_template': '{{ value_json.key }}',
+                    'payload': '{ "device": "toaster"}',
+                    'name': 'foo',
+                    'unit_of_measurement': 'MB',
+                    'verify_ssl': 'true',
+                    'authentication': 'basic',
+                    'username': 'my username',
+                    'password': 'my password',
+                    'headers': {'Accept': 'application/json'}
+                }
+            }))
+        self.assertEqual(1, mock_req.call_count)
 
 
 class TestRestBinarySensor(unittest.TestCase):

tests/components/sensor/test_rest.py

@@ -1,11 +1,13 @@
 """The tests for the REST sensor platform."""
 import unittest
+from pytest import raises
 from unittest.mock import patch, Mock
 
 import requests
 from requests.exceptions import Timeout, MissingSchema, RequestException
 import requests_mock
 
+from homeassistant.exceptions import PlatformNotReady
 from homeassistant.setup import setup_component
 import homeassistant.components.sensor as sensor
 import homeassistant.components.sensor.rest as rest
@@ -45,76 +47,78 @@ class TestRestSensorSetup(unittest.TestCase):
            side_effect=requests.exceptions.ConnectionError())
     def test_setup_failed_connect(self, mock_req):
         """Test setup when connection error occurs."""
-        self.assertTrue(rest.setup_platform(self.hass, {
-            'platform': 'rest',
-            'resource': 'http://localhost',
-        }, lambda devices, update=True: None) is None)
+        with raises(PlatformNotReady):
+            rest.setup_platform(self.hass, {
+                'platform': 'rest',
+                'resource': 'http://localhost',
+            }, lambda devices, update=True: None)
 
     @patch('requests.Session.send', side_effect=Timeout())
     def test_setup_timeout(self, mock_req):
         """Test setup when connection timeout occurs."""
-        self.assertTrue(rest.setup_platform(self.hass, {
-            'platform': 'rest',
-            'resource': 'http://localhost',
-        }, lambda devices, update=True: None) is None)
+        with raises(PlatformNotReady):
+            rest.setup_platform(self.hass, {
+                'platform': 'rest',
+                'resource': 'http://localhost',
+            }, lambda devices, update=True: None)
 
     @requests_mock.Mocker()
     def test_setup_minimum(self, mock_req):
         """Test setup with minimum configuration."""
         mock_req.get('http://localhost', status_code=200)
-        self.assertTrue(setup_component(self.hass, 'sensor', {
-            'sensor': {
-                'platform': 'rest',
-                'resource': 'http://localhost'
-            }
-        }))
+        with assert_setup_component(1, 'sensor'):
+            self.assertTrue(setup_component(self.hass, 'sensor', {
+                'sensor': {
+                    'platform': 'rest',
+                    'resource': 'http://localhost'
+                }
+            }))
         self.assertEqual(2, mock_req.call_count)
-        assert_setup_component(1, 'switch')
 
     @requests_mock.Mocker()
     def test_setup_get(self, mock_req):
         """Test setup with valid configuration."""
         mock_req.get('http://localhost', status_code=200)
-        self.assertTrue(setup_component(self.hass, 'sensor', {
-            'sensor': {
-                'platform': 'rest',
-                'resource': 'http://localhost',
-                'method': 'GET',
-                'value_template': '{{ value_json.key }}',
-                'name': 'foo',
-                'unit_of_measurement': 'MB',
-                'verify_ssl': 'true',
-                'authentication': 'basic',
-                'username': 'my username',
-                'password': 'my password',
-                'headers': {'Accept': 'application/json'}
-            }
-        }))
+        with assert_setup_component(1, 'sensor'):
+            self.assertTrue(setup_component(self.hass, 'sensor', {
+                'sensor': {
+                    'platform': 'rest',
+                    'resource': 'http://localhost',
+                    'method': 'GET',
+                    'value_template': '{{ value_json.key }}',
+                    'name': 'foo',
+                    'unit_of_measurement': 'MB',
+                    'verify_ssl': 'true',
+                    'authentication': 'basic',
+                    'username': 'my username',
+                    'password': 'my password',
+                    'headers': {'Accept': 'application/json'}
+                }
+            }))
         self.assertEqual(2, mock_req.call_count)
-        assert_setup_component(1, 'sensor')
 
     @requests_mock.Mocker()
     def test_setup_post(self, mock_req):
         """Test setup with valid configuration."""
         mock_req.post('http://localhost', status_code=200)
-        self.assertTrue(setup_component(self.hass, 'sensor', {
-            'sensor': {
-                'platform': 'rest',
-                'resource': 'http://localhost',
-                'method': 'POST',
-                'value_template': '{{ value_json.key }}',
-                'payload': '{ "device": "toaster"}',
-                'name': 'foo',
-                'unit_of_measurement': 'MB',
-                'verify_ssl': 'true',
-                'authentication': 'basic',
-                'username': 'my username',
-                'password': 'my password',
-                'headers': {'Accept': 'application/json'}
-            }
-        }))
+        with assert_setup_component(1, 'sensor'):
+            self.assertTrue(setup_component(self.hass, 'sensor', {
+                'sensor': {
+                    'platform': 'rest',
+                    'resource': 'http://localhost',
+                    'method': 'POST',
+                    'value_template': '{{ value_json.key }}',
+                    'payload': '{ "device": "toaster"}',
+                    'name': 'foo',
+                    'unit_of_measurement': 'MB',
+                    'verify_ssl': 'true',
+                    'authentication': 'basic',
+                    'username': 'my username',
+                    'password': 'my password',
+                    'headers': {'Accept': 'application/json'}
+                }
+            }))
         self.assertEqual(2, mock_req.call_count)
-        assert_setup_component(1, 'sensor')
 
 
 class TestRestSensor(unittest.TestCase):