Skip to content

Commit d958cfa

Browse files
fix: address CodeRabbit review feedback
- UserWishlistController: validate soft-deleted books before wishlist ops - ReservationManager/ReservationsController: return false/0 for missing books - PrestitiController: move duplicate check inside transaction with FOR UPDATE - QueryCache: add stampede prevention, safe unserialize, selective APCu flush - web.php: standardize 404 responses with i18n - layout.php: remove duplicate version.json, add defer to vendor.bundle.js - Add idx_libri_deleted_at to migration and schema - Add translation for "Libro non trovato"
1 parent b4784c0 commit d958cfa

12 files changed

Lines changed: 149 additions & 48 deletions

File tree

app/Controllers/PrestitiController.php

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -111,20 +111,6 @@ public function store(Request $request, Response $response, mysqli $db): Respons
111111
return $response->withHeader('Location', '/admin/prestiti/crea?error=missing_fields')->withStatus(302);
112112
}
113113

114-
// Case 8: Prevent multiple active reservations/loans for the same book by the same user
115-
$dupStmt = $db->prepare("
116-
SELECT id FROM prestiti
117-
WHERE libro_id = ? AND utente_id = ? AND attivo = 1
118-
AND stato IN ('in_corso', 'prenotato', 'pendente', 'in_ritardo')
119-
");
120-
$dupStmt->bind_param('ii', $libro_id, $utente_id);
121-
$dupStmt->execute();
122-
if ($dupStmt->get_result()->num_rows > 0) {
123-
$dupStmt->close();
124-
return $response->withHeader('Location', '/admin/prestiti/crea?error=duplicate_reservation')->withStatus(302);
125-
}
126-
$dupStmt->close();
127-
128114
// Verifica che la data di scadenza sia successiva alla data di prestito
129115
if (strtotime($data_scadenza) <= strtotime($data_prestito)) {
130116
return $response->withHeader('Location', '/admin/prestiti/crea?error=invalid_dates')->withStatus(302);
@@ -146,6 +132,23 @@ public function store(Request $request, Response $response, mysqli $db): Respons
146132
return $response->withHeader('Location', '/admin/prestiti/crea?error=book_not_found')->withStatus(302);
147133
}
148134

135+
// Case 8: Prevent multiple active reservations/loans for the same book by the same user
136+
// Moved inside transaction with FOR UPDATE to prevent race conditions
137+
$dupStmt = $db->prepare("
138+
SELECT id FROM prestiti
139+
WHERE libro_id = ? AND utente_id = ? AND attivo = 1
140+
AND stato IN ('in_corso', 'prenotato', 'pendente', 'in_ritardo')
141+
FOR UPDATE
142+
");
143+
$dupStmt->bind_param('ii', $libro_id, $utente_id);
144+
$dupStmt->execute();
145+
if ($dupStmt->get_result()->num_rows > 0) {
146+
$dupStmt->close();
147+
$db->rollback();
148+
return $response->withHeader('Location', '/admin/prestiti/crea?error=duplicate_reservation')->withStatus(302);
149+
}
150+
$dupStmt->close();
151+
149152
// Check if loan starts today (immediate loan) or in the future (scheduled loan)
150153
// Normalize to date-only to handle potential datetime inputs safely
151154
$today = gmdate('Y-m-d');

app/Controllers/ReservationManager.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,12 @@ public function isBookAvailableForImmediateLoan($bookId, ?string $startDate = nu
432432
$fallbackStmt->execute();
433433
$fallbackRow = $fallbackStmt->get_result()?->fetch_assoc();
434434
$fallbackStmt->close();
435-
$totalCopies = (int) ($fallbackRow['copie_totali'] ?? 1);
435+
436+
// If book doesn't exist or is soft-deleted, return false immediately
437+
if ($fallbackRow === null) {
438+
return false;
439+
}
440+
$totalCopies = (int) $fallbackRow['copie_totali'];
436441
}
437442

438443
if ($totalCopies === 0) {

app/Controllers/ReservationsController.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,11 @@ private function getBookTotalCopies(int $bookId): int
419419
$row = $result ? $result->fetch_assoc() : null;
420420
$stmt->close();
421421

422-
return (int) ($row['copie_totali'] ?? 1);
422+
// If book doesn't exist or is soft-deleted, return 0
423+
if ($row === null) {
424+
return 0;
425+
}
426+
427+
return (int) $row['copie_totali'];
423428
}
424429
}

app/Controllers/UserWishlistController.php

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,10 @@ public function status(Request $request, Response $response, mysqli $db): Respon
6464
$libroId = (int) ($q['libro_id'] ?? 0);
6565
$fav = false;
6666
if ($libroId > 0) {
67-
$stmt = $db->prepare('SELECT 1 FROM wishlist WHERE utente_id=? AND libro_id=? LIMIT 1');
67+
// Join with libri to exclude soft-deleted books
68+
$stmt = $db->prepare('SELECT 1 FROM wishlist w
69+
JOIN libri l ON l.id = w.libro_id AND l.deleted_at IS NULL
70+
WHERE w.utente_id = ? AND w.libro_id = ? LIMIT 1');
6871
$uid = (int) $user['id'];
6972
$stmt->bind_param('ii', $uid, $libroId);
7073
$stmt->execute();
@@ -101,6 +104,18 @@ public function toggle(Request $request, Response $response, mysqli $db): Respon
101104
// Item was removed
102105
$payload = ['favorite' => false];
103106
} else {
107+
// Validate book exists and is not soft-deleted before inserting
108+
$checkStmt = $db->prepare('SELECT id FROM libri WHERE id = ? AND deleted_at IS NULL');
109+
$checkStmt->bind_param('i', $libroId);
110+
$checkStmt->execute();
111+
$bookExists = $checkStmt->get_result()->num_rows > 0;
112+
$checkStmt->close();
113+
114+
if (!$bookExists) {
115+
$response->getBody()->write(json_encode(['error' => true, 'message' => __('Libro non trovato')]));
116+
return $response->withHeader('Content-Type', 'application/json')->withStatus(404);
117+
}
118+
104119
// Item didn't exist, insert it (use IGNORE to handle concurrent inserts)
105120
$stmt = $db->prepare('INSERT IGNORE INTO wishlist (utente_id, libro_id) VALUES (?, ?)');
106121
$stmt->bind_param('ii', $uid, $libroId);

app/Routes/web.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1512,7 +1512,7 @@
15121512
}
15131513
$stmt->close();
15141514
if (!$bookFound) {
1515-
$response->getBody()->write(json_encode(['error' => 'Book not found']));
1515+
$response->getBody()->write(json_encode(['success' => false, 'message' => __('Libro non trovato')]));
15161516
return $response->withStatus(404)->withHeader('Content-Type', 'application/json');
15171517
}
15181518
$data['available'] = ($data['copies_available'] > 0);
@@ -1719,7 +1719,7 @@
17191719
$book = $bookStmt->get_result()->fetch_assoc();
17201720
$bookStmt->close();
17211721
if (!$book) {
1722-
$response->getBody()->write(json_encode(['error' => 'Book not found']));
1722+
$response->getBody()->write(json_encode(['success' => false, 'message' => __('Libro non trovato')]));
17231723
return $response->withStatus(404)->withHeader('Content-Type', 'application/json');
17241724
}
17251725

app/Support/QueryCache.php

Lines changed: 91 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ private static function hasApcu(): bool
5252
/**
5353
* Get a value from cache or execute callback to generate it
5454
*
55+
* Uses mutex locking to prevent cache stampede (thundering herd problem).
56+
* Only one process computes the value while others wait.
57+
*
5558
* @param string $key Unique cache key
5659
* @param callable $callback Function to generate value if not cached
5760
* @param int $ttl Time to live in seconds (default: 300 = 5 minutes)
@@ -65,13 +68,46 @@ public static function remember(string $key, callable $callback, int $ttl = 300)
6568
return $cached;
6669
}
6770

68-
// Execute callback to get fresh value
69-
$value = $callback();
71+
// Acquire mutex lock to prevent stampede
72+
$lockKey = self::hashKey($key) . '.lock';
73+
$lockFile = self::getCacheDir() . '/' . $lockKey;
74+
$lockHandle = @fopen($lockFile, 'c');
75+
76+
if ($lockHandle === false) {
77+
// If we can't get a lock, just execute callback (graceful degradation)
78+
return $callback();
79+
}
80+
81+
try {
82+
// Try to acquire exclusive lock (non-blocking first)
83+
if (!flock($lockHandle, LOCK_EX | LOCK_NB)) {
84+
// Another process is computing, wait for it and try cache again
85+
flock($lockHandle, LOCK_EX);
86+
$cached = self::get($key);
87+
if ($cached !== null) {
88+
return $cached;
89+
}
90+
}
91+
92+
// Double-check cache after acquiring lock
93+
$cached = self::get($key);
94+
if ($cached !== null) {
95+
return $cached;
96+
}
97+
98+
// Execute callback to get fresh value
99+
$value = $callback();
70100

71-
// Store in cache
72-
self::set($key, $value, $ttl);
101+
// Store in cache
102+
self::set($key, $value, $ttl);
73103

74-
return $value;
104+
return $value;
105+
} finally {
106+
flock($lockHandle, LOCK_UN);
107+
fclose($lockHandle);
108+
// Clean up lock file (best effort)
109+
@unlink($lockFile);
110+
}
75111
}
76112

77113
/**
@@ -181,15 +217,25 @@ public static function clearByPrefix(string $prefix): int
181217
}
182218

183219
/**
184-
* Clear all cache entries
220+
* Clear all Pinakes cache entries
221+
*
222+
* Only clears entries with the 'pinakes_' prefix to avoid clearing
223+
* other applications' cache entries that may share the same APCu instance.
185224
*
186225
* @return bool Success status
187226
*/
188227
public static function flush(): bool
189228
{
190-
// Try APCu first
229+
// Try APCu first - only clear pinakes_* keys, not the entire cache
191230
if (self::hasApcu()) {
192-
return apcu_clear_cache();
231+
$success = true;
232+
$iterator = new \APCUIterator('/^pinakes_/');
233+
foreach ($iterator as $item) {
234+
if (!apcu_delete($item['key'])) {
235+
$success = false;
236+
}
237+
}
238+
return $success;
193239
}
194240

195241
// Fallback to file cache - delete all files
@@ -223,6 +269,9 @@ private static function hashKey(string $key): string
223269

224270
/**
225271
* Get value from file cache
272+
*
273+
* Uses file locking (flock) to prevent reading incomplete/corrupted data
274+
* and safe unserialize to prevent object injection attacks.
226275
*/
227276
private static function getFromFile(string $hashedKey): mixed
228277
{
@@ -232,24 +281,45 @@ private static function getFromFile(string $hashedKey): mixed
232281
return null;
233282
}
234283

235-
$content = @file_get_contents($path);
236-
if ($content === false) {
284+
// Open file with shared lock for reading
285+
$handle = @fopen($path, 'r');
286+
if ($handle === false) {
237287
return null;
238288
}
239289

240-
$data = @unserialize($content);
241-
if ($data === false || !is_array($data)) {
242-
@unlink($path);
243-
return null;
244-
}
290+
try {
291+
// Acquire shared lock for reading
292+
if (!flock($handle, LOCK_SH)) {
293+
return null;
294+
}
245295

246-
// Check expiration
247-
if (isset($data['expires']) && $data['expires'] < time()) {
248-
@unlink($path);
249-
return null;
250-
}
296+
$content = stream_get_contents($handle);
297+
if ($content === false || $content === '') {
298+
return null;
299+
}
300+
301+
// Use safe unserialize to prevent object injection attacks
302+
$data = @unserialize($content, ['allowed_classes' => false]);
303+
if ($data === false || !\is_array($data)) {
304+
flock($handle, LOCK_UN);
305+
fclose($handle);
306+
@unlink($path);
307+
return null;
308+
}
251309

252-
return $data['value'] ?? null;
310+
// Check expiration
311+
if (isset($data['expires']) && $data['expires'] < time()) {
312+
flock($handle, LOCK_UN);
313+
fclose($handle);
314+
@unlink($path);
315+
return null;
316+
}
317+
318+
return $data['value'] ?? null;
319+
} finally {
320+
flock($handle, LOCK_UN);
321+
fclose($handle);
322+
}
253323
}
254324

255325
/**

app/Views/frontend/layout.php

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,13 +1568,8 @@ class="fa-brands fa-bluesky"></i></a>
15681568
</div>
15691569
<hr class="my-4">
15701570
<div class="text-center">
1571-
<?php
1572-
$versionFile = dirname(dirname(dirname(__DIR__))) . '/version.json';
1573-
$versionData = file_exists($versionFile) ? json_decode(file_get_contents($versionFile), true) : null;
1574-
$version = $versionData['version'] ?? '0.1.1';
1575-
?>
15761571
<p><?= date('Y') ?><?= HtmlHelper::e($appName) ?> • Powered by Pinakes
1577-
v<?= HtmlHelper::e($version) ?></p>
1572+
v<?= HtmlHelper::e($appVersion) ?></p>
15781573
</div>
15791574
</div>
15801575
</footer>

app/Views/layout.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,7 @@ class="absolute z-50 w-full mt-2 bg-white border border-gray-200 rounded-2xl sha
695695
return translated;
696696
};
697697
</script>
698-
<script src="/assets/vendor.bundle.js?v=<?= $appVersion ?>"></script>
698+
<script src="/assets/vendor.bundle.js?v=<?= $appVersion ?>" defer></script>
699699
<script src="/assets/tinymce/tinymce.min.js" defer></script>
700700
<script src="/assets/flatpickr-init.js?v=<?= $appVersion ?>" defer></script>
701701
<script src="/assets/main.bundle.js?v=<?= $appVersion ?>" defer></script>

installer/database/indexes_optimization.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ ALTER TABLE libri ADD INDEX IF NOT EXISTS idx_isbn10 (isbn10);
3030
ALTER TABLE libri ADD INDEX IF NOT EXISTS idx_genere_scaffale (genere_id, scaffale_id);
3131
ALTER TABLE libri ADD INDEX IF NOT EXISTS idx_sottogenere_scaffale (sottogenere_id, scaffale_id);
3232

33+
-- Indice per soft-delete (SitemapGenerator, filtri deleted_at IS NULL)
34+
ALTER TABLE libri ADD INDEX IF NOT EXISTS idx_libri_deleted_at (deleted_at);
35+
3336
-- =====================================================
3437
-- TABELLA: libri_autori (CRITICA - mancano indici composti)
3538
-- Già presenti: libro_id, autore_id (singoli)
Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
-- Migration script for Pinakes 0.4.3
2-
-- Description: Add 'annullato' and 'scaduto' to prestiti status enum
2+
-- Description: Add 'annullato' and 'scaduto' to prestiti status enum, add index for soft-delete queries
33
-- Date: 2025-12-10
44

55
ALTER TABLE `prestiti` MODIFY COLUMN `stato` ENUM('pendente','prenotato','in_corso','restituito','in_ritardo','perso','danneggiato','annullato','scaduto') COLLATE utf8mb4_unicode_ci DEFAULT 'pendente';
6+
7+
-- Add index for deleted_at to optimize soft-delete queries (SitemapGenerator, etc.)
8+
ALTER TABLE `libri` ADD INDEX IF NOT EXISTS `idx_libri_deleted_at` (`deleted_at`);

0 commit comments

Comments
 (0)