diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php --- a/src/applications/feed/story/PhabricatorFeedStory.php +++ b/src/applications/feed/story/PhabricatorFeedStory.php @@ -78,10 +78,11 @@ $object_phids += $phids; } - $objects = id(new PhabricatorObjectQuery()) + $object_query = id(new PhabricatorObjectQuery()) ->setViewer($viewer) - ->withPHIDs(array_keys($object_phids)) - ->execute(); + ->withPHIDs(array_keys($object_phids)); + + $objects = $object_query->execute(); foreach ($key_phids as $key => $phids) { if (!$phids) { @@ -142,8 +143,13 @@ $handle_phids += $key_phids[$key]; } + // NOTE: This setParentQuery() is a little sketchy. Ideally, this whole + // method should be inside FeedQuery and it should be the parent query of + // both subqueries. We're just trying to share the workspace cache. + $handles = id(new PhabricatorHandleQuery()) ->setViewer($viewer) + ->setParentQuery($object_query) ->withPHIDs(array_keys($handle_phids)) ->execute(); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -1073,6 +1073,29 @@ } + /** + * Get a scalar string identifying this user. + * + * This is similar to using the PHID, but distinguishes between ominpotent + * and public users explicitly. This allows safe construction of cache keys + * or cache buckets which do not conflate public and omnipotent users. + * + * @return string Scalar identifier. + */ + public function getCacheFragment() { + if ($this->isOmnipotent()) { + return 'u.omnipotent'; + } + + $phid = $this->getPHID(); + if ($phid) { + return 'u.'.$phid; + } + + return 'u.public'; + } + + /* -( Managing Handles )--------------------------------------------------- */ diff --git a/src/applications/phid/query/PhabricatorHandleQuery.php b/src/applications/phid/query/PhabricatorHandleQuery.php --- a/src/applications/phid/query/PhabricatorHandleQuery.php +++ b/src/applications/phid/query/PhabricatorHandleQuery.php @@ -33,6 +33,7 @@ $object_query = id(new PhabricatorObjectQuery()) ->withPHIDs($phids) + ->setParentQuery($this) ->requireCapabilities($this->getRequiredObjectCapabilities()) ->setViewer($this->getViewer()); diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php --- a/src/applications/phid/query/PhabricatorObjectQuery.php +++ b/src/applications/phid/query/PhabricatorObjectQuery.php @@ -106,19 +106,6 @@ private function loadObjectsByPHID(array $types, array $phids) { $results = array(); - $workspace = $this->getObjectsFromWorkspace($phids); - - foreach ($phids as $key => $phid) { - if (isset($workspace[$phid])) { - $results[$phid] = $workspace[$phid]; - unset($phids[$key]); - } - } - - if (!$phids) { - return $results; - } - $groups = array(); foreach ($phids as $phid) { $type = phid_get_type($phid); @@ -127,6 +114,21 @@ $in_flight = $this->getPHIDsInFlight(); foreach ($groups as $type => $group) { + // We check the workspace for each group, because some groups may trigger + // other groups to load (for example, transactions load their objects). + $workspace = $this->getObjectsFromWorkspace($group); + + foreach ($group as $key => $phid) { + if (isset($workspace[$phid])) { + $results[$phid] = $workspace[$phid]; + unset($group[$key]); + } + } + + if (!$group) { + continue; + } + // Don't try to load PHIDs which are already "in flight"; this prevents // us from recursing indefinitely if policy checks or edges form a loop. // We will decline to load the corresponding objects. @@ -139,7 +141,10 @@ if ($group && isset($types[$type])) { $this->putPHIDsInFlight($group); $objects = $types[$type]->loadObjects($this, $group); - $results += mpull($objects, null, 'getPHID'); + + $map = mpull($objects, null, 'getPHID'); + $this->putObjectsInWorkspace($map); + $results += $map; } } diff --git a/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php b/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php --- a/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php +++ b/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php @@ -143,7 +143,7 @@ public static function getViewerSpaces(PhabricatorUser $viewer) { $cache = PhabricatorCaches::getRequestCache(); - $cache_key = self::KEY_VIEWER.'('.$viewer->getPHID().')'; + $cache_key = self::KEY_VIEWER.'('.$viewer->getCacheFragment().')'; $result = $cache->getKey($cache_key); if ($result === null) { diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -406,8 +406,8 @@ * * **Fully enrich objects pulled from the workspace.** After pulling objects * from the workspace, you still need to load and attach any additional - * content the query requests. Otherwise, a query might return objects without - * requested content. + * content the query requests. Otherwise, a query might return objects + * without requested content. * * Generally, you do not need to update the workspace yourself: it is * automatically populated as a side effect of objects surviving policy @@ -419,16 +419,22 @@ * @task workspace */ public function putObjectsInWorkspace(array $objects) { + $parent = $this->getParentQuery(); + if ($parent) { + $parent->putObjectsInWorkspace($objects); + return $this; + } + assert_instances_of($objects, 'PhabricatorPolicyInterface'); - $viewer_phid = $this->getViewer()->getPHID(); + $viewer_fragment = $this->getViewer()->getCacheFragment(); // The workspace is scoped per viewer to prevent accidental contamination. - if (empty($this->workspace[$viewer_phid])) { - $this->workspace[$viewer_phid] = array(); + if (empty($this->workspace[$viewer_fragment])) { + $this->workspace[$viewer_fragment] = array(); } - $this->workspace[$viewer_phid] += $objects; + $this->workspace[$viewer_fragment] += $objects; return $this; } @@ -445,20 +451,21 @@ * @task workspace */ public function getObjectsFromWorkspace(array $phids) { - $viewer_phid = $this->getViewer()->getPHID(); + $parent = $this->getParentQuery(); + if ($parent) { + return $parent->getObjectsFromWorkspace($phids); + } + + $viewer_fragment = $this->getViewer()->getCacheFragment(); $results = array(); foreach ($phids as $key => $phid) { - if (isset($this->workspace[$viewer_phid][$phid])) { - $results[$phid] = $this->workspace[$viewer_phid][$phid]; + if (isset($this->workspace[$viewer_fragment][$phid])) { + $results[$phid] = $this->workspace[$viewer_fragment][$phid]; unset($phids[$key]); } } - if ($phids && $this->getParentQuery()) { - $results += $this->getParentQuery()->getObjectsFromWorkspace($phids); - } - return $results; }