Merge ~apw/launchpad:archive-removeCopyNotification-fixes into launchpad:master

Proposed by Andy Whitcroft
Status: Needs review
Proposed branch: ~apw/launchpad:archive-removeCopyNotification-fixes
Merge into: launchpad:master
Diff against target: 34 lines (+3/-2)
2 files modified
lib/lp/soyuz/model/archive.py (+2/-1)
lib/lp/soyuz/model/packagecopyjob.py (+1/-1)
Reviewer Review Type Date Requested Status
Colin Watson (community) Needs Fixing
Review via email: mp+431805@code.launchpad.net

Commit message

removeCopyNotification: error handling fixes

Handle invalid and already acknowledged errors more cleanly reporting NotFound and Gone errors respectively.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

Strictly, I think 410 Gone is really meant to be for cases where the resource identified by the request URL is no longer available, which isn't quite the case here since the archive still exists. Similarly, 404 Not Found would indicate that the archive doesn't exist.

I think we should probably just use 400 Bad Request for this sort of thing. The clearest way to do that would be to invent a new exception type in `lp.soyuz.interfaces.archive` decorated with `@error_status(http.client.BAD_REQUEST)`; you could use the same exception with a different message for the two cases here if you wanted, sticking a `try:/except NotFoundError:` around the `PlainPackageCopyJob.get` call to re-raise it as an exception with the right error status decoration.

You'll also need to update tests for these corner cases. It looks as though they mostly already exist in `lp.soyuz.tests.test_archive.TestRemovingCopyNotifications`, although you might need to add a test for the case of a job ID that doesn't exist at all.

Finally, I wonder if `test_removeCopyNotification_raises_for_wrong_archive` really tests what it claims to be testing. I haven't stepped through it so I might be missing something, but I don't actually see any code that ensures that the job ID is for the context archive; I suspect that the test only passes because the job ID it supplies isn't for a failed job, so it ends up being silently equivalent to `test_removeCopyNotification_raises_for_not_failed`. We'll need to fix this. I'd suggest that all these tests should use `self.assertRaisesWithContent` to assert on the exception string as well as its type; being painfully precise about the exceptions we expect is a good way to avoid this sort of accidental-pass situation.

review: Needs Fixing

Unmerged commits

cbc27c7... by Andy Whitcroft

archive.removeCopyNotification: return GoneError if the job is not in an error state

When a job is not in an error state return GoneError on the assumption
it has been previously acknowledged, this rather than throwing an
AssertionError.

Signed-off-by: Andy Whitcroft <email address hidden>

Failed
[SUCCEEDED] docs:0 (build)
[FAILED] lint:0 (build)
[WAITING] mypy:0 (build)
13 of 3 results
06227ef... by Andy Whitcroft

PlainPackageCopyJob.get: report NotFoundError for invalid job_id

Where a job is completely missing report a NotFoundError rather than
cratering with an AttributeError.

Signed-off-by: Andy Whitcroft <email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
2index 61acec4..3a19341 100644
3--- a/lib/lp/soyuz/model/archive.py
4+++ b/lib/lp/soyuz/model/archive.py
5@@ -45,6 +45,7 @@ from lp.app.errors import (
6 IncompatibleArchiveStatus,
7 IncompatibleArguments,
8 NotFoundError,
9+ GoneError,
10 )
11 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
12 from lp.app.interfaces.security import IAuthorization
13@@ -3076,7 +3077,7 @@ class Archive(SQLBase):
14 pcj = PlainPackageCopyJob.get(job_id)
15 job = pcj.job
16 if job.status != JobStatus.FAILED:
17- raise AssertionError("Job is not failed")
18+ raise GoneError("Job is not failed")
19 Store.of(pcj.context).remove(pcj.context)
20 job.destroySelf()
21
22diff --git a/lib/lp/soyuz/model/packagecopyjob.py b/lib/lp/soyuz/model/packagecopyjob.py
23index aee140f..fc91c93 100644
24--- a/lib/lp/soyuz/model/packagecopyjob.py
25+++ b/lib/lp/soyuz/model/packagecopyjob.py
26@@ -189,7 +189,7 @@ class PackageCopyJobDerived(BaseRunnableJob, metaclass=EnumeratedSubclass):
27 its job_type does not match the desired subclass.
28 """
29 job = IStore(PackageCopyJob).get(PackageCopyJob, job_id)
30- if job.job_type != cls.class_job_type:
31+ if job is None or job.job_type != cls.class_job_type:
32 raise NotFoundError(
33 "No object found with id %d and type %s"
34 % (job_id, cls.class_job_type.title)

Subscribers

People subscribed via source and target branches

to status/vote changes: