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
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 61acec4..3a19341 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -45,6 +45,7 @@ from lp.app.errors import (
45 IncompatibleArchiveStatus,45 IncompatibleArchiveStatus,
46 IncompatibleArguments,46 IncompatibleArguments,
47 NotFoundError,47 NotFoundError,
48 GoneError,
48)49)
49from lp.app.interfaces.launchpad import ILaunchpadCelebrities50from lp.app.interfaces.launchpad import ILaunchpadCelebrities
50from lp.app.interfaces.security import IAuthorization51from lp.app.interfaces.security import IAuthorization
@@ -3076,7 +3077,7 @@ class Archive(SQLBase):
3076 pcj = PlainPackageCopyJob.get(job_id)3077 pcj = PlainPackageCopyJob.get(job_id)
3077 job = pcj.job3078 job = pcj.job
3078 if job.status != JobStatus.FAILED:3079 if job.status != JobStatus.FAILED:
3079 raise AssertionError("Job is not failed")3080 raise GoneError("Job is not failed")
3080 Store.of(pcj.context).remove(pcj.context)3081 Store.of(pcj.context).remove(pcj.context)
3081 job.destroySelf()3082 job.destroySelf()
30823083
diff --git a/lib/lp/soyuz/model/packagecopyjob.py b/lib/lp/soyuz/model/packagecopyjob.py
index aee140f..fc91c93 100644
--- a/lib/lp/soyuz/model/packagecopyjob.py
+++ b/lib/lp/soyuz/model/packagecopyjob.py
@@ -189,7 +189,7 @@ class PackageCopyJobDerived(BaseRunnableJob, metaclass=EnumeratedSubclass):
189 its job_type does not match the desired subclass.189 its job_type does not match the desired subclass.
190 """190 """
191 job = IStore(PackageCopyJob).get(PackageCopyJob, job_id)191 job = IStore(PackageCopyJob).get(PackageCopyJob, job_id)
192 if job.job_type != cls.class_job_type:192 if job is None or job.job_type != cls.class_job_type:
193 raise NotFoundError(193 raise NotFoundError(
194 "No object found with id %d and type %s"194 "No object found with id %d and type %s"
195 % (job_id, cls.class_job_type.title)195 % (job_id, cls.class_job_type.title)

Subscribers

People subscribed via source and target branches

to status/vote changes: