feat: added config load tests and code cleaning

Author: ymarcon

datashield/api.py

@@ -23,8 +23,9 @@ def __init__(self, names: list[str] = None):
         # load login information from configuration files, in order of precedence
         if names is not None and len(names) > 0:
             config = DSConfig.load()
+            name_set = set(names)
             if config.servers:
-                items = [x for x in config.servers if x.name in names]
+                items = [x for x in config.servers if x.name in name_set]
                 if len(items) == 0:
                     logging.warning(f"No matching server names found in configuration for: {', '.join(names)}")
                 else:

datashield/interface.py

@@ -6,7 +6,7 @@
 import logging
 import os
 import yaml
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, model_validator
 
 # Default configuration file paths to look for DataSHIELD login information, in order of precedence
 CONFIG_FILES = ["~/.config/datashield/config.yaml", "./.datashield/config.yaml"]
@@ -27,6 +27,12 @@ class DSLoginInfo(BaseModel):
 
     model_config = {"extra": "forbid"}
 
+    @model_validator(mode="after")
+    def validate_credentials(self) -> "DSLoginInfo":
+        if self.user is None and self.token is None:
+            raise ValueError("Either user or token must be provided")
+        return self
+
 
 class DSConfig(BaseModel):
     """
@@ -40,22 +46,26 @@ class DSConfig(BaseModel):
     @classmethod
     def load(cls) -> "DSConfig":
         """
-        Load the DataSHIELD configuration from default configuration files. The file must contain
-        a list of servers with their login details. The configuration from the first file found will be loaded,
-        in order of precedence. If multiple files are found, the configurations will be merged, with new server
-        details replacing existing ones by name.
+        Load the DataSHIELD configuration from the default configuration files.
+        Each file must contain a list of servers with their login details.
+        All readable configuration files listed in ``CONFIG_FILES`` are processed in
+        order. Their configurations are merged, with servers identified by their
+        ``name`` field. If the same server name appears in multiple files, the
+        definition from the later file in the list takes precedence and replaces
+        the earlier one. Servers that are only present in earlier files are kept.
 
         :return: The DataSHIELD configuration object
         """
         merged_config = None
         for config_file in CONFIG_FILES:
             try:
                 # check file exists and is readable, if not, silently ignore
-                if not os.path.exists(config_file):
+                path = os.path.expanduser(config_file)
+                if not os.path.exists(path):
                     continue
-                if not os.access(config_file, os.R_OK):
+                if not os.access(path, os.R_OK):
                     continue
-                config = cls.load_from_file(config_file)
+                config = cls.load_from_file(path)
                 if merged_config is None:
                     merged_config = config
                 else:
@@ -64,9 +74,9 @@ def load(cls) -> "DSConfig":
                     for server in config.servers:
                         existing_servers[server.name] = server
                     merged_config.servers = list(existing_servers.values())
-            except Exception as e:
-                # silently ignore errors, e.g. file not found or invalid format
-                logging.error(f"Failed to load login information from {config_file}: {e}")
+            except Exception:
+                # log and ignore errors, e.g. file not found or invalid format
+                logging.error(f"Failed to load login information from {config_file}")
         return merged_config if merged_config else cls()
 
     @classmethod

tests/test_config.py

@@ -16,15 +16,15 @@ def test_load_config_from_yaml():
   - name: server1
     url: https://opal-demo.obiba.org
     user: dsuser
-    password: P@ssw0rd
+    password: your-password-here
   - name: server2
     url: https://opal.example.org
     token: your-access-token-here
     profile: default
   - name: server3
     url: https://study.example.org/opal
     user: dsuser
-    password: P@ssw0rd
+    password: your-password-here
     profile: custom
     driver: datashield_opal.OpalDriver
 """
@@ -46,7 +46,7 @@ def test_load_config_from_yaml():
         assert config.servers[0].name == "server1"
         assert config.servers[0].url == "https://opal-demo.obiba.org"
         assert config.servers[0].user == "dsuser"
-        assert config.servers[0].password == "P@ssw0rd"
+        assert config.servers[0].password == "your-password-here"
         assert config.servers[0].token is None
         assert config.servers[0].profile == "default"
         assert config.servers[0].driver == "datashield_opal.OpalDriver"
@@ -64,7 +64,7 @@ def test_load_config_from_yaml():
         assert config.servers[2].name == "server3"
         assert config.servers[2].url == "https://study.example.org/opal"
         assert config.servers[2].user == "dsuser"
-        assert config.servers[2].password == "P@ssw0rd"
+        assert config.servers[2].password == "your-password-here"
         assert config.servers[2].token is None
         assert config.servers[2].profile == "custom"
         assert config.servers[2].driver == "datashield_opal.OpalDriver"
@@ -179,6 +179,14 @@ def test_create_dslogininfo_directly():
     assert login.driver == "datashield_opal.OpalDriver"
 
 
+def test_create_dslogininfo_requires_user_or_token():
+    """Test that DSLoginInfo requires at least one credential method."""
+    with pytest.raises(ValidationError) as exc_info:
+        DSLoginInfo(name="test", url="https://example.org")
+
+    assert "either user or token must be provided" in str(exc_info.value).lower()
+
+
 def test_create_dsconfig_directly():
     """Test creating DSConfig objects directly with Pydantic."""
     login1 = DSLoginInfo(name="server1", url="https://example1.org", user="user1", password="pass1")
@@ -229,3 +237,229 @@ def test_model_serialization():
     assert data["password"] == "testpass"
     assert data["profile"] == "default"
     assert data["driver"] == "datashield_opal.OpalDriver"
+
+
+def test_load_no_config_files(monkeypatch):
+    """Test loading configuration when no config files exist."""
+    # Mock the CONFIG_FILES to point to non-existent paths
+    monkeypatch.setattr(
+        "datashield.interface.CONFIG_FILES", ["/nonexistent/path/config.yaml", "/another/nonexistent/path/config.yaml"]
+    )
+
+    config = DSConfig.load()
+
+    assert config is not None
+    assert len(config.servers) == 0
+
+
+def test_load_from_first_config_file(monkeypatch):
+    """Test loading configuration from the first config file found."""
+    yaml_content = """
+servers:
+  - name: server1
+    url: https://example1.org
+    user: user1
+    password: pass1
+"""
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f:
+        f.write(yaml_content)
+        first_config_file = f.name
+
+    try:
+        # Mock CONFIG_FILES to use our temporary file
+        monkeypatch.setattr("datashield.interface.CONFIG_FILES", [first_config_file, "/nonexistent/second/config.yaml"])
+
+        config = DSConfig.load()
+
+        assert config is not None
+        assert len(config.servers) == 1
+        assert config.servers[0].name == "server1"
+        assert config.servers[0].url == "https://example1.org"
+        assert config.servers[0].user == "user1"
+
+    finally:
+        os.unlink(first_config_file)
+
+
+def test_load_from_second_config_file(monkeypatch):
+    """Test loading configuration from the second config file if first doesn't exist."""
+    yaml_content = """
+servers:
+  - name: server2
+    url: https://example2.org
+    user: user2
+    token: token123
+"""
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f:
+        f.write(yaml_content)
+        second_config_file = f.name
+
+    try:
+        # Mock CONFIG_FILES to use our temporary file as second option
+        monkeypatch.setattr("datashield.interface.CONFIG_FILES", ["/nonexistent/first/config.yaml", second_config_file])
+
+        config = DSConfig.load()
+
+        assert config is not None
+        assert len(config.servers) == 1
+        assert config.servers[0].name == "server2"
+        assert config.servers[0].url == "https://example2.org"
+        assert config.servers[0].token == "token123"
+
+    finally:
+        os.unlink(second_config_file)
+
+
+def test_load_merge_multiple_config_files(monkeypatch):
+    """Test merging configurations from multiple config files."""
+    yaml_content1 = """
+servers:
+  - name: server1
+    url: https://example1.org
+    user: user1
+    password: pass1
+  - name: server2
+    url: https://example2.org
+    user: user2
+    password: pass2
+"""
+
+    yaml_content2 = """
+servers:
+  - name: server2
+    url: https://example2-updated.org
+    user: user2_updated
+    password: pass2_updated
+  - name: server3
+    url: https://example3.org
+    token: token123
+"""
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f1:
+        f1.write(yaml_content1)
+        first_config_file = f1.name
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f2:
+        f2.write(yaml_content2)
+        second_config_file = f2.name
+
+    try:
+        # Mock CONFIG_FILES to use both our temporary files
+        monkeypatch.setattr("datashield.interface.CONFIG_FILES", [first_config_file, second_config_file])
+
+        config = DSConfig.load()
+
+        assert config is not None
+        assert len(config.servers) == 3
+
+        # Check server1 (from first file, unchanged)
+        server1 = next(s for s in config.servers if s.name == "server1")
+        assert server1.url == "https://example1.org"
+        assert server1.user == "user1"
+
+        # Check server2 (from first file, but updated by second file)
+        server2 = next(s for s in config.servers if s.name == "server2")
+        assert server2.url == "https://example2-updated.org"
+        assert server2.user == "user2_updated"
+        assert server2.password == "pass2_updated"
+
+        # Check server3 (from second file only)
+        server3 = next(s for s in config.servers if s.name == "server3")
+        assert server3.url == "https://example3.org"
+        assert server3.token == "token123"
+
+    finally:
+        os.unlink(first_config_file)
+        os.unlink(second_config_file)
+
+
+def test_load_handles_invalid_yaml_silently(monkeypatch):
+    """Test that load() silently handles invalid YAML files."""
+    invalid_yaml_content = """
+servers:
+  - name: server1
+    url: https://example.org
+    invalid: yaml: content:
+"""
+
+    valid_yaml_content = """
+servers:
+  - name: server2
+    url: https://example2.org
+    user: user2
+    password: pass2
+"""
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f1:
+        f1.write(invalid_yaml_content)
+        invalid_config_file = f1.name
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f2:
+        f2.write(valid_yaml_content)
+        valid_config_file = f2.name
+
+    try:
+        # Mock CONFIG_FILES with invalid file first, then valid file
+        monkeypatch.setattr("datashield.interface.CONFIG_FILES", [invalid_config_file, valid_config_file])
+
+        # Should not raise an exception, but load from the valid file
+        config = DSConfig.load()
+
+        assert config is not None
+        assert len(config.servers) == 1
+        assert config.servers[0].name == "server2"
+        assert config.servers[0].url == "https://example2.org"
+
+    finally:
+        os.unlink(invalid_config_file)
+        os.unlink(valid_config_file)
+
+
+def test_load_handles_permission_denied_silently(monkeypatch):
+    """Test that load() silently handles files without read permissions."""
+    yaml_content1 = """
+servers:
+  - name: server1
+    url: https://example1.org
+    user: user1
+    password: pass1
+"""
+
+    yaml_content2 = """
+servers:
+  - name: server2
+    url: https://example2.org
+    user: user2
+    password: pass2
+"""
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f1:
+        f1.write(yaml_content1)
+        no_read_file = f1.name
+
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f2:
+        f2.write(yaml_content2)
+        readable_file = f2.name
+
+    try:
+        # Remove read permissions from first file
+        os.chmod(no_read_file, 0o000)
+
+        # Mock CONFIG_FILES with unreadable file first, then readable file
+        monkeypatch.setattr("datashield.interface.CONFIG_FILES", [no_read_file, readable_file])
+
+        # Should not raise an exception, but load from the readable file
+        config = DSConfig.load()
+
+        assert config is not None
+        assert len(config.servers) == 1
+        assert config.servers[0].name == "server2"
+        assert config.servers[0].url == "https://example2.org"
+
+    finally:
+        # Restore permissions to allow cleanup
+        os.chmod(no_read_file, 0o644)
+        os.unlink(no_read_file)
+        os.unlink(readable_file)