Skip to content
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

[stable6] Backports of perf improvements for stable 6 on 26 #3446

Merged
merged 7 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/Controller/PollController.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use OCA\Polls\Service\PollService;
use OCP\AppFramework\Http\JSONResponse;
use OCP\IRequest;
use OCP\Server;

/**
* @psalm-api
Expand All @@ -55,7 +56,7 @@ public function __construct(
*/
public function list(): JSONResponse {
return $this->response(function () {
$appSettings = new AppSettings;
$appSettings = Server::get(AppSettings::class);
return [
'list' => $this->pollService->list(),
'pollCreationAllowed' => $appSettings->getPollCreationAllowed(),
Expand Down
19 changes: 12 additions & 7 deletions lib/Controller/PublicController.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
use OCP\Util;

/**
* Always use parent's classe response* methods to make sure, the token gets set correctly.
* Requesting the token inside the controller is not possible, because the token is submitted
* as a paramter and not known while contruction time
* i.e. ACL requests are not valid before calling the response* method
* @psalm-api
*/
class PublicController extends BasePublicController {
Expand Down Expand Up @@ -88,13 +92,14 @@ public function votePage(string $token) {
* @PublicPage
*/
public function getPoll(string $token): JSONResponse {
$this->acl->request(Acl::PERMISSION_POLL_VIEW);

// load poll through acl
return $this->response(fn () => [
'acl' => $this->acl,
'poll' => $this->acl->getPoll(),
], $token);
return $this->response(function () {
$this->acl->request(Acl::PERMISSION_POLL_VIEW);
// load poll through acl
return [
'acl' => $this->acl,
'poll' => $this->acl->getPoll(),
];
}, $token);
}

/**
Expand Down
6 changes: 2 additions & 4 deletions lib/Cron/JanitorCron.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use OCA\Polls\Model\Settings\AppSettings;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\TimedJob;
use OCP\Server;

/**
* @psalm-api
Expand All @@ -52,10 +53,7 @@ public function __construct(
) {
parent::__construct($time);
parent::setInterval(86400); // run once a day
$this->logMapper = $logMapper;
$this->pollMapper = $pollMapper;
$this->watchMapper = $watchMapper;
$this->appSettings = new AppSettings;
$this->appSettings = Server::get(AppSettings::class);
}

/**
Expand Down
36 changes: 32 additions & 4 deletions lib/Db/Poll.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ class Poll extends EntityWithUser implements JsonSerializable {
protected bool $hasOrphanedVotes = false;
protected int $maxDate = 0;
protected int $minDate = 0;
protected int $currentUserVotes = 0;
protected string $userRole = "none";
protected ?int $isCurrentUserLocked = 0;

public function __construct() {
$this->addType('created', 'int');
Expand All @@ -152,6 +155,7 @@ public function __construct() {
$this->addType('lastInteraction', 'int');
$this->addType('maxDate', 'int');
$this->addType('minDate', 'int');
$this->addType('currentUserVotes', 'int');
$this->urlGenerator = Container::queryClass(IURLGenerator::class);
$this->userMapper = Container::queryClass(UserMapper::class);
$this->voteMapper = Container::queryClass(VoteMapper::class);
Expand All @@ -165,6 +169,7 @@ public function __construct() {
public function jsonSerialize(): array {
return [
'id' => $this->getId(),
'type' => $this->getType(),
'title' => $this->getTitle(),
'description' => $this->getDescription(),
'descriptionSafe' => $this->getDescriptionSafe(),
Expand All @@ -182,13 +187,14 @@ public function jsonSerialize(): array {
'optionLimit' => $this->getOptionLimit(),
'proposalsExpire' => $this->getProposalsExpire(),
'showResults' => $this->getShowResults() === 'expired' ? Poll::SHOW_RESULTS_CLOSED : $this->getShowResults(),
'type' => $this->getType(),
'useNo' => $this->getUseNo(),
'voteLimit' => $this->getVoteLimit(),
'lastInteraction' => $this->getLastInteraction(),
'summary' => [
'orphanedVotes' => count($this->voteMapper->findOrphanedByPollandUser($this->id, $this->userMapper->getCurrentUserCached()->getId())),
'yesByCurrentUser' => count($this->voteMapper->getYesVotesByParticipant($this->getPollId(), $this->userMapper->getCurrentUserCached()->getId())),
'orphanedVotes' => $this->getCurrentUserOrphanedVotes(),
'yesByCurrentUser' => $this->getCurrentUserYesVotes(),
'countVotes' => $this->getCurrentUserCountVotes(),
'userRole' => $this->getUserRole(),
],
];
}
Expand Down Expand Up @@ -227,6 +233,13 @@ public function getExpired(): bool {
);
}

public function getUserRole(): string {
if ($this->userMapper->getCurrentUser()->getId() === $this->getOwner()) {
return 'owner';
}
return $this->userRole;
}

public function getVoteUrl(): string {
return $this->urlGenerator->linkToRouteAbsolute(
AppConstants::APP_ID . '.page.vote',
Expand Down Expand Up @@ -329,13 +342,28 @@ public function getRelevantThresholdNet(): int {
);
}

public function getCurrentUserCountVotes(): int {
return $this->currentUserVotes;
}

public function getIsCurrentUserLocked(): bool {
return (bool) $this->isCurrentUserLocked;
}

/**
* @psalm-return int<0, max>
*/
public function getOrphanedVotes(): int {
public function getCurrentUserOrphanedVotes(): int {
return count($this->voteMapper->findOrphanedByPollandUser($this->id, $this->userMapper->getCurrentUserCached()->getId()));
}

/**
* @psalm-return int<0, max>
*/
public function getCurrentUserYesVotes(): int {
return count($this->voteMapper->getYesVotesByParticipant($this->getPollId(), $this->userMapper->getCurrentUserCached()->getId()));
}

public function getDeadline(): int {
// if expiration is set return expiration date
if ($this->getExpire()) {
Expand Down
53 changes: 50 additions & 3 deletions lib/Db/PollMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ class PollMapper extends QBMapper {
/**
* @psalm-suppress PossiblyUnusedMethod
*/
public function __construct(IDBConnection $db) {
public function __construct(
IDBConnection $db,
private UserMapper $userMapper,
) {
parent::__construct($db, Poll::TABLE, Poll::class);
}

Expand Down Expand Up @@ -167,17 +170,41 @@ public function deleteByUserId(string $userId): void {
* Build the enhanced query with joined tables
*/
protected function buildQuery(): IQueryBuilder {
$currentUserId = $this->userMapper->getCurrentUser()->getId();
$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);
$this->joinOptionsForMaxDate($qb, self::TABLE);
$this->joinCurrentUserVotes($qb, self::TABLE, $currentUserId);
$this->joinUserRole($qb, self::TABLE, $currentUserId);
$qb->groupBy(self::TABLE . '.id');
return $qb;
}

/**
* Joins options to evaluate min and max option date for date polls
* if text poll or no options are set,
* the min value is the current time,
* the max value is null
*/
protected function joinUserRole(IQueryBuilder &$qb, string $fromAlias, string $currentUserId): void {
$joinAlias = 'shares';
$qb->addSelect($qb->createFunction('coalesce(' . $joinAlias . '.type, "") AS user_role'));

$qb->leftJoin(
$fromAlias,
Share::TABLE,
$joinAlias,
$qb->expr()->andX(
$qb->expr()->eq($fromAlias . '.id', $joinAlias . '.poll_id'),
$qb->expr()->eq($joinAlias . '.user_id', $qb->createNamedParameter($currentUserId, IQueryBuilder::PARAM_STR)),
)
);
}

/**
* Joins options to evaluate min and max option date for date polls
* if text poll or no options are set,
Expand All @@ -188,8 +215,6 @@ protected function joinOptionsForMaxDate(IQueryBuilder &$qb, string $fromAlias):
$joinAlias = 'options';
$saveMin = (string) time();

// force value into a MIN function to avoid grouping errors
// $qb->selectAlias($qb->func()->max($joinAlias . '.timestamp'), 'max_date');
$qb->addSelect($qb->createFunction('coalesce(MAX(' . $joinAlias . '.timestamp), 0) AS max_date'))
->addSelect($qb->createFunction('coalesce(MIN(' . $joinAlias . '.timestamp), ' . $saveMin . ') AS min_date'));

Expand All @@ -201,4 +226,26 @@ protected function joinOptionsForMaxDate(IQueryBuilder &$qb, string $fromAlias):
);
}

/**
* Joins options to evaluate min and max option date for date polls
* if text poll or no options are set,
* the min value is the current time,
* the max value is null
*/
protected function joinCurrentUserVotes(IQueryBuilder &$qb, string $fromAlias, $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');

$qb->leftJoin(
$fromAlias,
Vote::TABLE,
$joinAlias,
$qb->expr()->andX(
$qb->expr()->eq($joinAlias . '.poll_id', $fromAlias . '.id'),
$qb->expr()->eq($joinAlias . '.user_id', $qb->createNamedParameter($currentUserId, IQueryBuilder::PARAM_STR)),
)
);
}

}
6 changes: 3 additions & 3 deletions lib/Db/Share.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@

use JsonSerializable;
use OCA\Polls\AppConstants;
use OCA\Polls\Helper\Container;
use OCA\Polls\Model\Settings\AppSettings;
use OCP\IURLGenerator;
use OCP\Server;

/**
* @method int getId()
Expand Down Expand Up @@ -145,8 +145,8 @@ public function __construct() {
$this->addType('locked', 'int');
$this->addType('reminderSent', 'int');
$this->addType('deleted', 'int');
$this->urlGenerator = Container::queryClass(IURLGenerator::class);
$this->appSettings = new AppSettings;
$this->urlGenerator = Server::get(IURLGenerator::class);
$this->appSettings = Server::get(AppSettings::class);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/Db/UserMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public function getCurrentUser(): UserBase {
$this->currentUser = $this->getUserFromUserBase($this->userSession->getUser()->getUID());
} else {
try {
$this->currentUser = $this->getUserFromShareToken($this->getToken());
$this->currentUser = $this->getUserFromShareToken($this->getSessionStoredShareToken());
} catch (DoesNotExistException $e) {
$this->logger->debug('no user found, returned fake user');
$this->currentUser = new GenericUser('', Share::TYPE_PUBLIC);
Expand All @@ -103,7 +103,7 @@ public function getCurrentUserCached(): UserBase {
return $this->currentUser ?? $this->getCurrentUser();
}

private function getToken(): string {
public function getSessionStoredShareToken(): string {
return (string) $this->session->get(AppConstants::SESSION_KEY_SHARE_TOKEN);
}

Expand Down
13 changes: 0 additions & 13 deletions lib/Db/VoteMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,6 @@ public function findParticipantsByPoll(int $pollId): array {
return $this->findEntities($qb);
}


/**
* @throws \OCP\AppFramework\Db\DoesNotExistException if not found
* @return Vote[]
* @psalm-return array<array-key, Vote>
*/
public function findParticipantsVotes(int $pollId, string $userId): array {
$qb = $this->buildQuery();
$qb->andWhere($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)));
return $this->findEntities($qb);
}

public function deleteByPollAndUserId(int $pollId, string $userId): void {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())
Expand Down
19 changes: 6 additions & 13 deletions lib/Helper/Container.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,34 +31,27 @@
use OCA\Polls\Db\Share;
use OCA\Polls\Db\ShareMapper;
use OCP\App\IAppManager;
use OCP\AppFramework\App;
use OCP\IL10N;
use OCP\L10N\IFactory;
use Psr\Container\ContainerInterface;
use OCP\Server;

abstract class Container {
public static function getContainer(): ContainerInterface {
$app = new App(AppConstants::APP_ID);
return $app->getContainer();
}

public static function queryClass(string $class): mixed {
return self::getContainer()->get($class);
return Server::get($class);
}

public static function queryPoll(int $pollId): Poll {
return self::queryClass(PollMapper::class)->find($pollId);
return Server::get(PollMapper::class)->find($pollId);
}

public static function findShare(int $pollId, string $userId): Share {
return self::queryClass(ShareMapper::class)
->findByPollAndUser($pollId, $userId);
return Server::get(ShareMapper::class)->findByPollAndUser($pollId, $userId);
}

public static function getL10N(?string $lang = null): IL10N {
return self::queryClass(IFactory::class)->get(AppConstants::APP_ID, $lang);
return Server::get(IFactory::class)->get(AppConstants::APP_ID, $lang);
}
public static function isAppEnabled(string $app): bool {
return self::queryClass(IAppManager::class)->isEnabledForUser($app);
return Server::get(IAppManager::class)->isEnabledForUser($app);
}
}
Loading
Loading