Make Drydock more broadly aware of policies

epriestley · epriestley · commit 9b0fa5747bf4 · 2013-12-27T13:15:19.000-08:00
Summary: Ref T2015. Moves a bunch of raw object loads into modern policy-aware queries. Also straightens out the Log and Lease policies a little bit: there are legitimate states where these objects are not attached to a resource (particularly, while a lease is being acquired). Handle these more gracefully. Test Plan: Lint / browsed stuff. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D7836
diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
@@ -122,6 +122,7 @@ final public function allocateLease(
       $resource->beginReadLocking();
         $resource->reload();
 
+        // TODO: Policy stuff.
         $other_leases = id(new DrydockLease())->loadAllWhere(
           'status IN (%Ld) AND resourceID = %d',
           array(
@@ -388,13 +389,13 @@ public static function getAllBlueprintImplementationsForResource($type) {
   }
 
   protected function newResourceTemplate($name) {
-    $resource = new DrydockResource();
-    $resource->setBlueprintPHID($this->getInstance()->getPHID());
-    $resource->setBlueprintClass($this->getBlueprintClass());
-    $resource->setType($this->getType());
-    $resource->setStatus(DrydockResourceStatus::STATUS_PENDING);
-    $resource->setName($name);
-    $resource->save();
+    $resource = id(new DrydockResource())
+      ->setBlueprintPHID($this->getInstance()->getPHID())
+      ->setBlueprintClass($this->getBlueprintClass())
+      ->setType($this->getType())
+      ->setStatus(DrydockResourceStatus::STATUS_PENDING)
+      ->setName($name)
+      ->save();
 
     $this->activeResource = $resource;
 
diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -52,6 +52,7 @@ protected function executeAllocateResource(DrydockLease $lease) {
         throw new Exception("Unsupported VCS!");
     }
 
+    // TODO: Policy stuff here too.
     $host_lease = id(new DrydockLease())
       ->setResourceType('host')
       ->waitUntilActive();
diff --git a/src/applications/drydock/controller/DrydockBlueprintCreateController.php b/src/applications/drydock/controller/DrydockBlueprintCreateController.php
@@ -16,12 +16,12 @@ public function processRequest() {
         return $this->createDialog($implementations);
       }
 
-      $blueprint = new DrydockBlueprint();
-      $blueprint->setClassName($class);
-      $blueprint->setDetails(array());
-      $blueprint->setViewPolicy(PhabricatorPolicies::POLICY_ADMIN);
-      $blueprint->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN);
-      $blueprint->save();
+      $blueprint = id(new DrydockBlueprint())
+        ->setClassName($class)
+        ->setDetails(array())
+        ->setViewPolicy(PhabricatorPolicies::POLICY_ADMIN)
+        ->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN)
+        ->save();
 
       $edit_uri = $this->getApplicationURI(
         "blueprint/edit/".$blueprint->getID()."/");
diff --git a/src/applications/drydock/controller/DrydockBlueprintViewController.php b/src/applications/drydock/controller/DrydockBlueprintViewController.php
@@ -10,9 +10,12 @@ public function willProcessRequest(array $data) {
 
   public function processRequest() {
     $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();
 
-    $blueprint = id(new DrydockBlueprint())->load($this->id);
+    $blueprint = id(new DrydockBlueprintQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
     if (!$blueprint) {
       return new Aphront404Response();
     }
@@ -30,7 +33,7 @@ public function processRequest() {
 
     $resources = id(new DrydockResourceQuery())
       ->withBlueprintPHIDs(array($blueprint->getPHID()))
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->execute();
 
     $resource_list = $this->buildResourceListView($resources);
diff --git a/src/applications/drydock/controller/DrydockLeaseViewController.php b/src/applications/drydock/controller/DrydockLeaseViewController.php
@@ -10,9 +10,12 @@ public function willProcessRequest(array $data) {
 
   public function processRequest() {
     $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();
 
-    $lease = id(new DrydockLease())->load($this->id);
+    $lease = id(new DrydockLeaseQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
     if (!$lease) {
       return new Aphront404Response();
     }
@@ -32,7 +35,7 @@ public function processRequest() {
     $pager->setOffset($request->getInt('offset'));
 
     $logs = id(new DrydockLogQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->withLeaseIDs(array($lease->getID()))
       ->executeWithOffsetPager($pager);
 
diff --git a/src/applications/drydock/controller/DrydockResourceCloseController.php b/src/applications/drydock/controller/DrydockResourceCloseController.php
@@ -10,9 +10,12 @@ public function willProcessRequest(array $data) {
 
   public function processRequest() {
     $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();
 
-    $resource = id(new DrydockResource())->load($this->id);
+    $resource = id(new DrydockResourceQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
     if (!$resource) {
       return new Aphront404Response();
     }
@@ -22,7 +25,7 @@ public function processRequest() {
 
     if ($resource->getStatus() != DrydockResourceStatus::STATUS_OPEN) {
       $dialog = id(new AphrontDialogView())
-        ->setUser($user)
+        ->setUser($viewer)
         ->setTitle(pht('Resource Not Open'))
         ->appendChild(phutil_tag('p', array(), pht(
           'You can only close "open" resources.')))
@@ -31,22 +34,22 @@ public function processRequest() {
       return id(new AphrontDialogResponse())->setDialog($dialog);
     }
 
-    if (!$request->isDialogFormPost()) {
-      $dialog = id(new AphrontDialogView())
-        ->setUser($user)
-        ->setTitle(pht('Really close resource?'))
-        ->appendChild(phutil_tag('p', array(), pht(
-          'Closing a resource releases all leases and destroys the '.
-          'resource. It can not be undone. Continue?')))
-        ->addSubmitButton(pht('Close Resource'))
-        ->addCancelButton($resource_uri);
-
-      return id(new AphrontDialogResponse())->setDialog($dialog);
+    if ($request->isFormPost()) {
+      $resource->closeResource();
+      return id(new AphrontReloadResponse())->setURI($resource_uri);
     }
 
-    $resource->closeResource();
+    $dialog = id(new AphrontDialogView())
+      ->setUser($viewer)
+      ->setTitle(pht('Really close resource?'))
+      ->appendChild(
+        pht(
+          'Closing a resource releases all leases and destroys the '.
+          'resource. It can not be undone. Continue?'))
+      ->addSubmitButton(pht('Close Resource'))
+      ->addCancelButton($resource_uri);
 
-    return id(new AphrontReloadResponse())->setURI($resource_uri);
+    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }
diff --git a/src/applications/drydock/controller/DrydockResourceViewController.php b/src/applications/drydock/controller/DrydockResourceViewController.php
@@ -10,9 +10,12 @@ public function willProcessRequest(array $data) {
 
   public function processRequest() {
     $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();
 
-    $resource = id(new DrydockResource())->load($this->id);
+    $resource = id(new DrydockResourceQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
     if (!$resource) {
       return new Aphront404Response();
     }
@@ -29,7 +32,7 @@ public function processRequest() {
     $resource_uri = $this->getApplicationURI($resource_uri);
 
     $leases = id(new DrydockLeaseQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->withResourceIDs(array($resource->getID()))
       ->execute();
 
@@ -41,7 +44,7 @@ public function processRequest() {
     $pager->setOffset($request->getInt('offset'));
 
     $logs = id(new DrydockLogQuery())
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->withResourceIDs(array($resource->getID()))
       ->executeWithOffsetPager($pager);
 
diff --git a/src/applications/drydock/management/DrydockManagementCloseWorkflow.php b/src/applications/drydock/management/DrydockManagementCloseWorkflow.php
@@ -25,8 +25,15 @@ public function execute(PhutilArgumentParser $args) {
         "Specify one or more resource IDs to close.");
     }
 
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
+    $resources = id(new DrydockResourceQuery())
+      ->setViewer($viewer)
+      ->withIDs($ids)
+      ->execute();
+
     foreach ($ids as $id) {
-      $resource = id(new DrydockResource())->load($id);
+      $resource = idx($resources, $id);
       if (!$resource) {
         $console->writeErr("Resource %d does not exist!\n", $id);
       } else if ($resource->getStatus() != DrydockResourceStatus::STATUS_OPEN) {
diff --git a/src/applications/drydock/management/DrydockManagementCreateResourceWorkflow.php b/src/applications/drydock/management/DrydockManagementCreateResourceWorkflow.php
@@ -49,17 +49,22 @@ public function execute(PhutilArgumentParser $args) {
       $attributes = $options->parse($attributes);
     }
 
-    $blueprint = id(new DrydockBlueprint())->load((int)$blueprint_id);
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
+    $blueprint = id(new DrydockBlueprintQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($blueprint_id))
+      ->executeOne();
     if (!$blueprint) {
       throw new PhutilArgumentUsageException(
         "Specified blueprint does not exist.");
     }
 
-    $resource = new DrydockResource();
-    $resource->setBlueprintPHID($blueprint->getPHID());
-    $resource->setType($blueprint->getImplementation()->getType());
-    $resource->setName($resource_name);
-    $resource->setStatus(DrydockResourceStatus::STATUS_OPEN);
+    $resource = id(new DrydockResource())
+      ->setBlueprintPHID($blueprint->getPHID())
+      ->setType($blueprint->getImplementation()->getType())
+      ->setName($resource_name)
+      ->setStatus(DrydockResourceStatus::STATUS_OPEN);
     if ($attributes) {
       $resource->setAttributes($attributes);
     }
diff --git a/src/applications/drydock/query/DrydockLeaseQuery.php b/src/applications/drydock/query/DrydockLeaseQuery.php
@@ -44,17 +44,25 @@ public function loadPage() {
   }
 
   public function willFilterPage(array $leases) {
-    $resources = id(new DrydockResourceQuery())
-      ->setParentQuery($this)
-      ->setViewer($this->getViewer())
-      ->withIDs(mpull($leases, 'getResourceID'))
-      ->execute();
+    $resource_ids = array_filter(mpull($leases, 'getResourceID'));
+    if ($resource_ids) {
+      $resources = id(new DrydockResourceQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withIDs($resource_ids)
+        ->execute();
+    } else {
+      $resources = array();
+    }
 
     foreach ($leases as $key => $lease) {
-      $resource = idx($resources, $lease->getResourceID());
-      if (!$resource) {
-        unset($leases[$key]);
-        continue;
+      $resource = null;
+      if ($lease->getResourceID()) {
+        $resource = idx($resources, $lease->getResourceID());
+        if (!$resource) {
+          unset($leases[$key]);
+          continue;
+        }
       }
       $lease->attachResource($resource);
     }
diff --git a/src/applications/drydock/query/DrydockLogQuery.php b/src/applications/drydock/query/DrydockLogQuery.php
@@ -31,18 +31,60 @@ public function loadPage() {
   }
 
   public function willFilterPage(array $logs) {
-    $resource_ids = mpull($logs, 'getResourceID');
-    $resources = id(new DrydockResourceQuery())
-      ->setParentQuery($this)
-      ->setViewer($this->getViewer())
-      ->withIDs($resource_ids)
-      ->execute();
+    $resource_ids = array_filter(mpull($logs, 'getResourceID'));
+    if ($resource_ids) {
+      $resources = id(new DrydockResourceQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withIDs($resource_ids)
+        ->execute();
+    } else {
+      $resources = array();
+    }
 
     foreach ($logs as $key => $log) {
-      $resource = idx($resources, $log->getResourceID());
+      $resource = null;
+      if ($log->getResourceID()) {
+        $resource = idx($resources, $log->getResourceID());
+        if (!$resource) {
+          unset($logs[$key]);
+          continue;
+        }
+      }
       $log->attachResource($resource);
     }
 
+    $lease_ids = array_filter(mpull($logs, 'getLeaseID'));
+    if ($lease_ids) {
+      $leases = id(new DrydockLeaseQuery())
+        ->setParentQuery($this)
+        ->setViewer($this->getViewer())
+        ->withIDs($lease_ids)
+        ->execute();
+    } else {
+      $leases = array();
+    }
+
+    foreach ($logs as $key => $log) {
+      $lease = null;
+      if ($log->getLeaseID()) {
+        $lease = idx($leases, $log->getLeaseID());
+        if (!$lease) {
+          unset($logs[$key]);
+          continue;
+        }
+      }
+      $log->attachLease($lease);
+    }
+
+    // These logs are meaningless and their policies aren't computable. They
+    // shouldn't exist, but throw them away if they do.
+    foreach ($logs as $key => $log) {
+      if (!$log->getResource() && !$log->getLease()) {
+        unset($logs[$key]);
+      }
+    }
+
     return $logs;
   }
 
diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php
@@ -67,7 +67,7 @@ public function getResource() {
     return $this->assertAttached($this->resource);
   }
 
-  public function attachResource(DrydockResource $resource) {
+  public function attachResource(DrydockResource $resource = null) {
     $this->resource = $resource;
     return $this;
   }
@@ -194,11 +194,17 @@ public function getCapabilities() {
   }
 
   public function getPolicy($capability) {
-    return $this->getResource()->getPolicy($capability);
+    if ($this->getResource()) {
+      return $this->getResource()->getPolicy($capability);
+    }
+    return PhabricatorPolicies::getMostOpenPolicy();
   }
 
   public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    return $this->getResource()->hasAutomaticCapability($capability, $viewer);
+    if ($this->getResource()) {
+      return $this->getResource()->hasAutomaticCapability($capability, $viewer);
+    }
+    return false;
   }
 
   public function describeAutomaticCapability($capability) {
diff --git a/src/applications/drydock/storage/DrydockLog.php b/src/applications/drydock/storage/DrydockLog.php
diff --git a/src/applications/drydock/storage/DrydockResource.php b/src/applications/drydock/storage/DrydockResource.php
diff --git a/src/applications/drydock/worker/DrydockAllocatorWorker.php b/src/applications/drydock/worker/DrydockAllocatorWorker.php
diff --git a/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php b/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php
diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php

Original file line number	Diff line number	Diff line change
`@@ -52,6 +52,7 @@ protected function executeAllocateResource(DrydockLease $lease) {`
`52`	`52`	`throw new Exception("Unsupported VCS!");`
`53`	`53`	`}`
`54`	`54`
	`55`	`+ // TODO: Policy stuff here too.`
`55`	`56`	`$host_lease = id(new DrydockLease())`
`56`	`57`	`->setResourceType('host')`
`57`	`58`	`->waitUntilActive();`