From b3ae48d8ca2dfe5d2fa28bdad9f86af0251a7cd9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Jun 2015 11:25:01 -0700 Subject: [PATCH 1/2] Improve Differential query plans Summary: Ref T8575. We run a big "(A) UNION (B)" query on the home page and on the main Differential page. "A" can always be improved by using `%Ls`, so it can use the second half of the `(authorPHID, status)` key. "B" can sometimes be improved if the fraction of open revisions is smaller than the fraction of revisions you are reviewing. This is true for me on secure.phabricator.com (I'm a reviewer, either directly or via 'Blessed Reviewers', on about 80% of revisions, but <5% are open). In these cases, a `(status, phid)` key is more efficient. Test Plan: Tweaked queries and added keys on this server, saw less derpy query plans and performance. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8575 Differential Revision: https://secure.phabricator.com/D13325 --- .../query/DifferentialRevisionQuery.php | 16 ++++++++++------ .../storage/DifferentialRevision.php | 8 ++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 69e9477e0d..30731b6fed 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -771,19 +771,23 @@ final class DifferentialRevisionQuery $this->updatedEpochMax); } + // NOTE: Although the status constants are integers in PHP, the column is a + // string column in MySQL, and MySQL won't use keys on string columns if + // you put integers in the query. + switch ($this->status) { case self::STATUS_ANY: break; case self::STATUS_OPEN: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', DifferentialRevisionStatus::getOpenStatuses()); break; case self::STATUS_NEEDS_REVIEW: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, )); @@ -791,7 +795,7 @@ final class DifferentialRevisionQuery case self::STATUS_NEEDS_REVISION: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::NEEDS_REVISION, )); @@ -799,7 +803,7 @@ final class DifferentialRevisionQuery case self::STATUS_ACCEPTED: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::ACCEPTED, )); @@ -807,13 +811,13 @@ final class DifferentialRevisionQuery case self::STATUS_CLOSED: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', DifferentialRevisionStatus::getClosedStatuses()); break; case self::STATUS_ABANDONED: $where[] = qsprintf( $conn_r, - 'r.status IN (%Ld)', + 'r.status IN (%Ls)', array( ArcanistDifferentialRevisionStatus::ABANDONED, )); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 5f2131475b..affa0c4d9a 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -102,6 +102,14 @@ final class DifferentialRevision extends DifferentialDAO 'repositoryPHID' => array( 'columns' => array('repositoryPHID'), ), + // If you (or a project you are a member of) is reviewing a significant + // fraction of the revisions on an install, the result set of open + // revisions may be smaller than the result set of revisions where you + // are a reviewer. In these cases, this key is better than keys on the + // edge table. + 'key_status' => array( + 'columns' => array('status', 'phid'), + ), ), ) + parent::getConfiguration(); } From b3038dcaea2dda7aab7ac84d2a9dfc184b0c2896 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Jun 2015 11:25:19 -0700 Subject: [PATCH 2/2] When showing policy hints, if the Space policy is strictly stronger, show it instead Summary: Ref T8449. Before we show a policy hint in the header of an object, compare it to the space policy (if one exists). If the space policy is strictly stronger (more restrictive -- for example, the Space policy is 'members of X', and the object policy is 'public'), show the space policy instead. See discussion on T8376. Test Plan: {F509126} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8449 Differential Revision: https://secure.phabricator.com/D13328 --- .../__tests__/PhabricatorPolicyTestCase.php | 31 +++++ .../PhabricatorPolicyExplainController.php | 125 ++++++++++++++---- .../policy/storage/PhabricatorPolicy.php | 54 ++++++++ src/view/phui/PHUIHeaderView.php | 26 ++++ 4 files changed, 209 insertions(+), 27 deletions(-) diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php index b2475cdee5..9e7a9d11a9 100644 --- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php +++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php @@ -353,6 +353,37 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase { $this->assertEqual(array(), $result); } + public function testPolicyStrength() { + $public = PhabricatorPolicyQuery::getGlobalPolicy( + PhabricatorPolicies::POLICY_PUBLIC); + $user = PhabricatorPolicyQuery::getGlobalPolicy( + PhabricatorPolicies::POLICY_USER); + $admin = PhabricatorPolicyQuery::getGlobalPolicy( + PhabricatorPolicies::POLICY_ADMIN); + $noone = PhabricatorPolicyQuery::getGlobalPolicy( + PhabricatorPolicies::POLICY_NOONE); + + $this->assertFalse($public->isStrongerThan($public)); + $this->assertFalse($public->isStrongerThan($user)); + $this->assertFalse($public->isStrongerThan($admin)); + $this->assertFalse($public->isStrongerThan($noone)); + + $this->assertTrue($user->isStrongerThan($public)); + $this->assertFalse($user->isStrongerThan($user)); + $this->assertFalse($user->isStrongerThan($admin)); + $this->assertFalse($user->isStrongerThan($noone)); + + $this->assertTrue($admin->isStrongerThan($public)); + $this->assertTrue($admin->isStrongerThan($user)); + $this->assertFalse($admin->isStrongerThan($admin)); + $this->assertFalse($admin->isStrongerThan($noone)); + + $this->assertTrue($noone->isStrongerThan($public)); + $this->assertTrue($noone->isStrongerThan($user)); + $this->assertTrue($noone->isStrongerThan($admin)); + $this->assertFalse($admin->isStrongerThan($noone)); + } + /** * Test an object for visibility across multiple user specifications. diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php index 4224319d9c..46dfad3f57 100644 --- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php +++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php @@ -69,44 +69,115 @@ final class PhabricatorPolicyExplainController $capability_name = $capobj->getCapabilityName(); } - $space_info = null; - if ($object instanceof PhabricatorSpacesInterface) { - if (PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) { - $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( - $object); + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setClass('aphront-access-dialog'); - $handles = $viewer->loadHandles(array($space_phid)); + $this->appendSpaceInformation($dialog, $object, $policy, $capability); - $space_info = array( - pht( - 'This object is in %s, and can only be seen by users with '. - 'access to that space.', - $handles[$space_phid]->renderLink()), - phutil_tag('br'), - phutil_tag('br'), - ); - } - } - - $content = array( - $space_info, - pht('Users with the "%s" capability:', $capability_name), - $auto_info, - ); + $intro = pht( + 'Users with the "%s" capability for this object:', + $capability_name); $object_name = pht( '%s %s', $handle->getTypeName(), $handle->getObjectName()); - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) - ->setClass('aphront-access-dialog') + return $dialog ->setTitle(pht('Policy Details: %s', $object_name)) - ->appendChild($content) + ->appendParagraph($intro) + ->appendChild($auto_info) ->addCancelButton($object_uri, pht('Done')); + } - return id(new AphrontDialogResponse())->setDialog($dialog); + private function appendSpaceInformation( + AphrontDialogView $dialog, + PhabricatorPolicyInterface $object, + PhabricatorPolicy $policy, + $capability) { + $viewer = $this->getViewer(); + + if (!($object instanceof PhabricatorSpacesInterface)) { + return; + } + + if (!PhabricatorSpacesNamespaceQuery::getSpacesExist($viewer)) { + return; + } + + // NOTE: We're intentionally letting users through here, even if they only + // have access to one space. The intent is to help users in "space jail" + // understand who objects they create are visible to: + + $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( + $object); + + $handles = $viewer->loadHandles(array($space_phid)); + $doc_href = PhabricatorEnv::getDoclink('Spaces User Guide'); + + $dialog->appendParagraph( + array( + pht( + 'This object is in %s, and can only be seen or edited by users with '. + 'access to view objects in the space.', + $handles[$space_phid]->renderLink()), + ' ', + phutil_tag( + 'strong', + array(), + phutil_tag( + 'a', + array( + 'href' => $doc_href, + 'target' => '_blank', + ), + pht('Learn More'))), + )); + + $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer); + $space = idx($spaces, $space_phid); + if (!$space) { + return; + } + + $space_policies = PhabricatorPolicyQuery::loadPolicies($viewer, $space); + $space_policy = idx($space_policies, PhabricatorPolicyCapability::CAN_VIEW); + if (!$space_policy) { + return; + } + + $space_explanation = PhabricatorPolicy::getPolicyExplanation( + $viewer, + $space_policy->getPHID()); + $items = array(); + $items[] = $space_explanation; + + foreach ($items as $key => $item) { + $items[$key] = phutil_tag('li', array(), $item); + } + + $dialog->appendParagraph(pht('Users who can see objects in this space:')); + $dialog->appendChild(phutil_tag('ul', array(), $items)); + + $view_capability = PhabricatorPolicyCapability::CAN_VIEW; + if ($capability == $view_capability) { + $stronger = $space_policy->isStrongerThan($policy); + if ($stronger) { + $dialog->appendParagraph( + pht( + 'The space this object is in has a more restrictive view '. + 'policy ("%s") than the object does ("%s"), so the space\'s '. + 'view policy is shown as a hint instead of the object policy.', + $space_policy->getShortName(), + $policy->getShortName())); + } + } + + $dialog->appendParagraph( + pht( + 'After a user passes space policy checks, they must still pass '. + 'object policy checks.')); } } diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index e01e0dd416..c23858b0a8 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -362,6 +362,60 @@ final class PhabricatorPolicy } + /** + * Return `true` if this policy is stronger (more restrictive) than some + * other policy. + * + * Because policies are complicated, determining which policies are + * "stronger" is not trivial. This method uses a very coarse working + * definition of policy strength which is cheap to compute, unambiguous, + * and intuitive in the common cases. + * + * This method returns `true` if the //class// of this policy is stronger + * than the other policy, even if the policies are (or might be) the same in + * practice. For example, "Members of Project X" is considered a stronger + * policy than "All Users", even though "Project X" might (in some rare + * cases) contain every user. + * + * Generally, the ordering here is: + * + * - Public + * - All Users + * - (Everything Else) + * - No One + * + * In the "everything else" bucket, we can't make any broad claims about + * which policy is stronger (and we especially can't make those claims + * cheaply). + * + * Even if we fully evaluated each policy, the two policies might be + * "Members of X" and "Members of Y", each of which permits access to some + * set of unique users. In this case, neither is strictly stronger than + * the other. + * + * @param PhabricatorPolicy Other policy. + * @return bool `true` if this policy is more restrictive than the other + * policy. + */ + public function isStrongerThan(PhabricatorPolicy $other) { + $this_policy = $this->getPHID(); + $other_policy = $other->getPHID(); + + $strengths = array( + PhabricatorPolicies::POLICY_PUBLIC => -2, + PhabricatorPolicies::POLICY_USER => -1, + // (Default policies have strength 0.) + PhabricatorPolicies::POLICY_NOONE => 1, + ); + + $this_strength = idx($strengths, $this->getPHID(), 0); + $other_strength = idx($strengths, $other->getPHID(), 0); + + return ($this_strength > $other_strength); + } + + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php index 29aa2d07df..100ee93d4b 100644 --- a/src/view/phui/PHUIHeaderView.php +++ b/src/view/phui/PHUIHeaderView.php @@ -285,6 +285,32 @@ final class PHUIHeaderView extends AphrontTagView { return null; } + // If an object is in a Space with a strictly stronger (more restrictive) + // policy, we show the more restrictive policy. This better aligns the + // UI hint with the actual behavior. + + // NOTE: We'll do this even if the viewer has access to only one space, and + // show them information about the existence of spaces if they click + // through. + if ($object instanceof PhabricatorSpacesInterface) { + $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( + $object); + + $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer); + $space = idx($spaces, $space_phid); + if ($space) { + $space_policies = PhabricatorPolicyQuery::loadPolicies( + $viewer, + $space); + $space_policy = idx($space_policies, $view_capability); + if ($space_policy) { + if ($space_policy->isStrongerThan($policy)) { + $policy = $space_policy; + } + } + } + } + $phid = $object->getPHID(); $icon = id(new PHUIIconView())