From 105965851586c60eecfe632246a10d6c3e232efc Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Thu, 2 Jul 2026 07:55:22 +0200 Subject: [PATCH] refactor(profiles): remove dead registry surface Delete unused registry members (ProfileConnectionRegistry.insertIfAbsent / removeAllForProfile, ProfileRegistry.reorder, ProfilesView.countFor, ConnectionRegistry.getDefault) and their orphan tests, refreshing the stale docs that named removeAllForProfile as the profile-delete cleanup path. Repair the "one default per profile" join-row invariant: the connectionId FK cascade (foreign_keys=ON) silently drops a profile's default row while its other rows survive, leaving it defaultless. Add promoteMissingDefaults, share a deterministic re-promotion helper with remove(), and re-promote in removeAllForConnection. --- lib/connection/connection_registry.dart | 12 ----- lib/database/tables.dart | 6 +-- lib/profiles/profile_connection_registry.dart | 53 +++++++++++-------- lib/profiles/profile_registry.dart | 10 ---- lib/profiles/profiles_view.dart | 5 -- test/connection/connection_registry_test.dart | 28 ++++++---- test/database/app_database_test.dart | 8 +-- .../profile_connection_registry_test.dart | 32 +++++++++-- 8 files changed, 84 insertions(+), 70 deletions(-) diff --git a/lib/connection/connection_registry.dart b/lib/connection/connection_registry.dart index 3e0c3cb0..9fcf2cf9 100644 --- a/lib/connection/connection_registry.dart +++ b/lib/connection/connection_registry.dart @@ -1,7 +1,6 @@ import 'dart:async'; import 'dart:convert'; -import 'package:collection/collection.dart'; import 'package:drift/drift.dart'; import '../database/app_database.dart'; @@ -41,17 +40,6 @@ class ConnectionRegistry { return _rowToConnection(row); } - /// Returns the user's preferred connection — either the row marked - /// [Connections.isDefault], or the only row if exactly one exists, or - /// null when no connections are stored. - Future getDefault() async { - final rows = await _db.select(_db.connections).get(); - if (rows.isEmpty) return null; - final flagged = rows.firstWhereOrNull((r) => r.isDefault); - final picked = flagged ?? (rows.length == 1 ? rows.single : null); - return picked == null ? null : _rowToConnection(picked); - } - /// Insert or replace [connection]. If this is the first stored connection /// it is automatically marked default; re-upserting an existing row keeps /// the row's current `isDefault` (so token/metadata refreshes don't clear diff --git a/lib/database/tables.dart b/lib/database/tables.dart index ca4c60e9..6bce332f 100644 --- a/lib/database/tables.dart +++ b/lib/database/tables.dart @@ -193,9 +193,9 @@ class ProfileConnections extends Table { // No FK on profile_id: Plex Home profiles are virtual (built by // Profile.virtualPlexHome from PlexHomeService's live cache, never // persisted in `profiles`), so an FK here would reject every join row - // they need. The two profile-delete sites clean up join rows manually - // via ProfileConnectionRegistry.removeAllForProfile before calling - // ProfileRegistry.remove. + // they need. Profile deletion instead cleans up join rows explicitly + // (removeAllProfileConnectionsAndCleanup in profile_connection_cleanup) + // before calling ProfileRegistry.remove. TextColumn get profileId => text()(); TextColumn get connectionId => text().references(Connections, #id, onDelete: KeyAction.cascade)(); TextColumn get userToken => text().withDefault(const Constant(''))(); diff --git a/lib/profiles/profile_connection_registry.dart b/lib/profiles/profile_connection_registry.dart index 9e5e999f..1906cac9 100644 --- a/lib/profiles/profile_connection_registry.dart +++ b/lib/profiles/profile_connection_registry.dart @@ -118,13 +118,6 @@ class ProfileConnectionRegistry { ); } - /// Insert a new join row only if `(profileId, connectionId)` doesn't - /// already exist. Used by [ProfileSyncService] to surface new Plex Home - /// users without clobbering tokens cached by prior switches. - Future insertIfAbsent(ProfileConnection pc) async { - await _db.into(_db.profileConnections).insert(await _companion(pc), mode: InsertMode.insertOrIgnore); - } - /// Cache the freshly-acquired user token (e.g. after a `/home/users/switch` /// call). Updates `tokenAcquiredAt` to now. Future recordToken(String profileId, String connectionId, String token) async { @@ -149,12 +142,30 @@ class ProfileConnectionRegistry { await (_db.delete( _db.profileConnections, )..where((t) => t.profileId.equals(profileId) & t.connectionId.equals(connectionId))).go(); - // If we just removed the default, promote the oldest remaining row. - final remaining = await (_db.select(_db.profileConnections)..where((t) => t.profileId.equals(profileId))).get(); - if (remaining.isNotEmpty && !remaining.any((r) => r.isDefault)) { - await (_db.update(_db.profileConnections) - ..where((t) => t.profileId.equals(profileId) & t.connectionId.equals(remaining.first.connectionId))) - .write(const ProfileConnectionsCompanion(isDefault: Value(true))); + await _promoteDefaultIfMissing(profileId); + } + + /// Re-promote a default for [profileId] when it has join rows but none is + /// flagged default — removing the default row would otherwise leave the + /// profile defaultless. Deterministic: picks the lowest connectionId, + /// matching [listForProfile]'s secondary ordering. + Future _promoteDefaultIfMissing(String profileId) async { + final rows = await (_db.select(_db.profileConnections)..where((t) => t.profileId.equals(profileId))).get(); + if (rows.isEmpty || rows.any((r) => r.isDefault)) return; + final pick = rows.map((r) => r.connectionId).reduce((a, b) => a.compareTo(b) <= 0 ? a : b); + await (_db.update(_db.profileConnections) + ..where((t) => t.profileId.equals(profileId) & t.connectionId.equals(pick))) + .write(const ProfileConnectionsCompanion(isDefault: Value(true))); + } + + /// Repair the "exactly one default per profile" invariant across every + /// profile. The `connectionId` FK cascade (PRAGMA foreign_keys=ON) deletes + /// join rows silently when a Connection is removed, so a profile can be left + /// with surviving rows but no default flag. + Future promoteMissingDefaults() async { + final profileIds = (await _db.select(_db.profileConnections).get()).map((r) => r.profileId).toSet(); + for (final profileId in profileIds) { + await _promoteDefaultIfMissing(profileId); } } @@ -172,16 +183,14 @@ class ProfileConnectionRegistry { } /// Remove every join row referencing [connectionId] (e.g. when a Connection - /// is deleted). Drift's referential integrity isn't enabled by default for - /// SQLite without `PRAGMA foreign_keys=ON`, so we cascade explicitly. + /// is deleted). The `connectionId` FK (PRAGMA foreign_keys=ON) already + /// cascades these rows away when the Connection row itself is deleted; this + /// stays the explicit path for callers that drop the rows first, and either + /// way repairs any profile the removal left without a default. Future removeAllForConnection(String connectionId) async { - return await (_db.delete(_db.profileConnections)..where((t) => t.connectionId.equals(connectionId))).go(); - } - - /// Wipe every join row for [profileId] (e.g. when a Plex Home profile's - /// parent connection is removed). - Future removeAllForProfile(String profileId) async { - return await (_db.delete(_db.profileConnections)..where((t) => t.profileId.equals(profileId))).go(); + final removed = await (_db.delete(_db.profileConnections)..where((t) => t.connectionId.equals(connectionId))).go(); + await promoteMissingDefaults(); + return removed; } /// Wipe the entire join table. Used by sign-out so a fresh sign-in starts diff --git a/lib/profiles/profile_registry.dart b/lib/profiles/profile_registry.dart index 74c7a428..e15fc283 100644 --- a/lib/profiles/profile_registry.dart +++ b/lib/profiles/profile_registry.dart @@ -73,16 +73,6 @@ class ProfileRegistry { return (_db.delete(_db.profiles)..where((t) => t.kind.equals(ProfileKind.plexHome.id))).go(); } - Future reorder(List idsInOrder) async { - await _db.transaction(() async { - for (var i = 0; i < idsInOrder.length; i++) { - await (_db.update( - _db.profiles, - )..where((t) => t.id.equals(idsInOrder[i]))).write(ProfilesCompanion(sortOrder: Value(i))); - } - }); - } - Future clear() async { await _db.delete(_db.profiles).go(); } diff --git a/lib/profiles/profiles_view.dart b/lib/profiles/profiles_view.dart index c5d24ab2..5aa017c7 100644 --- a/lib/profiles/profiles_view.dart +++ b/lib/profiles/profiles_view.dart @@ -28,11 +28,6 @@ class ProfilesView { const ProfilesView({required this.profiles, required this.connectionsByProfile, required this.connectionsById}); static const empty = ProfilesView(profiles: [], connectionsByProfile: {}, connectionsById: {}); - - int countFor(Profile profile) { - if (profile.isPlexHome) return profile.parentConnectionId == null ? 0 : 1; - return connectionsByProfile[profile.id]?.length ?? 0; - } } /// Join-table rows that should be shown as explicit, user-manageable diff --git a/test/connection/connection_registry_test.dart b/test/connection/connection_registry_test.dart index 315243d9..fda42bc4 100644 --- a/test/connection/connection_registry_test.dart +++ b/test/connection/connection_registry_test.dart @@ -11,6 +11,15 @@ import 'package:plezy/services/plex_auth_service.dart'; import '../test_helpers/prefs.dart'; +/// The id of the connection currently flagged default, read straight from the +/// row (the registry maintains the flag; there is no public reader). +Future _defaultConnectionId(AppDatabase db) async { + for (final row in await db.select(db.connections).get()) { + if (row.isDefault) return row.id; + } + return null; +} + JellyfinConnection _jellyfin({String id = 'srv-1', String userName = 'edde'}) { return JellyfinConnection( id: id, @@ -71,7 +80,7 @@ void main() { group('ConnectionRegistry', () { test('list() returns empty when no connections stored', () async { expect(await registry.list(), isEmpty); - expect(await registry.getDefault(), isNull); + expect(await _defaultConnectionId(db), isNull); }); test('first upserted connection becomes the default', () async { @@ -80,8 +89,7 @@ void main() { expect(list.length, 1); expect(list.first.id, 'a'); - final defaultConn = await registry.getDefault(); - expect(defaultConn?.id, 'a'); + expect(await _defaultConnectionId(db), 'a'); }); test('upsert preserves type discriminator (Plex vs Jellyfin)', () async { @@ -141,10 +149,10 @@ void main() { await registry.upsert(_jellyfin(id: 'b')); // First is default by default; explicitly switch to b. await registry.setDefault('b'); - expect((await registry.getDefault())?.id, 'b'); + expect(await _defaultConnectionId(db), 'b'); // Switch back to a. await registry.setDefault('a'); - expect((await registry.getDefault())?.id, 'a'); + expect(await _defaultConnectionId(db), 'a'); }); test('remove deletes a row and re-elects a default when needed', () async { @@ -153,10 +161,10 @@ void main() { // a is default (first one in). await registry.remove('a'); // b should now be the default. - expect((await registry.getDefault())?.id, 'b'); + expect(await _defaultConnectionId(db), 'b'); // Removing the last clears the default cleanly. await registry.remove('b'); - expect(await registry.getDefault(), isNull); + expect(await _defaultConnectionId(db), isNull); }); test('re-upsert preserves the existing default flag', () async { @@ -165,15 +173,15 @@ void main() { // wrote `isFirst` (false on update). await registry.upsert(_jellyfin(id: 'a')); await registry.upsert(_jellyfin(id: 'b')); - expect((await registry.getDefault())?.id, 'a'); + expect(await _defaultConnectionId(db), 'a'); // Re-upsert the default with refreshed credentials. await registry.upsert(_jellyfin(id: 'a', userName: 'refreshed')); - expect((await registry.getDefault())?.id, 'a'); + expect(await _defaultConnectionId(db), 'a'); // And re-upserting a non-default row doesn't accidentally promote it. await registry.upsert(_jellyfin(id: 'b', userName: 'refreshed')); - expect((await registry.getDefault())?.id, 'a'); + expect(await _defaultConnectionId(db), 'a'); }); test('recordAuthSuccess updates lastAuthenticatedAt without losing config', () async { diff --git a/test/database/app_database_test.dart b/test/database/app_database_test.dart index 6616ea2d..e01aa6ed 100644 --- a/test/database/app_database_test.dart +++ b/test/database/app_database_test.dart @@ -58,10 +58,10 @@ class _AppDatabaseTestSuite { test('ProfileConnections has no profile_id FK (virtual plex_home profiles)', () async { // v20 dropped the profile_id FK so virtual Plex Home profiles can - // persist join rows without a parent `profiles` row. The two - // profile-delete sites (profile_detail_screen, profile_switch_screen) - // call ProfileConnectionRegistry.removeAllForProfile manually before - // deleting the profile, so the cascade isn't needed. + // persist join rows without a parent `profiles` row. Profile deletion + // instead cleans up join rows explicitly (via the teardown flow's + // removeAllProfileConnectionsAndCleanup) before deleting the profile, + // so the cascade isn't needed. final now = DateTime.now().millisecondsSinceEpoch; await db .into(db.connections) diff --git a/test/profiles/profile_connection_registry_test.dart b/test/profiles/profile_connection_registry_test.dart index 43441a00..82c0e66f 100644 --- a/test/profiles/profile_connection_registry_test.dart +++ b/test/profiles/profile_connection_registry_test.dart @@ -144,16 +144,40 @@ void main() { expect(await registry.listForConnection('c1'), isEmpty); }); - test('removeAllForProfile drops every row for a profile', () async { + test('removeAllForConnection re-promotes a default for a surviving profile', () async { await registry.upsert( const ProfileConnection(profileId: 'p1', connectionId: 'c1', userToken: 't', userIdentifier: 'u'), + makeDefault: true, ); await registry.upsert( const ProfileConnection(profileId: 'p1', connectionId: 'c2', userToken: 't', userIdentifier: 'u'), ); - final removed = await registry.removeAllForProfile('p1'); - expect(removed, 2); - expect(await registry.listForProfile('p1'), isEmpty); + // c1 is default; removing it connection-wide must leave p1 with c2 as + // its new default rather than defaultless. + await registry.removeAllForConnection('c1'); + final remaining = await registry.listForProfile('p1'); + expect(remaining, hasLength(1)); + expect(remaining.single.connectionId, 'c2'); + expect(remaining.single.isDefault, isTrue); + }); + + test('promoteMissingDefaults repairs a profile the FK cascade left defaultless', () async { + await registry.upsert( + const ProfileConnection(profileId: 'p1', connectionId: 'c1', userToken: 't', userIdentifier: 'u'), + makeDefault: true, + ); + await registry.upsert( + const ProfileConnection(profileId: 'p1', connectionId: 'c2', userToken: 't', userIdentifier: 'u'), + ); + // Deleting the connection cascades away p1's default join row (c1) via + // the connectionId FK, silently leaving p1 with no default flag. + await (db.delete(db.connections)..where((t) => t.id.equals('c1'))).go(); + final beforeRepair = await registry.listForProfile('p1'); + expect(beforeRepair.single.connectionId, 'c2'); + expect(beforeRepair.single.isDefault, isFalse); + + await registry.promoteMissingDefaults(); + expect((await registry.listForProfile('p1')).single.isDefault, isTrue); }); test('upsert succeeds for a virtual plex_home profile id with no parent row', () async {