Support on-demand content in repair_metadata

jobselko · jobselko · commit 7c3cb5c1954c · 2025-05-25T22:40:05.000+02:00
closes #849
diff --git a/CHANGES/849.feature b/CHANGES/849.feature
@@ -0,0 +1 @@
+Added support for on-demand content to `repair_metadata` endpoint.
diff --git a/pulp_python/app/tasks/repair.py b/pulp_python/app/tasks/repair.py
@@ -2,12 +2,19 @@
 import uuid
 from gettext import gettext as _
 
+from requests.exceptions import RequestException
 from django.db.models.query import QuerySet
 from pulpcore.plugin.models import ProgressReport
 from pulpcore.plugin.util import get_domain
 
 from pulp_python.app.models import PythonPackageContent, PythonRepository
-from pulp_python.app.utils import artifact_to_python_content_data
+from pulp_python.app.utils import (
+    artifact_to_python_content_data,
+    fetch_json_release_metadata,
+    parse_metadata,
+)
+from itertools import groupby
+from collections import defaultdict
 
 log = logging.getLogger(__name__)
 
@@ -32,11 +39,16 @@ def repair(repository_pk: uuid.UUID) -> None:
     content_set = repository.latest_version().content.values_list("pk", flat=True)
     content = PythonPackageContent.objects.filter(pk__in=content_set)
 
-    num_repaired = repair_metadata(content)
-    log.info(_("{} packages' metadata repaired.").format(num_repaired))
+    num_repaired, pkgs_not_repaired = repair_metadata(content)
+    log.info(
+        _(
+            "{} packages' metadata repaired. Not repaired packages due to either "
+            "inaccessible URL or mismatched sha256: {}."
+        ).format(num_repaired, pkgs_not_repaired)
+    )
 
 
-def repair_metadata(content: QuerySet[PythonPackageContent]) -> int:
+def repair_metadata(content: QuerySet[PythonPackageContent]) -> tuple[int, set[str]]:
     """
     Repairs metadata for a queryset of PythonPackageContent objects
     and updates the progress report.
@@ -45,25 +57,39 @@ def repair_metadata(content: QuerySet[PythonPackageContent]) -> int:
         content (QuerySet[PythonPackageContent]): The queryset of items to repair.
 
     Returns:
-        int: The number of packages that were repaired.
+        tuple[int, set[str]]: A tuple containing:
+            - The number of packages that were repaired.
+            - A set of primary keys of packages that were not repaired.
     """
-    # TODO: Add on_demand content repair
-    immediate_content = content.filter(contentartifact__artifact__isnull=False)
+    immediate_content = (
+        content.filter(contentartifact__artifact__isnull=False)
+        .distinct()
+        .prefetch_related("_artifacts")
+    )
+    on_demand_content = (
+        content.filter(contentartifact__artifact__isnull=True)
+        .distinct()
+        .prefetch_related("contentartifact_set__remoteartifact_set")
+        .order_by("name", "version")
+    )
     domain = get_domain()
 
     batch = []
     set_of_update_fields = set()
     total_repaired = 0
+    # Keep track of on-demand packages that need to be repaired
+    # A package is removed from this variable once it is successfully repaired
+    pkgs_not_repaired = set(on_demand_content.values_list("pk", flat=True))
 
     progress_report = ProgressReport(
         message="Repairing packages' metadata",
         code="repair.metadata",
-        total=immediate_content.count(),
+        total=content.count(),
     )
     progress_report.save()
     with progress_report:
         for package in progress_report.iter(
-            immediate_content.prefetch_related("_artifacts").iterator(chunk_size=1000)
+            immediate_content.iterator(chunk_size=1000)
         ):
             new_data = artifact_to_python_content_data(
                 package.filename, package._artifacts.get(), domain
@@ -82,8 +108,65 @@ def repair_metadata(content: QuerySet[PythonPackageContent]) -> int:
                 batch = []
                 set_of_update_fields.clear()
 
+        # For on-demand content, we expect that:
+        # 1. PythonPackageContent always has correct name and version, and one ContentArtifact
+        # 2. RemoteArtifact always has correct sha256
+        for (name, version), group in groupby(
+            on_demand_content.iterator(chunk_size=1000),
+            key=lambda x: (x.name, x.version),
+        ):
+            group_list = list(group)
+            grouped_by_url = defaultdict(list)
+
+            for package in group_list:
+                for ra in package.contentartifact_set.get().remoteartifact_set.all():
+                    grouped_by_url[ra.remote.url].append((package, ra))
+
+            # Prioritize the URL that can serve the most packages
+            for url, pkg_ra in sorted(
+                grouped_by_url.items(), key=lambda x: len(x[1]), reverse=True
+            ):
+                # All packages have the same URL, so pick a random remote
+                remote = pkg_ra[0][1].remote
+                try:
+                    json_data = fetch_json_release_metadata(name, version, remote)
+                except RequestException:
+                    continue
+
+                for package, ra in pkg_ra:
+                    if package.pk not in pkgs_not_repaired:
+                        continue
+                    # Extract data only for the specific distribution being checked
+                    dist_data = None
+                    for dist in json_data["urls"]:
+                        if ra.sha256 == dist["digests"]["sha256"]:
+                            dist_data = dist
+                            break
+                    if not dist_data:
+                        continue
+
+                    new_data = parse_metadata(json_data["info"], version, dist_data)
+                    # url belongs to RemoteArtifact
+                    new_data.pop("url")
+                    changed = False
+                    for field, value in new_data.items():
+                        if getattr(package, field) != value:
+                            setattr(package, field, value)
+                            set_of_update_fields.add(field)
+                            changed = True
+                    if changed:
+                        batch.append(package)
+                    if len(batch) == 1000:
+                        total_repaired += len(batch)
+                        PythonPackageContent.objects.bulk_update(
+                            batch, set_of_update_fields
+                        )
+                        batch = []
+                        set_of_update_fields.clear()
+                    pkgs_not_repaired.remove(package.pk)
+
     if batch:
         total_repaired += len(batch)
         PythonPackageContent.objects.bulk_update(batch, set_of_update_fields)
 
-    return total_repaired
+    return total_repaired, pkgs_not_repaired
diff --git a/pulp_python/app/utils.py b/pulp_python/app/utils.py
@@ -1,5 +1,6 @@
 import pkginfo
 import re
+import requests
 import shutil
 import tempfile
 import json
@@ -9,6 +10,7 @@
 from packaging.utils import canonicalize_name
 from packaging.requirements import Requirement
 from packaging.version import parse, InvalidVersion
+from pulpcore.plugin.models import Remote
 
 
 PYPI_LAST_SERIAL = "X-PYPI-LAST-SERIAL"
@@ -189,6 +191,19 @@ def artifact_to_python_content_data(filename, artifact, domain=None):
     return data
 
 
+def fetch_json_release_metadata(name: str, version: str, remote: Remote) -> dict:
+    """
+    Fetches metadata for a specific release from PyPI's JSON API. A release can contain
+    multiple distributions. See https://docs.pypi.org/api/json/#get-a-release for more details.
+
+    Returns dict containing "info", "last_serial", "urls", and "vulnerabilities" keys.
+    """
+    url = f"{remote.url}pypi/{name}/{version}/json"
+    response = requests.get(url, timeout=10)
+    response.raise_for_status()
+    return response.json()
+
+
 def python_content_to_json(base_path, content_query, version=None, domain=None):
     """
     Converts a QuerySet of PythonPackageContent into the PyPi JSON format
diff --git a/pulp_python/tests/functional/api/test_repair.py b/pulp_python/tests/functional/api/test_repair.py
@@ -23,7 +23,42 @@ def _create(artifact_filename, filename, content_data):
             "ca.save(); "
             "print(get_url(c))"
         )
-        process = subprocess.run(["pulpcore-manager", "shell", "-c", commands], capture_output=True)
+        process = subprocess.run(
+            ["pulpcore-manager", "shell", "-c", commands], capture_output=True
+        )
+
+        assert process.returncode == 0
+        content_href = process.stdout.decode().strip()
+        return python_bindings.ContentPackagesApi.read(content_href)
+
+    return _create
+
+
+@pytest.fixture
+def create_content_remote(python_bindings):
+    def _create(filename, content, remote, remote_2=None):
+        commands = (
+            "from pulpcore.plugin.models import ContentArtifact, RemoteArtifact; "
+            "from pulpcore.plugin.util import extract_pk, get_url; "
+            "from pulp_python.app.models import PythonPackageContent, PythonRemote; "
+            f"c = PythonPackageContent(filename={filename!r}, **{content!r}); "
+            "c.save(); "
+            f"ca = ContentArtifact(content=c, relative_path={filename!r}); "
+            "ca.save(); "
+            f"r = PythonRemote.objects.get(pk=extract_pk({remote.pulp_href!r})); "
+            f"ra = RemoteArtifact(content_artifact=ca, remote=r, sha256={content['sha256']!r}); "
+            "ra.save(); "
+        )
+        if remote_2:
+            commands += (
+                f"r2 = PythonRemote.objects.get(pk=extract_pk({remote_2.pulp_href!r})); "
+                f"ra2 = RemoteArtifact(content_artifact=ca, remote=r2, sha256={content['sha256']!r}); "  # noqa: E501
+                "ra2.save(); "
+            )
+        commands += "print(get_url(c))"
+        process = subprocess.run(
+            ["pulpcore-manager", "shell", "-c", commands], capture_output=True
+        )
 
         assert process.returncode == 0
         content_href = process.stdout.decode().strip()
@@ -36,7 +71,9 @@ def _create(artifact_filename, filename, content_data):
 def move_to_repository(python_bindings, monitor_task):
     def _move(repo_href, content_hrefs):
         body = {"add_content_units": content_hrefs}
-        task = monitor_task(python_bindings.RepositoriesPythonApi.modify(repo_href, body).task)
+        task = monitor_task(
+            python_bindings.RepositoriesPythonApi.modify(repo_href, body).task
+        )
         assert len(task.created_resources) == 1
         return python_bindings.RepositoriesPythonApi.read(repo_href)
 
@@ -68,8 +105,13 @@ def test_metadata_repair_command(
 
     move_to_repository(python_repo.pulp_href, [content.pulp_href])
     process = subprocess.run(
-        ["pulpcore-manager", "repair-python-metadata", "--repositories", python_repo.pulp_href],
-        capture_output=True
+        [
+            "pulpcore-manager",
+            "repair-python-metadata",
+            "--repositories",
+            python_repo.pulp_href,
+        ],
+        capture_output=True,
     )
     assert process.returncode == 0
     output = process.stdout.decode().strip()
@@ -84,6 +126,7 @@ def test_metadata_repair_command(
 
 def test_metadata_repair_endpoint(
     create_content_direct,
+    delete_orphans_pre,
     download_python_file,
     monitor_task,
     move_to_repository,
@@ -124,3 +167,98 @@ def test_metadata_repair_endpoint(
     assert content.packagetype == "sdist"
     assert content.requires_python == ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*"
     assert content.author == ""
+
+
+def test_metadata_repair_endpoint_on_demand(
+    create_content_remote,
+    delete_orphans_pre,
+    monitor_task,
+    move_to_repository,
+    python_bindings,
+    python_remote_factory,
+    python_repo_factory,
+):
+    """
+    Test repairing of package metadata via `Repositories.repair_metadata` endpoint
+    when only RemoteArtifacts are present.
+    """
+    # 1. Set up tested data
+    python_remote = python_remote_factory()
+    python_remote_bad = python_remote_factory(url="https://fixtures.pulpproject.org/")
+    python_repo = python_repo_factory(remote=python_remote)
+
+    scipy_filename_1 = "scipy-1.1.0.tar.gz"
+    scipy_data_1 = {
+        "name": "scipy",
+        "version": "1.1.0",
+        "sha256": "878352408424dffaa695ffedf2f9f92844e116686923ed9aa8626fc30d32cfd1",
+        # Wrong metadata
+        "author": "ME",
+        "packagetype": "bdist",
+        "requires_python": ">=3.8",
+    }
+
+    scipy_filename_2 = "scipy-1.1.0-cp36-none-win32.whl"
+    scipy_data_2 = scipy_data_1.copy()
+    scipy_data_2["sha256"] = (
+        "0e9bb7efe5f051ea7212555b290e784b82f21ffd0f655405ac4f87e288b730b3"
+    )
+
+    celery_filename = "celery-2.4.1.tar.gz"
+    celery_data = {
+        "name": "celery",
+        "version": "2.4.1",
+        "sha256": "c77652ca179d14473975822dbfb1b5dab950c88c171ef6bc2257ddb9066e6790",
+        # Wrong metadata
+        "author": "ME",
+        "packagetype": "bdist",
+        "requires_python": ">=3.8",
+    }
+
+    # 2. Create content
+    content_1 = create_content_remote(
+        scipy_filename_1, scipy_data_1, python_remote, python_remote_bad
+    )
+    content_2 = create_content_remote(scipy_filename_2, scipy_data_2, python_remote_bad)
+    content_3 = create_content_remote(celery_filename, celery_data, python_remote)
+
+    content_hrefs = {}
+    for filename, data, content in [
+        (scipy_filename_1, scipy_data_1, content_1),
+        (scipy_filename_2, scipy_data_2, content_2),
+        (celery_filename, celery_data, content_3),
+    ]:
+        for field, test_value in data.items():
+            assert getattr(content, field) == test_value
+            content_hrefs[filename] = content.pulp_href
+    move_to_repository(python_repo.pulp_href, list(content_hrefs.values()))
+
+    # 3. Repair metadata
+    response = python_bindings.RepositoriesPythonApi.repair_metadata(
+        python_repo.pulp_href
+    )
+    monitor_task(response.task)
+
+    # 4. Check new metadata
+    new_metadata = [
+        # repaired
+        ("celery-2.4.1.tar.gz", "celery", "2.4.1", "Ask Solem", "sdist", ""),
+        (
+            "scipy-1.1.0.tar.gz",
+            "scipy",
+            "1.1.0",
+            "",
+            "sdist",
+            ">=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*",
+        ),
+        # not repaired
+        ("scipy-1.1.0-cp36-none-win32.whl", "scipy", "1.1.0", "ME", "bdist", ">=3.8"),
+    ]
+    for filename, name, version, author, packagetype, requires_python in new_metadata:
+        new_content = python_bindings.ContentPackagesApi.read(content_hrefs[filename])
+        assert new_content.filename == filename
+        assert new_content.name == name
+        assert new_content.version == version
+        assert new_content.author == author
+        assert new_content.packagetype == packagetype
+        assert new_content.requires_python == requires_python

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Added support for on-demand content to `repair_metadata` endpoint.