refactor: extract _normalise_version and _directory_label from export_updateSoftware

Extract the version validation/normalisation logic and the filesystem
directory-label derivation into two module-level helper functions
(_normalise_version, _directory_label) so they can be unit-tested
directly without standing up a full DIRAC service.

Update the unit tests to import and exercise the real production
functions instead of maintaining local mirror implementations.

Author: fstagni

src/DIRAC/FrameworkSystem/Service/SystemAdministratorHandler.py

@@ -47,6 +47,92 @@ def loadDIRACCFG():
     return S_OK((cfgPath, diracCFG))
 
 
+def _normalise_version(version):
+    """Validate and normalise a raw version string supplied by the operator.
+
+    Parameters
+    ----------
+    version:
+        Raw string as received from the client (may contain surrounding
+        whitespace or use the spaced ``pkg @ url`` pip syntax).
+
+    Returns
+    -------
+    tuple(str, str | None, bool, bool)
+        ``(version, primaryExtension, released_version, isPrerelease)``
+
+        - *version* – normalised version string ready to be passed to pip.
+        - *primaryExtension* – package name when the caller used
+          ``extension==version`` syntax; ``None`` otherwise.
+        - *released_version* – ``True`` when installing a PEP 440 release,
+          ``False`` when installing from a VCS URL.
+        - *isPrerelease* – ``True`` when the PEP 440 version is a pre-release.
+
+    Raises
+    ------
+    ValueError
+        When the version string is empty or not a valid PEP 440 version and
+        does not contain a recognised VCS URL.
+    """
+    version = version.strip()
+    if not version:
+        raise ValueError("No version specified")
+
+    primaryExtension = None
+    if "==" in version:
+        primaryExtension, version = version.split("==", 1)
+
+    released_version = True
+    isPrerelease = False
+
+    # Special aliases: install DIRAC from the integration branch
+    if version.lower() in ("integration", "devel", "master", "main"):
+        released_version = False
+        version = "DIRAC[server] @ git+https://github.com/DIRACGrid/DIRAC.git@integration"
+        return version, primaryExtension, released_version, isPrerelease
+
+    # Try to parse as a PEP 440 version number
+    try:
+        parsed = Version(version)
+        isPrerelease = parsed.is_prerelease
+        version = f"v{parsed}"
+    except InvalidVersion:
+        if "https://" in version:
+            # Treat as a VCS URL (e.g. "DIRAC[server] @ git+https://...")
+            released_version = False
+        else:
+            raise ValueError(f"Invalid version passed {version!r}")
+
+    return version, primaryExtension, released_version, isPrerelease
+
+
+def _directory_label(version, released_version):
+    """Derive the filesystem directory label for a given version.
+
+    For released versions this is the version string itself.  For VCS URLs
+    (pip ``pkg @ url`` syntax) it is the URL part, stripped of any
+    ``#egg=...`` fragment and surrounding whitespace.
+
+    Parameters
+    ----------
+    version:
+        Normalised version string as returned by :func:`_normalise_version`.
+    released_version:
+        ``True`` when *version* is a PEP 440 release string.
+
+    Returns
+    -------
+    str
+        A filesystem-safe label derived from *version*.
+    """
+    if released_version:
+        return version
+    # version is "pkg @ git+https://host/repo.git@branch"
+    # Split on the *first* "@" (the pip separator) only, then strip spaces
+    # and drop any "#egg=..." fragment so the branch name is preserved.
+    return version.split("@", 1)[1].strip().split("#")[0]
+
+
 class SystemAdministratorHandler(RequestHandler):
     @classmethod
     def initializeHandler(cls, serviceInfo):
@@ -262,35 +348,11 @@ def export_updateSoftware(self, version):
         - a git tag/branch like "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
         """
         # Validate and normalise the requested version
-        # Strip surrounding whitespace and collapse any internal spaces around "@"
-        # so that both "DIRAC[server] @ git+https://..." and
-        # "DIRAC[server]@git+https://..." are accepted.
-        version = version.strip()
-        if not version:
-            return S_ERROR("No version specified")
-        primaryExtension = None
-        if "==" in version:
-            primaryExtension, version = version.split("==")
-
-        released_version = True
-        isPrerelease = False
-
-        # Special cases (e.g. installing the integration/main branch)
-        if version.lower() in ["integration", "devel", "master", "main"]:
-            released_version = False
-            version = "DIRAC[server] @ git+https://github.com/DIRACGrid/DIRAC.git@integration"
-
-        if released_version:
-            try:
-                version = Version(version)
-                isPrerelease = version.is_prerelease
-                version = f"v{version}"
-            except InvalidVersion:
-                if "https://" in version:
-                    released_version = False
-                else:
-                    self.log.exception("Invalid version passed", version)
-                    return S_ERROR(f"Invalid version passed {version!r}")
+        try:
+            version, primaryExtension, released_version, isPrerelease = _normalise_version(version)
+        except ValueError as e:
+            self.log.exception("Invalid version passed", version)
+            return S_ERROR(str(e))
 
         # Find what to install
         otherExtensions = []
@@ -316,7 +378,7 @@ def export_updateSoftware(self, version):
             installer.flush()
             self.log.info("Downloaded DIRACOS installer to", installer.name)
 
-            directory = version if released_version else version.split("@", 1)[1].strip().split("#")[0]
+            directory = _directory_label(version, released_version)
             newProPrefix = os.path.join(
                 rootPath,
                 "versions",

src/DIRAC/FrameworkSystem/Service/test/test_SystemAdministratorHandler.py

@@ -1,65 +1,18 @@
-"""Unit tests for SystemAdministratorHandler version normalisation logic
-and the SystemAdministratorClientCLI do_update input validation.
+"""Unit tests for SystemAdministratorHandler helper functions and
+the SystemAdministratorClientCLI do_update input validation.
 """
 
 import pytest
 from unittest.mock import MagicMock, patch
 
-
-# ---------------------------------------------------------------------------
-# Helpers – replicate the version-normalisation logic from
-# SystemAdministratorHandler.export_updateSoftware so we can test it
-# without standing up a full DIRAC service.
-# ---------------------------------------------------------------------------
-
-
-def _normalise_version(version):
-    """Mirror the normalisation applied at the top of export_updateSoftware.
-
-    Returns the normalised version string, or raises ValueError if invalid.
-    Mirrors the side-effects relevant to the directory-name derivation and
-    the pip command construction.
-    """
-    from packaging.version import Version, InvalidVersion
-
-    version = version.strip()
-    if not version:
-        raise ValueError("No version specified")
-
-    released_version = True
-    isPrerelease = False
-
-    if version.lower() in ["integration", "devel", "master", "main"]:
-        released_version = False
-        version = "DIRAC[server] @ git+https://github.com/DIRACGrid/DIRAC.git@integration"
-
-    if released_version:
-        try:
-            parsed = Version(version)
-            isPrerelease = parsed.is_prerelease
-            version = f"v{parsed}"
-        except InvalidVersion:
-            if "https://" in version:
-                released_version = False
-            else:
-                raise ValueError(f"Invalid version passed {version!r}")
-
-    return version, released_version, isPrerelease
-
-
-def _directory_from_version(version, released_version):
-    """Mirror the directory-name derivation in export_updateSoftware.
-
-    Split on the *first* "@" only (the pip package @ URL separator), strip
-    whitespace, then drop any "#egg=..." fragment.
-    """
-    if released_version:
-        return version
-    return version.split("@", 1)[1].strip().split("#")[0]
+from DIRAC.FrameworkSystem.Service.SystemAdministratorHandler import (
+    _directory_label,
+    _normalise_version,
+)
 
 
 # ---------------------------------------------------------------------------
-# Tests: version normalisation
+# Tests: _normalise_version
 # ---------------------------------------------------------------------------
 
 
@@ -73,72 +26,80 @@ def test_whitespace_only_raises(self):
             _normalise_version("   ")
 
     def test_released_version(self):
-        version, released, pre = _normalise_version("9.0.18")
+        version, primary, released, pre = _normalise_version("9.0.18")
         assert released is True
         assert pre is False
         assert version == "v9.0.18"
+        assert primary is None
 
     def test_released_prerelease_version(self):
-        version, released, pre = _normalise_version("9.0.18a1")
+        version, primary, released, pre = _normalise_version("9.0.18a1")
         assert released is True
         assert pre is True
         assert version == "v9.0.18a1"
 
+    def test_extension_syntax_splits_primary(self):
+        version, primary, released, pre = _normalise_version("MyExtension==9.0.18")
+        assert primary == "MyExtension"
+        assert version == "v9.0.18"
+        assert released is True
+
     @pytest.mark.parametrize("keyword", ["integration", "devel", "master", "main"])
     def test_special_keywords(self, keyword):
-        version, released, pre = _normalise_version(keyword)
+        version, primary, released, pre = _normalise_version(keyword)
         assert released is False
+        assert pre is False
         assert "DIRACGrid/DIRAC" in version
         assert "@integration" in version
 
     def test_git_url_without_spaces(self):
         raw = "DIRAC[server]@git+https://github.com/fstagni/DIRAC.git@test_branch"
-        version, released, pre = _normalise_version(raw)
+        version, primary, released, pre = _normalise_version(raw)
         assert released is False
         assert version == raw
 
     def test_git_url_with_spaces_around_at(self):
-        """The CLI now sends the raw user input; leading/trailing spaces must be stripped."""
-        raw = "  DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch  "
-        version, released, pre = _normalise_version(raw)
+        """Leading/trailing whitespace is stripped; internal pip spaces are kept."""
+        raw = "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
+        version, primary, released, pre = _normalise_version(f"  {raw}  ")
         assert released is False
-        # Internal spaces around "@" are preserved (pip accepts them)
-        assert version.strip() == raw.strip()
+        assert version == raw
 
     def test_invalid_version_no_url_raises(self):
         with pytest.raises(ValueError, match="Invalid version passed"):
             _normalise_version("not-a-valid-version")
 
 
 # ---------------------------------------------------------------------------
-# Tests: directory derivation from version string
+# Tests: _directory_label
 # ---------------------------------------------------------------------------
 
 
-class TestDirectoryFromVersion:
+class TestDirectoryLabel:
     def test_released_version_uses_version_directly(self):
-        d = _directory_from_version("v9.0.18", released_version=True)
-        assert d == "v9.0.18"
+        assert _directory_label("v9.0.18", released_version=True) == "v9.0.18"
 
     def test_git_url_without_spaces(self):
-        """No spaces around '@' separator — branch name must be included."""
+        """Branch name after the second '@' must be preserved."""
         version = "DIRAC[server]@git+https://github.com/fstagni/DIRAC.git@test_branch"
-        d = _directory_from_version(version, released_version=False)
-        # Split on first "@" → "git+https://github.com/fstagni/DIRAC.git@test_branch"
-        assert d == "git+https://github.com/fstagni/DIRAC.git@test_branch"
+        assert (
+            _directory_label(version, released_version=False) == "git+https://github.com/fstagni/DIRAC.git@test_branch"
+        )
 
-    def test_git_url_with_spaces_around_at_separator(self):
-        """Spaces around the pip '@' separator must be stripped; branch part kept."""
-        # Simulate what the handler receives after version.strip()
+    def test_git_url_with_spaces_around_pip_separator(self):
+        """Spaces around the pip '@' separator are stripped; branch part kept."""
         version = "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
-        d = _directory_from_version(version, released_version=False)
-        assert d == "git+https://github.com/fstagni/DIRAC.git@test_branch"
+        assert (
+            _directory_label(version, released_version=False) == "git+https://github.com/fstagni/DIRAC.git@test_branch"
+        )
 
     def test_git_url_with_hash_fragment(self):
-        """#egg= fragment must be stripped from directory name."""
+        """#egg= fragment must be stripped from the directory label."""
         version = "DIRAC[server]@git+https://github.com/DIRACGrid/DIRAC.git@integration#egg=DIRAC"
-        d = _directory_from_version(version, released_version=False)
-        assert d == "git+https://github.com/DIRACGrid/DIRAC.git@integration"
+        assert (
+            _directory_label(version, released_version=False)
+            == "git+https://github.com/DIRACGrid/DIRAC.git@integration"
+        )
 
 
 # ---------------------------------------------------------------------------
@@ -150,15 +111,12 @@ class TestDoUpdate:
     """Test SystemAdministratorClientCLI.do_update input validation."""
 
     def _make_cli(self):
-        from DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI import (
-            SystemAdministratorClientCLI,
-        )
+        from DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI import SystemAdministratorClientCLI
 
-        with patch("DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.SystemAdministratorClient"):
-            cli = SystemAdministratorClientCLI.__new__(SystemAdministratorClientCLI)
-            cli.host = "localhost"
-            cli.port = 9162
-            return cli
+        cli = SystemAdministratorClientCLI.__new__(SystemAdministratorClientCLI)
+        cli.host = "localhost"
+        cli.port = 9162
+        return cli
 
     def test_empty_args_prints_usage_and_returns(self):
         cli = self._make_cli()
@@ -169,12 +127,10 @@ def test_empty_args_prints_usage_and_returns(self):
             patch("DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.gLogger") as mock_logger,
         ):
             cli.do_update("")
-            # Client must NOT be contacted
             mock_client_cls.assert_not_called()
-            # Usage should be printed
             assert mock_logger.notice.called
 
-    def test_whitespace_only_args_prints_usage_and_returns(self):
+    def test_whitespace_only_args_does_not_contact_server(self):
         cli = self._make_cli()
         with (
             patch(
@@ -202,8 +158,8 @@ def test_valid_version_calls_client(self):
             mock_client_cls.assert_called_once_with(cli.host, cli.port)
             mock_instance.updateSoftware.assert_called_once_with("9.0.18", timeout=600)
 
-    def test_git_url_with_spaces_passes_stripped_version(self):
-        """Spaces are stripped from the outer edges but the version body is preserved."""
+    def test_git_url_with_spaces_passes_full_version_to_server(self):
+        """The version body (including internal spaces) is forwarded as-is."""
         cli = self._make_cli()
         with (
             patch(