From c42c5983aa6af18d4c26f1b5bd8677732ecc0ebf Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 3 Feb 2020 08:40:57 -0800
Subject: [PATCH] Fix an issue where loading a mangled project graph could fail
 too abruptly

Summary:
Ref T13484. If you load a subproject S which has a mangled/invalid `parentPath`, the query currently tries to execute an empty edge query and fatals.

Instead, we want to deny-by-default in the policy layer but not fail the query. The subproject should become restricted but not fatal anything related to it.

See T13484 for a future refinement where we could identify "broken / data integrity issue" objects explicilty.

Test Plan:
  - Modified the `projectPath` of some subproject in the database to `QQQQ...`.
  - Loaded that project page.
  - Before patch: fatal after issuing bad edge query.
  - After patch: "functionally correct" policy layer failure, although an explicit "data integrity issue" failure would be better.

Maniphest Tasks: T13484

Differential Revision: https://secure.phabricator.com/D20963
---
 .../project/query/PhabricatorProjectQuery.php | 19 +++++++++++++++++++
 .../edges/query/PhabricatorEdgeQuery.php      | 12 +++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php
index b68c638363..96efcfba4b 100644
--- a/src/applications/project/query/PhabricatorProjectQuery.php
+++ b/src/applications/project/query/PhabricatorProjectQuery.php
@@ -271,6 +271,25 @@ final class PhabricatorProjectQuery
 
     $all_graph = $this->getAllReachableAncestors($projects);
 
+    // See T13484. If the graph is damaged (and contains a cycle or an edge
+    // pointing at a project which has been destroyed), some of the nodes we
+    // started with may be filtered out by reachability tests. If any of the
+    // projects we are linking up don't have available ancestors, filter them
+    // out.
+
+    foreach ($projects as $key => $project) {
+      $project_phid = $project->getPHID();
+      if (!isset($all_graph[$project_phid])) {
+        $this->didRejectResult($project);
+        unset($projects[$key]);
+        continue;
+      }
+    }
+
+    if (!$projects) {
+      return array();
+    }
+
     // NOTE: Although we may not need much information about ancestors, we
     // always need to test if the viewer is a member, because we will return
     // ancestor projects to the policy filter via ExtendedPolicy calls. If
diff --git a/src/infrastructure/edges/query/PhabricatorEdgeQuery.php b/src/infrastructure/edges/query/PhabricatorEdgeQuery.php
index 6519c47724..f63e827431 100644
--- a/src/infrastructure/edges/query/PhabricatorEdgeQuery.php
+++ b/src/infrastructure/edges/query/PhabricatorEdgeQuery.php
@@ -46,6 +46,13 @@ final class PhabricatorEdgeQuery extends PhabricatorQuery {
    * @task config
    */
   public function withSourcePHIDs(array $source_phids) {
+    if (!$source_phids) {
+      throw new Exception(
+        pht(
+          'Edge list passed to "withSourcePHIDs(...)" is empty, but it must '.
+          'be nonempty.'));
+    }
+
     $this->sourcePHIDs = $source_phids;
     return $this;
   }
@@ -158,11 +165,10 @@ final class PhabricatorEdgeQuery extends PhabricatorQuery {
    * @task exec
    */
   public function execute() {
-    if (!$this->sourcePHIDs) {
+    if ($this->sourcePHIDs === null) {
       throw new Exception(
         pht(
-          'You must use %s to query edges.',
-          'withSourcePHIDs()'));
+          'You must use "withSourcePHIDs()" to query edges.'));
     }
 
     $sources = phid_group_by_type($this->sourcePHIDs);