@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Skip loading attached objects for files when we know the file is visible
Summary:
Depends on D19222. Ref T13106. We currently execute an edge query (and possibly an object query) when loading builtin files, but this is never necessary because we know these files are always visible.
Instead, skip this logic for builtin files and profile image files; these files have global visibility and will never get a different policy result because of file attachment information.
(In theory, we could additionally skip this for files with the most open visibility policy or some other trivially visible policy like the user's PHID, but we do actually care about the attachment data some of the time.)
Test Plan: Saw queries drop from 151 to 145 on local test page. Checked file attachment data in Files, saw it still working correctly.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19223
+55
-27
+55
-27
src/applications/files/query/PhabricatorFileQuery.php
+55
-27
src/applications/files/query/PhabricatorFileQuery.php
···
152
152
}
153
153
154
154
protected function loadPage() {
155
-
$files = $this->loadStandardPage(new PhabricatorFile());
155
+
$files = $this->loadStandardPage($this->newResultObject());
156
156
157
157
if (!$files) {
158
158
return $files;
159
159
}
160
160
161
-
$viewer = $this->getViewer();
162
-
$is_omnipotent = $viewer->isOmnipotent();
161
+
// Figure out which files we need to load attached objects for. In most
162
+
// cases, we need to load attached objects to perform policy checks for
163
+
// files.
163
164
164
-
// We need to load attached objects to perform policy checks for files.
165
-
// First, load the edges.
166
-
167
-
$edge_type = PhabricatorFileHasObjectEdgeType::EDGECONST;
168
-
$file_phids = mpull($files, 'getPHID');
169
-
$edges = id(new PhabricatorEdgeQuery())
170
-
->withSourcePHIDs($file_phids)
171
-
->withEdgeTypes(array($edge_type))
172
-
->execute();
173
-
174
-
$object_phids = array();
165
+
// However, in some special cases where we know files will always be
166
+
// visible, we skip this. See T8478 and T13106.
167
+
$need_objects = array();
175
168
foreach ($files as $file) {
176
-
$phids = array_keys($edges[$file->getPHID()][$edge_type]);
177
-
$file->attachObjectPHIDs($phids);
169
+
$always_visible = false;
178
170
179
171
if ($file->getIsProfileImage()) {
180
-
// If this is a profile image, don't bother loading related files.
181
-
// It will always be visible, and we can get into trouble if we try
182
-
// to load objects and end up stuck in a cycle. See T8478.
183
-
continue;
172
+
$always_visible = true;
184
173
}
185
174
186
-
if ($is_omnipotent) {
187
-
// If the viewer is omnipotent, we don't need to load the associated
188
-
// objects either since they can certainly see the object. Skipping
189
-
// this can improve performance and prevent cycles.
175
+
if ($file->isBuiltin()) {
176
+
$always_visible = true;
177
+
}
178
+
179
+
if ($always_visible) {
180
+
// We just treat these files as though they aren't attached to
181
+
// anything. This saves a query in common cases when we're loading
182
+
// profile images or builtins. We could be slightly more nuanced
183
+
// about this and distinguish between "not attached to anything" and
184
+
// "might be attached but policy checks don't need to care".
185
+
$file->attachObjectPHIDs(array());
190
186
continue;
191
187
}
192
188
193
-
foreach ($phids as $phid) {
194
-
$object_phids[$phid] = true;
189
+
$need_objects[] = $file;
190
+
}
191
+
192
+
$viewer = $this->getViewer();
193
+
$is_omnipotent = $viewer->isOmnipotent();
194
+
195
+
// If we have any files left which do need objects, load the edges now.
196
+
$object_phids = array();
197
+
if ($need_objects) {
198
+
$edge_type = PhabricatorFileHasObjectEdgeType::EDGECONST;
199
+
$file_phids = mpull($need_objects, 'getPHID');
200
+
201
+
$edges = id(new PhabricatorEdgeQuery())
202
+
->withSourcePHIDs($file_phids)
203
+
->withEdgeTypes(array($edge_type))
204
+
->execute();
205
+
206
+
foreach ($need_objects as $file) {
207
+
$phids = array_keys($edges[$file->getPHID()][$edge_type]);
208
+
$file->attachObjectPHIDs($phids);
209
+
210
+
if ($is_omnipotent) {
211
+
// If the viewer is omnipotent, we don't need to load the associated
212
+
// objects either since the viewer can certainly see the object.
213
+
// Skipping this can improve performance and prevent cycles. This
214
+
// could possibly become part of the profile/builtin code above which
215
+
// short circuits attacment policy checks in cases where we know them
216
+
// to be unnecessary.
217
+
continue;
218
+
}
219
+
220
+
foreach ($phids as $phid) {
221
+
$object_phids[$phid] = true;
222
+
}
195
223
}
196
224
}
197
225
···
203
231
204
232
$xforms = id(new PhabricatorTransformedFile())->loadAllWhere(
205
233
'transformedPHID IN (%Ls)',
206
-
$file_phids);
234
+
mpull($files, 'getPHID'));
207
235
$xform_phids = mpull($xforms, 'getOriginalPHID', 'getTransformedPHID');
208
236
foreach ($xform_phids as $derived_phid => $original_phid) {
209
237
$object_phids[$original_phid] = true;