Merge pull request #267 from lsst/tickets/DM-24658

DM-24658: Improve error reporting in connection string.

CI 31750 passed, merged.

python/lsst/daf/butler/registry/_dbAuth.py

@@ -34,6 +34,10 @@ class DbAuthError(RuntimeError):
     pass
 
 
+class DbAuthNotFoundError(DbAuthError):
+    """Credentials file does not exist or no match was found in it."""
+
+
 class DbAuthPermissionsError(DbAuthError):
     """Credentials file has incorrect permissions."""
 
@@ -66,12 +70,12 @@ def __init__(self, path=None, envVar=None, authList=None):
         if envVar is not None and envVar in os.environ:
             secretPath = os.path.expanduser(os.environ[envVar])
         elif path is None:
-            raise DbAuthError(
+            raise DbAuthNotFoundError(
                 "No default path provided to DbAuth configuration file")
         else:
             secretPath = os.path.expanduser(path)
         if not os.path.isfile(secretPath):
-            raise DbAuthError("No DbAuth configuration file: " + secretPath)
+            raise DbAuthNotFoundError(f"No DbAuth configuration file: {secretPath}")
         mode = os.stat(secretPath).st_mode
         if mode & (stat.S_IRWXG | stat.S_IRWXO) != 0:
             raise DbAuthPermissionsError(
@@ -206,7 +210,7 @@ def getAuth(self, drivername, username, host, port, database):
                     return (None, authDict["password"])
                 return (authDict["username"], authDict["password"])
 
-        raise DbAuthError(
+        raise DbAuthNotFoundError(
             "No matching DbAuth configuration for: "
             f"({drivername}, {username}, {host}, {port}, {database})")
 

python/lsst/daf/butler/registry/connectionString.py

@@ -22,7 +22,7 @@
 __all__ = ("DB_AUTH_ENVVAR", "DB_AUTH_PATH", "ConnectionStringFactory")
 
 from sqlalchemy.engine import url
-from ._dbAuth import DbAuth, DbAuthError, DbAuthPermissionsError
+from ._dbAuth import DbAuth, DbAuthNotFoundError
 
 DB_AUTH_ENVVAR = "LSST_DB_AUTH"
 """Default name of the environmental variable that will be used to locate DB
@@ -67,8 +67,10 @@ def fromConfig(cls, registryConfig):
 
         Raises
         ------
-        DBAuthError
+        DbAuthPermissionsError
             If the credentials file has incorrect permissions.
+        DbAuthError
+            A problem occured when retrieving DB authentication.
         """
         # this import can not live on the top because of circular import issue
         from lsst.daf.butler.registry import RegistryConfig
@@ -91,13 +93,12 @@ def fromConfig(cls, registryConfig):
             dbAuth = DbAuth(DB_AUTH_PATH, DB_AUTH_ENVVAR)
             auth = dbAuth.getAuth(conStr.drivername, conStr.username, conStr.host,
                                   conStr.port, conStr.database)
-        except DbAuthPermissionsError:
-            # re-raise permission error for safety
-            raise
-        except DbAuthError:
-            # credentials file doesn't exist or no match was found
+        except DbAuthNotFoundError:
+            # credentials file doesn't exist or no matches were found
             pass
         else:
+            # only assign auth when *no* errors were raised, otherwise assume
+            # connection string was correct
             conStr.username = auth[0]
             conStr.password = auth[1]
 

tests/config/dbAuth/registryConf2.yaml

@@ -0,0 +1,7 @@
+# minimal configuration required when using explicit 'host-port',
+# also tests '/' are placed correctly when not explicit in dbname
+registry:
+  db: 'postgresql://host.example.com'
+  port: 5432
+  database: 'my_database'
+  expected: 'postgresql://user:test3@host.example.com:5432/my_database'

tests/config/dbAuth/registryConf3.yaml

@@ -0,0 +1,5 @@
+# test that combinations of 'db' keys and explicit keys work (f.e. port)
+registry:
+  db: 'postgresql://host.example.com:5432'
+  database: 'my_database'
+  expected: 'postgresql://user:test3@host.example.com:5432/my_database'

tests/test_connectionString.py

@@ -25,8 +25,9 @@
 import unittest
 import os
 import os.path
+import glob
 
-from lsst.daf.butler.registry import RegistryConfig
+from lsst.daf.butler.registry import RegistryConfig, DbAuthError
 import lsst.daf.butler.registry.connectionString as ConnectionStringModule
 from lsst.daf.butler.registry.connectionString import ConnectionStringFactory
 
@@ -35,9 +36,9 @@
 
 class ConnectionStringBuilderTestCase(unittest.TestCase):
     """Tests for ConnectionStringBuilder."""
-    configDir = os.path.join(TESTDIR, "config/basic/connectionStringConfs/")
-    configFiles = os.listdir(configDir)
-    credentialsFile = os.path.join(TESTDIR, "testDbAuth.yaml")
+    configDir = os.path.join(TESTDIR, "config", "dbAuth")
+    configFiles = glob.glob(os.path.join(configDir, "registryConf*"))
+    credentialsFile = os.path.join(configDir, "db-auth.yaml")
 
     def setUp(self):
         self.resetDbAuthPathValue = ConnectionStringModule.DB_AUTH_PATH
@@ -48,7 +49,7 @@ def tearDown(self):
         ConnectionStringModule.DB_AUTH_PATH = self.resetDbAuthPathValue
 
     def testBuilder(self):
-        """Tests ConnectionStringBuilder builds correct connection strings.
+        """Tests ConnectionStringFactory returns correct connection strings.
         """
         regConfigs = [RegistryConfig(os.path.join(self.configDir, name)) for name in self.configFiles]
 
@@ -61,13 +62,20 @@ def testBuilder(self):
 
     def testRelVsAbsPath(self):
         """Tests that relative and absolute paths are preserved."""
-        regConf = RegistryConfig(os.path.join(self.configDir, 'conf1.yaml'))
+        regConf = RegistryConfig(os.path.join(self.configDir, 'registryConf1.yaml'))
         regConf['db'] = 'sqlite:///relative/path/conf1.sqlite3'
-
         conStrFactory = ConnectionStringFactory()
         conStr = conStrFactory.fromConfig(regConf)
         self.assertEqual(str(conStr), 'sqlite:///relative/path/conf1.sqlite3')
 
+    def testRaises(self):
+        """Test that DbAuthError propagates through the class."""
+        ConnectionStringModule.DB_AUTH_PATH = os.path.join(self.configDir, "badDbAuth2.yaml")
+        regConf = RegistryConfig(os.path.join(self.configDir, 'registryConf2.yaml'))
+        conStrFactory = ConnectionStringFactory()
+        with self.assertRaises(DbAuthError):
+            conStrFactory.fromConfig(regConf)
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_dbAuth.py

@@ -26,7 +26,7 @@
 from lsst.daf.butler.registry import DbAuth, DbAuthError
 
 TESTDIR = os.path.join(
-    os.path.abspath(os.path.dirname(__file__)), "data", "dbAuth")
+    os.path.abspath(os.path.dirname(__file__)), "config", "dbAuth")
 
 
 class DbAuthTestCase(unittest.TestCase):
@@ -74,7 +74,7 @@ def test_connStrings(self):
 
     def test_load(self):
         """Test loading from a YAML file and matching in order."""
-        filePath = os.path.join(TESTDIR, "good_test.yaml")
+        filePath = os.path.join(TESTDIR, "db-auth.yaml")
         os.chmod(filePath, 0o600)
         auth = DbAuth(filePath)
         self.assertEqual(
@@ -201,16 +201,16 @@ def test_errors(self):
                           envVar="LSST_NONEXISTENT")
         with self.assertRaisesRegex(
                 DbAuthError,
-                r"^DbAuth configuration file .*bad_test1.yaml has incorrect "
+                r"^DbAuth configuration file .*badDbAuth1.yaml has incorrect "
                 r"permissions: \d*644$"):
-            filePath = os.path.join(TESTDIR, "bad_test1.yaml")
+            filePath = os.path.join(TESTDIR, "badDbAuth1.yaml")
             os.chmod(filePath, 0o644)
             auth = DbAuth(filePath)
         with self.assertRaisesRegex(
                 DbAuthError,
                 r"^Unable to load DbAuth configuration file: "
-                r".*bad_test2.yaml"):
-            filePath = os.path.join(TESTDIR, "bad_test2.yaml")
+                r".*badDbAuth2.yaml"):
+            filePath = os.path.join(TESTDIR, "badDbAuth2.yaml")
             os.chmod(filePath, 0o600)
             os.environ["LSST_DB_AUTH_TEST"] = filePath
             auth = DbAuth(envVar="LSST_DB_AUTH_TEST")
@@ -298,7 +298,7 @@ def test_getUrl(self):
             auth.getUrl("postgresql://host.example.com:5432/my_database"),
             "postgresql://foo:bar@host.example.com:5432/my_database"),
 
-        filePath = os.path.join(TESTDIR, "good_test.yaml")
+        filePath = os.path.join(TESTDIR, "db-auth.yaml")
         os.chmod(filePath, 0o600)
         auth = DbAuth(filePath)
         self.assertEqual(