From 615c6ca6968c6880fd4c23b0ec149bc9469955ed Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 18 Jan 2022 06:35:18 +0000 Subject: [PATCH 1/2] Issue 10935: Improved "GROUP BY" handling --- src/Database/Database.php | 42 ++++++++++++++++++++++- src/Module/Api/Friendica/Photo/Create.php | 2 +- src/Module/Update/Profile.php | 33 +++++++++--------- src/Worker/Notifier.php | 23 ++----------- 4 files changed, 62 insertions(+), 38 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index a812efbdb9..292625bb57 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -1367,6 +1367,45 @@ class Database return $this->toArray($this->select($table, $fields, $condition, $params)); } + /** + * Escape fields, adding special treatment for "group by" handling + * + * @param array $fields + * @param array $options + * @return array + */ + private function escapeFields(array $fields, array $options) + { + // In the case of a "GROUP BY" we have to add all the ORDER fields to the fieldlist. + // This needs to done to apply the "MAX(...)" treatment from below to them. + // Otherwise MySQL would report errors. + if (!empty($options['group_by']) && !empty($options['order'])) { + foreach ($options['order'] as $key => $field) { + if (!is_int($key)) { + if (!in_array($key, $fields)) { + $fields[] = $key; + } + } else { + if (!in_array($field, $fields)) { + $fields[] = $field; + } + } + } + } + + array_walk($fields, function(&$value, $key) use ($options) + { + $field = $value; + $value = '`' . str_replace('`', '``', $value) . '`'; + + if (!empty($options['group_by']) && !in_array($field, $options['group_by'])) { + $value = 'MAX(' . $value . ') AS ' . $value; + } + }); + + return $fields; + } + /** * Select rows from a table * @@ -1403,7 +1442,8 @@ class Database } if (count($fields) > 0) { - $select_string = implode(', ', array_map([DBA::class, 'quoteIdentifier'], $fields)); + $fields = $this->escapeFields($fields, $params); + $select_string = implode(', ', $fields); } else { $select_string = '*'; } diff --git a/src/Module/Api/Friendica/Photo/Create.php b/src/Module/Api/Friendica/Photo/Create.php index 62889d328c..bb3dc63b22 100644 --- a/src/Module/Api/Friendica/Photo/Create.php +++ b/src/Module/Api/Friendica/Photo/Create.php @@ -55,7 +55,7 @@ class Create extends BaseApi $type = $this->getRequestValue($this->parameters, 'extension', 'json'); // input params - $desc = $this->getRequestValue($request, 'desc') ?? ''; + $desc = $this->getRequestValue($request, 'desc', ''); $album = $this->getRequestValue($request, 'album'); $allow_cid = $this->getRequestValue($request, 'allow_cid'); $deny_cid = $this->getRequestValue($request, 'deny_cid'); diff --git a/src/Module/Update/Profile.php b/src/Module/Update/Profile.php index 22e6e4a997..b97b9f89e5 100644 --- a/src/Module/Update/Profile.php +++ b/src/Module/Update/Profile.php @@ -70,32 +70,35 @@ class Profile extends BaseModule $last_updated = $last_updated_array[$last_updated_key] ?? 0; + $condition = ["`uid` = ? AND NOT `contact-blocked` AND NOT `contact-pending` + AND `visible` AND (NOT `deleted` OR `gravity` = ?) + AND `wall` " . $sql_extra, $a->getProfileOwner(), GRAVITY_ACTIVITY]; + if ($_GET['force'] && !empty($_GET['item'])) { // When the parent is provided, we only fetch this - $sql_extra4 = " AND `parent` = " . intval($_GET['item']); + $condition = DBA::mergeConditions($condition, ['parent' => $_GET['item']]); } elseif ($is_owner || !$last_updated) { // If the page user is the owner of the page we should query for unseen // items. Otherwise use a timestamp of the last succesful update request. - $sql_extra4 = " AND `unseen`"; + $condition = DBA::mergeConditions($condition, ['unseen' => true]); } else { $gmupdate = gmdate(DateTimeFormat::MYSQL, $last_updated); - $sql_extra4 = " AND `received` > '" . $gmupdate . "'"; + $condition = DBA::mergeConditions($condition, ["`received` > ?", $gmupdate]); } - $items_stmt = DBA::p( - "SELECT `parent-uri-id` AS `uri-id`, MAX(`created`), MAX(`received`) FROM `post-user-view` - WHERE `uid` = ? AND NOT `contact-blocked` AND NOT `contact-pending` - AND `visible` AND (NOT `deleted` OR `gravity` = ?) - AND `wall` $sql_extra4 $sql_extra - GROUP BY `parent-uri-id` ORDER BY `received` DESC", - $a->getProfileOwner(), - GRAVITY_ACTIVITY - ); - - if (!DBA::isResult($items_stmt)) { + $items = Post::selectToArray(['parent-uri-id', 'created', 'received'], $condition, ['group_by' => ['parent-uri-id'], 'order' => ['received' => true]]); + if (!DBA::isResult($items)) { return; } + // @todo the DBA functions should handle "SELECT field AS alias" in the future, + // so that this workaround here could be removed. + $items = array_map(function ($item) { + $item['uri-id'] = $item['parent-uri-id']; + unset($item['parent-uri-id']); + return $item; + }, $items); + // Set a time stamp for this page. We will make use of it when we // search for new items (update routine) $last_updated_array[$last_updated_key] = time(); @@ -113,8 +116,6 @@ class Profile extends BaseModule } } - $items = DBA::toArray($items_stmt); - $o .= DI::conversation()->create($items, 'profile', $a->getProfileOwner(), false, 'received', $a->getProfileOwner()); System::htmlUpdateExit($o); diff --git a/src/Worker/Notifier.php b/src/Worker/Notifier.php index bb66270860..f46b1b0d9a 100644 --- a/src/Worker/Notifier.php +++ b/src/Worker/Notifier.php @@ -446,26 +446,9 @@ class Notifier if ($diaspora_delivery && !$unlisted) { $batch_delivery = true; - $participants_stmt = DBA::p( - "SELECT - `batch`, `network`, `protocol`, - ANY_VALUE(`id`) AS `id`, - ANY_VALUE(`url`) AS `url`, - ANY_VALUE(`name`) AS `name` - FROM `contact` - WHERE `network` = ? - AND `batch` != '' - AND `uid` = ? - AND `rel` != ? - AND NOT `blocked` - AND NOT `pending` - AND NOT `archive` - GROUP BY `batch`, `network`, `protocol`", - Protocol::DIASPORA, - $owner['uid'], - Contact::SHARING - ); - $participants = DBA::toArray($participants_stmt); + $participants = DBA::selectToArray('contact', ['batch', 'network', 'protocol', 'id', 'url', 'name'], + ["`network` = ? AND `batch` != '' AND `uid` = ? AND `rel` != ? AND NOT `blocked` AND NOT `pending` AND NOT `archive`", Protocol::DIASPORA, $owner['uid'], Contact::SHARING], + ['group_by' => ['batch', 'network', 'protocol']]); // Fetch the participation list // The function will ensure that there are no duplicates From a46b21590d2ee40596cafb9bfaa045f4e33f49e7 Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 18 Jan 2022 06:59:02 +0000 Subject: [PATCH 2/2] use "ANY_VALUE" instead of "MAX" --- src/Database/Database.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 292625bb57..cc7f754ee6 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -1377,7 +1377,7 @@ class Database private function escapeFields(array $fields, array $options) { // In the case of a "GROUP BY" we have to add all the ORDER fields to the fieldlist. - // This needs to done to apply the "MAX(...)" treatment from below to them. + // This needs to done to apply the "ANY_VALUE(...)" treatment from below to them. // Otherwise MySQL would report errors. if (!empty($options['group_by']) && !empty($options['order'])) { foreach ($options['order'] as $key => $field) { @@ -1399,7 +1399,7 @@ class Database $value = '`' . str_replace('`', '``', $value) . '`'; if (!empty($options['group_by']) && !in_array($field, $options['group_by'])) { - $value = 'MAX(' . $value . ') AS ' . $value; + $value = 'ANY_VALUE(' . $value . ') AS ' . $value; } });