diff --git a/resources/sql/autopatches/20151223.proj.04.keycol.sql b/resources/sql/autopatches/20151223.proj.04.keycol.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20151223.proj.04.keycol.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_project.project + ADD projectPathKey BINARY(4) NOT NULL; diff --git a/resources/sql/autopatches/20151223.proj.05.updatekeys.php b/resources/sql/autopatches/20151223.proj.05.updatekeys.php new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20151223.proj.05.updatekeys.php @@ -0,0 +1,24 @@ +establishConnection('w'); + +foreach (new LiskMigrationIterator($table) as $project) { + $path = $project->getProjectPath(); + $key = $project->getProjectPathKey(); + + if (strlen($path) && ($key !== "\0\0\0\0")) { + continue; + } + + $path_key = PhabricatorHash::digestForIndex($project->getPHID()); + $path_key = substr($path_key, 0, 4); + + queryfx( + $conn_w, + 'UPDATE %T SET projectPath = %s, projectPathKey = %s WHERE id = %d', + $project->getTableName(), + $path_key, + $path_key, + $project->getID()); +} diff --git a/resources/sql/autopatches/20151223.proj.06.uniq.sql b/resources/sql/autopatches/20151223.proj.06.uniq.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20151223.proj.06.uniq.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_project.project + ADD UNIQUE KEY `key_pathkey` (projectPathKey); diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -243,50 +243,6 @@ } private function applyExtendedPolicyChecks(array $extended_objects) { - // First, we're going to detect cycles and reject any objects which are - // part of a cycle. We don't want to loop forever if an object has a - // self-referential or nonsense policy. - - static $in_flight = array(); - - $all_phids = array(); - foreach ($extended_objects as $key => $object) { - $phid = $object->getPHID(); - if (isset($in_flight[$phid])) { - // TODO: This could be more user-friendly. - $this->rejectObject($extended_objects[$key], false, ''); - unset($extended_objects[$key]); - continue; - } - - // We might throw from rejectObject(), so we don't want to actually mark - // anything as in-flight until we survive this entire step. - $all_phids[$phid] = $phid; - } - - foreach ($all_phids as $phid) { - $in_flight[$phid] = true; - } - - $caught = null; - try { - $extended_objects = $this->executeExtendedPolicyChecks($extended_objects); - } catch (Exception $ex) { - $caught = $ex; - } - - foreach ($all_phids as $phid) { - unset($in_flight[$phid]); - } - - if ($caught) { - throw $caught; - } - - return $extended_objects; - } - - private function executeExtendedPolicyChecks(array $extended_objects) { $viewer = $this->viewer; $filter_capabilities = $this->capabilities; @@ -396,10 +352,11 @@ } if ($objects_in) { - $objects_out = id(new PhabricatorPolicyFilter()) - ->setViewer($viewer) - ->requireCapabilities($capabilities) - ->apply($objects_in); + $objects_out = $this->executeExtendedPolicyChecks( + $viewer, + $capabilities, + $objects_in, + $key_map); $objects_out = mpull($objects_out, null, 'getPHID'); } else { $objects_out = array(); @@ -435,6 +392,53 @@ return $extended_objects; } + private function executeExtendedPolicyChecks( + PhabricatorUser $viewer, + array $capabilities, + array $objects, + array $key_map) { + + // Do crude cycle detection by seeing if we have a huge stack depth. + // Although more sophisticated cycle detection is possible in theory, + // it is difficult with hierarchical objects like subprojects. Many other + // checks make it difficult to create cycles normally, so just do a + // simple check here to limit damage. + + static $depth; + + $depth++; + + if ($depth > 32) { + foreach ($objects as $key => $object) { + $this->rejectObject($objects[$key], false, ''); + unset($objects[$key]); + continue; + } + } + + if (!$objects) { + return array(); + } + + $caught = null; + try { + $result = id(new PhabricatorPolicyFilter()) + ->setViewer($viewer) + ->requireCapabilities($capabilities) + ->apply($objects); + } catch (Exception $ex) { + $caught = $ex; + } + + $depth--; + + if ($caught) { + throw $caught; + } + + return $result; + } + private function checkCapability( PhabricatorPolicyInterface $object, $capability) { diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -113,6 +113,80 @@ $this->assertTrue($caught instanceof Exception); } + public function testAncestryQueries() { + $user = $this->createUser(); + $user->save(); + + $ancestor = $this->createProject($user); + $parent = $this->createProject($user, $ancestor); + $child = $this->createProject($user, $parent); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withAncestorProjectPHIDs(array($ancestor->getPHID())) + ->execute(); + $this->assertEqual(2, count($projects)); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withParentProjectPHIDs(array($ancestor->getPHID())) + ->execute(); + $this->assertEqual(1, count($projects)); + $this->assertEqual( + $parent->getPHID(), + head($projects)->getPHID()); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withAncestorProjectPHIDs(array($ancestor->getPHID())) + ->withDepthBetween(2, null) + ->execute(); + $this->assertEqual(1, count($projects)); + $this->assertEqual( + $child->getPHID(), + head($projects)->getPHID()); + + $parent2 = $this->createProject($user, $ancestor); + $child2 = $this->createProject($user, $parent2); + $grandchild2 = $this->createProject($user, $child2); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withAncestorProjectPHIDs(array($ancestor->getPHID())) + ->execute(); + $this->assertEqual(5, count($projects)); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withParentProjectPHIDs(array($ancestor->getPHID())) + ->execute(); + $this->assertEqual(2, count($projects)); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withAncestorProjectPHIDs(array($ancestor->getPHID())) + ->withDepthBetween(2, null) + ->execute(); + $this->assertEqual(3, count($projects)); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withAncestorProjectPHIDs(array($ancestor->getPHID())) + ->withDepthBetween(3, null) + ->execute(); + $this->assertEqual(1, count($projects)); + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withPHIDs( + array( + $child->getPHID(), + $grandchild2->getPHID(), + )) + ->execute(); + $this->assertEqual(2, count($projects)); + } + public function testParentProject() { $user = $this->createUser(); $user->save(); diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -11,6 +11,11 @@ private $nameTokens; private $icons; private $colors; + private $ancestorPHIDs; + private $parentPHIDs; + private $isMilestone; + private $minDepth; + private $maxDepth; private $status = 'status-any'; const STATUS_ANY = 'status-any'; @@ -69,6 +74,27 @@ return $this; } + public function withParentProjectPHIDs($parent_phids) { + $this->parentPHIDs = $parent_phids; + return $this; + } + + public function withAncestorProjectPHIDs($ancestor_phids) { + $this->ancestorPHIDs = $ancestor_phids; + return $this; + } + + public function withIsMilestone($is_milestone) { + $this->isMilestone = $is_milestone; + return $this; + } + + public function withDepthBetween($min, $max) { + $this->minDepth = $min; + $this->maxDepth = $max; + return $this; + } + public function needMembers($need_members) { $this->needMembers = $need_members; return $this; @@ -337,6 +363,65 @@ $this->colors); } + if ($this->parentPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'parentProjectPHID IN (%Ls)', + $this->parentPHIDs); + } + + if ($this->ancestorPHIDs !== null) { + $ancestor_paths = queryfx_all( + $conn, + 'SELECT projectPath, projectDepth FROM %T WHERE phid IN (%Ls)', + id(new PhabricatorProject())->getTableName(), + $this->ancestorPHIDs); + if (!$ancestor_paths) { + throw new PhabricatorEmptyQueryException(); + } + + $sql = array(); + foreach ($ancestor_paths as $ancestor_path) { + $sql[] = qsprintf( + $conn, + '(projectPath LIKE %> AND projectDepth > %d)', + $ancestor_path['projectPath'], + $ancestor_path['projectDepth']); + } + + $where[] = '('.implode(' OR ', $sql).')'; + + $where[] = qsprintf( + $conn, + 'parentProjectPHID IS NOT NULL'); + } + + if ($this->isMilestone !== null) { + if ($this->isMilestone) { + $where[] = qsprintf( + $conn, + 'milestoneNumber IS NOT NULL'); + } else { + $where[] = qsprintf( + $conn, + 'milestoneNumber IS NULL'); + } + } + + if ($this->minDepth !== null) { + $where[] = qsprintf( + $conn, + 'projectDepth >= %d', + $this->minDepth); + } + + if ($this->maxDepth !== null) { + $where[] = qsprintf( + $conn, + 'projectDepth <= %d', + $this->maxDepth); + } + return $where; } diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -33,6 +33,7 @@ protected $projectPath; protected $projectDepth; + protected $projectPathKey; private $memberPHIDs = self::ATTACHABLE; private $watcherPHIDs = self::ATTACHABLE; @@ -196,6 +197,7 @@ 'milestoneNumber' => 'uint32?', 'projectPath' => 'hashpath64', 'projectDepth' => 'uint32', + 'projectPathKey' => 'bytes4', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, @@ -224,6 +226,10 @@ 'key_path' => array( 'columns' => array('projectPath', 'projectDepth'), ), + 'key_pathkey' => array( + 'columns' => array('projectPathKey'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -310,6 +316,12 @@ $this->setPHID($this->generatePHID()); } + if (!strlen($this->getProjectPathKey())) { + $hash = PhabricatorHash::digestForIndex($this->getPHID()); + $hash = substr($hash, 0, 4); + $this->setProjectPathKey($hash); + } + $path = array(); $depth = 0; if ($this->parentProjectPHID) { @@ -317,15 +329,12 @@ $path[] = $parent->getProjectPath(); $depth = $parent->getProjectDepth() + 1; } - $hash = PhabricatorHash::digestForIndex($this->getPHID()); - $path[] = substr($hash, 0, 4); - + $path[] = $this->getProjectPathKey(); $path = implode('', $path); $limit = self::getProjectDepthLimit(); - if (strlen($path) > ($limit * 4)) { - throw new Exception( - pht('Unable to save project: path length is too long.')); + if ($depth >= $limit) { + throw new Exception(pht('Project depth is too great.')); } $this->setProjectPath($path); @@ -403,7 +412,7 @@ $path = $this->getProjectPath(); $parent_length = (strlen($path) - 4); - for ($ii = $parent_length; $ii >= 0; $ii -= 4) { + for ($ii = $parent_length; $ii > 0; $ii -= 4) { $parts[] = substr($path, 0, $ii); }