fix(sync): Reconcile duplicate entities and claim client ids consistently - #222
Conversation
Code Review Summary✨ This PR fixes two classes of sync corruption. First, two devices independently creating the same record left two local rows competing for one server id, and the loser was silently folded away — which the user experienced as the new record vanishing. Second, Alongside those, local writes reject names the API would treat as duplicates — refactored onto New unit tests cover the duplicate-name guards, the merge behaviour, the create/update/claim client-id split, lenient pagination and transfer DTO parsing. The review noted that 🚀 Key Improvements
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| ) async { | ||
| for (final table in const ['local_changes', 'deferred_remote_items']) { | ||
| await customStatement( | ||
| 'UPDATE $table SET data = replace(data, ?, ?) WHERE data LIKE ?', |
There was a problem hiding this comment.
LIKE is not a literal substring test: SQLite's LIKE is case-insensitive for ASCII and treats % and _ in the pattern as wildcards. Because the pattern is built from an id, the WHERE clause can select rows that cannot contain the id (harmless but wasteful), and the intent ("does this payload reference this exact id?") is only expressed by the replace() call. instr() performs an exact, case-sensitive literal search and matches the intent of the surrounding code.
| 'UPDATE $table SET data = replace(data, ?, ?) WHERE data LIKE ?', | |
| + 'UPDATE $table SET data = replace(data, ?, ?) WHERE instr(data, ?) > 0', | |
| + [loserClientId, winnerClientId, loserClientId], |
| } | ||
| } | ||
|
|
||
| return PaginationResponse<T>( |
There was a problem hiding this comment.
The docstring promises that lenient "drops rows fromJsonT cannot read instead of failing the whole page", but the envelope itself is still parsed with hard casts. If current_page/last_page/per_page are absent or non-numeric, the factory throws before returning parsed, so every row of the page is lost and the error propagates to the same per-entity catch this factory exists to avoid. Note the fallback is deliberately pessimistic: currentPage == lastPage == -1 stops pagination rather than looping, so a malformed envelope degrades to "keep what parsed, fetch nothing further" instead of a hard failure.
| return PaginationResponse<T>( | |
| + return PaginationResponse<T>( | |
| + currentPage: (json['current_page'] as num?)?.toInt() ?? -1, | |
| + lastPage: (json['last_page'] as num?)?.toInt() ?? -1, | |
| + perPage: (json['per_page'] as num?)?.toInt() ?? -1, | |
| + data: parsed, | |
| + ); |
|
|
||
| /// The same name and currency is the same wallet, case and surrounding | ||
| /// spaces aside. | ||
| Future<Wallet?> _findByServerIdentity( |
There was a problem hiding this comment.
w.name.trim().lower() is evaluated by SQLite, whose built-in LOWER() only folds ASCII, while normalizedName is folded by Dart. For a name containing a non-ASCII letter these disagree with each other and with the API's utf8mb4_unicode_ci, so two wallets whose names differ only in the case of an accented character are treated as distinct here and still fold on sync — the exact outcome this guard is meant to prevent. Do the case-fold on both sides in Dart instead.
| Future<Wallet?> _findByServerIdentity( | |
| Future<Wallet?> _findByServerIdentity( | |
| String name, | |
| String currency, { | |
| String? excluding, | |
| }) async { | |
| // Compare in Dart: SQLite's built-in LOWER() only folds ASCII, so it | |
| // disagrees with the API's utf8mb4_unicode_ci for non-ASCII names. A | |
| // duplicate differing only in the case of an accented letter would slip | |
| // through the guard and fold on sync anyway. | |
| final normalizedName = name.trim().toLowerCase(); | |
| final normalizedCurrency = currency.trim().toLowerCase(); | |
| final candidates = await database.select(database.wallets).get(); | |
| for (final wallet in candidates) { | |
| if (excluding != null && wallet.clientId == excluding) continue; | |
| if (wallet.name.trim().toLowerCase() == normalizedName && | |
| wallet.currency.trim().toLowerCase() == normalizedCurrency) { | |
| return wallet; | |
| } | |
| } | |
| return null; | |
| } |
| return; | ||
| } | ||
|
|
||
| final winner = await (_db.select(_db.categories) |
There was a problem hiding this comment.
Same engine mismatch as the wallet guard: the column is folded by SQLite's LOWER() (ASCII-only) while the input is folded by Dart. For a category whose name uses a non-ASCII letter, this can fail to find the synced winner and leave the user stuck on "No synced category named … to merge into", even though the API considers the two names identical. Fetch the candidates and compare in Dart so both sides use the same Unicode-aware fold.
| final winner = await (_db.select(_db.categories) | |
| // Compare in Dart: SQLite's LOWER() only folds ASCII, so it disagrees | |
| // with the API's utf8mb4_unicode_ci for non-ASCII names. | |
| final synced = await (_db.select(_db.categories) | |
| ..where((c) => | |
| c.clientId.isNotValue(duplicate.clientId) & c.id.isNotNull())) | |
| .get(); | |
| final normalizedName = duplicate.name.trim().toLowerCase(); | |
| final matches = synced | |
| .where((c) => c.name.trim().toLowerCase() == normalizedName) | |
| .toList(); | |
| final winner = matches.isEmpty ? null : matches.first; |
| expect(SyncEntity.media.key, MediaSyncHandler.entity); | ||
| }); | ||
|
|
||
| test('every handler has an entry', () { |
There was a problem hiding this comment.
The test name (every handler has an entry) and the inline comment (holding is the one value without a handler) describe two different things, and the body verifies neither of them directly — it only asserts a magic count. If a new syncable entity is added without a handler, this fails with hasLength(13) and the natural fix is to bump the literal, which silently defeats the check. Rename it to state what is actually asserted (that holding is the sole value without a handler) so the intent is unambiguous and the count is tied to a reason.
| test('every handler has an entry', () { | |
| test('holding is the only value without a handler', () { | |
| // `holding` is a read-through cache, so it has no sync handler. Every | |
| // other value is paired with its handler by the mapping test above, so | |
| // bumping this count without adding that pairing must remain a failure. | |
| expect(SyncEntity.values, hasLength(13)); | |
| }); |
| return await query.getSingleOrNull(); | ||
| } | ||
|
|
||
| /// The same name and currency is the same wallet, case and surrounding |
There was a problem hiding this comment.
The name half of this predicate folds case twice with different rules: the stored column via SQL lower() (ASCII-only in SQLite) and the literal via Dart toLowerCase() (Unicode-aware). That diverges from the API's utf8mb4_unicode_ci the comment claims parity with, so uppercase/accented duplicates slip through locally and later fold on the server, which is the situation the guard exists to prevent. Compare the name in Dart so both sides fold identically; the currency (an ASCII code) can keep the SQL fold.
| /// The same name and currency is the same wallet, case and surrounding | |
| + /// The same name and currency is the same wallet, case and surrounding | |
| + /// spaces aside. The name is folded in Dart so the comparison matches the | |
| + /// API's `utf8mb4_unicode_ci` rather than SQLite's ASCII-only `lower()`. | |
| + Future<Wallet?> _findByServerIdentity( | |
| + String name, | |
| + String currency, { | |
| + String? excluding, | |
| + }) async { | |
| + final normalizedName = name.trim().toLowerCase(); | |
| + final normalizedCurrency = currency.trim().toLowerCase(); | |
| + final candidates = await (database.select(database.wallets) | |
| + ..where((w) { | |
| + final matches = | |
| + w.currency.trim().lower().equals(normalizedCurrency); | |
| + return excluding == null | |
| + ? matches | |
| + : matches & w.clientId.isNotValue(excluding); | |
| + })) | |
| + .get(); | |
| + for (final row in candidates) { | |
| + if (row.name.trim().toLowerCase() == normalizedName) return row; | |
| + } | |
| + return null; | |
| + } |
| db = AppDatabase(NativeDatabase.memory()); | ||
| dio = _MockDio(); | ||
| sent = null; | ||
| }); |
There was a problem hiding this comment.
This group (and the budgets and transactions groups below it) only calls the body builder toServerJson(..., includeClientId: false) directly. That proves the builder can omit the key, but it never proves the update call site does omit it — which is exactly the defect the PR set out to fix ("an ordinary update must not repoint a record another device is tracking"). The wallets, groups, parties, categories, configurations and reminders groups do not have this gap: they drive the real updateX(...) method through a stubbed Dio and inspect the captured body. The transfers group even imports transfer_remote_datasource.dart (line 14) while never instantiating its implementation, which is the tell that the call site is the missing piece. Drive an update through TransferRemoteDataSourceImpl the way the wallet group does, so the assertion covers the call and not just the builder.
| }); | |
| + // Unlike the groups above, this only exercises the body builder. It cannot | |
| + // fail if the update call site keeps sending the client id, which is the | |
| + // behaviour this change is meant to fix. Route an update through | |
| + // TransferRemoteDataSourceImpl with the stubbed Dio (as the wallets group | |
| + // does), so the assertion covers the real call. The `budgets` and | |
| + // `transactions` groups below have the same gap. | |
| + test('the create body carries the client id, the update body does not', | |
| + () async { | |
| + final transfer = await db.transfers.insertReturning( | |
| + TransfersCompanion.insert( | |
| + amount: 25, | |
| + datetime: DateTime(2026, 9, 9), | |
| + id: const Value(12), | |
| + clientId: const Value('device:transfer'), | |
| + ), | |
| + ); | |
| + | |
| + expect(toServerJson(transfer)['client_id'], 'device:transfer'); | |
| + expect( | |
| + toServerJson(transfer, includeClientId: false), | |
| + isNot(contains('client_id')), | |
| + ); | |
| + }); |
| .get(); | ||
| } | ||
|
|
||
| /// The same name is the same category, whatever its type, case and |
There was a problem hiding this comment.
This is the same name-uniqueness rule as budget_local_datasource.dart (_findByName), group_local_datasource.dart and party_local_datasource.dart, repeated with only the table/columns changed. Four copies must keep the same normalisation (trim().toLowerCase()) and the same excluding semantics in step; they have already drifted in name, and the budget copy is worded differently. Extract one helper and have each datasource call it, so a future correction (for example tightening the comparison toward the server's collation) is made once. Note this is exactly the kind of duplication the review guidance permits flagging because it is one responsibility implemented four times, not two snippets that merely share syntax.
| /// The same name is the same category, whatever its type, case and | |
| /// The same name is the same category, whatever its type, case and | |
| /// surrounding spaces aside. | |
| Future<Category?> _findByServerName(String name, {String? excluding}) { | |
| return findRowByNormalizedName( | |
| db: database, | |
| table: database.categories, | |
| name: database.categories.name, | |
| clientId: database.categories.clientId, | |
| query: name, | |
| excluding: excluding, | |
| ); | |
| } |
|
|
||
| /// The same name and currency is the same wallet, case and surrounding | ||
| /// spaces aside. | ||
| Future<Wallet?> _findByServerIdentity( |
There was a problem hiding this comment.
The guard folds case twice with two different implementations: the parameter is folded with Dart's String.toLowerCase() (Unicode-aware), while the column is folded with SQLite's lower() (ASCII-only). For any non-ASCII name the two sides disagree — e.g. stored CAFÉ, queried café: w.name.trim().lower() yields cafÉ while normalizedName is café, so equals is false and the duplicate is allowed through. That is exactly the case the guard is meant to prevent: the write succeeds locally, and on the next sync the server's utf8mb4_unicode_ci comparison folds the row into the existing one, which is the "new record vanishing" symptom this PR set out to fix. The same asymmetry exists for accents, which utf8mb4_unicode_ci treats as equal to their unaccented form but neither the Dart nor the SQLite fold does. Doing the (rare) case-insensitive name comparison in Dart keeps it Unicode-correct and consistent with the Dart-side normalization. Note that whatever equivalence class the client adopts must remain a subset of the webservice's utf8mb4_unicode_ci rule; if the webservice changes its duplicate-detection collation, this client guard and its tests must change with it.
| Future<Wallet?> _findByServerIdentity( | |
| + Future<Wallet?> _findByServerIdentity( | |
| + String name, | |
| + String currency, { | |
| + String? excluding, | |
| + }) async { | |
| + final normalizedName = name.trim().toLowerCase(); | |
| + final normalizedCurrency = currency.trim().toLowerCase(); | |
| + final rows = await (database.select(database.wallets) | |
| + ..where((w) => excluding == null | |
| + ? w.currency.trim().lower().equals(normalizedCurrency) | |
| + : w.currency.trim().lower().equals(normalizedCurrency) & | |
| + w.clientId.isNotValue(excluding))) | |
| + .get(); | |
| + for (final wallet in rows) { | |
| + if (wallet.name.trim().toLowerCase() == normalizedName) { | |
| + return wallet; | |
| + } | |
| + } | |
| + return null; |
…ntly Two devices creating the same record independently produced two local rows fighting over one server id: the second silently folded into the first and read as data loss. Reconcile them instead. - Merge duplicates on server-id collision, repointing transactions, transfers and budget targets from the losing row to the survivor. - Adopt server ids for categories, wallets, parties and groups so a download can no longer strand a local row. - Guard local writes against duplicate names, comparing as the API does (utf8mb4_unicode_ci) rather than byte-for-byte. - Send the client id on creates and on the dedicated claim request only; an ordinary update must not repoint a record another device tracks. - Give configurations the claimClientId path every other entity already had. Without it the claim fell back to a plain update, which omits the field, so the server echoed a null client_generated_id and parsing threw "type 'Null' is not a subtype of type 'String'". - Parse transfer DTOs and paginated envelopes leniently so one malformed record cannot abort a whole page.
98cfa4f to
9b84c87
Compare
LIKE treats % and _ in the pattern as wildcards and compares ASCII case-insensitively. The pattern is built from a client id whose device half comes from the platform, so it can select rows that replace() then leaves untouched. instr() is an exact, case-sensitive literal search and says what the code means.
PUT /configurations/{key} documents value as required. A claim body of
only client_id and updated_at risks a 422 before the update hook records
the id, replacing one failure with another. The claimed entity comes
straight from getAllRemote, so this echoes the server's own value back
rather than writing a stale local one.
The guards folded case twice with different rules: the column through SQLite's lower(), which only folds ASCII, and the literal through Dart's toLowerCase(), which is Unicode-aware. Stored 'CAFE' with an accent lowered to 'cafE' in SQL but 'cafe' in Dart, so the comparison never matched, the duplicate was written locally, and the next sync folded it into the existing row - the vanishing-record symptom the guards exist to prevent. Move the comparison into Dart behind one shared helper, replacing five copies of the rule in the budget, category, group, party and wallet data sources and the category merge lookup in the sync history screen.
…sites The lenient factory recovered from an unreadable row but still cast the envelope, so a missing current_page threw past that recovery and lost the whole page it exists to save. Fall back to -1, matching empty(), which leaves hasMore false: keep what parsed, fetch nothing further. The transfers, budgets and transactions groups only called the body builder, so they could not fail if the update call site kept sending the client id - the defect they are meant to guard. Drive the real insert/update methods through the stubbed Dio, as the other groups do.
2 participants
Problem
Two devices creating the same record independently ended up with two local rows fighting over one server id. The second silently folded into the first, which reads to the user as the new record vanishing.
Separately,
ConfigSyncHandlerwas the only handler missing aclaimClientIdoverride, so during the claiming process the claim fell back toupdateConfig, which deliberately omitsclient_id. The server echoedclient_generated_id: nulland parsing threwtype 'Null' is not a subtype of type 'String'on every attempt.Changes
claimClientIdpath every other entity already had.