Skip to content

Commit

Permalink
Optimize username/name handling [DB update]
Browse files Browse the repository at this point in the history 
Avoid UPDATE on the `users` table to update the username unless the
username has actually changed, and remove unused `joined`/`lastSyncTime`
columns.

Also use the user's real name instead of the username in most API
responses.  The real name comes from the WWW DB, so fall back to the
username on master if unavailable. It would be nice not to cache the
username on master, but we want to avoid making the WWW DB a hard
dependency except when absolutely necessary (e.g., creating a new API
key).
  • Loading branch information
@dstillman
dstillman committed Oct 19, 2021
1 parent 9b96401 commit 85d520c
Show file tree
Hide file tree
Showing 8 changed files with 53 additions and 59 deletions.
2 changes: 1 addition & 1 deletion controllers/GroupsController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public function groups() {
// Multiple groups
else {
if ($this->objectUserID) {
$title = Zotero_Users::getUsername($this->objectUserID) . "’s Groups";
$title = Zotero_Users::getName($this->objectUserID) . "’s Groups";
}
else {
// For now, only root can do unrestricted group searches
Expand Down
7 changes: 7 additions & 0 deletions misc/db-updates/2021-10-17/removeUserTimestamps
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/usr/bin/php -d mysqlnd.net_read_timeout=3600
<?php
set_include_path("../../../include");
require("header.inc.php");

// Run this after rolling out code changes
Zotero_Admin_DB::query("ALTER TABLE `users` DROP `joined`, DROP `lastSyncTime`");
2 changes: 0 additions & 2 deletions misc/master.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,6 @@ CREATE TABLE `users` (
`userID` int(10) unsigned NOT NULL,
`libraryID` int(10) unsigned NOT NULL,
`username` varchar(255) NOT NULL,
`joined` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
`lastSyncTime` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
PRIMARY KEY (`userID`),
UNIQUE KEY `libraryID` (`libraryID`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
Expand Down
8 changes: 4 additions & 4 deletions model/Group.inc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ public function toHTML() {
"",
true
);
$tr->td->a = Zotero_Users::getUsername($this->ownerUserID);
$tr->td->a = Zotero_Users::getName($this->ownerUserID);
$tr->td->a['href'] = Zotero_URI::getUserURI($this->ownerUserID);

Zotero_Atom::addHTMLRow($html, '', "Type", preg_replace('/([a-z])([A-Z])/', '$1 $2', $this->type));
Expand All @@ -748,7 +748,7 @@ public function toHTML() {
$ul = $tr->td->addChild('ul');
foreach ($admins as $admin) {
$li = $ul->addChild('li');
$li->a = Zotero_Users::getUsername($admin);
$li->a = Zotero_Users::getName($admin);
$li->a['href'] = Zotero_URI::getUserURI($admin);
}
}
Expand Down Expand Up @@ -905,7 +905,7 @@ public function toAtom($queryParams) {

$author = $xml->addChild('author');
$ownerLibraryID = Zotero_Users::getLibraryIDFromUserID($this->ownerUserID);
$author->name = Zotero_Users::getUsername($this->ownerUserID);
$author->name = Zotero_Users::getName($this->ownerUserID);
$author->uri = Zotero_URI::getLibraryURI($ownerLibraryID);

$xml->id = Zotero_URI::getGroupURI($this);
Expand Down Expand Up @@ -995,7 +995,7 @@ public function memberToAtom($userID) {
// If we know the username, provide that
// TODO: get and cache full names
if (Zotero_Users::exists($userID)) {
$title = Zotero_Users::getUsername($userID);
$title = Zotero_Users::getName($userID);
}
else {
$title = "User $userID";
Expand Down
8 changes: 4 additions & 4 deletions model/Items.inc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ public static function search($libraryID, $onlyTopLevel = false, array $params =
foreach ($createdByUserIDs as $createdByUserID) {
$toAdd[] = array(
$createdByUserID,
Zotero_Users::getUsername($createdByUserID)
Zotero_Users::getName($createdByUserID)
);
}

Expand Down Expand Up @@ -981,7 +981,7 @@ public static function convertItemToAtom(Zotero_Item $item, $queryParams, $permi
);
$cachedParams = Z_Array::filterKeys($queryParams, $allowedParams);

$cacheVersion = 3;
$cacheVersion = 4;
$cacheKey = "atomEntry_" . $item->libraryID . "/" . $item->id . "_"
. md5(
$version
Expand Down Expand Up @@ -1136,7 +1136,7 @@ public static function convertItemToAtom(Zotero_Item $item, $queryParams, $permi
}
if ($createdByUserID) {
try {
$author->name = Zotero_Users::getUsername($createdByUserID);
$author->name = Zotero_Users::getName($createdByUserID);
$author->uri = Zotero_URI::getUserURI($createdByUserID);
}
// If user no longer exists, use library for author instead
Expand Down Expand Up @@ -1215,7 +1215,7 @@ public static function convertItemToAtom(Zotero_Item $item, $queryParams, $permi
try {
$xml->addChild(
'zapi:lastModifiedByUser',
Zotero_Users::getUsername($lastModifiedByUserID),
Zotero_Users::getName($lastModifiedByUserID),
Zotero_Atom::$nsZoteroAPI
);
}
Expand Down
4 changes: 2 additions & 2 deletions model/Libraries.inc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ public static function getName($libraryID) {
switch ($type) {
case 'user':
$userID = Zotero_Users::getUserIDFromLibraryID($libraryID);
return Zotero_Users::getUsername($userID);
return Zotero_Users::getName($userID);

case 'publications':
$userID = Zotero_Users::getUserIDFromLibraryID($libraryID);
return Zotero_Users::getUsername($userID) . "’s Publications";
return Zotero_Users::getName($userID) . "’s Publications";

case 'group':
$groupID = Zotero_Groups::getGroupIDFromLibraryID($libraryID);
Expand Down
78 changes: 34 additions & 44 deletions model/Users.inc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,6 @@ public static function getUserIDFromLibraryID($libraryID, $libraryType=null) {
}


/**
* Warning: This method may lie or return false..
*/
public static function getUserIDFromUsername($username) {
$sql = "SELECT userID FROM users WHERE username=?";
return Zotero_DB::valueQuery($sql, $username);
}


public static function getUserIDFromSessionID($sessionID) {
$sql = "SELECT userID FROM sessions WHERE id=?
AND UNIX_TIMESTAMP() < modified + lifetime";
Expand Down Expand Up @@ -164,21 +155,26 @@ public static function getUsername($userID, $skipAutoAdd=false) {
return $username;
}

if (!$skipAutoAdd && !self::isDeletedUser($userID)) {
$sql = "SELECT username FROM users WHERE userID=?";
$username = Zotero_DB::valueQuery($sql, $userID);
try {
$wwwUsername = self::getUsernameFromWWW($userID);
}
catch (Exception $e) {
Z_Core::logError("WARNING: $e -- can't get username from www");
}

if ($wwwUsername
&& $username != $wwwUsername
&& !$skipAutoAdd
&& !self::isDeletedUser($userID)) {
if (!self::exists($userID)) {
self::addFromWWW($userID);
self::add($userID, $wwwUsername);
}
else {
self::updateFromWWW($userID);
self::updateUsername($userID, $wwwUsername);
}
}

$sql = "SELECT username FROM users WHERE userID=?";
$username = Zotero_DB::valueQuery($sql, $userID);
// Fall back to WWW. Deleting accounts currently deletes the dataserver user, so we need to
// get the username from WWW.
if (!$username) {
$username = self::getUsernameFromWWW($userID);
$username = $wwwUsername;
}

self::$usernamesByID[$userID] = $username;
Expand Down Expand Up @@ -222,12 +218,24 @@ public static function getRealName($userID) {
}

self::$realNamesByID[$userID] = $name;
Z_Core::$MC->set($cacheKey, $name);
Z_Core::$MC->set($cacheKey, $name, 43200);

return $name;
}


/**
* Get real name, falling back to username if unavailable
*/
public static function getName($userID) {
$name = self::getRealName($userID);
if (!$name) {
$name = self::getUsername($userID);
}
return $name;
}


public static function toJSON($userID) {
$realName = Zotero_Users::getRealName($userID);
$json = [
Expand Down Expand Up @@ -292,31 +300,9 @@ public static function addFromWWW($userID) {
}


public static function updateFromWWW($userID) {
if (self::isDeletedUser($userID)) {
throw new Exception("User $userID was deleted", Z_ERROR_USER_NOT_FOUND);
}
$username = self::getUsernameFromWWW($userID);
self::updateUsername($userID, $username);
}


public static function update($userID, $username=false) {
$sql = "UPDATE users SET ";
$params = array();
if ($username) {
$sql .= "username=?, ";
$params[] = $username;
}
$sql .= "lastSyncTime=NOW() WHERE userID=?";
$params[] = $userID;
return Zotero_DB::query($sql, $params);
}


public static function updateUsername($userID, $username) {
$sql = "UPDATE users SET username=? WHERE userID=?";
return Zotero_DB::query(
Zotero_DB::query(
$sql,
[
$username,
Expand All @@ -327,6 +313,10 @@ public static function updateUsername($userID, $username) {
'writeInReadMode' => true
]
);

self::$usernamesByID[$userID] = $username;
$cacheKey = "usernameByID_" . $userID;
Z_Core::$MC->set($cacheKey, $username, 43200);
}


Expand Down
3 changes: 1 addition & 2 deletions model/auth/Password.inc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,11 @@ private static function updateUser($userID, $username) {
if (Zotero_Users::exists($userID)) {
$currentUsername = Zotero_Users::getUsername($userID, true);
if ($currentUsername != $username) {
Zotero_Users::update($userID, $username);
Zotero_Users::updateUsername($userID, $username);
}
}
else {
Zotero_Users::add($userID, $username);
Zotero_Users::update($userID);
}
}
}
Expand Down

0 comments on commit 85d520c

Please sign in to comment.