Merge lp:~allenap/launchpad/job-status-validator into lp:launchpad

[Gavin Panella]

Consolidate the purpose of Job's status/_status/_set_status code into a single validator.

This removes the status/_status/_set_status issue in Job (where status
is a gettable property, _status represents the database column, and
_set_status is what Job's state transition methods call).

Why?

- I've lost time before wondering why queries aren't working before
  realising I need to use Job._status, so this clears that up.

- It simplifies the code because everything goes via the status
  property, and all transitions are validated.

- Even code within Job can't (accidentally) cheat on status
  transitions, to the extent that a helper, prepare_job(), was
  introduced in the tests in order to get a Job in a desired starting
  state.

[Ian Booth (wallyworld)]

This is a great change. As you say, everything is much clearer now.

review: Approve (code*)

[Jeroen T. Vermeulen (jtv)]

This one's bitten me too, so I'll be very glad to see it go.

One thing I missed was a pre-imp. Gavin, did you verify that there isn't some good but invisible reason for the current design? I believe Aaron wrote this, and he seems to prefer explaining his code in person over leaving comments.

review: Needs Fixing (code)

[Jeroen T. Vermeulen (jtv)]

Ahem. Something went wrong with my vote there.

review: Needs Information (code)

[Gavin Panella (allenap)]

Hi Aaron,

I forgot to ping you today about this, so for now this will have to
do. When you have a chance can you have a gander at this, especially
with respect to Jeroen's comment:

> One thing I missed was a pre-imp. Gavin, did you verify that there
> isn't some good but invisible reason for the current design? I
> believe Aaron wrote this, and he seems to prefer explaining his code
> in person over leaving comments.

I'm very bad at doing pre-implementation calls, so I apologise in
advance if I've done something stupid because of the lack of that.

I'm back on Tuesday, so if you'd like to talk about it let's try to
talk then.

Thanks, Gavin.

[Aaron Bentley (abentley)]

I didn't make _status private because only some transitions are valid. That would be silly, because you can just make a read/write property for that. And the solution you've given would be even better in that case.

I made status private because status is part of a more complex state that includes the date_started and date_finished. It should not be set in isolation, so I made it private and provided methods for setting it to the desired state. By providing a writable status, you encourage people to set it directly, which will produce inconsistent states (e.g. Job.status == JobStatus.RUNNING, but Job.date_started is None).

For example, prepare_job will apparently produce inconsistent states. IIUC, prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED) will produce a job that is COMPLETED, but has no date_started or date_finished. Now obviously, this is test code and it doesn't matter for those particular tests, but at least in the previous version, it was clear that they were doing something slightly dirty, because they were setting a private variable.

review: Disapprove

[Julian Edwards (julian-edwards)]

Why not have a public status that you can read and use in queries, and make the validator reject everything because you're not using the mutator methods.

This is what happens on PackageUpload - see lib/lp/soyuz/model/queue.py and search for validate_status.

The current situation is plainly suboptimal and I agree with the intention of this branch.

[Aaron Bentley (abentley)]

If we can have a readonly "status" attribute and still write to the database column via mutators, that certainly works for me.

[Julian Edwards (julian-edwards)]

That's exactly what the PackageUpload code is doing.

[lp:~allenap/launchpad/job-status-validator]

13127. By Gavin Panella on 2011-05-31

Test to demonstrate Job's attribute access.

13128. By Gavin Panella on 2011-05-31

Test access of all attributes at once, so that we get a complete picture on failure.

13129. By Gavin Panella on 2011-08-15

Merge devel, resolving several conflicts.

13130. By Gavin Panella on 2011-08-15

Change prepare_job() to automatically figure out how to transition.

13131. By Gavin Panella on 2011-08-15

Switch to a single argument version of prepare_job().

13132. By Gavin Panella on 2011-08-15

Move 'invariants' code into validate_status_transition().

13133. By Gavin Panella on 2011-08-15

Use super(); Exception is now a new-style class.

13134. By Gavin Panella on 2011-08-19

Merge devel.

13135. By Gavin Panella on 2011-08-19

Move prepare_job into a testing package.

13136. By Gavin Panella on 2011-08-22

Merge devel.

13137. By Gavin Panella on 2011-08-22

Adjust some more parts of Launchpad to the Job._status -> .status change.

13138. By Gavin Panella on 2011-10-07

Merge devel.

Unmerged revisions

13138. By Gavin Panella on 2011-10-07

Merge devel.

13137. By Gavin Panella on 2011-08-22

Adjust some more parts of Launchpad to the Job._status -> .status change.

13136. By Gavin Panella on 2011-08-22

Merge devel.

13135. By Gavin Panella on 2011-08-19

Move prepare_job into a testing package.

13134. By Gavin Panella on 2011-08-19

Merge devel.

13133. By Gavin Panella on 2011-08-15

Use super(); Exception is now a new-style class.

13132. By Gavin Panella on 2011-08-15

Move 'invariants' code into validate_status_transition().

13131. By Gavin Panella on 2011-08-15

Switch to a single argument version of prepare_job().

13130. By Gavin Panella on 2011-08-15

Change prepare_job() to automatically figure out how to transition.

13129. By Gavin Panella on 2011-08-15

Merge devel, resolving several conflicts.

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2011-09-13 05:23:16 +0000
+++ lib/lp/buildmaster/model/builder.py	2011-10-07 13:40:29 +0000
@@ -854,7 +854,7 @@
             BuildQueue,
             BuildQueue.job == Job.id,
             BuildQueue.builder == self.id,
-            Job._status == JobStatus.RUNNING,
+            Job.status == JobStatus.RUNNING,
             Job.date_started != None).one()
 
     def getCurrentBuildFarmJob(self):
@@ -914,7 +914,7 @@
             Coalesce(BuildQueue.virtualized, True)),
             Processor.id == BuildQueue.processorID,
             Job.id == BuildQueue.jobID,
-            Job._status == JobStatus.WAITING).group_by(
+            Job.status == JobStatus.WAITING).group_by(
                 Processor, Coalesce(BuildQueue.virtualized, True))
 
         result_dict = {'virt': {}, 'nonvirt': {}}
 
=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2010-08-30 15:00:23 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2011-10-07 13:40:29 +0000
@@ -553,6 +553,6 @@
             # Avoid corrupt build jobs where the builder is None.
             BuildQueue.builder != None,
             # status is a property. Let's use _status.
-            Job._status == JobStatus.RUNNING,
+            Job.status == JobStatus.RUNNING,
             Job.date_started != None)
         return result_set
 
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-09-27 00:57:13 +0000
+++ lib/lp/code/model/branch.py	2011-10-07 13:40:29 +0000
@@ -1176,8 +1176,8 @@
             BranchJob,
             BranchJob.branch == self,
             Job.id == BranchJob.jobID,
-            Job._status != JobStatus.COMPLETED,
-            Job._status != JobStatus.FAILED,
+            Job.status != JobStatus.COMPLETED,
+            Job.status != JobStatus.FAILED,
             BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
         return jobs.count() > 0
 
 
=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2011-08-26 00:43:31 +0000
+++ lib/lp/code/model/branchjob.py	2011-10-07 13:40:29 +0000
@@ -926,8 +926,8 @@
             Job.id == BranchJob.jobID,
             BranchJob.branch == branch,
             BranchJob.job_type == BranchJobType.ROSETTA_UPLOAD,
-            Job._status != JobStatus.COMPLETED,
-            Job._status != JobStatus.FAILED)
+            Job.status != JobStatus.COMPLETED,
+            Job.status != JobStatus.FAILED)
         if since is not None:
             match = And(match, Job.date_created > since)
         jobs = store.using(BranchJob, Job).find((BranchJob), match)
 
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2011-09-14 10:19:43 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2011-10-07 13:40:29 +0000
@@ -206,7 +206,7 @@
             BranchMergeProposalJob.job_type ==
             BranchMergeProposalJobType.UPDATE_PREVIEW_DIFF,
             BranchMergeProposalJob.job == Job.id,
-            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
+            Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
         job = jobs.order_by(Job.scheduled_start, Job.date_created).first()
         if job is not None:
             return BranchMergeProposalJobFactory.create(job)
 
=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2011-09-26 20:00:50 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2011-10-07 13:40:29 +0000
@@ -779,7 +779,7 @@
         TargetBranch = ClassAlias(Branch)
         clauses = [
             BranchMergeProposalJob.job == Job.id,
-            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
+            Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
             BranchMergeProposalJob.branch_merge_proposal ==
             BranchMergeProposal.id, BranchMergeProposal.source_branch ==
             SourceBranch.id, BranchMergeProposal.target_branch ==
@@ -794,7 +794,7 @@
         # the date_created, then job type.  This should give us all creation
         # jobs before comment jobs.
         jobs = jobs.order_by(
-            Desc(Job._status), Job.date_created,
+            Desc(Job.status), Job.date_created,
             Desc(BranchMergeProposalJob.job_type))
         # Now only return one job for any given merge proposal.
         ready_jobs = []
 
=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2011-10-06 16:26:36 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2011-10-07 13:40:29 +0000
@@ -1227,7 +1227,7 @@
         job = self._makeRosettaUploadJob()
         job.job.start()
         job.job.complete()
-        job.job._status = JobStatus.FAILED
+        job.job.status = JobStatus.FAILED
         unfinished_jobs = list(RosettaUploadJob.findUnfinishedJobs(
             self.branch))
         self.assertEqual([], unfinished_jobs)
 
=== modified file 'lib/lp/code/scripts/tests/test_scan_branches.py'
--- lib/lp/code/scripts/tests/test_scan_branches.py	2010-10-26 15:47:24 +0000
+++ lib/lp/code/scripts/tests/test_scan_branches.py	2011-10-07 13:40:29 +0000
@@ -76,7 +76,7 @@
         result = store.find(
             BranchJob,
             BranchJob.jobID == Job.id,
-            Job._status == JobStatus.WAITING,
+            Job.status == JobStatus.WAITING,
             BranchJob.job_type == BranchJobType.REVISION_MAIL,
             BranchJob.branch == db_branch)
         self.assertEqual(result.count(), 1)
 
=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py	2011-08-12 14:39:51 +0000
+++ lib/lp/registry/model/persontransferjob.py	2011-10-07 13:40:29 +0000
@@ -370,7 +370,7 @@
         conditions = [
             PersonTransferJob.job_type == cls.class_job_type,
             PersonTransferJob.job_id == Job.id,
-            Job._status.is_in(Job.PENDING_STATUSES)]
+            Job.status.is_in(Job.PENDING_STATUSES)]
         arg_conditions = []
         if from_person is not None:
             arg_conditions.append(
 
=== modified file 'lib/lp/registry/tests/test_person_merge_job.py'
--- lib/lp/registry/tests/test_person_merge_job.py	2011-07-30 09:14:44 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py	2011-10-07 13:40:29 +0000
@@ -174,7 +174,7 @@
     def test_find_only_pending_or_running(self):
         # find() only returns jobs that are pending.
         for status in JobStatus.items:
-            removeSecurityProxy(self.job.job)._status = status
+            removeSecurityProxy(self.job.job).status = status
             if status in Job.PENDING_STATUSES:
                 self.assertEqual([self.job], self.find())
             else:
 
=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py	2011-09-21 13:06:53 +0000
+++ lib/lp/services/job/model/job.py	2011-10-07 13:40:29 +0000
@@ -48,9 +48,33 @@
     """Invalid transition from one job status to another attempted."""
 
     def __init__(self, current_status, requested_status):
-        Exception.__init__(
-            self, 'Transition from %s to %s is invalid.' %
-            (current_status, requested_status))
+        super(InvalidTransition, self).__init__(
+            'Transition from %s to %s is invalid.' % (
+                current_status, requested_status))
+
+
+def validate_status_transition(job, attr, status):
+    """Validate the status transition and maintain "invariants"."""
+    assert attr == "status"
+    if status not in job._valid_transitions[job.status]:
+        raise InvalidTransition(job.status, status)
+
+    # General invariants.
+    if status == JobStatus.RUNNING:
+        job.date_started = datetime.datetime.now(UTC)
+        job.date_finished = None
+        job.attempt_count += 1
+    if status in (JobStatus.COMPLETED, JobStatus.FAILED):
+        job.date_finished = datetime.datetime.now(UTC)
+
+    # Transition-specific invariants.
+    transition = job.status, status
+    if transition == (JobStatus.RUNNING, JobStatus.WAITING):
+        job.date_finished = datetime.datetime.now(UTC)
+    if transition == (JobStatus.SUSPENDED, JobStatus.WAITING):
+        job.lease_expires = None
+
+    return status
 
 
 class Job(SQLBase):
@@ -70,8 +94,9 @@
 
     log = StringCol()
 
-    _status = EnumCol(enum=JobStatus, notNull=True, default=JobStatus.WAITING,
-                      dbName='status')
+    status = EnumCol(
+        enum=JobStatus, notNull=True, default=JobStatus.WAITING,
+        dbName='status', storm_validator=validate_status_transition)
 
     attempt_count = IntCol(default=0)
 
@@ -102,13 +127,6 @@
          JobStatus.RUNNING,
          JobStatus.SUSPENDED))
 
-    def _set_status(self, status):
-        if status not in self._valid_transitions[self._status]:
-            raise InvalidTransition(self._status, status)
-        self._status = status
-
-    status = property(lambda x: x._status)
-
     @property
     def is_pending(self):
         """See `IJob`."""
@@ -153,42 +171,35 @@
 
     def start(self):
         """See `IJob`."""
-        self._set_status(JobStatus.RUNNING)
-        self.date_started = datetime.datetime.now(UTC)
-        self.date_finished = None
-        self.attempt_count += 1
+        self.status = JobStatus.RUNNING
 
     def complete(self):
         """See `IJob`."""
-        self._set_status(JobStatus.COMPLETED)
-        self.date_finished = datetime.datetime.now(UTC)
+        self.status = JobStatus.COMPLETED
 
     def fail(self):
         """See `IJob`."""
-        self._set_status(JobStatus.FAILED)
-        self.date_finished = datetime.datetime.now(UTC)
+        self.status = JobStatus.FAILED
 
     def queue(self):
         """See `IJob`."""
-        self._set_status(JobStatus.WAITING)
-        self.date_finished = datetime.datetime.now(UTC)
+        self.status = JobStatus.WAITING
 
     def suspend(self):
         """See `IJob`."""
-        self._set_status(JobStatus.SUSPENDED)
+        self.status = JobStatus.SUSPENDED
 
     def resume(self):
         """See `IJob`."""
         if self.status is not JobStatus.SUSPENDED:
-            raise InvalidTransition(self._status, JobStatus.WAITING)
-        self._set_status(JobStatus.WAITING)
-        self.lease_expires = None
+            raise InvalidTransition(self.status, JobStatus.WAITING)
+        self.status = JobStatus.WAITING
 
 
 Job.ready_jobs = Select(
     Job.id,
     And(
-        Job._status == JobStatus.WAITING,
+        Job.status == JobStatus.WAITING,
         Or(Job.lease_expires == None, Job.lease_expires < UTC_NOW),
         Or(Job.scheduled_start == None, Job.scheduled_start <= UTC_NOW),
         ))
 
=== added directory 'lib/lp/services/job/testing'
=== added file 'lib/lp/services/job/testing/__init__.py'
--- lib/lp/services/job/testing/__init__.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/job/testing/__init__.py	2011-10-07 13:40:29 +0000
@@ -0,0 +1,37 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Testing resources for lp.services.job."""
+
+__metaclass__ = type
+
+from lp.services.job.model.job import Job
+
+
+def find_transition(s_from, s_to):
+    """Find a valid transition route between `s_from` and `s_to`."""
+    if s_to == s_from:
+        return ()
+    else:
+        valid_transitions = Job._valid_transitions.iteritems()
+        for from_status, to_statuses in valid_transitions:
+            if s_to in to_statuses:
+                if s_from == from_status:
+                    return (s_to,)
+                else:
+                    possible = find_transition(s_from, from_status)
+                    if possible is not None:
+                        return possible + (s_to,)
+        else:
+            return None
+
+
+def prepare_job(desrired_status, job=None):
+    """Creates a new job and transitions its status to `desrired_status`."""
+    if job is None:
+        job = Job()
+    transitions = find_transition(
+        job.status, desrired_status)
+    for status in transitions:
+        job.status = status
+    return job
 
=== modified file 'lib/lp/services/job/tests/test_job.py'
--- lib/lp/services/job/tests/test_job.py	2011-09-21 13:06:53 +0000
+++ lib/lp/services/job/tests/test_job.py	2011-10-07 13:40:29 +0000
@@ -8,6 +8,12 @@
 
 import pytz
 from storm.locals import Store
+from zope.interface.interface import (
+    Attribute,
+    Method,
+    )
+from zope.security.interfaces import ForbiddenAttribute
+from zope.security.proxy import ProxyFactory
 
 from canonical.database.constants import UTC_NOW
 from canonical.launchpad.interfaces.lpstorm import IStore
@@ -22,6 +28,7 @@
     Job,
     LeaseHeld,
     )
+from lp.services.job.testing import prepare_job
 from lp.testing import (
     TestCase,
     TestCaseWithFactory,
@@ -100,17 +107,17 @@
 
     def test_start_when_completed_is_invalid(self):
         """When a job is completed, attempting to start is invalid."""
-        job = Job(_status=JobStatus.COMPLETED)
+        job = prepare_job(JobStatus.COMPLETED)
         self.assertRaises(InvalidTransition, job.start)
 
     def test_start_when_failed_is_invalid(self):
         """When a job is failed, attempting to start is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.start)
 
     def test_start_when_running_is_invalid(self):
         """When a job is running, attempting to start is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.start)
 
     def test_complete(self):
@@ -118,7 +125,7 @@
 
         It should set date_finished and set the job status to COMPLETED.
         """
-        job = Job(_status=JobStatus.RUNNING)
+        job = Job(status=JobStatus.RUNNING)
         self.assertEqual(None, job.date_finished)
         job.complete()
         self.assertNotEqual(None, job.date_finished)
@@ -126,17 +133,17 @@
 
     def test_complete_when_waiting_is_invalid(self):
         """When a job is waiting, attempting to complete is invalid."""
-        job = Job(_status=JobStatus.WAITING)
+        job = prepare_job(JobStatus.WAITING)
         self.assertRaises(InvalidTransition, job.complete)
 
     def test_complete_when_completed_is_invalid(self):
         """When a job is completed, attempting to complete is invalid."""
-        job = Job(_status=JobStatus.COMPLETED)
+        job = prepare_job(JobStatus.COMPLETED)
         self.assertRaises(InvalidTransition, job.complete)
 
     def test_complete_when_failed_is_invalid(self):
         """When a job is failed, attempting to complete is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.complete)
 
     def test_fail(self):
@@ -144,7 +151,7 @@
 
         It should set date_finished and set the job status to FAILED.
         """
-        job = Job(_status=JobStatus.RUNNING)
+        job = Job(status=JobStatus.RUNNING)
         self.assertEqual(None, job.date_finished)
         job.fail()
         self.assertNotEqual(None, job.date_finished)
@@ -152,17 +159,17 @@
 
     def test_fail_when_waiting_is_invalid(self):
         """When a job is waiting, attempting to fail is invalid."""
-        job = Job(_status=JobStatus.WAITING)
+        job = prepare_job(JobStatus.WAITING)
         self.assertRaises(InvalidTransition, job.fail)
 
     def test_fail_when_completed_is_invalid(self):
         """When a job is completed, attempting to fail is invalid."""
-        job = Job(_status=JobStatus.COMPLETED)
+        job = prepare_job(JobStatus.COMPLETED)
         self.assertRaises(InvalidTransition, job.fail)
 
     def test_fail_when_failed_is_invalid(self):
         """When a job is failed, attempting to fail is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.fail)
 
     def test_queue(self):
@@ -170,7 +177,7 @@
 
         It should set date_finished, and set status to WAITING.
         """
-        job = Job(_status=JobStatus.RUNNING)
+        job = Job(status=JobStatus.RUNNING)
         self.assertEqual(None, job.date_finished)
         job.queue()
         self.assertNotEqual(None, job.date_finished)
@@ -178,22 +185,22 @@
 
     def test_queue_when_completed_is_invalid(self):
         """When a job is completed, attempting to queue is invalid."""
-        job = Job(_status=JobStatus.COMPLETED)
+        job = prepare_job(JobStatus.COMPLETED)
         self.assertRaises(InvalidTransition, job.queue)
 
     def test_queue_when_waiting_is_invalid(self):
         """When a job is waiting, attempting to queue is invalid."""
-        job = Job(_status=JobStatus.WAITING)
+        job = prepare_job(JobStatus.WAITING)
         self.assertRaises(InvalidTransition, job.queue)
 
     def test_queue_when_failed_is_invalid(self):
         """When a job is failed, attempting to queue is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.queue)
 
     def test_suspend(self):
         """A job that is in the WAITING state can be suspended."""
-        job = Job(_status=JobStatus.WAITING)
+        job = prepare_job(JobStatus.WAITING)
         job.suspend()
         self.assertEqual(
             job.status,
@@ -201,23 +208,23 @@
 
     def test_suspend_when_running(self):
         """When a job is running, attempting to suspend is valid."""
-        job = Job(_status=JobStatus.RUNNING)
+        job = Job(status=JobStatus.RUNNING)
         job.suspend()
         self.assertEqual(JobStatus.SUSPENDED, job.status)
 
     def test_suspend_when_completed(self):
         """When a job is completed, attempting to suspend is invalid."""
-        job = Job(_status=JobStatus.COMPLETED)
+        job = prepare_job(JobStatus.COMPLETED)
         self.assertRaises(InvalidTransition, job.suspend)
 
     def test_suspend_when_failed(self):
         """When a job is failed, attempting to suspend is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.suspend)
 
     def test_resume(self):
         """A job that is suspended can be resumed."""
-        job = Job(_status=JobStatus.SUSPENDED)
+        job = Job(status=JobStatus.SUSPENDED)
         job.resume()
         self.assertEqual(
             job.status,
@@ -225,30 +232,30 @@
 
     def test_resume_clears_lease_expiry(self):
         """A job that resumes should null out the lease_expiry."""
-        job = Job(_status=JobStatus.SUSPENDED)
+        job = Job(status=JobStatus.SUSPENDED)
         job.lease_expires = UTC_NOW
         job.resume()
         self.assertIs(None, job.lease_expires)
 
     def test_resume_when_running(self):
         """When a job is running, attempting to resume is invalid."""
-        job = Job(_status=JobStatus.RUNNING)
+        job = Job(status=JobStatus.RUNNING)
         self.assertRaises(InvalidTransition, job.resume)
 
     def test_resume_when_completed(self):
         """When a job is completed, attempting to resume is invalid."""
-        job = Job(_status=JobStatus.COMPLETED)
+        job = prepare_job(JobStatus.COMPLETED)
         self.assertRaises(InvalidTransition, job.resume)
 
     def test_resume_when_failed(self):
         """When a job is failed, attempting to resume is invalid."""
-        job = Job(_status=JobStatus.FAILED)
+        job = prepare_job(JobStatus.FAILED)
         self.assertRaises(InvalidTransition, job.resume)
 
     def test_is_pending(self):
         """is_pending is True when the job can possibly complete."""
         for status in JobStatus.items:
-            job = Job(_status=status)
+            job = prepare_job(status)
             self.assertEqual(
                 status in Job.PENDING_STATUSES, job.is_pending)
 
@@ -272,7 +279,7 @@
     def test_ready_jobs_started(self):
         """Job.ready_jobs should not jobs that have been started."""
         preexisting = self._sampleData()
-        job = Job(_status=JobStatus.RUNNING)
+        job = Job(status=JobStatus.RUNNING)
         self.assertEqual(
             preexisting, list(Store.of(job).execute(Job.ready_jobs)))
 
@@ -340,3 +347,46 @@
         job = Job()
         job.acquireLease(-300)
         self.assertEqual(0, job.getTimeout())
+
+    def test_attribute_access(self):
+        # Job's security proxy allows anyone to its attributes, but no one may
+        # set them.
+        job = ProxyFactory(Job())
+
+        def can_read(name):
+            try:
+                getattr(job, name)
+            except ForbiddenAttribute:
+                return False
+            else:
+                return True
+
+        def can_write(name):
+            try:
+                setattr(job, name, None)
+            except ForbiddenAttribute:
+                return False
+            else:
+                return True
+
+        attributes = set(
+            name for name in IJob
+            if isinstance(IJob[name], Attribute)
+            and not isinstance(IJob[name], Method))
+
+        expected = dict.fromkeys(attributes, "r")
+
+        observed = {}
+        for attribute in attributes:
+            read = can_read(attribute)
+            write = can_write(attribute)
+            if read and write:
+                observed[attribute] = "rw"
+            elif read:
+                observed[attribute] = "r"
+            elif write:
+                observed[attribute] = "w"
+            else:
+                observed[attribute] = "-"
+
+        self.assertEqual(expected, observed)
 
=== modified file 'lib/lp/soyuz/model/distroseriesdifferencejob.py'
--- lib/lp/soyuz/model/distroseriesdifferencejob.py	2011-09-09 19:34:10 +0000
+++ lib/lp/soyuz/model/distroseriesdifferencejob.py	2011-10-07 13:40:29 +0000
@@ -255,7 +255,7 @@
             DistributionJob,
             DistributionJob.job_type == cls.class_job_type,
             Job.id == DistributionJob.job_id,
-            Job._status.is_in(Job.PENDING_STATUSES),
+            Job.status.is_in(Job.PENDING_STATUSES),
             DistributionJob.distroseries == derived_series)
 
         parent_series_ids = set(
 
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-09-13 13:41:36 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-10-07 13:40:29 +0000
@@ -325,7 +325,7 @@
             PackageCopyJob.job_type == cls.class_job_type,
             PackageCopyJob.target_archive == target_archive,
             Job.id == PackageCopyJob.job_id,
-            Job._status == JobStatus.WAITING)
+            Job.status == JobStatus.WAITING)
         jobs = jobs.order_by(PackageCopyJob.id)
         return DecoratedResultSet(jobs, cls)
 
@@ -337,7 +337,7 @@
             Job.id == PackageCopyJob.job_id,
             PackageCopyJob.job_type == cls.class_job_type,
             PackageCopyJob.target_distroseries == target_series,
-            Job._status.is_in(Job.PENDING_STATUSES))
+            Job.status.is_in(Job.PENDING_STATUSES))
         raw_jobs = raw_jobs.order_by(PackageCopyJob.id)
         return DecoratedResultSet(raw_jobs, cls)
 
 
=== modified file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-08-31 14:12:38 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-10-07 13:40:29 +0000
@@ -27,6 +27,7 @@
 from lp.services.database import bulk
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.testing import prepare_job
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
@@ -506,7 +507,7 @@
         dsd = self.factory.makeDistroSeriesDifference()
         job = create_job(
             dsd.derived_series, dsd.source_package_name, dsd.parent_series)
-        removeSecurityProxy(job).job._status = JobStatus.COMPLETED
+        prepare_job(JobStatus.COMPLETED, removeSecurityProxy(job))
         self.assertEqual(
             {},
             self.getJobSource().getPendingJobsForDifferences(
 
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-08-30 14:24:13 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-10-07 13:40:29 +0000
@@ -33,6 +33,8 @@
     JobStatus,
     SuspendJobException,
     )
+from lp.services.job.model.job import Job
+from lp.services.job.testing import prepare_job
 from lp.soyuz.adapters.overrides import SourceOverride
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -234,7 +236,7 @@
     def test_getActiveJobs_only_returns_waiting_jobs(self):
         # getActiveJobs ignores jobs that aren't in the WAITING state.
         job = self.makeJob()
-        removeSecurityProxy(job).job._status = JobStatus.RUNNING
+        removeSecurityProxy(job).job.status = JobStatus.RUNNING
         source = getUtility(IPlainPackageCopyJobSource)
         self.assertContentEqual([], source.getActiveJobs(job.target_archive))
 
@@ -480,21 +482,16 @@
     def test_getPendingJobsPerPackage_only_returns_pending_jobs(self):
         # getPendingJobsPerPackage ignores jobs that have already been
         # run.
-        dsd = self.factory.makeDistroSeriesDifference()
-        job = self.makeJob(dsd)
+        series = self.factory.makeDistroSeries()
         job_source = getUtility(IPlainPackageCopyJobSource)
-        found_by_state = {}
         for status in JobStatus.items:
-            removeSecurityProxy(job).job._status = status
-            result = job_source.getPendingJobsPerPackage(dsd.derived_series)
-            if len(result) > 0:
-                found_by_state[status] = result[dsd.source_package_name.name]
-        expected = {
-            JobStatus.WAITING: job,
-            JobStatus.RUNNING: job,
-            JobStatus.SUSPENDED: job,
-        }
-        self.assertEqual(expected, found_by_state)
+            dsd = self.factory.makeDistroSeriesDifference(
+                derived_series=series)
+            job = self.makeJob(dsd)
+            prepare_job(status, removeSecurityProxy(job))
+        jobs = job_source.getPendingJobsPerPackage(series)
+        statuses = set(job.status for job in jobs.itervalues())
+        self.assertEqual(Job.PENDING_STATUSES, statuses)
 
     def test_getPendingJobsPerPackage_distinguishes_jobs(self):
         # getPendingJobsPerPackage associates the right job with the
 
=== modified file 'lib/lp/translations/model/translationsharingjob.py'
--- lib/lp/translations/model/translationsharingjob.py	2011-08-01 08:12:35 +0000
+++ lib/lp/translations/model/translationsharingjob.py	2011-10-07 13:40:29 +0000
@@ -232,7 +232,7 @@
             (TranslationSharingJob.productseries_id ==
              packaging.productseries.id),
             TranslationSharingJob.job_type == cls.class_job_type,
-            Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
+            Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
         result.order_by(TranslationSharingJob.id)
         job = result.first()
         if job is None: