From aba40a4068a546c85a564d3f836eb17c3e5817e1 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 14 Aug 2024 15:16:14 -0400 Subject: [PATCH] Properly close the connections to Koha from Aspen It is best practice to close all database connections after the data from a query has been ingested and parsed. This change calls close() on all mysqli result sets that do not already call close after use. Test Plan: 1) Set up an instance of Aspen Discovery connected to an instance of Koha 2) Perform all Koha related actions in Aspen Discovery 3) No changes in Aspen behavior should be noted, but this should result in a reduction of used connections to the database. --- code/web/Drivers/Koha.php | 85 ++++++++++++++++++++++++++---- code/web/release_notes/24.09.00.MD | 1 + 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/code/web/Drivers/Koha.php b/code/web/Drivers/Koha.php index 92a4e51902..8e05f1f137 100644 --- a/code/web/Drivers/Koha.php +++ b/code/web/Drivers/Koha.php @@ -46,6 +46,7 @@ function updateHomeLibrary(User $patron, string $homeLibraryCode) { $curRow = $results->fetch_assoc(); $address = $curRow['address']; $city = $curRow['city']; + $results->close(); } $postVariables = [ @@ -142,6 +143,7 @@ function updatePatronInfo($patron, $canUpdateContactInfo, $fromMasquerade = fals $city = $curRow['city']; $lastname = $curRow['surname']; } + $results->close(); } $postVariables = [ @@ -437,7 +439,6 @@ public function getCheckouts(User $patron): array { if ($renewPrefRow = $renewPrefResults->fetch_assoc()) { $renewPref = $renewPrefRow['autorenew_checkouts']; } - $renewPrefResults->close(); } $timer->logTime("Loaded borrower preference for autorenew_checkouts"); @@ -455,7 +456,6 @@ public function getCheckouts(User $patron): array { $patronIsExpired = true; } } - $patronExpirationResults->close(); } $timer->logTime("Loaded patron expiration date"); @@ -709,6 +709,7 @@ public function getCheckouts(User $patron): array { $checkouts[$curCheckout->source . $curCheckout->sourceId . $curCheckout->userId] = $curCheckout; } + $results->close(); //Check to see if any checkouts are Claims Returned $allIssueIdsAsString = implode(',', $allIssueIds); @@ -897,9 +898,11 @@ public function patronLogin($username, $password, $validatedViaSSO) { $patronId = $lookupUserRow['borrowernumber']; $newUser = $this->loadPatronInfoFromDB($patronId, null, $barcode); if (!empty($newUser) && !($newUser instanceof AspenError)) { + $lookupUserResult->close(); return $newUser; } } + $lookupUserResult->close(); } else if ($this->getKohaVersion() >= 22.1110) { //Authenticate the user using KOHA API $oauthToken = $this->getOAuthToken(); @@ -944,9 +947,11 @@ public function patronLogin($username, $password, $validatedViaSSO) { $expiredPasswordResult = $this->processExpiredPassword($lookupUserRow['borrowernumber'], $barcode); if ($expiredPasswordResult != null) { + $lookupUserResult->close(); return $expiredPasswordResult; } } + $lookupUserResult->close(); } $result['messages'][] = translate([ 'text' => 'Unable to authenticate with the ILS. Please try again later or contact the library.', @@ -1015,6 +1020,7 @@ public function patronLogin($username, $password, $validatedViaSSO) { if ($lookupUserResult->num_rows > 0) { $userExistsInDB = true; $lookupUserRow = $lookupUserResult->fetch_assoc(); + $lookupUserResult->close(); if (UserAccount::isUserMasquerading()) { $patronId = $lookupUserRow['borrowernumber']; $newUser = $this->loadPatronInfoFromDB($patronId, null, $barcode); @@ -1035,6 +1041,8 @@ public function patronLogin($username, $password, $validatedViaSSO) { return new AspenError('Maximum number of failed login attempts reached, your account has been locked.'); } } + } else { + $lookupUserResult->close(); } } } else { @@ -1087,6 +1095,7 @@ public function processExpiredPassword($borrowernumber, $barcode) : ?ExpiredPass } } } + $passwordExpirationResult->close(); } catch (Exception $e) { //This happens if password expiration is not enabled } @@ -1209,6 +1218,7 @@ private function loadPatronInfoFromDB($patronId, $password, $suppliedUsernameOrB global $logger; $logger->log("Could not get information about patron category", Logger::LOG_ERROR); } + $patronCategoryResult->close(); } else { global $logger; $logger->log("Could not get information about patron category", Logger::LOG_ERROR); @@ -1324,6 +1334,7 @@ private function loadPatronInfoFromDB($patronId, $password, $suppliedUsernameOrB return $user; } + return $userExistsInDB; } @@ -1434,6 +1445,7 @@ public function doReadingHistoryAction(User $patron, string $action, array $sele $address = $curRow['address']; $city = $curRow['city']; } + $results->close(); } else { //We could not connect to the database, don't update, so we don't corrupt the DB global $logger; @@ -1602,6 +1614,8 @@ public function getReadingHistory($patron, $page = 1, $recordsPerPage = -1, $sor } $readingHistoryTitles[] = $curTitle; } + + $readingHistoryTitleRS->close(); } } @@ -2528,7 +2542,7 @@ public function getHolds($patron, $page = 1, $recordsPerPage = -1, $sortOption = $holds['available'][$curHold->source . $curHold->cancelId . $curHold->userId] = $curHold; } } - + $results->close(); //Load additional ILL Requests that are not shipped $oauthToken = $this->getOAuthToken(); @@ -2969,7 +2983,6 @@ public function renewCheckout($patron, $recordId, $itemId = null, $itemIndex = n $renewSql = "SELECT issues.*, items.biblionumber, items.itype, items.itemcallnumber, items.enumchron, title, author, issues.renewals from issues left join items on items.itemnumber = issues.itemnumber left join biblio ON items.biblionumber = biblio.biblionumber where borrowernumber = '" . mysqli_escape_string($this->dbConnection, $patron->unique_ils_id) . "' AND issues.itemnumber = $itemId limit 1"; } - $renewResults = mysqli_query($this->dbConnection, $renewSql); $maxRenewals = 0; $params = [ @@ -2985,15 +2998,16 @@ public function renewCheckout($patron, $recordId, $itemId = null, $itemIndex = n //Parse the result if (isset($renewResponse->success) && ($renewResponse->success == 1)) { + $renewResults = mysqli_query($this->dbConnection, $renewSql); while ($curRow = mysqli_fetch_assoc($renewResults)) { $patronType = $patron->patronType; $itemType = $curRow['itype']; $checkoutBranch = $curRow['branchcode']; if ($this->getKohaVersion() >= 22.11) { - $renewCount = $curRow['renewals_count'] + 1; + $renewCount = $curRow['renewals_count']; } else { - $renewCount = $curRow['renewals'] + 1; + $renewCount = $curRow['renewals']; } /** @noinspection SqlResolve */ $issuingRulesSql = "SELECT * FROM circulation_rules where rule_name = 'renewalsallowed' AND (categorycode IN ('$patronType', '*') OR categorycode IS NULL) and (itemtype IN('$itemType', '*') OR itemtype is null) and (branchcode IN ('$checkoutBranch', '*') OR branchcode IS NULL) order by branchcode desc, categorycode desc, itemtype desc limit 1"; @@ -3005,6 +3019,7 @@ public function renewCheckout($patron, $recordId, $itemId = null, $itemIndex = n $issuingRulesRS->close(); } } + $renewResults->close(); $renewsRemaining = ($maxRenewals - $renewCount); @@ -3726,6 +3741,7 @@ function getSelfRegistrationFields($type = 'selfReg') { while ($curRow = $results->fetch_assoc()) { $kohaPreferences[$curRow['variable']] = $curRow['value']; } + $results->close(); if ($type == 'selfReg') { $unwantedFields = explode('|', $kohaPreferences['PatronSelfRegistrationBorrowerUnwantedField']); @@ -4845,6 +4861,7 @@ function getNewMaterialsRequestForm(User $user) { while ($curRow = $results->fetch_assoc()) { $kohaPreferences[$curRow['variable']] = $curRow['value']; } + $results->close(); if (isset($kohaPreferences['OPACSuggestionMandatoryFields'])) { $mandatoryFields = array_flip(explode('|', $kohaPreferences['OPACSuggestionMandatoryFields'])); @@ -5177,6 +5194,7 @@ function getNumMaterialsRequests(User $user) { if ($curRow = $results->fetch_assoc()) { $numRequests = $curRow['numRequests']; } + $results->close(); return $numRequests; } @@ -5301,6 +5319,7 @@ function getMaterialsRequests(User $user) { } $allRequests[] = $request; } + $results->close(); return $allRequests; } @@ -5406,6 +5425,7 @@ function getPatronUpdateForm($user) { } } } + $results->close(); //Set default values for extended patron attributes if ($this->getKohaVersion() > 21.05) { @@ -5719,6 +5739,7 @@ function importListsFromIls($patron) { } $results['totalLists']++; } + $listResults->close(); return $results; } @@ -5917,6 +5938,7 @@ public function findNewUser($patronBarcode, $patronUsername) { return $newUser; } } + $lookupUserResult->close(); }else{ //search by username /** @noinspection SqlResolve */ @@ -5931,6 +5953,7 @@ public function findNewUser($patronBarcode, $patronUsername) { return $newUser; } } + $lookupUserResult->close(); } return false; @@ -5955,6 +5978,7 @@ public function findNewUserByEmail($patronEmail): mixed { } else if ($lookupUserResult->num_rows > 1) { return 'Found more than one user.'; } + $lookupUserResult->close(); return false; } @@ -5969,18 +5993,21 @@ public function findUserByField($field, $value) { $sql = "SELECT borrowernumber, cardnumber, " . mysqli_escape_string($this->dbConnection, $field) . " from borrowers where " . mysqli_escape_string($this->dbConnection, $field) . " = '" . mysqli_escape_string($this->dbConnection, $value) . "'"; $lookupUserResult = mysqli_query($this->dbConnection, $sql); + $return_value = false; if ($lookupUserResult->num_rows == 1) { $lookupUserRow = $lookupUserResult->fetch_assoc(); $patronId = $lookupUserRow['borrowernumber']; $newUser = $this->loadPatronInfoFromDB($patronId, null, $lookupUserRow['cardnumber']); if (!empty($newUser) && !($newUser instanceof AspenError)) { - return $newUser; + $return_value = $newUser; } } else if ($lookupUserResult->num_rows > 1) { - return 'Found more than one user.'; + $return_value = 'Found more than one user.'; } - return false; + $lookupUserResult->close(); + + return $return_value; } /** @@ -5997,6 +6024,7 @@ public function showMessagingSettings(): bool { $allowed = false; } } + $preferenceRS->close(); return $allowed; } @@ -6030,6 +6058,7 @@ public function getMessagingSettingsTemplate(User $patron): ?string { $enablePhoneMessaging |= !empty($systemPreference['value']); } } + $systemPreferencesRS->close(); $interface->assign('enablePhoneMessaging', $enablePhoneMessaging); /** @noinspection SqlResolve */ @@ -6039,6 +6068,7 @@ public function getMessagingSettingsTemplate(User $patron): ?string { $interface->assign('smsAlertNumber', $borrowerRow['smsalertnumber']); $interface->assign('smsProviderId', $borrowerRow['sms_provider_id']); } + $borrowerRS->close(); //Lookup which transports are allowed /** @noinspection SqlResolve */ @@ -6057,6 +6087,7 @@ public function getMessagingSettingsTemplate(User $patron): ?string { } $messagingSettings[$transportId]['allowableTransports'][$transportSetting['message_transport_type']] = $transportSetting['message_transport_type']; } + $transportSettingRS->close(); //Get the list of notices to display information for /** @noinspection SqlResolve */ @@ -6100,6 +6131,7 @@ public function getMessagingSettingsTemplate(User $patron): ?string { } $messageAttributes[] = $messageType; } + $messageAttributesRS->close(); $interface->assign('messageAttributes', $messageAttributes); //Get messaging settings for the user @@ -6125,6 +6157,7 @@ public function getMessagingSettingsTemplate(User $patron): ?string { $messagingSettings[$messageType]['selectedTransports'][$userMessagingSetting['message_transport_type']] = $userMessagingSetting['message_transport_type']; } } + $userMessagingSettingsRS->close(); $interface->assign('messagingSettings', $messagingSettings); $validNoticeDays = []; @@ -6154,6 +6187,7 @@ public function getMessagingSettingsTemplate(User $patron): ?string { } else { $noticeLanguages[$language] = $language; } + $languageRS->close(); } /** @noinspection SqlResolve */ $borrowerLanguageSql = "SELECT lang FROM borrowers where borrowernumber = '" . mysqli_escape_string($this->dbConnection, $patron->unique_ils_id) . "'"; @@ -6161,8 +6195,9 @@ public function getMessagingSettingsTemplate(User $patron): ?string { if ($borrowerLanguageRow = $borrowerLanguageRS->fetch_assoc()) { $preferredNoticeLanguage = $borrowerLanguageRow['lang']; } - + $borrowerLanguageRS->close(); } + $interface->assign('canTranslateNotices', $canTranslateNotices); $interface->assign('noticeLanguages', $noticeLanguages); $interface->assign('preferredNoticeLanguage', $preferredNoticeLanguage); @@ -6568,6 +6603,7 @@ public function getShowAutoRenewSwitch(User $patron) { while ($curRow = $results->fetch_assoc()) { $showAutoRenew = $curRow['value']; } + $results->close(); return $showAutoRenew; } @@ -6583,6 +6619,7 @@ public function isAutoRenewalEnabledForUser(User $patron) { $autoRenewEnabled = $curRow['autorenew_checkouts']; break; } + $results->close(); } return $autoRenewEnabled; } @@ -6605,6 +6642,7 @@ public function updateAutoRenewal(User $patron, bool $allowAutoRenewal) { $address = $curRow['address']; $city = $curRow['city']; } + $results->close(); } $postVariables = [ @@ -6687,6 +6725,7 @@ function getPasswordRecoveryTemplate() { $uniqueKeyValid = true; } } + $lookupResult->close(); if (!$uniqueKeyValid) { $error = translate([ 'text' => 'The link you clicked is either invalid, or expired.
Be sure you used the link from the email, or contact library staff for assistance.
Please contact the library if you need further assistance.', @@ -6728,6 +6767,8 @@ function processPasswordRecovery() { $uniqueKeyValid = true; } } + $lookupResult->close(); + if (!$uniqueKeyValid) { $error = translate([ 'text' => 'The link you clicked is either invalid, or expired.
Be sure you used the link from the email, or contact library staff for assistance.
Please contact the library if you need further assistance.', @@ -6841,6 +6882,8 @@ function setExtendedAttributes() { $extendedAttributes[] = $attribute; } + $borrowerAttributeTypesRS->close(); + return $extendedAttributes; } @@ -7028,6 +7071,7 @@ public function getEditableUsername(User $user) { if ($curRow = $results->fetch_assoc()) { return $curRow['userId']; } + $results->close(); } return null; } @@ -7049,6 +7093,7 @@ public function updateEditableUsername(User $patron, string $username): array { 'message' => 'Sorry, that username is not available.', ]; } + $results->close(); } //Load required fields from Koha here to make sure we don't wipe them out /** @noinspection SqlResolve */ @@ -7061,6 +7106,7 @@ public function updateEditableUsername(User $patron, string $username): array { $address = $curRow['address']; $city = $curRow['city']; } + $results->close(); } $postVariables = [ @@ -7143,6 +7189,7 @@ public function getILSMessages(User $user) { 'messageStyle' => 'info', ]; } + $results->close(); } } @@ -7186,6 +7233,7 @@ public function getILSMessages(User $user) { ]; } } + $results->close(); } return $messages; @@ -7424,6 +7472,7 @@ public function getCurbsidePickupSettings($locationCode) { } } } + $results->close(); } } else { @@ -7680,6 +7729,7 @@ function validateUniqueId(User $user) { $lookupUserResult = mysqli_query($this->dbConnection, $sql); if ($lookupUserResult->num_rows > 0) { $lookupUserRow = $lookupUserResult->fetch_assoc(); + $lookupUserResult->close(); if ($lookupUserRow['borrowernumber'] != $user->unique_ils_id) { global $logger; $logger->log("Updating unique id for user from $user->unique_ils_id to {$lookupUserRow['borrowernumber']}", Logger::LOG_WARNING); @@ -7912,7 +7962,9 @@ public function lookupAccountByEmail(string $email) : array { 'name' => trim(trim($curRow['firstname'] . ' ' . $curRow['middle_name']) . ' ' . $curRow['surname']), ]; } + $results->close(); } + if (count($cardNumbers) == 0) { return [ 'success' => false, @@ -7947,6 +7999,7 @@ public function lookupAccountByPhoneNumber(string $phone) : array { 'patronId' => $curRow['borrowernumber'] ]; } + $results->close(); } if (count($cardNumbers) == 0) { return [ @@ -7977,6 +8030,7 @@ public function getBasicRegistrationForm() : array { while ($curRow = $results->fetch_assoc()) { $kohaPreferences[$curRow['variable']] = $curRow['value']; } + $results->close(); $unwantedFields = explode('|', $kohaPreferences['PatronSelfRegistrationBorrowerUnwantedField']); $requiredFields = explode('|', $kohaPreferences['PatronSelfRegistrationBorrowerMandatoryField']); @@ -8185,6 +8239,8 @@ public function bypassReadingHistoryUpdate($patron, $isNightlyUpdate) : bool { $expirationDate = strtotime($curRow['dateexpiry']); } } + + $results->close(); } //Don't update reading history if we've never seen the patron or the patron was last seen before we last updated reading history @@ -8383,6 +8439,8 @@ public function checkoutByAPI(User $patron, $barcode, Location $currentLocation) 'isPublicFacing' => true, ]); } + + $lookupItemResult->close(); } return $result; @@ -8414,6 +8472,7 @@ public function getMessageTypes(): array { $transports[$curRow['module']][$i]['name'] = $curRow['name']; $i++; } + $results->close(); } return $transports; @@ -8473,6 +8532,8 @@ public function updateMessageQueue(): array { } } + $results->close(); + return [ 'success' => true, 'message' => 'Added ' . $numAdded . ' to message queue' @@ -8532,6 +8593,8 @@ public function updateUserMessageQueue(User $patron): array { } + $results->close(); + return [ 'success' => true, 'message' => 'Updated user message queue' @@ -8564,4 +8627,4 @@ protected function getUserMessageTranslation($code, User $patron): array { return $result; } -} \ No newline at end of file +} diff --git a/code/web/release_notes/24.09.00.MD b/code/web/release_notes/24.09.00.MD index 78591c86f7..146cf17910 100644 --- a/code/web/release_notes/24.09.00.MD +++ b/code/web/release_notes/24.09.00.MD @@ -132,6 +132,7 @@ - Remove superfluous loop in Koha driver function updateHomeLibrary #1968 (*KMH*) - Hide empty item groups for volume-level holds in Koha (*KMH*) - Remove old pre-production Koha volumes code (*KMH*) +- Properly close the connections to Koha from Aspen (*KMH*) ### GitHub Actions - Add GitHub Actions to check pull requests for release notes (*KMH*)