Repository: DMTF/python-redfish-library

Issue:
`get_root_object` call in `RestClientBase` `__init__` method
Hey folks!

We just started a project that is utilizing this library. We notice that there is a network call during instantiation of the object [here](https://github.com/DMTF/python-redfish-library/blob/7c7df89a70d5419d0fed4010b2d8e3668584b7be/src/redfish/rest/v1.py#L500). I want to propose that we make this network call optional, by introducing a keyword argument to the `__init__` method of the redfish client that is defaulted to `True` to preserve the current behavior. Here are a few reasons why:

* If the process that is utilizing this library doesn't know the BMC IP up front it prevents a client from being created.
* If for some reason the BMC IP never returns a response, instantiation _can_ be permanently stuck behind waiting for a response on the socket.
* It's more flexible overall, allows for new/different use-cases, and doesn't impede anyone who is expecting to get that for free.

I'll kick up a PR for this.

Discussion:
@mraineri: I'm certainly open to it and can see the benefits you're pointing out. One thing to be cautious of is the call to `get_service_root` caches the service root info automatically on behalf of the caller. I'd like to ensure that we maintain that automatic caching. Perhaps we can add a second call to it as part of `login` if it wasn't cached previously?@jpavlav: Sounds good! I can definitely add that in. I've already added a basic test for the original intent, but I'll flesh it out a bit so we can make sure we walk the new behavior 👍@jpavlav: @mraineri Kicked up a PR for this. Should be good to go for review 👍 @mraineri: Closing; merged the PR for this request.

Solution patch:
diff --git a/README.rst b/README.rst
index 65ccb77..2a30172 100644
--- a/README.rst
+++ b/README.rst
@@ -91,6 +91,7 @@ There are several optional parameters:
 * ``timeout``: The number of seconds to wait for a response before closing the connection.  The default value is ``None``.
 * ``max_retry``: The number of retries to perform an operation before giving up.  The default value is ``10``.
 * ``proxies``: A dictionary containing protocol to proxy URL mappings.  The default value is ``None``.  See `Using proxies`_.
+* ``check_connectivity``: A boolean value to determine whether the client immediately attempts a connection to the base_url. The default is ``True``.
 
 To create a Redfish object, call the ``redfish_client`` method:
 
diff --git a/src/redfish/rest/v1.py b/src/redfish/rest/v1.py
index db9d0ec..fd54842 100644
--- a/src/redfish/rest/v1.py
+++ b/src/redfish/rest/v1.py
@@ -454,7 +454,7 @@ class RestClientBase(object):
     def __init__(self, base_url, username=None, password=None,
                                 default_prefix='/redfish/v1/', sessionkey=None,
                                 capath=None, cafile=None, timeout=None,
-                                max_retry=None, proxies=None):
+                                max_retry=None, proxies=None, check_connectivity=True):
         """Initialization of the base class RestClientBase
 
         :param base_url: The URL of the remote system
@@ -477,6 +477,9 @@ def __init__(self, base_url, username=None, password=None,
         :type max_retry: int
         :param proxies: Dictionary containing protocol to proxy URL mappings
         :type proxies: dict
+        :param check_connectivity: A boolean to determine whether the client immediately checks for
+        connectivity to the base_url or not.
+        :type check_connectivity: bool
 
         """
 
@@ -497,7 +500,9 @@ def __init__(self, base_url, username=None, password=None,
         self.default_prefix = default_prefix
         self.capath = capath
         self.cafile = cafile
-        self.get_root_object()
+
+        if check_connectivity:
+            self.get_root_object()
 
     def __enter__(self):
         self.login()
@@ -965,6 +970,8 @@ def login(self, username=None, password=None, auth=AuthMethod.SESSION):
         :type auth: object/instance of class AuthMethod
 
         """
+        if getattr(self, "root_resp", None) is None:
+            self.get_root_object()
 
         self.__username = username if username else self.__username
         self.__password = password if password else self.__password
@@ -999,7 +1006,7 @@ def login(self, username=None, password=None, auth=AuthMethod.SESSION):
             message_item = search_message(resp, "Base", "PasswordChangeRequired")
             if not message_item is None:
                 raise RedfishPasswordChangeRequiredError("Password Change Required\n", message_item["MessageArgs"][0])
-            
+
             if not self.__session_key and resp.status not in [200, 201, 202, 204]:
                 if resp.status == 401:
                     # Invalid credentials supplied
@@ -1042,7 +1049,7 @@ def __init__(self, base_url, username=None, password=None,
                                 default_prefix='/redfish/v1/',
                                 sessionkey=None, capath=None,
                                 cafile=None, timeout=None,
-                                max_retry=None, proxies=None):
+                                max_retry=None, proxies=None, check_connectivity=True):
         """Initialize HttpClient
 
         :param base_url: The url of the remote system
@@ -1065,17 +1072,21 @@ def __init__(self, base_url, username=None, password=None,
         :type max_retry: int
         :param proxies: Dictionary containing protocol to proxy URL mappings
         :type proxies: dict
+        :param check_connectivity: A boolean to determine whether the client immediately checks for
+        connectivity to the base_url or not.
+        :type check_connectivity: bool
 
         """
         super(HttpClient, self).__init__(base_url, username=username,
                             password=password, default_prefix=default_prefix,
                             sessionkey=sessionkey, capath=capath,
                             cafile=cafile, timeout=timeout,
-                            max_retry=max_retry, proxies=proxies)
+                            max_retry=max_retry, proxies=proxies,
+                            check_connectivity=check_connectivity)
 
         try:
             self.login_url = self.root.Links.Sessions['@odata.id']
-        except KeyError:
+        except (KeyError, AttributeError):
             # While the "Links/Sessions" property is required, we can fallback
             # on the URI hardened in 1.6.0 of the specification if not found
             LOGGER.debug('"Links/Sessions" not found in Service Root.')
@@ -1128,7 +1139,7 @@ def redfish_client(base_url=None, username=None, password=None,
                                 default_prefix='/redfish/v1/',
                                 sessionkey=None, capath=None,
                                 cafile=None, timeout=None,
-                                max_retry=None, proxies=None):
+                                max_retry=None, proxies=None, check_connectivity=True):
     """Create and return appropriate REDFISH client instance."""
     """ Instantiates appropriate Redfish object based on existing"""
     """ configuration. Use this to retrieve a pre-configured Redfish object
@@ -1153,6 +1164,9 @@ def redfish_client(base_url=None, username=None, password=None,
     :type max_retry: int
     :param proxies: Dictionary containing protocol to proxy URL mappings
     :type proxies: dict
+    :param check_connectivity: A boolean to determine whether the client immediately checks for
+    connectivity to the base_url or not.
+    :type check_connectivity: bool
     :returns: a client object.
 
     """
@@ -1162,4 +1176,4 @@ def redfish_client(base_url=None, username=None, password=None,
     return HttpClient(base_url=base_url, username=username, password=password,
                         default_prefix=default_prefix, sessionkey=sessionkey,
                         capath=capath, cafile=cafile, timeout=timeout,
-                        max_retry=max_retry, proxies=proxies)
+                        max_retry=max_retry, proxies=proxies, check_connectivity=check_connectivity)


Test patch:
diff --git a/tests/rest/test_v1.py b/tests/rest/test_v1.py
index 4171893..c953c04 100644
--- a/tests/rest/test_v1.py
+++ b/tests/rest/test_v1.py
@@ -4,40 +4,76 @@
 # https://github.com/DMTF/python-redfish-library/blob/main/LICENSE.md
 
 # -*- encoding: utf-8 -*-
+import json
 import unittest
+from unittest import mock
 
-from redfish.rest.v1 import HttpClient
-from redfish.rest.v1 import RetriesExhaustedError
-from redfish.rest.v1 import redfish_client
+from redfish.rest.v1 import HttpClient, RetriesExhaustedError, redfish_client
 
 
 class TestRedFishClient(unittest.TestCase):
-    def test_redfish_client(self):
-        base_url = "http://foo.bar"
-        username = "rstallman"
-        password = "123456"
-        default_prefix = "/custom/redfish/v1/"
-        sessionkey = "fg687glgkf56vlgkf"
-        capath = "/path/to/the/dir"
-        cafile = "filename.test"
-        timeout = 666
-        max_retry = 42
+    def setUp(self) -> None:
+        self.base_url = "http://foo.bar"
+        self.username = "rstallman"
+        self.password = "123456"
+        self.default_prefix = "/custom/redfish/v1/"
+        self.sessionkey = "fg687glgkf56vlgkf"
+        self.capath = "/path/to/the/dir"
+        self.cafile = "filename.test"
+        self.timeout = 666
+        self.max_retry = 42
+
+    def test_redfish_client(self) -> None:
         # NOTE(hberaud) the client try to connect when we initialize the
         # http client object so we need to catch the retries exception first.
         # In a second time we need to mock the six.http_client to simulate
         # server responses and do some other tests
         with self.assertRaises(RetriesExhaustedError):
-            client = redfish_client(base_url=base_url)
+            client = redfish_client(base_url=self.base_url)
             # Check the object type
             self.assertTrue(isinstance(client, HttpClient))
             # Check the object attributes values.
             # Here we check if the client object is properly initialized
-            self.assertEqual(client.base_url, base_url)
-            self.assertEqual(client.username, username)
-            self.assertEqual(client.password, password)
-            self.assertEqual(client.default_prefix, default_prefix)
-            self.assertEqual(client.sessionkey, sessionkey)
-            self.assertEqual(client.capath, capath)
-            self.assertEqual(client.cafile, cafile)
-            self.assertEqual(client.timeout, timeout)
-            self.assertEqual(client.max_retry, max_retry)
+            self.assertEqual(client.base_url, self.base_url)
+            self.assertEqual(client.username, self.username)
+            self.assertEqual(client.password, self.password)
+            self.assertEqual(client.default_prefix, self.default_prefix)
+            self.assertEqual(client.sessionkey, self.sessionkey)
+            self.assertEqual(client.capath, self.capath)
+            self.assertEqual(client.cafile, self.cafile)
+            self.assertEqual(client.timeout, self.timeout)
+            self.assertEqual(client.max_retry, self.max_retry)
+
+    def test_redfish_client_no_root_resp(self) -> None:
+        client = redfish_client(base_url=self.base_url, check_connectivity=False)
+        self.assertIsNone(getattr(client, "root_resp", None))
+
+    @mock.patch("requests.Session.request")
+    def test_redfish_client_root_object_initialized_after_login(
+        self, mocked_request: mock.Mock
+    ) -> None:
+        dummy_root_data = '{"Links": {"Sessions": {"@data.id": "/redfish/v1/SessionService/Sessions"}}}'
+        dummy_session_response = (
+            '{"@odata.type": "#Session.v1_1_2.Session", '
+            '"@odata.id": "/redfish/v1/SessionService/Sessions/1", '
+            '"Id": "1", "Name": "User Session", "Description": "Manager User Session", '
+            '"UserName": "user", "Oem": {}}'
+        )
+        root_resp = mock.Mock(content=dummy_root_data, status_code=200)
+        auth_resp = mock.Mock(
+            content=dummy_session_response,
+            status_code=200,
+            headers={"location": "fake", "x-auth-token": "fake"},
+        )
+        mocked_request.side_effect = [
+            root_resp,
+            auth_resp,
+        ]
+        client = redfish_client(base_url=self.base_url, check_connectivity=False)
+        client.login()
+
+        self.assertEqual(client.root, json.loads(dummy_root_data))
+
+
+if __name__ == "__main__":
+    unittest.main()