Fix MySQL error 1390 (too many placeholders) in album/photo deletion path#4225
Fix MySQL error 1390 (too many placeholders) in album/photo deletion path#4225
Conversation
… with many photos Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com> Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/4ddd5192-7894-4390-85ae-d0eaf37842db
…SQL placeholder limit Co-authored-by: ildyria <627094+ildyria@users.noreply.github.com> Agent-Logs-Url: https://github.com/LycheeOrg/Lychee/sessions/07aec537-3849-4561-90cc-10d5fb571821
|
@coderabbitai review please. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors delete operations across album and photo DTOs to use chunking and subqueries instead of materializing large ID arrays into memory, preventing MySQL prepared statement placeholder limit errors while maintaining existing deletion order semantics and public interfaces. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/Actions/Album/Delete.php (2)
120-129:deleteTagAlbumsuses unchunkedwhereIncalls.The tag album deletion path uses multiple
whereIn($tag_album_ids)calls without chunking. While tag albums are typically fewer in number than photos, extremely large bulk deletions could still hit the placeholder limit.♻️ Optional: Apply consistent chunking pattern
private function deleteTagAlbums(array $tag_album_ids): void { + if (count($tag_album_ids) === 0) { + return; + } $purchasable_service = resolve(PurchasableService::class); $purchasable_service->deleteMultipleAlbumPurchasables($tag_album_ids); - DB::table('live_metrics')->whereIn('album_id', $tag_album_ids)->delete(); - DB::table(APC::ACCESS_PERMISSIONS)->whereIn(APC::BASE_ALBUM_ID, $tag_album_ids)->delete(); - DB::table('statistics')->whereIn('album_id', $tag_album_ids)->delete(); - DB::table('tag_albums')->whereIn('id', $tag_album_ids)->delete(); - DB::table('base_albums')->whereIn('id', $tag_album_ids)->delete(); + collect($tag_album_ids)->chunk(1000)->each(function ($chunk): void { + DB::table('live_metrics')->whereIn('album_id', $chunk->all())->delete(); + DB::table(APC::ACCESS_PERMISSIONS)->whereIn(APC::BASE_ALBUM_ID, $chunk->all())->delete(); + DB::table('statistics')->whereIn('album_id', $chunk->all())->delete(); + DB::table('tag_albums')->whereIn('id', $chunk->all())->delete(); + DB::table('base_albums')->whereIn('id', $chunk->all())->delete(); + }); }
152-164: Good use of subquery to avoid photo_id placeholder explosion, but consider album_id array size.The subquery approach elegantly avoids materializing potentially millions of photo IDs. However,
whereIn(PA::ALBUM_ID, $album_ids)in both the subquery (line 159) and the initial query (line 144) could theoretically hit the placeholder limit if the recursive album collection infindAllAlbumsToDeleteproduces a very large array. This is unlikely in practice for most installations, but the code already acknowledges the memory risk with an explicit comment. Consider chunking the album_ids array as defensive programming for deeply nested or exceptionally large album trees.app/DTO/Delete/PhotosToBeDeletedDTO.php (1)
107-107:exclude_size_variants_idsis unchunked in both fetch and usage.Line 107 fetches all
size_variant_idfromorder_itemsinto memory, and line 134 uses it in awhereInwithout chunking. If the order history grows large, this could hit limits.However, this is likely acceptable in practice since order items referencing size variants that are about to be deleted would be an unusual edge case (orders typically reference photos that shouldn't be deleted).
Also applies to: 134-134
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8889789-cf90-41da-8059-49e5c5d7249e
📒 Files selected for processing (3)
app/Actions/Album/Delete.phpapp/DTO/Delete/AlbumsToBeDeletedDTO.phpapp/DTO/Delete/PhotosToBeDeletedDTO.php
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
Deleting albums with large photo counts hits MySQL's 65,535 prepared-statement placeholder limit across multiple
whereIncalls that receive unbounded PHP arrays of photo/album IDs.Root cause
Two patterns:
whereIn('photo_id', $photos_ids)— array can be arbitrarily large (one entry per photo)whereIn('album_id', $album_ids)— same risk for deeply recursive album treesFixes
app/Actions/Album/Delete.phpfindAllPhotosToDelete(): replacewhereIn(PA::PHOTO_ID, $photos_ids)(PHP array round-trip) with a correlated subquery — eliminates the intermediate array entirelyapp/DTO/Delete/PhotosToBeDeletedDTO.php— addedCHUNK_SIZE = 1000softDelete(): 2-D chunking on bothsoft_delete_photo_ids(outer) andalbum_ids(inner)forceDelete(): chunkedforce_delete_photo_idsacross all writes — albumheader_id/cover_idnullification and five table deletes (size_variants,statistics,palettes,photo_album,photos)collectSizeVariantPathsByPhotoID()/collectLivePhotoPathsByPhotoID(): chunkphoto_ids, accumulate withreduce+concatapp/DTO/Delete/AlbumsToBeDeletedDTO.php— addedCHUNK_SIZE = 1000containsshort-circuits on first hit)_lft DESC(fetched in chunks, sorted in PHP) so children precede parents across chunk boundaries, preserving nested-set FK integrityalbums(leaf-first) →base_albums💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.
Summary by CodeRabbit
Refactor