Repository: dwavesystems/dwave-cloud-client
Pull request #93 (2018-04-09 23:04:45)

Better control over config loading
Legacy config skip flag, explicit error when config load fails.


Patch:
diff --git a/dwave/cloud/client.py b/dwave/cloud/client.py
index c1b5114..cbafdbd 100644
--- a/dwave/cloud/client.py
+++ b/dwave/cloud/client.py
@@ -67,12 +67,19 @@ class Client(object):
     @classmethod
     def from_config(cls, config_file=None, profile=None, client=None,
                     endpoint=None, token=None, solver=None, proxy=None,
-                    **kwargs):
+                    legacy_config_fallback=True, **kwargs):
         """Client factory method which loads configuration from file(s),
         process environment variables and explicitly provided values, creating
         and returning the appropriate client instance
         (:class:`dwave.cloud.qpu.Client` or :class:`dwave.cloud.sw.Client`).
 
+        Args:
+            TODO
+
+            legacy_config_fallback (bool, default=True):
+                If loading from a ``dwave.conf`` config file fails, try
+                loading the ``.dwrc`` legacy config.
+
         Example:
             Create ``dwave.conf`` in your current directory or
             ``~/.config/dwave/dwave.conf``::
@@ -90,6 +97,13 @@ class Client(object):
                     computation = solver.sample_ising({}, {})
                     samples = computation.result()
 
+        Raises:
+            :exc:`~dwave.cloud.exceptions.ConfigFileReadError`:
+                Config file specified or detected could not be opened or read.
+
+            :exc:`~dwave.cloud.exceptions.ConfigFileParseError`:
+                Config file parse failed.
+
         TODO: describe config loading, new config in broad strokes, refer to
         actual loaders' doc; include examples for config and usage.
 
@@ -108,8 +122,9 @@ class Client(object):
                 endpoint=endpoint, token=token, solver=solver, proxy=proxy,
                 client=client)
 
-        # and failback to the legacy `.dwrc`
-        if config.get('token') is None or config.get('endpoint') is None:
+        # and fallback to the legacy `.dwrc`
+        if legacy_config_fallback and (
+                config.get('token') is None or config.get('endpoint') is None):
             try:
                 _endpoint, _token, _proxy, _solver = legacy_load_config(
                     key=profile,
diff --git a/dwave/cloud/config.py b/dwave/cloud/config.py
index 9d50d1e..1593e5c 100644
--- a/dwave/cloud/config.py
+++ b/dwave/cloud/config.py
@@ -2,6 +2,7 @@ import os
 import configparser
 import homebase
 
+from dwave.cloud.exceptions import ConfigFileReadError, ConfigFileParseError
 
 CONF_APP = "dwave"
 CONF_AUTHOR = "dwavesystem"
@@ -102,22 +103,29 @@ def load_config_from_file(filename=None):
 
     Raises:
         :exc:`ValueError`:
-            Config file not found, or format invalid (parsing failed).
+            Config file location unspecified and undetected.
+
+        :exc:`~dwave.cloud.exceptions.ConfigFileReadError`:
+            Config file specified or detected could not be opened or read.
+
+        :exc:`~dwave.cloud.exceptions.ConfigFileParseError`:
+            Config file parse failed.
     """
     if filename is None:
         filename = detect_configfile_path()
         if not filename:
-            raise ValueError("Config filename not given, and could not be detected")
+            raise ValueError("Config file not given, and could not be detected")
 
     config = configparser.ConfigParser(default_section="defaults")
     try:
-        files_read = config.read(filename)
-    except configparser.Error:
-        files_read = []
+        with open(filename, 'r') as f:
+            config.read_file(f, filename)
 
-    if not files_read:
-        raise ValueError("Failed to parse the config file "\
-                         "given or detected: {}".format(filename))
+    except (IOError, OSError):
+        raise ConfigFileReadError("Failed to read {!r}".format(filename))
+
+    except configparser.Error:
+        raise ConfigFileParseError("Failed to parse {!r}".format(filename))
 
     return config
 
@@ -173,6 +181,23 @@ def load_config(config_file=None, profile=None, client=None,
     config dirs). For details on format and location detection, see
     :func:`load_config_from_file`.
 
+    If location of ``config_file`` is explicitly specified (via arguments or
+    environment variable), but the file does not exits, or is not readable,
+    config loading will fail with
+    :exc:`~dwave.cloud.exceptions.ConfigFileReadError`. Config loading will fail
+    with :exc:`~dwave.cloud.exceptions.ConfigFileParseError` if file is
+    readable, but it's not a valid config file.
+
+    Similarly, if ``profile`` is explicitly specified (via arguments or
+    environment variable), config loading will fail with :exc:`ValueError` if
+    that profile is not present in the config loaded. If config file is not
+    specified explicitly, nor detected on file system, not defined via
+    environment, resulting in an empty config, explicit profile selection will
+    also fail.
+
+    If profile is not explicitly specified, selection of profile is described
+    under ``profile`` argument below.
+
     Environment variables:
 
         ``DWAVE_CONFIG_FILE``:
@@ -256,28 +281,33 @@ def load_config(config_file=None, profile=None, client=None,
                 }
 
     Raises:
-        Nothing, except in unexpected circumstances.
-            Cases of invalid config file, file/disk read error, invalid
-            environment values, etc. are handled and result in `None` values
-            for one or all of the keys in the result.
+        :exc:`ValueError`:
+            Invalid (non-existing) profile name.
+
+        :exc:`~dwave.cloud.exceptions.ConfigFileReadError`:
+            Config file specified or detected could not be opened or read.
 
+        :exc:`~dwave.cloud.exceptions.ConfigFileParseError`:
+            Config file parse failed.
     """
-    # load config file
-    # lookup priority: explicitly specified, environment specified, auto-detected
     if config_file is None:
         config_file = os.getenv("DWAVE_CONFIG_FILE")
     try:
         config = load_config_from_file(config_file)
-        # last-resort profile name:
+        # determine profile name fallback:
         #  (1) profile key under [defaults],
         #  (2) first non-[defaults] section
         first_section = next(iter(config.sections() + [None]))
         config_defaults = config.defaults()
         default_profile = config_defaults.get('profile', first_section)
     except ValueError:
+        # config file not specified, or not detected: start with null-config
         config = {}
         config_defaults = {}
         default_profile = None
+    except (ConfigFileReadError, ConfigFileParseError):
+        # unable to access/read/parse config file(s): explicitly fail
+        raise
 
     if profile is None:
         profile = os.getenv("DWAVE_PROFILE", default_profile)
@@ -285,7 +315,7 @@ def load_config(config_file=None, profile=None, client=None,
         try:
             section = dict(config[profile])
         except KeyError:
-            raise ValueError("Profile {!r} not defined in config file".format(profile))
+            raise ValueError("Config profile {!r} not found".format(profile))
     else:
         # as the very last resort (unspecified profile name and
         # no profiles defined in config), try to use [defaults]
@@ -294,7 +324,7 @@ def load_config(config_file=None, profile=None, client=None,
         else:
             section = {}
 
-    # override (or defined) a selected subset of values via env or kwargs,
+    # override a selected subset of values via env or kwargs,
     # pass-through the rest unmodified
     section['client'] = client or os.getenv("DWAVE_API_CLIENT", section.get('client'))
     section['endpoint'] = endpoint or os.getenv("DWAVE_API_ENDPOINT", section.get('endpoint'))
@@ -417,4 +447,4 @@ def legacy_load_config(key=None, endpoint=None, token=None, solver=None, proxy=N
             pass  # Just ignore any malformed lines
             # TODO issue a warning
 
-    raise ValueError("No configuration for the connection could be discovered.")
+    raise ValueError("No configuration for the client could be discovered.")
diff --git a/dwave/cloud/exceptions.py b/dwave/cloud/exceptions.py
index dd72ce4..3d0709c 100644
--- a/dwave/cloud/exceptions.py
+++ b/dwave/cloud/exceptions.py
@@ -1,3 +1,11 @@
+class ConfigFileReadError(Exception):
+    """Non-existing or unreadable config file specified or implied."""
+
+
+class ConfigFileParseError(Exception):
+    """Invalid format of config file."""
+
+
 class SolverError(Exception):
     """Generic base class for all solver-related errors."""
 


Test patch:
diff --git a/tests/test_client.py b/tests/test_client.py
index a68187c..8012a42 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -109,6 +109,21 @@ class ClientFactory(unittest.TestCase):
                 init.assert_called_once_with(
                     endpoint='endpoint', token='token', custom='new-custom')
 
+    def test_legacy_config_load_fallback(self):
+        conf = 'endpoint token proxy solver'.split()
+        with mock.patch("dwave.cloud.client.load_config", return_value={}):
+            with mock.patch("dwave.cloud.client.legacy_load_config", lambda **kwargs: conf):
+                # test fallback works (legacy config is loaded)
+                with dwave.cloud.Client.from_config(legacy_config_fallback=True) as client:
+                    self.assertEqual(client.endpoint, 'endpoint')
+                    self.assertEqual(client.token, 'token')
+                    self.assertEqual(client.default_solver, 'solver')
+                    self.assertEqual(client.session.proxies['http'], 'proxy')
+
+                # test fallback is avoided (legacy config skipped)
+                self.assertRaises(
+                    ValueError, dwave.cloud.Client.from_config, legacy_config_fallback=False)
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/test_config.py b/tests/test_config.py
index 79f42d1..671c29b 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -4,9 +4,10 @@ import os
 import sys
 import unittest
 import configparser
-
 from functools import partial
 
+from dwave.cloud.exceptions import ConfigFileParseError, ConfigFileReadError
+
 try:
     # python 3
     import unittest.mock as mock
@@ -64,7 +65,7 @@ class TestConfig(unittest.TestCase):
         return config
 
     def test_config_load_from_file(self):
-        with mock.patch(configparser_open_namespace, iterable_mock_open(self.config_body), create=True):
+        with mock.patch('dwave.cloud.config.open', iterable_mock_open(self.config_body), create=True):
             config = load_config_from_file(filename="filename")
             self.assertEqual(config.sections(), ['dw2000', 'software', 'alpha'])
             self.assertEqual(config['dw2000']['client'], 'qpu')
@@ -78,26 +79,27 @@ class TestConfig(unittest.TestCase):
                 os.environ.pop(key, None)
 
     def test_config_load_from_file__invalid_format__duplicate_sections(self):
-        """Config loading should fail with `ValueError` on file load error."""
+        """Config loading should fail with ``ConfigFileParseError`` for invalid
+        config files."""
         myconfig = u"""
             [section]
             key = val
             [section]
             key = val
         """
-        with mock.patch(configparser_open_namespace, iterable_mock_open(myconfig), create=True):
-            self.assertRaises(ValueError, load_config_from_file, filename="filename")
-            self.assertRaises(ValueError, load_config, config_file="filename", profile="section")
+        with mock.patch('dwave.cloud.config.open', iterable_mock_open(myconfig), create=True):
+            self.assertRaises(ConfigFileParseError, load_config_from_file, filename="filename")
+            self.assertRaises(ConfigFileParseError, load_config, config_file="filename", profile="section")
 
     def test_no_config_detected(self):
         with mock.patch("dwave.cloud.config.detect_configfile_path", lambda: None):
             self.assertRaises(ValueError, load_config_from_file)
 
     def test_invalid_filename_given(self):
-        self.assertRaises(ValueError, load_config_from_file, filename='/path/to/non/existing/config')
+        self.assertRaises(ConfigFileReadError, load_config_from_file, filename='/path/to/non/existing/config')
 
     def test_config_load_profile(self):
-        with mock.patch(configparser_open_namespace, iterable_mock_open(self.config_body), create=True):
+        with mock.patch('dwave.cloud.config.open', iterable_mock_open(self.config_body), create=True):
             profile = load_profile(name="alpha", filename="filename")
             self.assertEqual(profile['token'], 'alpha-token')
             self.assertRaises(KeyError, load_profile, name="non-existing-section", filename="filename")
diff --git a/tests/test_mock_solver_loading.py b/tests/test_mock_solver_loading.py
index a4583c5..2010c9e 100644
--- a/tests/test_mock_solver_loading.py
+++ b/tests/test_mock_solver_loading.py
@@ -12,7 +12,7 @@ except ImportError:
     import mock
 
 from dwave.cloud.qpu import Client, Solver
-from dwave.cloud.exceptions import InvalidAPIResponseError
+from dwave.cloud.exceptions import InvalidAPIResponseError, ConfigFileReadError
 from dwave.cloud.config import legacy_load_config
 
 from .test_config import iterable_mock_open, configparser_open_namespace
@@ -217,8 +217,7 @@ class MockConfiguration(unittest.TestCase):
 
     def test_explicit_only(self):
         """Specify information only through function arguments."""
-        with Client.from_config(config_file='nonexisting',
-                                endpoint='arg-url', token='arg-token') as client:
+        with Client.from_config(endpoint='arg-url', token='arg-token') as client:
             client.session.get = GetEvent.handle
             try:
                 client.get_solver('arg-solver')
@@ -227,10 +226,10 @@ class MockConfiguration(unittest.TestCase):
                 return
             self.fail()
 
-    def test_nothing(self):
+    def test_nonexisting_file(self):
         """With no values set, we should get an error when trying to create Client."""
-        with self.assertRaises(ValueError):
-            with Client.from_config(config_file='nonexisting') as client:
+        with self.assertRaises(ConfigFileReadError):
+            with Client.from_config(config_file='nonexisting', legacy_config_fallback=False) as client:
                 pass
 
     def test_explicit_with_file(self):