launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32939
[Merge] ~enriqueesanchz/launchpad:add-import-export-endpoints into launchpad:master
Enrique Sánchez has proposed merging ~enriqueesanchz/launchpad:add-import-export-endpoints into launchpad:master.
Commit message:
Add ImportVulnerabilityJob check commit exists
Launchpad only stores the last commit sha1 for a GitRef so we need to
call turnip's API to check if a commit exists in a git repository.
ImportVulnerabilityJob will catch the exception and set an error
message.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/491985
`Vulnerability.importData` wanted to check if the `import_since_commmit_sha1` parameter existed in the targeted GitRef (before starting the job). We thought this could be done just using Launchpad data but it turns out that we only store the last commit for each branch in Launchpad's database (`gitref`).
So we need to make a call to turnip's API to check if the commit exists. `ImportVulnerabilityJob` was already doing a call and raising an exception in the case it does not exists. We now catch that exception and use it to show the custom error message.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-import-export-endpoints into launchpad:master.
diff --git a/lib/lp/bugs/model/importvulnerabilityjob.py b/lib/lp/bugs/model/importvulnerabilityjob.py
index 942a834..4ef6e08 100644
--- a/lib/lp/bugs/model/importvulnerabilityjob.py
+++ b/lib/lp/bugs/model/importvulnerabilityjob.py
@@ -27,6 +27,7 @@ from lp.bugs.model.vulnerabilityjob import (
)
from lp.bugs.scripts.soss.models import SOSSRecord
from lp.bugs.scripts.soss.sossimport import SOSSImporter
+from lp.code.errors import GitRepositoryScanFault
from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.interfaces.gitlookup import IGitLookup
from lp.services.config import config
@@ -206,12 +207,20 @@ class ImportVulnerabilityJob(VulnerabilityJobDerived):
raise exception
# turnip API call to get added/modified files
- stats = getUtility(IGitHostingClient).getDiffStats(
- path=repository.getInternalPath(),
- old=self.import_since_commit_sha1,
- new=ref.commit_sha1,
- logger=logger,
- )
+ try:
+ stats = getUtility(IGitHostingClient).getDiffStats(
+ path=repository.getInternalPath(),
+ old=self.import_since_commit_sha1,
+ new=ref.commit_sha1,
+ logger=logger,
+ )
+ except GitRepositoryScanFault:
+ exception = VulnerabilityJobException(
+ f"Git diff between {self.import_since_commit_sha1} and "
+ f"{ref.commit_sha1} for {self.git_ref} not found"
+ )
+ self.notifyUserError(exception)
+ raise exception
files = [*stats.get("added", ()), *stats.get("modified", ())]
for file in files:
diff --git a/lib/lp/bugs/model/tests/test_vulnerability.py b/lib/lp/bugs/model/tests/test_vulnerability.py
index 55f233a..effa984 100644
--- a/lib/lp/bugs/model/tests/test_vulnerability.py
+++ b/lib/lp/bugs/model/tests/test_vulnerability.py
@@ -850,8 +850,9 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
)
def test_importData_wrong_import_since_commit_sha1(self):
- """Test that we cannot create a ImportVulnerabilityJob when
- import_since_commit_sha1 is not in git_ref.
+ """Test that we can create a ImportVulnerabilityJob when
+ import_since_commit_sha1 is not in git_ref. The job will check that
+ later.
"""
self.useContext(feature_flags())
set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
@@ -869,11 +870,7 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1 = "1" * 40
with person_logged_in(self.requester):
- self.assertRaisesWithContent(
- NotFoundError,
- f"'{import_since_commit_sha1} does not exist in "
- f"{self.git_ref}'",
- getUtility(IVulnerabilitySet).importData,
+ getUtility(IVulnerabilitySet).importData(
self.requester,
self.handler,
repo,
@@ -883,6 +880,19 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1,
)
+ job = getUtility(IImportVulnerabilityJobSource).get(self.handler)
+ naked_job = removeSecurityProxy(job)
+ self.assertIsInstance(naked_job, ImportVulnerabilityJob)
+ self.assertEqual(naked_job.git_repository, repo.id)
+ self.assertEqual(naked_job.git_ref, self.git_ref)
+ self.assertEqual(naked_job.git_paths, self.git_paths)
+ self.assertEqual(
+ naked_job.information_type, self.information_type.value
+ )
+ self.assertEqual(
+ naked_job.import_since_commit_sha1, import_since_commit_sha1
+ )
+
def test_importData_duplicated(self):
"""Test that we cannot create a duplicated ImportVulnerabilityJob
if there is already a peding one for the same handler.
diff --git a/lib/lp/bugs/model/vulnerability.py b/lib/lp/bugs/model/vulnerability.py
index 5f2acd1..c337b8f 100644
--- a/lib/lp/bugs/model/vulnerability.py
+++ b/lib/lp/bugs/model/vulnerability.py
@@ -443,13 +443,8 @@ class VulnerabilitySet:
f"{git_ref} does not exist in the specified git repository"
)
- # Check import_since_commit_sha1 exists in git_ref
- if import_since_commit_sha1 and not git_repository.checkCommitInRef(
- import_since_commit_sha1, git_ref
- ):
- raise NotFoundError(
- f"{import_since_commit_sha1} does not exist in {git_ref}"
- )
+ # The job will check that import_since_commit_sha1 exists in git_ref
+ # making a call to turnip's API
# Trigger the import job after validations pass
job_source = getUtility(IImportVulnerabilityJobSource)
diff --git a/lib/lp/bugs/tests/test_importvulnerabilityjob.py b/lib/lp/bugs/tests/test_importvulnerabilityjob.py
index 104a4b7..8dd6d2a 100644
--- a/lib/lp/bugs/tests/test_importvulnerabilityjob.py
+++ b/lib/lp/bugs/tests/test_importvulnerabilityjob.py
@@ -1,8 +1,11 @@
# Copyright 2025 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+import re
+from contextlib import contextmanager
from pathlib import Path
+import responses
import transaction
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -21,6 +24,10 @@ from lp.code.tests.helpers import GitHostingFixture
from lp.services.features.testing import FeatureFixture
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.tests import block_on_job
+from lp.services.timeout import (
+ get_default_timeout_function,
+ set_default_timeout_function,
+)
from lp.testing import TestCaseWithFactory, person_logged_in
from lp.testing.layers import CeleryJobLayer, DatabaseFunctionalLayer
@@ -30,6 +37,19 @@ class ImportVulnerabilityJobTests(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
+ @contextmanager
+ def mockRequests(self, method, set_default_timeout=True, **kwargs):
+ with responses.RequestsMock() as requests_mock:
+ requests_mock.add(method, re.compile(r".*"), **kwargs)
+ original_timeout_function = get_default_timeout_function()
+ if set_default_timeout:
+ set_default_timeout_function(lambda: 60.0)
+ try:
+ yield
+ finally:
+ set_default_timeout_function(original_timeout_function)
+ self.requests = [call.request for call in requests_mock.calls]
+
def setUp(self):
super().setUp()
self.repository = self.factory.makeGitRepository()
@@ -653,6 +673,54 @@ class ImportVulnerabilityJobTests(TestCaseWithFactory):
},
)
+ def test_run_import_with_wrong_import_since_commit_sha1(self):
+ """Run ImportVulnerabilityJob using import_since_commit_sha1"""
+ self.factory.makeDistribution(name="soss")
+ commit_sha1 = "1" * 40
+
+ # This is the second ref we created in setUp()
+ ref = "ref/tags/v1.0"
+ self.assertEqual(self.repository.refs[1].path, ref)
+
+ job = self.job_source.create(
+ handler=VulnerabilityHandlerEnum.SOSS,
+ git_repository=self.repository.id,
+ git_ref=ref,
+ git_paths=["cves"],
+ information_type=InformationType.PRIVATESECURITY.value,
+ import_since_commit_sha1=commit_sha1,
+ )
+
+ error_msg = (
+ f"Git diff between {commit_sha1} and "
+ f"{self.repository.refs[1].commit_sha1} for {ref} not found"
+ )
+
+ with self.mockRequests("GET", status=404):
+ self.assertRaisesWithContent(
+ VulnerabilityJobException,
+ error_msg,
+ job.run,
+ )
+
+ self.assertEqual(
+ job.metadata,
+ {
+ "request": {
+ "git_repository": self.repository.id,
+ "git_ref": ref,
+ "git_paths": ["cves"],
+ "information_type": InformationType.PRIVATESECURITY.value,
+ "import_since_commit_sha1": commit_sha1,
+ },
+ "result": {
+ "error_description": [error_msg],
+ "succeeded": [],
+ "failed": [],
+ },
+ },
+ )
+
def test_get(self):
"""ImportVulnerabilityJob.get() returns the import job for the given
handler.
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 58f3215..eae554c 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -770,12 +770,6 @@ class IGitRepositoryView(IHasRecipes, IAccessTokenTarget):
"yet been scanned."
)
- def checkCommitInRef(commit, ref):
- """Check if a commit exists in a git ref.
- :param commit: the commit sha1 to look for.
- :param ref: the git reference path where it will look.
- """
-
def updateMergeCommitIDs(paths):
"""Update commit SHA1s of merge proposals for this repository.
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 465b876..483f986 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1424,16 +1424,6 @@ class GitRepository(
)
return not jobs.is_empty()
- def checkCommitInRef(self, commit, ref):
- """See `IGitRepository`."""
- store = Store.of(self)
- return not store.find(
- GitRef.commit_sha1,
- GitRef.repository_id == self.id,
- GitRef.commit_sha1 == commit,
- GitRef.path == ref,
- ).is_empty()
-
def updateMergeCommitIDs(self, paths):
"""See `IGitRepository`."""
store = Store.of(self)
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index e1fe89f..4e48291 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -2560,80 +2560,6 @@ class TestGitRepositoryRefs(TestCaseWithFactory):
hosting_fixture.getRefs.extract_kwargs(),
)
- master_sha1 = hashlib.sha1(b"refs/heads/master").hexdigest()
- author = self.factory.makePerson()
- with person_logged_in(author):
- author_email = author.preferredemail.email
- author_date = datetime(2015, 1, 1, tzinfo=timezone.utc)
- committer_date = datetime(2015, 1, 2, tzinfo=timezone.utc)
- self.useFixture(
- GitHostingFixture(
- commits=[
- {
- "sha1": master_sha1,
- "message": "tip of master",
- "author": {
- "name": author.displayname,
- "email": author_email,
- "time": int(seconds_since_epoch(author_date)),
- },
- "committer": {
- "name": "New Person",
- "email": "new-person@xxxxxxxxxxx",
- "time": int(seconds_since_epoch(committer_date)),
- },
- "parents": [],
- "tree": hashlib.sha1(b"").hexdigest(),
- }
- ]
- )
- )
-
- def test_checkCommitInRef(self):
- """Test that a commit is on a GitRef."""
- repository = self.factory.makeGitRepository()
- paths = ["refs/heads/master", "refs/heads/other-branch"]
- ref = self.factory.makeGitRefs(repository=repository, paths=paths)
-
- # Check that ref[0] is master
- self.assertEqual(ref[0].path, "refs/heads/master")
-
- # Check that the commit we created exists in the specified branch
- result = repository.checkCommitInRef(
- ref[0].commit_sha1, "refs/heads/master"
- )
- self.assertEqual(result, True)
-
- def test_checkCommitInRef_wrong_branch(self):
- """Test that checkCommitInRef returns False when checking a commit from
- a different branch.
- """
- repository = self.factory.makeGitRepository()
- paths = ["refs/heads/master", "refs/heads/other-branch"]
- ref = self.factory.makeGitRefs(repository=repository, paths=paths)
-
- # Check that ref[0] is master
- self.assertEqual(ref[0].path, "refs/heads/master")
-
- # Check that the commit from master does not exist in other-branch
- result = repository.checkCommitInRef(
- ref[0].commit_sha1, "refs/heads/other-branch"
- )
- self.assertEqual(result, False)
-
- def test_checkCommitInRef_wrong_commit_sha1(self):
- """Test that checkCommitInRef returns False when checking a wrong
- commit_sha1.
- """
- repository = self.factory.makeGitRepository()
- paths = ["refs/heads/master", "refs/heads/other-branch"]
- self.factory.makeGitRefs(repository=repository, paths=paths)
-
- # Check that a commit that does not exist in this repo does not exist
- # in master
- result = repository.checkCommitInRef("1" * 40, "refs/heads/master")
- self.assertEqual(result, False)
-
def test_fetchRefCommits(self):
# fetchRefCommits fetches detailed tip commit metadata for the
# requested refs.
