refactor: use helper + optional temp_path instead of separate functions

Address review feedback:
- Add copy_artifact_to_temp_file helper that copies artifact to disk
- artifact_to_python_content_data accepts optional temp_path param
- artifact_to_metadata_artifact accepts optional temp_path param
- Remove artifact_to_metadata_artifact_from_path (no longer needed)
- Repair loop creates temp file once via helper, passes to both functions
- Use try/finally for temp file cleanup

JIRA: PULP-1573

Author: decko

pulp_python/app/tasks/repair.py

@@ -10,8 +10,8 @@
 from pulp_python.app.models import PythonPackageContent, PythonRepository
 from pulp_python.app.utils import (
     artifact_to_metadata_artifact,
-    artifact_to_metadata_artifact_from_path,
     artifact_to_python_content_data,
+    copy_artifact_to_temp_file,
     fetch_json_release_metadata,
     parse_metadata,
 )
@@ -120,30 +120,24 @@ def repair_metadata(content: QuerySet[PythonPackageContent]) -> tuple[int, set[s
                 .first()
                 .artifact
             )
-            # For wheels, keep the temp file so we can reuse it for metadata artifact
-            is_wheel = package.filename.endswith(".whl")
-            if is_wheel:
-                new_data, temp_path = artifact_to_python_content_data(
-                    package.filename, main_artifact, domain, keep_temp_file=True
+            # Copy artifact to temp file once, reuse for both content data and metadata
+            temp_path = copy_artifact_to_temp_file(main_artifact, package.filename)
+            try:
+                new_data = artifact_to_python_content_data(
+                    package.filename, main_artifact, domain, temp_path=temp_path
                 )
-            else:
-                new_data = artifact_to_python_content_data(package.filename, main_artifact, domain)
-                temp_path = None
-
-            total_metadata_repaired += update_metadata_artifact_if_needed(
-                package,
-                new_data.get("metadata_sha256"),
-                main_artifact,
-                metadata_batch,
-                pkgs_metadata_not_repaired,
-                temp_path=temp_path,
-            )
-            total_repaired += update_package_if_needed(
-                package, new_data, batch, set_of_update_fields
-            )
-
-            # Clean up temp file after processing
-            if temp_path and os.path.exists(temp_path):
+                total_metadata_repaired += update_metadata_artifact_if_needed(
+                    package,
+                    new_data.get("metadata_sha256"),
+                    main_artifact,
+                    metadata_batch,
+                    pkgs_metadata_not_repaired,
+                    temp_path=temp_path,
+                )
+                total_repaired += update_package_if_needed(
+                    package, new_data, batch, set_of_update_fields
+                )
+            finally:
                 os.unlink(temp_path)
 
         # For on-demand content, we expect that:
@@ -315,10 +309,9 @@ def _process_metadata_batch(metadata_batch: list[tuple]) -> set[str]:
     content_artifacts = []
 
     for package, main_artifact, temp_path in metadata_batch:
-        if temp_path and os.path.exists(temp_path):
-            metadata_artifact = artifact_to_metadata_artifact_from_path(package.filename, temp_path)
-        else:
-            metadata_artifact = artifact_to_metadata_artifact(package.filename, main_artifact)
+        metadata_artifact = artifact_to_metadata_artifact(
+            package.filename, main_artifact, temp_path=temp_path
+        )
         if metadata_artifact:
             ca = ContentArtifact(
                 artifact=metadata_artifact,

pulp_python/app/utils.py

@@ -240,59 +240,67 @@ def compute_metadata_sha256(filename: str) -> str | None:
     return hashlib.sha256(metadata_content).hexdigest() if metadata_content else None
 
 
-def artifact_to_python_content_data(filename, artifact, domain=None, keep_temp_file=False):
+def copy_artifact_to_temp_file(artifact, filename, tmp_dir="."):
+    """
+    Copy an artifact's file to a temporary file on disk.
+
+    Returns the path to the temp file. The caller is responsible for cleanup.
+    """
+    temp_file = tempfile.NamedTemporaryFile("wb", dir=tmp_dir, suffix=filename, delete=False)
+    artifact.file.seek(0)
+    shutil.copyfileobj(artifact.file, temp_file)
+    temp_file.flush()
+    temp_file.close()
+    return temp_file.name
+
+
+def artifact_to_python_content_data(filename, artifact, domain=None, temp_path=None):
     """
     Takes the artifact/filename and returns the metadata needed to create a PythonPackageContent.
 
-    If keep_temp_file is True, returns a tuple of (data, temp_file_path) instead of just data.
-    The caller is responsible for cleaning up the temp file.
-    """
-    if keep_temp_file:
-        temp_file = tempfile.NamedTemporaryFile("wb", dir=".", suffix=filename, delete=False)
-        artifact.file.seek(0)
-        shutil.copyfileobj(artifact.file, temp_file)
-        temp_file.flush()
-        temp_file.close()
-        metadata = get_project_metadata_from_file(temp_file.name)
-        data = parse_project_metadata(vars(metadata))
-        data["sha256"] = artifact.sha256
-        data["size"] = artifact.size
-        data["filename"] = filename
-        data["pulp_domain"] = domain or artifact.pulp_domain
-        data["_pulp_domain"] = data["pulp_domain"]
-        return data, temp_file.name
+    If temp_path is provided, uses it instead of copying the artifact to a new temp file.
+    """
+    # Copy file to a temp directory under the user provided filename, we do this
+    # because pkginfo validates that the filename has a valid extension before
+    # reading it
+    if temp_path:
+        metadata = get_project_metadata_from_file(temp_path)
     else:
-        # Copy file to a temp directory under the user provided filename, we do this
-        # because pkginfo validates that the filename has a valid extension before
-        # reading it
         with tempfile.NamedTemporaryFile("wb", dir=".", suffix=filename) as temp_file:
             artifact.file.seek(0)
             shutil.copyfileobj(artifact.file, temp_file)
             temp_file.flush()
             metadata = get_project_metadata_from_file(temp_file.name)
-        data = parse_project_metadata(vars(metadata))
-        data["sha256"] = artifact.sha256
-        data["size"] = artifact.size
-        data["filename"] = filename
-        data["pulp_domain"] = domain or artifact.pulp_domain
-        data["_pulp_domain"] = data["pulp_domain"]
-        return data
+    data = parse_project_metadata(vars(metadata))
+    data["sha256"] = artifact.sha256
+    data["size"] = artifact.size
+    data["filename"] = filename
+    data["pulp_domain"] = domain or artifact.pulp_domain
+    data["_pulp_domain"] = data["pulp_domain"]
+    return data
 
 
 def artifact_to_metadata_artifact(
-    filename: str, artifact: Artifact, tmp_dir: str = "."
+    filename: str, artifact: Artifact, tmp_dir: str = ".", temp_path: str | None = None
 ) -> Artifact | None:
     """
     Creates artifact for metadata from the provided wheel artifact.
+
+    If temp_path is provided, uses it instead of copying the artifact to a new temp file.
     """
     if not filename.endswith(".whl"):
         return None
 
-    with tempfile.NamedTemporaryFile("wb", dir=tmp_dir, suffix=filename, delete=False) as temp_file:
-        temp_wheel_path = temp_file.name
-        artifact.file.seek(0)
-        shutil.copyfileobj(artifact.file, temp_file)
-        temp_file.flush()
+    if temp_path:
+        temp_wheel_path = temp_path
+    else:
+        with tempfile.NamedTemporaryFile(
+            "wb", dir=tmp_dir, suffix=filename, delete=False
+        ) as temp_file:
+            temp_wheel_path = temp_file.name
+            artifact.file.seek(0)
+            shutil.copyfileobj(artifact.file, temp_file)
+            temp_file.flush()
 
     metadata_content = extract_wheel_metadata(temp_wheel_path)
     if not metadata_content:
@@ -315,36 +323,6 @@ def artifact_to_metadata_artifact(
     return metadata_artifact
 
 
-def artifact_to_metadata_artifact_from_path(filename: str, temp_wheel_path: str) -> Artifact | None:
-    """
-    Creates artifact for metadata from an already-extracted wheel temp file.
-
-    Unlike artifact_to_metadata_artifact, this avoids re-reading the artifact
-    from storage by using a temp file that was already written during metadata
-    extraction.
-    """
-    if not filename.endswith(".whl"):
-        return None
-
-    metadata_content = extract_wheel_metadata(temp_wheel_path)
-    if not metadata_content:
-        return None
-
-    with tempfile.NamedTemporaryFile("wb", dir=".", suffix=".metadata", delete=False) as temp_md:
-        temp_metadata_path = temp_md.name
-        temp_md.write(metadata_content)
-        temp_md.flush()
-
-    metadata_artifact = Artifact.init_and_validate(temp_metadata_path)
-    try:
-        metadata_artifact.save()
-    except IntegrityError:
-        metadata_artifact = Artifact.objects.get(
-            sha256=metadata_artifact.sha256, pulp_domain=get_domain()
-        )
-    return metadata_artifact
-
-
 def fetch_json_release_metadata(name: str, version: str, remotes: set[Remote]) -> dict:
     """
     Fetches metadata for a specific release from PyPI's JSON API. A release can contain