-
-
Notifications
You must be signed in to change notification settings - Fork 519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
collection query populate depth is off by 1 #1450
Comments
And actually, I was playing around with things and ended up in infinite recursion when So maybe something like this as well could help prevent recursion. function cockpit_populate_collection(&$items, $maxlevel = -1, $level = 0, $fieldsFilter = [], &$stack = []) {
if (!is_array($items)) {
return $items;
}
if (is_numeric($maxlevel) && ($maxlevel == 0 || $maxlevel > -1 && $level > $maxlevel)) {
return $items;
}
foreach ($items as $k => &$v) {
if ($level > 0 && is_array($v) && isset($v['_id'], $v['link'])) {
$id = $v['_id'];
$link = $v['link'];
if (in_array([$id, $link], $stack)) { // if _id is globally unique, then we can drop $link
continue
}
$items[$k] = cockpit('collections')->_resolveLinkedItem($v['link'], (string)$v['_id'], $fieldsFilter);
$items[$k]['_link'] = $link;
$stack[] = [$id, $link];
}
$items[$k] = cockpit_populate_collection($items[$k], $maxlevel, ($level + 1), $fieldsFilter, $stack);
}
return $items;
} |
Thanks for your effort! Could you please create a PR? |
I can try maybe late this week or next, but I'll need to find an extended window for testing and whatnot. Quick question - is |
tl;dr: change here:
What is this about?
This is in reference to the
populate=1
query argument that is available when querying collection items that contain aCollectionLink
field.When looking into the source code, I noticed that the value of
populate=1
also doubles as a max depth parameter, meaning that you can passpopulate=2
which will populate 1 depth deeper (populate=3
etc.).Here's the function signature that shows that max depth is used:
cockpit/modules/Collections/bootstrap.php
Line 628 in 568b012
And where it is called and gets passed the
populate
parameter.:cockpit/modules/Collections/bootstrap.php
Line 269 in 568b012
So what's the issue?
The problem is that the code is currently structured so that the max depth parameter uses
0
to signify a depth of1
and1
to signify a depth of2
and on top of being a bit confusing, it's problematic when you're also using it as a boolean.used as a boolean to determine if we should populate here:
cockpit/modules/Collections/bootstrap.php
Line 268 in 568b012
So what that means is that if you pass
populate=0
, nothing gets populated, and if you passpopulate=1
, two depths get populated (populate=2
is 3 depths).Here's an example of the response format where I have a cyclical relationship between two collections:
What's the solution
Well the most intuitive thing I can think of would be to make
$maxlevel=0
mean don't populate,$maxdepth=1
be depth of 1, etc., and keep$maxlevel=-1
to mean populate everything.Here's a draft solution
Compared to the original:
cockpit/modules/Collections/bootstrap.php
Lines 628 to 636 in 568b012
So in the updated version that means:
$items
(array of associative arrays):maxlevel=1, level=0
,level > maxlevel == false
,keys = [0, 1, 2...]
level > 0 == false
$item
(single associative array):maxlevel=1, level=1
,level > maxlevel == false
,keys = [val1, linked, ...]
linked
is resolved because it is a collectionlink field$item[linked]
(single associative array):maxlevel=1, level=2
,level > maxlevel == true
, exit early because of maxlevelThe text was updated successfully, but these errors were encountered: