From 6f574de0b9625e67df79b2113e2126ea6e243250 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:18:33 +0200 Subject: [PATCH] fix: keep small contact photos on the device and on the server #34 ContactsProvider keeps a photo within its 96 px thumbnail as the thumbnail alone, without a PHOTO_FILE_ID, and the thumbnail does not read back. Such a Photo row looked empty to the planner. Every download of the card inserted the photo again (two, then three rows), and any edit of the contact on the device deleted the photo on the server. A Photo row now always counts as the contact's photo. Photo rows that earlier downloads added again are deleted by the contact's next clean write, one row is kept, and nothing reaches the server for them. A photo set on the device is still uploaded. Checked with planner tests and end to end through the engine, with a provider that stores small photos as thumbnails as the device does. --- src/device-sync/__tests__/contacts/harness.ts | 44 ++++++++++++- .../__tests__/contacts/lifecycle.test.ts | 61 +++++++++++++++++++ .../__tests__/contacts/real-engine.test.ts | 26 +++++++- src/device-sync/contacts/project.ts | 9 ++- 4 files changed, 134 insertions(+), 6 deletions(-) diff --git a/src/device-sync/__tests__/contacts/harness.ts b/src/device-sync/__tests__/contacts/harness.ts index 663dfbd0..27f1a243 100644 --- a/src/device-sync/__tests__/contacts/harness.ts +++ b/src/device-sync/__tests__/contacts/harness.ts @@ -4,7 +4,7 @@ * way the engine applies them (one op group per batch); uploads go through * the fake server's patch handling and come back through its normalisation. */ -import { GroupMembership, Groups, RawContacts } from '../../android-columns'; +import { Data, GroupMembership, Groups, RawContacts } from '../../android-columns'; import { makeKeyMinter, parseObjectRef, uuidFrom } from '../../common/ids'; import type { GroupRights } from '../../contacts/members'; import { contactsPlanner } from '../../contacts/planner'; @@ -17,7 +17,7 @@ import type { UploadAction, UploadPlan, } from '../../planner'; -import { CONTACTS_AUTHORITY, type BatchResult, type ProviderPort, type Row } from '../../types'; +import { CONTACTS_AUTHORITY, type BatchResult, type OpResult, type ProviderOp, type ProviderPort, type Row } from '../../types'; import { FakeJmapServer } from '../fakes/fake-jmap-server'; import { FakeDeviceProviders } from '../fakes/fake-provider'; @@ -35,6 +35,40 @@ function rowsOf(res: { columns: string[]; rows: unknown[][] }): Row[] { return res.rows.map((cells) => Object.fromEntries(res.columns.map((c, i) => [c, cells[i]])) as Row); } +/** + * ContactsProvider keeps a photo no larger than its 96 px thumbnail as the + * thumbnail only: PHOTO_FILE_ID (DATA14) stays NULL, and the thumbnail + * (DATA15) does not read back. The fake stores every photo with a display + * photo; this makes the photo rows a batch wrote thumbnail-only, as a device + * does with the test fixtures' 1×1 JPEGs. + */ +export function keepThumbnailsOnly(device: FakeDeviceProviders, ops: ProviderOp[], results: OpResult[]): void { + ops.forEach((op, i) => { + if ((op.op !== 'insert' && op.op !== 'update') || op.table !== 'data') return; + const bytes = op.values[Data.DATA15]; + if (!bytes || typeof bytes !== 'object') return; + const id = op.op === 'insert' ? results[i]?.id : op.id; + const row = id === undefined ? undefined : device.table('data').get(id); + if (row) row[Data.DATA14] = null; + }); +} + +/** A provider port whose photo writes come out thumbnail-only (see `keepThumbnailsOnly`). */ +export function thumbnailOnlyPort(device: FakeDeviceProviders, port: ProviderPort): ProviderPort { + return { + accountName: port.accountName, + authority: port.authority, + query: (q) => port.query(q), + readSyncState: () => port.readSyncState(), + readPhoto: (id, px) => port.readPhoto(id, px), + applyBatch: async (ops) => { + const res = await port.applyBatch(ops); + if (res.ok) keepThumbnailsOnly(device, ops, res.results); + return res; + }, + }; +} + export class Harness { readonly device = new FakeDeviceProviders(); readonly port: ProviderPort = this.device.port(ACCOUNT, CONTACTS_AUTHORITY); @@ -49,6 +83,8 @@ export class Harness { readonly unselected = new Set(); /** When false the context has no `groupsOf` (memberships not synced). */ groupIndex = true; + /** When true the photos the planner writes come out thumbnail-only (see `keepThumbnailsOnly`). */ + thumbnailPhotos = false; readonly ctx: ContactsContext & GroupRights; constructor() { @@ -98,7 +134,9 @@ export class Harness { async apply(group: OpGroup): Promise { if (!group.ops.length) return { ok: true, results: [] }; - return this.port.applyBatch(group.ops); + const res = await this.port.applyBatch(group.ops); + if (res.ok && this.thumbnailPhotos) keepThumbnailsOnly(this.device, group.ops, res.results); + return res; } async applyOk(group: OpGroup): Promise { diff --git a/src/device-sync/__tests__/contacts/lifecycle.test.ts b/src/device-sync/__tests__/contacts/lifecycle.test.ts index 9e69cd74..8d66eff4 100644 --- a/src/device-sync/__tests__/contacts/lifecycle.test.ts +++ b/src/device-sync/__tests__/contacts/lifecycle.test.ts @@ -181,6 +181,67 @@ describe('contacts lifecycle', () => { }); }); + describe('a small photo the provider keeps as its thumbnail only (no PHOTO_FILE_ID)', () => { + const photo = { ph: { kind: 'photo', uri: `data:image/jpeg;base64,${JPEG}`, mediaType: 'image/jpeg' } }; + const small = () => ({ name: { full: 'Photo Smalltest' }, emails: { e1: { address: 'small@example.org' } }, notes: { n1: { note: 'v1' } }, media: photo }); + const photos = (h: Harness, rawId: number) => h.device.rows('data', 'raw_contact_id = ? AND mimetype = ?', [rawId, MimeType.PHOTO]); + + it('keeps one row through downloads and merges, and never reads it as deleted', async () => { + const h = new Harness(); + h.thumbnailPhotos = true; + const { id, rawId } = await h.seed(small()); + expect(photos(h, rawId)).toMatchObject([{ data14: null, data_sync1: 'media:ph' }]); + await expectSettled(h, id, rawId); + for (const note of ['v2', 'v3']) { + h.server.serverUpdate('ContactCard', JMAP, id, { 'notes/n1/note': note }); + await h.download(id); + expect(photos(h, rawId)).toHaveLength(1); + } + // A merge into an edited contact. + h.device.user.updateData(h.rowsByKey(rawId)['emails:e1']._id as number, { data1: 'small2@example.org' }); + h.server.serverUpdate('ContactCard', JMAP, id, { 'notes/n1/note': 'v4' }); + await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops); + expect(photos(h, rawId)).toHaveLength(1); + // An edit of another field uploads alone. + expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toEqual({ + kind: 'upload', actions: [{ kind: 'update', id, patch: { 'emails/e1/address': 'small2@example.org' } }], + }); + await h.upload(rawId); + expect(h.card(id).media).toEqual(photo); + expect(photos(h, rawId)).toHaveLength(1); + await expectSettled(h, id, rawId); + // A photo set on the device (it gets a display photo) replaces it. + h.device.user.setPhoto(rawId, JPEG2); + h.devicePhotos.set(rawId, JPEG2); + expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toEqual({ + kind: 'upload', actions: [{ kind: 'update', id, patch: { 'media/ph/uri': `data:image/jpeg;base64,${JPEG2}`, 'media/ph/mediaType': 'image/jpeg' } }], + }); + }); + + it('heals the photo rows the same photo was inserted as again, keeping one and the server photo', async () => { + const h = new Harness(); + h.thumbnailPhotos = true; + const { id, rawId } = await h.seed(small()); + const [row] = photos(h, rawId); + const again = { [Data.MIMETYPE]: MimeType.PHOTO, [Data.DATA15]: { b64: JPEG }, data_sync1: row.data_sync1, data_sync2: row.data_sync2, data_sync3: row.data_sync3 }; + for (let i = 0; i < 2; i++) await h.applyOk({ ref: 'again', ops: [{ op: 'insert', table: 'data', values: { ...again, [Data.RAW_CONTACT_ID]: rawId } }] }); + expect(photos(h, rawId)).toHaveLength(3); + // An edit on the device uploads without touching the photo, and its accepted write keeps one row. + h.device.user.updateData(h.rowsByKey(rawId)['notes:n1']._id as number, { data1: 'v2 edited on device' }); + expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toEqual({ + kind: 'upload', actions: [{ kind: 'update', id, patch: { 'notes/n1/note': 'v2 edited on device' } }], + }); + await h.upload(rawId); + expect(photos(h, rawId)).toHaveLength(1); + expect(h.card(id).media).toEqual(photo); + await expectSettled(h, id, rawId); + // An unchanged card downloaded again (a full reconcile) heals too. + for (let i = 0; i < 2; i++) await h.applyOk({ ref: 'again', ops: [{ op: 'insert', table: 'data', values: { ...again, [Data.RAW_CONTACT_ID]: rawId } }] }); + await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops); + expect(photos(h, rawId)).toHaveLength(1); + }); + }); + it('heals a baseline the provider stored differently from the prediction', async () => { const h = new Harness(); const { id, rawId } = await h.seed({ name: { full: 'Dr. Jane Q. Public' } }); diff --git a/src/device-sync/__tests__/contacts/real-engine.test.ts b/src/device-sync/__tests__/contacts/real-engine.test.ts index f6a160a8..df779301 100644 --- a/src/device-sync/__tests__/contacts/real-engine.test.ts +++ b/src/device-sync/__tests__/contacts/real-engine.test.ts @@ -3,11 +3,13 @@ // themselves are tested in the other files of this folder. import { describe, expect, it } from 'vitest'; -import { Groups, MimeType } from '../../android-columns'; +import { Data, Groups, MimeType } from '../../android-columns'; import { calendarPlanner } from '../../calendar/planner'; import { contactsPlanner } from '../../contacts/planner'; import { addServerCards, CONTACTS_AUTHORITY, createHarness, rowWrites, type Harness } from '../engine/harness'; import type { SetTarget } from '../fakes/fake-jmap-server'; +import { JPEG } from './fixtures'; +import { thumbnailOnlyPort } from './harness'; function real(options: Parameters[0] = {}): Harness { const h = createHarness(options); @@ -78,6 +80,28 @@ describe('contacts through the engine', () => { expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 0 }); }); + it('keeps one row for a small photo the provider stores as its thumbnail, and never deletes it on the server', async () => { + const h = real(); + const base = h.deps.provider; + h.deps.provider = (name, authority) => thumbnailOnlyPort(h.device, base(name, authority)); + const media = { ph: { kind: 'photo', uri: `data:image/jpeg;base64,${JPEG}`, mediaType: 'image/jpeg' } }; + const card = h.server.addCard('a', { uid: 'u-small', name: { full: 'Photo Smalltest' }, notes: { n1: { note: 'v1' } }, addressBookIds: { [h.book]: true }, media }); + expect((await h.run()).outcome).toBe('ok'); + const id = h.contactNamed('Photo Smalltest')!.id; + const rowsOf = (mimetype: string) => h.device.rows('data').filter((d) => Number(d.raw_contact_id) === id && d.mimetype === mimetype); + expect(rowsOf(MimeType.PHOTO)).toMatchObject([{ data14: null }]); + + for (const note of ['v2', 'v3']) { + h.server.serverUpdate('ContactCard', 'a', card, { 'notes/n1/note': note }); + expect((await h.run()).outcome).toBe('ok'); + expect(rowsOf(MimeType.PHOTO)).toHaveLength(1); + } + h.device.user.updateData(Number(rowsOf(MimeType.NOTE)[0]._id), { [Data.DATA1]: 'v4 edited on device' }); + expect(await h.run()).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { updated: 1 } } }); + expect(h.server.get('ContactCard', 'a', card)).toMatchObject({ notes: { n1: { note: 'v4 edited on device' } }, media }); + expect(rowsOf(MimeType.PHOTO)).toHaveLength(1); + }); + it('syncs a contact whose photo is too large for a provider batch, without the photo', async () => { const h = real(); // About 900 KB of JPEG as a data: URI (a camera photo set over CardDAV). diff --git a/src/device-sync/contacts/project.ts b/src/device-sync/contacts/project.ts index b548fd13..1ff0a655 100644 --- a/src/device-sync/contacts/project.ts +++ b/src/device-sync/contacts/project.ts @@ -318,11 +318,16 @@ export function rowMatchesUnit(u: Pick, stored: Row, return spec.columns.every((c) => same(u.cells[c], stored[c], c)); } -/** Whether a row has nothing in it (an editor's blank nickname or note). */ +/** + * Whether a row has nothing in it (an editor's blank nickname or note). A + * photo row always holds a photo: ContactsProvider keeps one within its 96 px + * thumbnail as the thumbnail alone, without PHOTO_FILE_ID, and the thumbnail + * (DATA15, a blob) does not read back. + */ export function isEmptyRow(mimetype: string, cells: Row): boolean { const spec = SPECS[mimetype]; if (!spec) return true; - if (mimetype === MimeType.PHOTO) return cellText(cells[Data.DATA14]) === null; + if (mimetype === MimeType.PHOTO) return false; if (mimetype === MimeType.GROUP_MEMBERSHIP) return cellText(cells[GroupMembership.GROUP_SOURCE_ID]) === null && cellText(cells[Data.DATA1]) === null; return spec.valueColumns.every((c) => text(cells[c]) === null); }