fix: make sure to update commit info

The pre-process commit task doesn't check if the commit data has
been already pulled from the provider. This can cause an error where
the parent commit is not found to carryforward the reports.

These changes make sure that we try to update the commit info if necessary.
It also moves the function - that was repeated in a few places - to
RepositoryService.

services/repository.py

@@ -126,6 +126,30 @@ async def fetch_appropriate_parent_for_commit(
     return closest_parent_without_message
 
 
+async def possibly_update_commit_from_provider_info(commit, repository_service):
+    repoid = commit.repoid
+    commitid = commit.commitid
+    try:
+        if not commit.message:
+            log.info(
+                "Commit does not have all needed info. Reaching provider to fetch info",
+                extra=dict(repoid=repoid, commit=commitid),
+            )
+            await update_commit_from_provider_info(repository_service, commit)
+            return True
+    except TorngitObjectNotFoundError:
+        log.warning(
+            "Could not update commit with info because it was not found at the provider",
+            extra=dict(repoid=repoid, commit=commitid),
+        )
+        return False
+    log.debug(
+        "Not updating commit because it already seems to be populated",
+        extra=dict(repoid=repoid, commit=commitid),
+    )
+    return False
+
+
 async def update_commit_from_provider_info(repository_service, commit):
     """
     Takes the result from the torngit commit details, and updates the commit

tasks/commit_update.py

@@ -12,6 +12,7 @@
 from helpers.exceptions import RepositoryWithoutValidBotError
 from services.repository import (
     get_repo_provider_service,
+    possibly_update_commit_from_provider_info,
     update_commit_from_provider_info,
 )
 from tasks.base import BaseCodecovTask
@@ -38,7 +39,7 @@ async def run_async(
         was_updated = False
         try:
             repository_service = get_repo_provider_service(repository, commit)
-            was_updated = await self.possibly_update_commit_from_provider_info(
+            was_updated = await possibly_update_commit_from_provider_info(
                 commit, repository_service
             )
         except RepositoryWithoutValidBotError:
@@ -64,32 +65,6 @@ async def run_async(
             )
         return {"was_updated": was_updated}
 
-    # TODO move this into services as it is used in upload task also
-    async def possibly_update_commit_from_provider_info(
-        self, commit, repository_service
-    ):
-        repoid = commit.repoid
-        commitid = commit.commitid
-        try:
-            if not commit.message:
-                log.info(
-                    "Commit does not have all needed info. Reaching provider to fetch info",
-                    extra=dict(repoid=repoid, commit=commitid),
-                )
-                await update_commit_from_provider_info(repository_service, commit)
-                return True
-        except TorngitObjectNotFoundError:
-            log.warning(
-                "Could not update commit with info because it was not found at the provider",
-                extra=dict(repoid=repoid, commit=commitid),
-            )
-            return False
-        log.debug(
-            "Not updating commit because it already seems to be populated",
-            extra=dict(repoid=repoid, commit=commitid),
-        )
-        return False
-
 
 RegisteredCommitUpdateTask = celery_app.register_task(CommitUpdateTask())
 commit_update_task = celery_app.tasks[RegisteredCommitUpdateTask.name]

tasks/preprocess_upload.py

@@ -12,7 +12,10 @@
 from helpers.save_commit_error import save_commit_error
 from services.redis import get_redis_connection
 from services.report import ReportService
-from services.repository import get_repo_provider_service
+from services.repository import (
+    get_repo_provider_service,
+    possibly_update_commit_from_provider_info,
+)
 from services.yaml import save_repo_yaml_to_database_if_needed
 from services.yaml.fetcher import fetch_commit_yaml_from_provider
 from tasks.base import BaseCodecovTask
@@ -82,8 +85,14 @@ async def process_async_within_lock(
         )
         commit = commits.first()
         assert commit, "Commit not found in database."
+
         repository = commit.repository
         repository_service = self.get_repo_service(commit)
+        # Makes sure that we can properly carry forward reports
+        # By populating the commit info (if needed)
+        updated_commit = await possibly_update_commit_from_provider_info(
+            commit=commit, repository_service=repository_service
+        )
         if repository_service:
             commit_yaml = await self.fetch_commit_yaml_and_possibly_store(
                 commit, repository_service
@@ -99,7 +108,11 @@ async def process_async_within_lock(
         commit_report = await report_service.initialize_and_save_report(
             commit, report_code
         )
-        return {"preprocessed_upload": True, "reportid": str(commit_report.external_id)}
+        return {
+            "preprocessed_upload": True,
+            "reportid": str(commit_report.external_id),
+            "updated_commit": updated_commit,
+        }
 
     def get_repo_service(self, commit):
         repository_service = None

tasks/tests/unit/test_commit_update.py

@@ -159,7 +159,7 @@ async def test_update_commit_not_found(
     ):
 
         mock_update_commit_from_provider = mocker.patch(
-            "tasks.commit_update.update_commit_from_provider_info"
+            "tasks.commit_update.possibly_update_commit_from_provider_info"
         )
         mock_update_commit_from_provider.side_effect = TorngitObjectNotFoundError(
             "fake_response", "message"

tasks/tests/unit/test_preprocess_upload.py

@@ -155,6 +155,7 @@ def fake_possibly_shift(report, base, head):
         assert result == {
             "preprocessed_upload": True,
             "reportid": str(report.external_id),
+            "updated_commit": False,
         }
         mocked_fetch_yaml.assert_called()
         mock_possibly_shift.assert_called()

tasks/tests/unit/test_upload_task.py

@@ -234,8 +234,8 @@ async def test_upload_task_upload_processing_delay_not_enough_delay(
         mock_redis,
         celery_app,
     ):
-        mock_possibly_update_commit_from_provider_info = mocker.patch.object(
-            UploadTask, "possibly_update_commit_from_provider_info", return_value=True
+        mock_possibly_update_commit_from_provider_info = mocker.patch(
+            "tasks.upload.possibly_update_commit_from_provider_info", return_value=True
         )
         mocker.patch.object(UploadTask, "possibly_setup_webhooks", return_value=True)
         mocker.patch.object(UploadTask, "fetch_commit_yaml_and_possibly_store")
@@ -281,8 +281,8 @@ async def test_upload_task_upload_processing_delay_enough_delay(
         mock_redis,
         celery_app,
     ):
-        mocker.patch.object(
-            UploadTask, "possibly_update_commit_from_provider_info", return_value=True
+        mocker.patch(
+            "tasks.upload.possibly_update_commit_from_provider_info", return_value=True
         )
         mocker.patch.object(UploadTask, "possibly_setup_webhooks", return_value=True)
         mocker.patch.object(UploadTask, "fetch_commit_yaml_and_possibly_store")
@@ -300,9 +300,6 @@ async def test_upload_task_upload_processing_delay_enough_delay(
         ).timestamp()
         mock_configuration.set_params({"setup": {"upload_processing_delay": 1000}})
         mocker.patch.object(UploadTask, "app", celery_app)
-        mocker.patch.object(
-            UploadTask, "possibly_update_commit_from_provider_info", return_value=True
-        )
         mocker.patch.object(UploadTask, "possibly_setup_webhooks", return_value=True)
         mocker.patch.object(UploadTask, "fetch_commit_yaml_and_possibly_store")
         mocked_chain = mocker.patch("tasks.upload.chain")
@@ -334,8 +331,8 @@ async def test_upload_task_upload_processing_delay_upload_is_none(
     ):
         mock_configuration.set_params({"setup": {"upload_processing_delay": 1000}})
         mocker.patch.object(UploadTask, "app", celery_app)
-        mocker.patch.object(
-            UploadTask, "possibly_update_commit_from_provider_info", return_value=True
+        mocker.patch(
+            "tasks.upload.possibly_update_commit_from_provider_info", return_value=True
         )
         mocker.patch.object(UploadTask, "possibly_setup_webhooks", return_value=True)
         mocker.patch.object(UploadTask, "fetch_commit_yaml_and_possibly_store")
@@ -713,8 +710,8 @@ async def test_upload_task_upload_already_created(
         mock_storage,
     ):
         mocked_schedule_task = mocker.patch.object(UploadTask, "schedule_task")
-        mock_possibly_update_commit_from_provider_info = mocker.patch.object(
-            UploadTask, "possibly_update_commit_from_provider_info", return_value=True
+        mock_possibly_update_commit_from_provider_info = mocker.patch(
+            "tasks.upload.possibly_update_commit_from_provider_info", return_value=True
         )
         mock_create_upload = mocker.patch.object(ReportService, "create_report_upload")
 

tasks/upload.py

@@ -31,6 +31,7 @@
 from services.repository import (
     create_webhook_on_provider,
     get_repo_provider_service,
+    possibly_update_commit_from_provider_info,
     update_commit_from_provider_info,
 )
 from services.yaml import save_repo_yaml_to_database_if_needed
@@ -297,7 +298,7 @@ async def run_async_within_lock(
         was_updated, was_setup = False, False
         try:
             repository_service = get_repo_provider_service(repository, commit)
-            was_updated = await self.possibly_update_commit_from_provider_info(
+            was_updated = await possibly_update_commit_from_provider_info(
                 commit, repository_service
             )
             was_setup = await self.possibly_setup_webhooks(commit, repository_service)
@@ -534,31 +535,6 @@ async def possibly_setup_webhooks(self, commit, repository_service):
                 )
         return False
 
-    async def possibly_update_commit_from_provider_info(
-        self, commit, repository_service
-    ):
-        repoid = commit.repoid
-        commitid = commit.commitid
-        try:
-            if not commit.message:
-                log.info(
-                    "Commit does not have all needed info. Reaching provider to fetch info",
-                    extra=dict(repoid=repoid, commit=commitid),
-                )
-                await update_commit_from_provider_info(repository_service, commit)
-                return True
-        except TorngitObjectNotFoundError:
-            log.warning(
-                "Could not update commit with info because it was not found at the provider",
-                extra=dict(repoid=repoid, commit=commitid),
-            )
-            return False
-        log.debug(
-            "Not updating commit because it already seems to be populated",
-            extra=dict(repoid=repoid, commit=commitid),
-        )
-        return False
-
     def normalize_upload_arguments(
         self, commit: Commit, arguments: Mapping[str, Any], redis_connection: Redis
     ):