mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-10 08:52:39 +01:00
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
This commit is contained in:
parent
42e46bbe5a
commit
c42c5983aa
2 changed files with 28 additions and 3 deletions
|
@ -271,6 +271,25 @@ final class PhabricatorProjectQuery
|
||||||
|
|
||||||
$all_graph = $this->getAllReachableAncestors($projects);
|
$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
|
// 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
|
// always need to test if the viewer is a member, because we will return
|
||||||
// ancestor projects to the policy filter via ExtendedPolicy calls. If
|
// ancestor projects to the policy filter via ExtendedPolicy calls. If
|
||||||
|
|
|
@ -46,6 +46,13 @@ final class PhabricatorEdgeQuery extends PhabricatorQuery {
|
||||||
* @task config
|
* @task config
|
||||||
*/
|
*/
|
||||||
public function withSourcePHIDs(array $source_phids) {
|
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;
|
$this->sourcePHIDs = $source_phids;
|
||||||
return $this;
|
return $this;
|
||||||
}
|
}
|
||||||
|
@ -158,11 +165,10 @@ final class PhabricatorEdgeQuery extends PhabricatorQuery {
|
||||||
* @task exec
|
* @task exec
|
||||||
*/
|
*/
|
||||||
public function execute() {
|
public function execute() {
|
||||||
if (!$this->sourcePHIDs) {
|
if ($this->sourcePHIDs === null) {
|
||||||
throw new Exception(
|
throw new Exception(
|
||||||
pht(
|
pht(
|
||||||
'You must use %s to query edges.',
|
'You must use "withSourcePHIDs()" to query edges.'));
|
||||||
'withSourcePHIDs()'));
|
|
||||||
}
|
}
|
||||||
|
|
||||||
$sources = phid_group_by_type($this->sourcePHIDs);
|
$sources = phid_group_by_type($this->sourcePHIDs);
|
||||||
|
|
Loading…
Reference in a new issue