fix PollMapper for PostgreSQL usage

[dartcafe]

fixes #3393
fixes #3396
fixes #3399

• added testFind to unit tests
• adding and maintaining grouping statements

[joergmschulz]

sorry, in Postgres we still have

[index] Fehler: An exception occurred while executing a query: SQLSTATE[42803]: Grouping error: 7 ERROR:  column "shares.type" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: ...."vote_answer") AS "current_user_votes", coalesce(shares.typ...
                                                             ^
	GET /apps/polls/polls?time=1711655716981
	...

and

[core] Fehler: Error while running background job (class: OCA\Polls\Cron\AutoReminderCron, arguments: )
	von ? von -- um 28.03.2024, 20:55:01

[dartcafe]

@joergmschulz Are you referring to the app store installed version or the changed version with this PR?

[dartcafe]

OK. It turned out, that IQueryBuilder::addGroupBy() did not really add the grouping statement for the joined table, without prior IQueryBuilder::groupBy() statement.

The order matters.

[joergmschulz]

Cool, thank you for fixing. I can confirm polls work using postgres and nextcloud 28.0.4

[dartcafe]

Great, thank you for feedback.

[pierreozoux]

Hi!

we are hitting this issue on PG and version 6.3.0, would it be possible to backport this fix on 6.3 line, and publish 6.3.1?

Thanks for your help!

[unteem]

this patch below seems to fix it. I can make a PR for v6 in the coming days

--- apps/polls/lib/Db/CommentMapper.php
+++ apps/polls/lib/Db/CommentMapper.php
@@ -110,17 +110,10 @@ class CommentMapper extends QBMapperWithUser {
 		$qb = $this->db->getQueryBuilder();
 
 		$qb->select(self::TABLE . '.*')
-			->from($this->getTableName(), self::TABLE);
-		$anonAlias = $this->joinAnon($qb, self::TABLE);
-
-		$qb->groupBy(
-			self::TABLE . '.id',
-			$anonAlias . '.anonymous',
-			$anonAlias . '.owner',
-			$anonAlias . '.show_results',
-			$anonAlias . '.expire',
-		);
-
+			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.id');
+		
+		$this->joinAnon($qb, self::TABLE);
 		return $qb;
 	}
 }
--- apps/polls/lib/Db/PollMapper.php
+++ apps/polls/lib/Db/PollMapper.php
@@ -174,13 +174,13 @@ class PollMapper extends QBMapper {
 		$qb = $this->db->getQueryBuilder();
 
 		$qb->select(self::TABLE . '.*')
-			// TODO: check if this is necessary, in case of empty table to avoid possibly nulled columns
-			// ->groupBy(self::TABLE . '.id')
-			->from($this->getTableName(), self::TABLE);
+			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.id');
+
+
 		$this->joinOptionsForMaxDate($qb, self::TABLE);
 		$this->joinCurrentUserVotes($qb, self::TABLE, $currentUserId);
 		$this->joinUserRole($qb, self::TABLE, $currentUserId);
-		$qb->groupBy(self::TABLE . '.id');
 		return $qb;
 	}
 
@@ -192,7 +192,13 @@ class PollMapper extends QBMapper {
 	 */
 	protected function joinUserRole(IQueryBuilder &$qb, string $fromAlias, string $currentUserId): void {
 		$joinAlias = 'shares';
-		$qb->addSelect($qb->createFunction('coalesce(' . $joinAlias . '.type, "") AS user_role'));
+		$emptyString = $qb->createNamedParameter("", IQueryBuilder::PARAM_STR);
+
+		$qb->addSelect($qb->createFunction('coalesce(' . $joinAlias . '.type, '. $emptyString . ') AS user_role'))
+			->addGroupBy($joinAlias . '.type');
+
+		$qb->selectAlias($joinAlias . '.locked', 'is_current_user_locked')
+			->addGroupBy($joinAlias . '.locked');
 
 		$qb->leftJoin(
 			$fromAlias,
@@ -201,8 +207,10 @@ class PollMapper extends QBMapper {
 			$qb->expr()->andX(
 				$qb->expr()->eq($fromAlias . '.id', $joinAlias . '.poll_id'),
 				$qb->expr()->eq($joinAlias . '.user_id', $qb->createNamedParameter($currentUserId, IQueryBuilder::PARAM_STR)),
+				$qb->expr()->eq($joinAlias . '.deleted', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)),
 			)
 		);
+
 	}
 
 	/**
@@ -213,9 +221,11 @@ class PollMapper extends QBMapper {
 	 */
 	protected function joinOptionsForMaxDate(IQueryBuilder &$qb, string $fromAlias): void {
 		$joinAlias = 'options';
-		$saveMin = (string) time();
 
-		$qb->addSelect($qb->createFunction('coalesce(MAX(' . $joinAlias . '.timestamp), 0) AS max_date'))
+		$zero = $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT);
+		$saveMin = $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT);
+
+		$qb->addSelect($qb->createFunction('coalesce(MAX(' . $joinAlias . '.timestamp), '. $zero  . ') AS max_date'))
 			->addSelect($qb->createFunction('coalesce(MIN(' . $joinAlias . '.timestamp), ' . $saveMin . ') AS min_date'));
 
 		$qb->leftJoin(
@@ -232,7 +242,7 @@ class PollMapper extends QBMapper {
 	 * the min value is the current time,
 	 * the max value is null
 	 */
-	protected function joinCurrentUserVotes(IQueryBuilder &$qb, string $fromAlias, $currentUserId): void {
+	protected function joinCurrentUserVotes(IQueryBuilder &$qb, string $fromAlias, string $currentUserId): void {
 		$joinAlias = 'user_vote';
 		// force value into a MIN function to avoid grouping errors
 		$qb->selectAlias($qb->func()->count($joinAlias . '.vote_answer'), 'current_user_votes');

--- apps/polls/lib/Db/Preferences.php
+++ apps/polls/lib/Db/Preferences.php
@@ -69,7 +69,7 @@ class Preferences extends Entity implements JsonSerializable {
 	}
 
 	public function getPreferences_decoded(): mixed {
-		return json_decode((string) $this->getPreferences());
+		return json_decode($this->getPreferences() ?? '');
 	}
 
 	/**

--- apps/polls/lib/Db/QBMapperWithUser.php
+++ apps/polls/lib/Db/QBMapperWithUser.php
@@ -50,14 +50,19 @@ abstract class QBMapperWithUser extends QBMapper {
 	/**
 	 * Joins anonymous setting of poll
 	 */
-	protected function joinAnon(IQueryBuilder &$qb, string $fromAlias): string {
+	protected function joinAnon(IQueryBuilder &$qb, string $fromAlias): void {
 		$joinAlias = 'anon';
 
 		$qb->selectAlias($joinAlias . '.anonymous', 'anonymized')
 			->selectAlias($joinAlias . '.owner', 'poll_owner_id')
 			->selectAlias($joinAlias . '.show_results', 'poll_show_results')
 			->selectAlias($joinAlias . '.expire', 'poll_expire')
-		;
+			->addGroupBy(
+				$joinAlias . '.anonymous',
+				$joinAlias . '.owner',
+				$joinAlias . '.show_results',
+				$joinAlias . '.expire',
+			);
 
 		$qb->leftJoin(
 			$fromAlias,
@@ -65,6 +70,6 @@ abstract class QBMapperWithUser extends QBMapper {
 			$joinAlias,
 			$qb->expr()->eq($joinAlias . '.id', $fromAlias . '.poll_id'),
 		);
-		return $joinAlias;
+
 	}
 }

--- apps/polls/lib/Db/ShareMapper.php
+++ apps/polls/lib/Db/ShareMapper.php
@@ -60,7 +60,7 @@ class ShareMapper extends QBMapper {
 		$qb = $this->db->getQueryBuilder();
 
 		$qb->select(self::TABLE . '.*')
-		->from($this->getTableName(), self::TABLE)
+		    ->from($this->getTableName(), self::TABLE)
 			->groupBy(self::TABLE . '.id')
 			->where($qb->expr()->eq(self::TABLE . '.poll_id', $qb->createNamedParameter($pollId, IQueryBuilder::PARAM_INT)));
 
@@ -120,7 +120,8 @@ class ShareMapper extends QBMapper {
 		$qb = $this->db->getQueryBuilder();
 
 		$qb->select(self::TABLE . '.*')
-		->from($this->getTableName(), self::TABLE)
+			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.id')
 			->where($qb->expr()->eq(self::TABLE . '.poll_id', $qb->createNamedParameter($pollId, IQueryBuilder::PARAM_INT)))
 			->andWhere($qb->expr()->eq(self::TABLE . '.user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
 			->andWhere($qb->expr()->isNotNull(self::TABLE . '.id'));
@@ -160,7 +161,8 @@ class ShareMapper extends QBMapper {
 		$qb = $this->db->getQueryBuilder();
 
 		$qb->select(self::TABLE . '.*')
-		->from($this->getTableName(), self::TABLE)
+			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.id')
 			->where($qb->expr()->eq(self::TABLE . '.token', $qb->createNamedParameter($token, IQueryBuilder::PARAM_STR)));
 
 		if (!$getDeleted) {
@@ -176,6 +178,7 @@ class ShareMapper extends QBMapper {
 		}
 	}
 
+
 	/**
 	 * @return void
 	 */
@@ -218,8 +221,6 @@ class ShareMapper extends QBMapper {
 				$qb->expr()->eq($fromAlias . '.user_id', $joinAlias . '.user_id'),
 			)
 		);
-		// avoid result with nulled columns
-		$qb->groupBy($fromAlias . '.id');
 	}
 
 }

--- apps/polls/lib/Db/VoteMapper.php
+++ apps/polls/lib/Db/VoteMapper.php
@@ -118,10 +118,10 @@ class VoteMapper extends QBMapperWithUser {
 
 		$qb->selectDistinct([self::TABLE . '.user_id', self::TABLE . '.poll_id'])
 			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.user_id', self::TABLE . '.poll_id')
 			->where(
 				$qb->expr()->eq(self::TABLE . '.poll_id', $qb->createNamedParameter($pollId, IQueryBuilder::PARAM_INT))
 			);
-		$qb->addGroupBy(self::TABLE . '.user_id', self::TABLE . '.poll_id');
 
 		return $this->findEntities($qb);
 	}
@@ -211,28 +211,20 @@ class VoteMapper extends QBMapperWithUser {
 
 	protected function buildQuery(bool $findOrphaned = false): IQueryBuilder {
 		$qb = $this->db->getQueryBuilder();
-
+		
 		$qb->select(self::TABLE . '.*')
-			->from($this->getTableName(), self::TABLE);
+			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.id');
 
 		$optionAlias = $this->joinOption($qb, self::TABLE);
 		
-		
 		if ($findOrphaned) {
 			$qb->where($qb->expr()->isNull($optionAlias . '.id'));
 		} else {
 			$qb->where($qb->expr()->isNotNull($optionAlias . '.id'));
 		}
-		$anonAlias = $this->joinAnon($qb, self::TABLE);
+		$this->joinAnon($qb, self::TABLE);
 
-		$qb->groupBy(
-			self::TABLE . '.id',
-			$optionAlias . '.id',
-			$anonAlias . '.anonymous',
-			$anonAlias . '.owner',
-			$anonAlias . '.show_results',
-			$anonAlias . '.expire',
-		);
 		
 		return $qb;
 	}
@@ -244,7 +236,8 @@ class VoteMapper extends QBMapperWithUser {
 	protected function joinOption(IQueryBuilder &$qb, string $fromAlias): string {
 		$joinAlias = 'options';
 		
-		$qb->selectAlias($joinAlias . '.id', 'option_id');
+		$qb->selectAlias($joinAlias . '.id', 'option_id')
+			->addGroupBy($joinAlias . '.id');
 
 		$qb->leftJoin(
 			$fromAlias,

--- apps/polls/lib/Db/OptionMapper.php
+++ apps/polls/lib/Db/OptionMapper.php
@@ -176,21 +176,15 @@ class OptionMapper extends QBMapperWithUser {
 
 		$qb->select(self::TABLE . '.*')
 			->from($this->getTableName(), self::TABLE)
+			->groupBy(self::TABLE . '.id')
 			->orderBy('order', 'ASC');
 
+
 		$this->joinVotesCount($qb, self::TABLE, $hideVotes);
 		$this->joinPollForLimits($qb, self::TABLE);
 		$this->joinCurrentUserVote($qb, self::TABLE, $currentUserId);
 		$this->joinCurrentUserVoteCount($qb, self::TABLE, $currentUserId);
-		$anonAlias = $this->joinAnon($qb, self::TABLE);
-
-		$qb->groupBy(
-			self::TABLE . '.id',
-			$anonAlias . '.anonymous',
-			$anonAlias . '.owner',
-			$anonAlias . '.show_results',
-			$anonAlias . '.expire',
-		);
+		$this->joinAnon($qb, self::TABLE);
 
 		return $qb;
 	}