From 7c12f548d7e1817fee465b1f6df3e926c49056cb Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Sun, 6 Sep 2026 14:04:05 +0700 Subject: [PATCH] fix(library): bound playlist picker SQL parameters Deduplicate requested track keys and aggregate match counts in batches of 500 parameters. Use scalar preview lookups instead of expanding another playlist ID list. Cover large selections, duplicate keys, empty selections, and cover selection in regression tests. --- .../library_collections_database.dart | 200 ++++++++---------- test/playlist_picker_database_test.dart | 118 +++++++++++ 2 files changed, 208 insertions(+), 110 deletions(-) create mode 100644 test/playlist_picker_database_test.dart diff --git a/lib/services/library_collections_database.dart b/lib/services/library_collections_database.dart index 060b37a9..65f0f99c 100644 --- a/lib/services/library_collections_database.dart +++ b/lib/services/library_collections_database.dart @@ -310,116 +310,7 @@ class LibraryCollectionsDatabase { Future> loadPlaylistPickerSummaries( List requestedTrackKeys, - ) async { - final db = await database; - final uniqueTrackKeys = requestedTrackKeys - .where((key) => key.trim().isNotEmpty) - .toSet() - .toList(growable: false); - - final playlistRows = await db.rawQuery(''' - SELECT - p.id, - p.name, - p.cover_image_path, - p.created_at, - p.updated_at, - COUNT(pt.track_key) AS track_count - FROM $_tablePlaylists p - LEFT JOIN $_tablePlaylistTracks pt ON pt.playlist_id = p.id - GROUP BY p.id - ORDER BY p.created_at DESC, p.rowid DESC - '''); - - final matchedCountsByPlaylistId = {}; - if (uniqueTrackKeys.isNotEmpty) { - final placeholders = List.filled(uniqueTrackKeys.length, '?').join(', '); - final matchedRows = await db.rawQuery(''' - SELECT playlist_id, COUNT(*) AS matched_count - FROM $_tablePlaylistTracks - WHERE track_key IN ($placeholders) - GROUP BY playlist_id - ''', uniqueTrackKeys); - for (final row in matchedRows) { - final playlistId = row['playlist_id']?.toString(); - if (playlistId == null || playlistId.isEmpty) continue; - matchedCountsByPlaylistId[playlistId] = - (row['matched_count'] as num?)?.toInt() ?? 0; - } - } - - final playlistIdsNeedingPreview = playlistRows - .where((row) { - final coverPath = row['cover_image_path']?.toString(); - return coverPath == null || coverPath.isEmpty; - }) - .map((row) => row['id']?.toString() ?? '') - .where((id) => id.isNotEmpty) - .toList(growable: false); - - final previewCoverByPlaylistId = {}; - if (playlistIdsNeedingPreview.isNotEmpty) { - final placeholders = List.filled( - playlistIdsNeedingPreview.length, - '?', - ).join(', '); - final previewRows = await db.rawQuery(''' - SELECT outer_tracks.playlist_id, outer_tracks.track_json - FROM $_tablePlaylistTracks outer_tracks - WHERE outer_tracks.playlist_id IN ($placeholders) - AND outer_tracks.rowid = ( - SELECT inner_tracks.rowid - FROM $_tablePlaylistTracks inner_tracks - WHERE inner_tracks.playlist_id = outer_tracks.playlist_id - ORDER BY inner_tracks.added_at ASC, inner_tracks.rowid ASC - LIMIT 1 - ) - ''', playlistIdsNeedingPreview); - - for (final row in previewRows) { - final playlistId = row['playlist_id']?.toString(); - final trackJson = row['track_json'] as String?; - if (playlistId == null || - playlistId.isEmpty || - trackJson == null || - trackJson.isEmpty) { - continue; - } - try { - final decoded = jsonDecode(trackJson); - if (decoded is! Map) continue; - final coverUrl = decoded['coverUrl']?.toString(); - if (coverUrl != null && coverUrl.isNotEmpty) { - previewCoverByPlaylistId[playlistId] = coverUrl; - } - } catch (_) {} - } - } - - return playlistRows - .map((row) { - final id = row['id']?.toString() ?? ''; - final createdAt = - DateTime.tryParse(row['created_at']?.toString() ?? '') ?? - DateTime.now(); - final updatedAt = - DateTime.tryParse(row['updated_at']?.toString() ?? '') ?? - createdAt; - return PlaylistPickerSummaryRow( - id: id, - name: row['name']?.toString() ?? '', - coverImagePath: row['cover_image_path'] as String?, - previewCover: previewCoverByPlaylistId[id], - createdAt: createdAt, - updatedAt: updatedAt, - trackCount: (row['track_count'] as num?)?.toInt() ?? 0, - containsAllRequestedTracks: - uniqueTrackKeys.isNotEmpty && - matchedCountsByPlaylistId[id] == uniqueTrackKeys.length, - ); - }) - .toList(growable: false); - } + ) async => readPlaylistPickerSummaries(await database, requestedTrackKeys); Future _upsertEntry( String table, @@ -703,3 +594,92 @@ class LibraryCollectionsDatabase { _log.i('Restored collections from backup'); } } + +/// Reads picker rows with bounded selection queries, preserving playlist order. +Future> readPlaylistPickerSummaries( + DatabaseExecutor db, + List requestedTrackKeys, +) async { + final uniqueTrackKeys = requestedTrackKeys + .where((key) => key.trim().isNotEmpty) + .toSet() + .toList(growable: false); + + final playlistRows = await db.rawQuery(''' + SELECT + p.id, + p.name, + p.cover_image_path, + p.created_at, + p.updated_at, + COUNT(pt.track_key) AS track_count, + CASE WHEN p.cover_image_path IS NULL OR p.cover_image_path = '' THEN ( + SELECT preview.track_json + FROM $_tablePlaylistTracks preview + WHERE preview.playlist_id = p.id + ORDER BY preview.added_at ASC, preview.rowid ASC + LIMIT 1 + ) END AS preview_track_json + FROM $_tablePlaylists p + LEFT JOIN $_tablePlaylistTracks pt ON pt.playlist_id = p.id + GROUP BY p.id + ORDER BY p.created_at DESC, p.rowid DESC + '''); + + final matchedCountsByPlaylistId = {}; + const chunkSize = 500; + for (var offset = 0; offset < uniqueTrackKeys.length; offset += chunkSize) { + final chunk = uniqueTrackKeys.sublist( + offset, + (offset + chunkSize).clamp(0, uniqueTrackKeys.length), + ); + final placeholders = List.filled(chunk.length, '?').join(', '); + final matchedRows = await db.rawQuery(''' + SELECT playlist_id, COUNT(*) AS matched_count + FROM $_tablePlaylistTracks + WHERE track_key IN ($placeholders) + GROUP BY playlist_id + ''', chunk); + for (final row in matchedRows) { + final playlistId = row['playlist_id']?.toString(); + if (playlistId == null || playlistId.isEmpty) continue; + matchedCountsByPlaylistId[playlistId] = + (matchedCountsByPlaylistId[playlistId] ?? 0) + + ((row['matched_count'] as num?)?.toInt() ?? 0); + } + } + + return playlistRows + .map((row) { + final id = row['id']?.toString() ?? ''; + final createdAt = + DateTime.tryParse(row['created_at']?.toString() ?? '') ?? + DateTime.now(); + final updatedAt = + DateTime.tryParse(row['updated_at']?.toString() ?? '') ?? createdAt; + String? previewCover; + final previewJson = row['preview_track_json'] as String?; + if (previewJson != null && previewJson.isNotEmpty) { + try { + final decoded = jsonDecode(previewJson); + if (decoded is Map) { + final cover = decoded['coverUrl']?.toString(); + if (cover != null && cover.isNotEmpty) previewCover = cover; + } + } catch (_) {} + } + return PlaylistPickerSummaryRow( + id: id, + name: row['name']?.toString() ?? '', + coverImagePath: row['cover_image_path'] as String?, + previewCover: previewCover, + createdAt: createdAt, + updatedAt: updatedAt, + trackCount: (row['track_count'] as num?)?.toInt() ?? 0, + containsAllRequestedTracks: + uniqueTrackKeys.isNotEmpty && + matchedCountsByPlaylistId[id] == uniqueTrackKeys.length, + ); + }) + .toList(growable: false); +} diff --git a/test/playlist_picker_database_test.dart b/test/playlist_picker_database_test.dart new file mode 100644 index 00000000..94a8007b --- /dev/null +++ b/test/playlist_picker_database_test.dart @@ -0,0 +1,118 @@ +import 'dart:convert'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:sqflite/sqflite.dart'; +import 'package:spotiflac_android/services/library_collections_database.dart'; + +// Exercises the database boundary's bounded requests and aggregation without +// requiring a platform SQLite plugin. The production SQL is passed unchanged. +class _PickerDatabase implements DatabaseExecutor { + final List> playlistRows; + final Map> membership; + final batches = >[]; + int summaryQueries = 0; + + _PickerDatabase(this.playlistRows, this.membership); + + @override + Future>> rawQuery( + String sql, [ + List? arguments, + ]) async { + if (arguments == null) { + summaryQueries++; + expect(sql, contains('preview_track_json')); + return playlistRows; + } + // Emulate a database with a conservative parameter budget. Regressing to + // one unbounded query must fail even on hosts with a larger SQLite limit. + if (arguments.length > 500) throw StateError('too many SQL variables'); + expect('?'.allMatches(sql).length, arguments.length); + batches.add(List.of(arguments)); + return [ + for (final entry in membership.entries) + { + 'playlist_id': entry.key, + 'matched_count': arguments.where(entry.value.contains).length, + }, + ]; + } + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +Map _playlist(String id, {String? cover, String? preview}) => { + 'id': id, + 'name': id, + 'cover_image_path': cover, + 'preview_track_json': preview, + 'created_at': '2026-01-01T00:00:00Z', + 'updated_at': '2026-01-02T00:00:00Z', + 'track_count': 1205, +}; + +void main() { + test( + 'large selections aggregate across batches and deduplicate keys', + () async { + final keys = List.generate(1205, (i) => 'track-$i'); + final db = _PickerDatabase( + [ + _playlist('partial'), + _playlist( + 'full', + preview: jsonEncode({'coverUrl': 'https://example.test/cover'}), + ), + _playlist('empty'), + ], + {'full': keys.toSet(), 'partial': keys.take(1204).toSet(), 'empty': {}}, + ); + final rows = await readPlaylistPickerSummaries(db, [ + ...keys, + ...keys.take(10), + '', + ' ', + ]); + expect(db.batches.map((batch) => batch.length), [500, 500, 205]); + expect(db.batches.expand((batch) => batch).toList(), keys); + expect(db.summaryQueries, 1); + expect(rows.map((row) => row.id), ['partial', 'full', 'empty']); + expect(rows.map((row) => row.containsAllRequestedTracks), [ + false, + true, + false, + ]); + expect(rows[1].previewCover, 'https://example.test/cover'); + expect(rows[1].trackCount, 1205); + }, + ); + + test( + 'empty selection performs no matching query or implicit all-match', + () async { + final db = _PickerDatabase([_playlist('playlist')], {}); + final rows = await readPlaylistPickerSummaries(db, ['', ' ']); + expect(db.batches, isEmpty); + expect(rows.single.containsAllRequestedTracks, isFalse); + }, + ); + + test( + 'many playlist previews need no parameter list and tolerate bad JSON', + () async { + final db = _PickerDatabase([ + for (var i = 0; i < 1200; i++) _playlist('$i'), + _playlist('custom', cover: '/local/cover.jpg'), + _playlist('bad', preview: '{broken'), + _playlist('blank', preview: '{"coverUrl":""}'), + ], {}); + final rows = await readPlaylistPickerSummaries(db, []); + expect(rows, hasLength(1203)); + expect(db.summaryQueries, 1); + expect(db.batches, isEmpty); + expect(rows[1200].coverImagePath, '/local/cover.jpg'); + expect(rows.map((row) => row.previewCover), everyElement(isNull)); + }, + ); +}