Skip to content

Commit

Permalink
Merge pull request #3446 from nextcloud/fix/stable-6-for-26-perf-back…
Browse files Browse the repository at this point in the history
…ports

[stable6] Backports of perf improvements for stable 6 on 26
  • Loading branch information
artonge authored Apr 24, 2024
2 parents 3097e50 + ec3d982 commit 8d347b7
Show file tree
Hide file tree
Showing 20 changed files with 178 additions and 171 deletions.
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

0 comments on commit 8d347b7

Please sign in to comment.