SL-329 optimize db - #31
Conversation
Summary of ChangesHello @MarijusDilys, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the database performance of the SaferPay module. It achieves this by strategically adding new indexes to several critical tables and optimizing existing database queries. The changes are designed to make data access faster and more efficient, particularly for frequently queried fields, which should lead to a more responsive user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on optimizing database performance by introducing indexes on several tables and refining query construction to ensure indexes are utilized effectively. The changes in src/Install/Installer.php and the new upgrade script upgrade/install-1.2.8.php correctly add necessary indexes. The modifications in the repository classes to remove quotes from integer comparisons in WHERE clauses are also a good improvement. My review includes a couple of suggestions to further refine the database schema by removing a redundant index and to improve the maintainability of the upgrade script by refactoring duplicated code.
| foreach ($orderIndexes as $indexSql) { | ||
| try { | ||
| $result = $db->execute($indexSql); | ||
| if (!$result) { | ||
| $error = $db->getMsgError(); | ||
| if (strpos($error, 'Duplicate key name') === false) { | ||
| $success = false; | ||
| PrestaShopLogger::addLog('SaferPay: Failed to add order index - ' . $error, 3, null, 'SaferPayOrder'); | ||
| } | ||
| } | ||
| } catch (Exception $e) { | ||
| PrestaShopLogger::addLog('SaferPay: Order index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayOrder'); | ||
| } | ||
| } | ||
|
|
||
| // Add indexes for saferpay_card_alias table | ||
| $cardAliasIndexes = [ | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_id_customer` (`id_customer`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_payment_method` (`payment_method`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_customer_payment` (`id_customer`, `payment_method`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_valid_till` (`valid_till`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_alias_id` (`alias_id`)", | ||
| ]; | ||
|
|
||
| foreach ($cardAliasIndexes as $indexSql) { | ||
| try { | ||
| $result = $db->execute($indexSql); | ||
| if (!$result) { | ||
| $error = $db->getMsgError(); | ||
| if (strpos($error, 'Duplicate key name') === false) { | ||
| $success = false; | ||
| PrestaShopLogger::addLog('SaferPay: Failed to add card alias index - ' . $error, 3, null, 'SaferPayCardAlias'); | ||
| } | ||
| } | ||
| } catch (Exception $e) { | ||
| PrestaShopLogger::addLog('SaferPay: Card alias index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayCardAlias'); | ||
| } | ||
| } | ||
|
|
||
| // Add indexes for saferpay_assert table | ||
| $assertIndexes = [ | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_assert` ADD INDEX `idx_id_saferpay_order` (`id_saferpay_order`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_assert` ADD INDEX `idx_payment_method` (`payment_method`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_assert` ADD INDEX `idx_brand` (`brand`)", | ||
| ]; | ||
|
|
||
| foreach ($assertIndexes as $indexSql) { | ||
| try { | ||
| $result = $db->execute($indexSql); | ||
| if (!$result) { | ||
| $error = $db->getMsgError(); | ||
| if (strpos($error, 'Duplicate key name') === false) { | ||
| $success = false; | ||
| PrestaShopLogger::addLog('SaferPay: Failed to add assert index - ' . $error, 3, null, 'SaferPayAssert'); | ||
| } | ||
| } | ||
| } catch (Exception $e) { | ||
| PrestaShopLogger::addLog('SaferPay: Assert index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayAssert'); | ||
| } | ||
| } | ||
|
|
||
| // Add indexes for saferpay_order_refund table | ||
| $refundIndexes = [ | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order_refund` ADD INDEX `idx_id_saferpay_order` (`id_saferpay_order`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order_refund` ADD INDEX `idx_id_order` (`id_order`)", | ||
| "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order_refund` ADD INDEX `idx_transaction_id` (`transaction_id`)", | ||
| ]; | ||
|
|
||
| foreach ($refundIndexes as $indexSql) { | ||
| try { | ||
| $result = $db->execute($indexSql); | ||
| if (!$result) { | ||
| $error = $db->getMsgError(); | ||
| if (strpos($error, 'Duplicate key name') === false) { | ||
| $success = false; | ||
| PrestaShopLogger::addLog('SaferPay: Failed to add refund index - ' . $error, 3, null, 'SaferPayOrderRefund'); | ||
| } | ||
| } | ||
| } catch (Exception $e) { | ||
| PrestaShopLogger::addLog('SaferPay: Refund index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayOrderRefund'); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication in this function. The logic for adding indexes is repeated four times with only minor differences in variable names and log messages. This can be refactored into a helper function to improve readability and maintainability.
For example, you could create a function like this:
function _add_saferpay_indexes(Db $db, array $indexQueries, string $entityName, string $logObjectName, bool &$success)
{
foreach ($indexQueries as $indexSql) {
try {
$result = $db->execute($indexSql);
if (!$result) {
$error = $db->getMsgError();
if (strpos($error, 'Duplicate key name') === false) {
$success = false;
PrestaShopLogger::addLog(
"SaferPay: Failed to add {$entityName} index - " . $error,
3,
null,
$logObjectName
);
}
}
} catch (Exception $e) {
PrestaShopLogger::addLog(
"SaferPay: {$entityName} index creation skipped - " . $e->getMessage(),
1,
null,
$logObjectName
);
}
}
}And then call it for each table:
_add_saferpay_indexes($db, $orderIndexes, 'order', 'SaferPayOrder', $success);
_add_saferpay_indexes($db, $cardAliasIndexes, 'card alias', 'SaferPayCardAlias', $success);
// ... and so on for other tables.1 participant
Self-Checks
JIRA task link
Summary
QA Checklist Labels
QA Checklist
Additional Context
Frontend Changes