From 43c5b41e2fd1b5eb374952f4b7ae4d5596d3fd6c Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Thu, 6 Jun 2024 13:42:27 -0400 Subject: [PATCH] Fix issues with search database table and entity interface (#3757) --- .../10.0/002-modify-search-user-column.sql | 11 +++++ module/VuFind/sql/mysql.sql | 2 +- module/VuFind/sql/pgsql.sql | 2 +- .../src/VuFind/Controller/AbstractSearch.php | 4 +- .../Controller/MyResearchController.php | 4 +- .../Controller/Plugin/ResultScroller.php | 2 +- .../VuFind/Controller/UpgradeController.php | 20 ++++++++- .../Db/Entity/SearchEntityInterface.php | 20 ++++----- module/VuFind/src/VuFind/Db/Row/Search.php | 44 ++++++++++++------- .../src/VuFind/Db/Service/SearchService.php | 29 +++++++++++- .../Db/Service/SearchServiceInterface.php | 6 +++ module/VuFind/src/VuFind/Db/Table/Search.php | 4 +- module/VuFind/src/VuFind/Search/History.php | 2 +- module/VuFind/src/VuFind/Search/Memory.php | 2 +- .../Command/ScheduledSearch/NotifyCommand.php | 2 +- 15 files changed, 114 insertions(+), 40 deletions(-) create mode 100644 module/VuFind/sql/migrations/pgsql/10.0/002-modify-search-user-column.sql diff --git a/module/VuFind/sql/migrations/pgsql/10.0/002-modify-search-user-column.sql b/module/VuFind/sql/migrations/pgsql/10.0/002-modify-search-user-column.sql new file mode 100644 index 00000000000..aaeb990f437 --- /dev/null +++ b/module/VuFind/sql/migrations/pgsql/10.0/002-modify-search-user-column.sql @@ -0,0 +1,11 @@ +-- +-- Modifications to user_id column in table `comments` +-- + +ALTER TABLE "search" + ALTER COLUMN user_id DROP NOT NULL; + +ALTER TABLE "search" + ALTER COLUMN user_id SET DEFAULT NULL; + +UPDATE "search" SET user_id=NULL WHERE user_id='0' OR user_id NOT IN (SELECT DISTINCT id FROM "user"); diff --git a/module/VuFind/sql/mysql.sql b/module/VuFind/sql/mysql.sql index 5149e7c600d..fc57cc9b93f 100644 --- a/module/VuFind/sql/mysql.sql +++ b/module/VuFind/sql/mysql.sql @@ -133,7 +133,7 @@ CREATE TABLE `resource_tags` ( /*!40101 SET character_set_client = utf8mb4 */; CREATE TABLE `search` ( `id` bigint unsigned NOT NULL AUTO_INCREMENT, - `user_id` int(11) NOT NULL DEFAULT '0', + `user_id` int(11) DEFAULT NULL, `session_id` varchar(128) DEFAULT NULL, `created` datetime NOT NULL DEFAULT '2000-01-01 00:00:00', `title` varchar(20) DEFAULT NULL, diff --git a/module/VuFind/sql/pgsql.sql b/module/VuFind/sql/pgsql.sql index 05ce2f8a08b..3d87dbf2864 100644 --- a/module/VuFind/sql/pgsql.sql +++ b/module/VuFind/sql/pgsql.sql @@ -90,7 +90,7 @@ DROP TABLE IF EXISTS "search"; CREATE TABLE search ( id BIGSERIAL, -user_id int NOT NULL DEFAULT '0', +user_id int DEFAULT NULL, session_id varchar(128), created timestamp NOT NULL DEFAULT '1970-01-01 00:00:00', title varchar(20) DEFAULT NULL, diff --git a/module/VuFind/src/VuFind/Controller/AbstractSearch.php b/module/VuFind/src/VuFind/Controller/AbstractSearch.php index 36d05e2a379..314bac0b477 100644 --- a/module/VuFind/src/VuFind/Controller/AbstractSearch.php +++ b/module/VuFind/src/VuFind/Controller/AbstractSearch.php @@ -141,7 +141,7 @@ protected function redirectToSavedSearch($id) // If we got this far, the user is allowed to view the search, so we can // deminify it to a new object. - $minSO = $search->getSearchObject(); + $minSO = $search->getSearchObjectOrThrowException(); $savedSearch = $minSO->deminify($this->getResultsManager()); // Now redirect to the URL associated with the saved search; this @@ -562,7 +562,7 @@ protected function restoreAdvancedSearch($searchId) } // Restore the full search object: - $minSO = $search->getSearchObject(); + $minSO = $search->getSearchObjectOrThrowException(); $savedSearch = $minSO->deminify($this->getResultsManager()); // Fail if this is not the right type of search: diff --git a/module/VuFind/src/VuFind/Controller/MyResearchController.php b/module/VuFind/src/VuFind/Controller/MyResearchController.php index eff89029083..c068d076cc4 100644 --- a/module/VuFind/src/VuFind/Controller/MyResearchController.php +++ b/module/VuFind/src/VuFind/Controller/MyResearchController.php @@ -593,7 +593,7 @@ public function schedulesearchAction() // Now fetch all the results: $resultsManager = $this->serviceLocator ->get(\VuFind\Search\Results\PluginManager::class); - $results = $search->getSearchObject()->deminify($resultsManager); + $results = $search->getSearchObjectOrThrowException()->deminify($resultsManager); // Build the form. return $this->createViewModel( @@ -623,7 +623,7 @@ protected function isDuplicateOfSavedSearch( $normalizer = $this->serviceLocator ->get(\VuFind\Search\SearchNormalizer::class); $normalized = $normalizer - ->normalizeMinifiedSearch($rowToCheck->getSearchObject()); + ->normalizeMinifiedSearch($rowToCheck->getSearchObjectOrThrowException()); $matches = $searchTable->getSearchRowsMatchingNormalizedSearch( $normalized, $sessId, diff --git a/module/VuFind/src/VuFind/Controller/Plugin/ResultScroller.php b/module/VuFind/src/VuFind/Controller/Plugin/ResultScroller.php index 13595a56a83..6c66d1a7d0c 100644 --- a/module/VuFind/src/VuFind/Controller/Plugin/ResultScroller.php +++ b/module/VuFind/src/VuFind/Controller/Plugin/ResultScroller.php @@ -666,7 +666,7 @@ protected function restoreSearch(int $searchId): ?Results null ); if (!empty($row)) { - $minSO = $row->getSearchObject(); + $minSO = $row->getSearchObjectOrThrowException(); $search = $minSO->deminify($this->resultsManager); // The saved search does not remember its original limit or sort; // we should reapply them from the session data: diff --git a/module/VuFind/src/VuFind/Controller/UpgradeController.php b/module/VuFind/src/VuFind/Controller/UpgradeController.php index 260797f7f99..16de9e34d3c 100644 --- a/module/VuFind/src/VuFind/Controller/UpgradeController.php +++ b/module/VuFind/src/VuFind/Controller/UpgradeController.php @@ -49,6 +49,7 @@ use VuFind\Crypt\Base62; use VuFind\Db\AdapterFactory; use VuFind\Db\Service\ResourceServiceInterface; +use VuFind\Db\Service\SearchServiceInterface; use VuFind\Exception\RecordMissing as RecordMissingException; use VuFind\Record\ResourcePopulator; use VuFind\Search\Results\PluginManager as ResultsManager; @@ -327,6 +328,20 @@ protected function fixVuFindSourceInDatabase() } } + /** + * Support method for fixdatabaseAction() -- clean up invalid user ID + * values in the search table. + * + * @return void + */ + protected function fixInvalidUserIdsInSearchTable(): void + { + $count = $this->getDbService(SearchServiceInterface::class)->cleanUpInvalidUserIds(); + if ($count) { + $this->session->warnings->append("Converted $count invalid user_id values in search table"); + } + } + /** * Support method for fixdatabaseAction() -- add checksums to search table rows. * @@ -340,7 +355,7 @@ protected function fixSearchChecksumsInDatabase() $searchRows = $search->select($searchWhere); if (count($searchRows) > 0) { foreach ($searchRows as $searchRow) { - $searchObj = $searchRow->getSearchObject()->deminify($manager); + $searchObj = $searchRow->getSearchObjectOrThrowException()->deminify($manager); $url = $searchObj->getUrlQuery()->getParams(); $checksum = crc32($url) & 0xFFFFFFF; $searchRow->checksum = $checksum; @@ -586,6 +601,9 @@ public function fixdatabaseAction() // Clean up the "VuFind" source, if necessary. $this->fixVuFindSourceInDatabase(); + + // Fix invalid user IDs in search table, if necessary. + $this->fixInvalidUserIdsInSearchTable(); } catch (Exception $e) { $this->flashMessenger()->addMessage( 'Database upgrade failed: ' . $e->getMessage(), diff --git a/module/VuFind/src/VuFind/Db/Entity/SearchEntityInterface.php b/module/VuFind/src/VuFind/Db/Entity/SearchEntityInterface.php index 8d4b27fce19..ca6bf3a8cec 100644 --- a/module/VuFind/src/VuFind/Db/Entity/SearchEntityInterface.php +++ b/module/VuFind/src/VuFind/Db/Entity/SearchEntityInterface.php @@ -52,18 +52,18 @@ public function getId(): ?int; /** * Get user. * - * @return UserEntityInterface + * @return ?UserEntityInterface */ - public function getUser(): UserEntityInterface; + public function getUser(): ?UserEntityInterface; /** * Set user. * - * @param int $user User + * @param ?UserEntityInterface $user User * * @return SearchEntityInterface */ - public function setUser(UserEntityInterface $user): SearchEntityInterface; + public function setUser(?UserEntityInterface $user): SearchEntityInterface; /** * Get session identifier. @@ -75,11 +75,11 @@ public function getSessionId(): ?string; /** * Set session identifier. * - * @param ?string $session_id Session id + * @param ?string $sessionId Session id * * @return SearchEntityInterface */ - public function setSessionId(?string $session_id): SearchEntityInterface; + public function setSessionId(?string $sessionId): SearchEntityInterface; /** * Get created date. @@ -132,18 +132,18 @@ public function setSaved(bool $saved): SearchEntityInterface; /** * Get the search object from the row. * - * @return \VuFind\Search\Minified + * @return ?\VuFind\Search\Minified */ - public function getSearchObject(): \VuFind\Search\Minified; + public function getSearchObject(): ?\VuFind\Search\Minified; /** * Set search object. * - * @param \VuFind\Search\Minified $searchObject Search object + * @param ?\VuFind\Search\Minified $searchObject Search object * * @return SearchEntityInterface */ - public function setSearchObject(\VuFind\Search\Minified $searchObject): SearchEntityInterface; + public function setSearchObject(?\VuFind\Search\Minified $searchObject): SearchEntityInterface; /** * Get checksum. diff --git a/module/VuFind/src/VuFind/Db/Row/Search.php b/module/VuFind/src/VuFind/Db/Row/Search.php index 9482df584af..97ad370f367 100644 --- a/module/VuFind/src/VuFind/Db/Row/Search.php +++ b/module/VuFind/src/VuFind/Db/Row/Search.php @@ -97,14 +97,24 @@ protected function normalizeSearchObject() /** * Get the search object from the row. * - * @return \VuFind\Search\Minified + * @return ?\VuFind\Search\Minified */ - public function getSearchObject(): \VuFind\Search\Minified + public function getSearchObject(): ?\VuFind\Search\Minified { // We need to make sure the search object is a string before unserializing: $this->normalizeSearchObject(); - $result = unserialize($this->search_object); - if (!($result instanceof \VuFind\Search\Minified)) { + return $this->search_object ? unserialize($this->search_object) : null; + } + + /** + * Get the search object from the row, and throw an exception if it is missing. + * + * @return \VuFind\Search\Minified + * @throws \Exception + */ + public function getSearchObjectOrThrowException(): \VuFind\Search\Minified + { + if (!($result = $this->getSearchObject())) { throw new \Exception('Problem decoding saved search'); } return $result; @@ -185,23 +195,25 @@ public function getId(): ?int /** * Get user. * - * @return UserEntityInterface + * @return ?UserEntityInterface */ - public function getUser(): UserEntityInterface + public function getUser(): ?UserEntityInterface { - return $this->getDbServiceManager()->get(UserServiceInterface::class)->getUserById($this->user_id); + return $this->user_id + ? $this->getDbServiceManager()->get(UserServiceInterface::class)->getUserById($this->user_id) + : null; } /** * Set user. * - * @param UserEntityInterface $user User + * @param ?UserEntityInterface $user User * * @return SearchEntityInterface */ - public function setUser(UserEntityInterface $user): SearchEntityInterface + public function setUser(?UserEntityInterface $user): SearchEntityInterface { - $this->user_id = $user->getId(); + $this->user_id = $user?->getId(); return $this; } @@ -218,13 +230,13 @@ public function getSessionId(): ?string /** * Set session identifier. * - * @param ?string $session_id Session id + * @param ?string $sessionId Session id * * @return SearchEntityInterface */ - public function setSessionId(?string $session_id): SearchEntityInterface + public function setSessionId(?string $sessionId): SearchEntityInterface { - $this->session_id = $session_id; + $this->session_id = $sessionId; return $this; } @@ -300,13 +312,13 @@ public function setSaved(bool $saved): SearchEntityInterface /** * Set search object. * - * @param \VuFind\Search\Minified $searchObject Search object + * @param ?\VuFind\Search\Minified $searchObject Search object * * @return SearchEntityInterface */ - public function setSearchObject(\VuFind\Search\Minified $searchObject): SearchEntityInterface + public function setSearchObject(?\VuFind\Search\Minified $searchObject): SearchEntityInterface { - $this->search_object = serialize($searchObject); + $this->search_object = $searchObject ? serialize($searchObject) : null; return $this; } diff --git a/module/VuFind/src/VuFind/Db/Service/SearchService.php b/module/VuFind/src/VuFind/Db/Service/SearchService.php index fd35005f08b..3d00c17e012 100644 --- a/module/VuFind/src/VuFind/Db/Service/SearchService.php +++ b/module/VuFind/src/VuFind/Db/Service/SearchService.php @@ -29,6 +29,11 @@ namespace VuFind\Db\Service; +use VuFind\Db\Table\DbTableAwareInterface; +use VuFind\Db\Table\DbTableAwareTrait; + +use function count; + /** * Database service for search. * @@ -38,6 +43,28 @@ * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License * @link https://vufind.org/wiki/development:plugins:database_gateways Wiki */ -class SearchService extends AbstractDbService implements SearchServiceInterface +class SearchService extends AbstractDbService implements SearchServiceInterface, DbTableAwareInterface { + use DbTableAwareTrait; + + /** + * Set invalid user_id values in the table to null; return count of affected rows. + * + * @return int + */ + public function cleanUpInvalidUserIds(): int + { + $searchTable = $this->getDbTable('search'); + $allIds = $this->getDbTable('user')->getSql()->select()->columns(['id']); + $searchCallback = function ($select) use ($allIds) { + $select->where->equalTo('user_id', '0') + ->OR->notIn('user_id', $allIds); + }; + $badRows = $searchTable->select($searchCallback); + $count = count($badRows); + if ($count > 0) { + $searchTable->update(['user_id' => null], $searchCallback); + } + return $count; + } } diff --git a/module/VuFind/src/VuFind/Db/Service/SearchServiceInterface.php b/module/VuFind/src/VuFind/Db/Service/SearchServiceInterface.php index 82ab5b6d56e..05d584cc3a0 100644 --- a/module/VuFind/src/VuFind/Db/Service/SearchServiceInterface.php +++ b/module/VuFind/src/VuFind/Db/Service/SearchServiceInterface.php @@ -40,4 +40,10 @@ */ interface SearchServiceInterface extends DbServiceInterface { + /** + * Set invalid user_id values in the table to null; return count of affected rows. + * + * @return int + */ + public function cleanUpInvalidUserIds(): int; } diff --git a/module/VuFind/src/VuFind/Db/Table/Search.php b/module/VuFind/src/VuFind/Db/Table/Search.php index 38fa0f03198..b2d6122f20c 100644 --- a/module/VuFind/src/VuFind/Db/Table/Search.php +++ b/module/VuFind/src/VuFind/Db/Table/Search.php @@ -251,7 +251,7 @@ public function getSearchRowsMatchingNormalizedSearch( }; $results = []; foreach ($this->select($callback) as $match) { - $minified = $match->getSearchObject(); + $minified = $match->getSearchObjectOrThrowException(); if ($normalized->isEquivalentToMinifiedSearch($minified)) { $results[] = $match; if (count($results) >= $limit) { @@ -293,7 +293,7 @@ public function saveSearch( $existingRow->created = date('Y-m-d H:i:s'); // Keep the ID of the old search: $minified = $normalized->getMinified(); - $minified->id = $existingRow->getSearchObject()->id; + $minified->id = $existingRow->getSearchObjectOrThrowException()->id; $existingRow->search_object = serialize($minified); $existingRow->session_id = $sessionId; $existingRow->save(); diff --git a/module/VuFind/src/VuFind/Search/History.php b/module/VuFind/src/VuFind/Search/History.php index 33066f5019e..10b9ba67c41 100644 --- a/module/VuFind/src/VuFind/Search/History.php +++ b/module/VuFind/src/VuFind/Search/History.php @@ -119,7 +119,7 @@ public function getSearchHistory($userId = null) // Loop through and sort the history $saved = $schedule = $unsaved = []; foreach ($searchHistory as $current) { - $search = $current->getSearchObject()->deminify($this->resultsManager); + $search = $current->getSearchObjectOrThrowException()->deminify($this->resultsManager); // $current->saved may be 1 (MySQL) or true (PostgreSQL), so we should // avoid a strict === comparison here: if ($current->saved == 1) { diff --git a/module/VuFind/src/VuFind/Search/Memory.php b/module/VuFind/src/VuFind/Search/Memory.php index 634a0852007..6ac1a4c3f5c 100644 --- a/module/VuFind/src/VuFind/Search/Memory.php +++ b/module/VuFind/src/VuFind/Search/Memory.php @@ -304,7 +304,7 @@ protected function getSearchById(int $id): ?\VuFind\Search\Base\Results $search = $this->searchTable->getOwnedRowById($id, $this->sessionId, null); if ($search) { - $minSO = $search->getSearchObject(); + $minSO = $search->getSearchObjectOrThrowException(); $this->searchCache[$id] = $minSO->deminify($this->resultsManager); } else { $this->searchCache[$id] = null; diff --git a/module/VuFindConsole/src/VuFindConsole/Command/ScheduledSearch/NotifyCommand.php b/module/VuFindConsole/src/VuFindConsole/Command/ScheduledSearch/NotifyCommand.php index a76fd342794..7fc00ebcbe1 100644 --- a/module/VuFindConsole/src/VuFindConsole/Command/ScheduledSearch/NotifyCommand.php +++ b/module/VuFindConsole/src/VuFindConsole/Command/ScheduledSearch/NotifyCommand.php @@ -362,7 +362,7 @@ protected function setLanguage($userLang) */ protected function getObjectForSearch($s) { - $minSO = $s->getSearchObject(); + $minSO = $s->getSearchObjectOrThrowException(); $searchObject = $minSO->deminify($this->resultsManager); if (!$searchObject->getOptions()->supportsScheduledSearch()) { $this->err(