diff --git a/.secrets.baseline b/.secrets.baseline index de7e1018..08b8ca14 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -221,6 +221,15 @@ "line_number": 5 } ], + "data/local/schemas/io.theficos.ereader.data.local.db.EReaderDatabase/11.json": [ + { + "type": "Hex High Entropy String", + "filename": "data/local/schemas/io.theficos.ereader.data.local.db.EReaderDatabase/11.json", + "hashed_secret": "38835e0a4c1e854823be4e6cd0d20980141ac6d1", + "is_verified": false, + "line_number": 5 + } + ], "data/local/schemas/io.theficos.ereader.data.local.db.EReaderDatabase/2.json": [ { "type": "Hex High Entropy String", @@ -351,7 +360,7 @@ "filename": "docs/sync-api.md", "hashed_secret": "cb0cb287efd5879d5295028d6c29f1111334f3bb", "is_verified": false, - "line_number": 1080 + "line_number": 1156 } ], "server/alembic.ini": [ @@ -378,7 +387,7 @@ "filename": "server/quire_server/api/account_lifecycle.py", "hashed_secret": "5baa1b6af7d89ab9e691824782122b2c496d5475", "is_verified": false, - "line_number": 167, + "line_number": 169, "is_secret": false } ], @@ -528,7 +537,7 @@ "filename": "server/tests/integration/test_browser_identity.py", "hashed_secret": "df77a6b3e1381f280f62cc2eceea74ec575efde6", "is_verified": false, - "line_number": 25, + "line_number": 27, "is_secret": false }, { @@ -536,7 +545,7 @@ "filename": "server/tests/integration/test_browser_identity.py", "hashed_secret": "a4b48a81cdab1e1a5dd37907d6c85ca1c61ddc7c", "is_verified": false, - "line_number": 347, + "line_number": 381, "is_secret": false } ], @@ -596,5 +605,5 @@ } ] }, - "generated_at": "2026-10-30T09:54:50Z" + "generated_at": "2026-10-30T18:09:32Z" } diff --git a/README.md b/README.md index 062e19b1..fcde048f 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ credentials are included. See the [screenshot notes](docs/assets/README.md). - **Keep passages.** Enter Highlight mode to save coloured passages, change their colour or remove them. Open Highlights to browse the book's saved passages and return to their exact pages. From a text selection, Copy as quote includes - the book title and author. [Highlights stay on your device](docs/reader-highlights.md), + the book title and author. [Highlights work offline and sync with Quire Server](docs/reader-highlights.md), including when you remove a download and later import the same book again. - **Keep a useful shelf.** Search, sort, continue reading, and filter finished or abandoned books. With Quire Server, Library Stats brings together reading diff --git a/app/src/main/java/io/theficos/ereader/di/AppContainer.kt b/app/src/main/java/io/theficos/ereader/di/AppContainer.kt index ba3bcd46..b2755de3 100644 --- a/app/src/main/java/io/theficos/ereader/di/AppContainer.kt +++ b/app/src/main/java/io/theficos/ereader/di/AppContainer.kt @@ -25,6 +25,8 @@ import io.theficos.ereader.data.opds.BookDownloader import io.theficos.ereader.data.opds.OpdsClient import io.theficos.ereader.data.opds.OpdsHttpClient import io.theficos.ereader.data.opds.OpenLibraryClient +import io.theficos.ereader.data.sync.HighlightSyncer +import io.theficos.ereader.data.sync.HighlightSyncSession import io.theficos.ereader.data.sync.SyncClient import io.theficos.ereader.data.sync.SyncDependencies import io.theficos.ereader.data.sync.SyncEnqueuer @@ -94,7 +96,12 @@ class AppContainer(context: Context) { private val db: EReaderDatabase = EReaderDatabase.build(appContext) val documentRepository = DocumentRepository(db.documentDao()) val progressRepository = ProgressRepository(db.progressDao()) - val highlightRepository = HighlightRepository(db.highlightDao()) + val highlightRepository = HighlightRepository( + db.highlightDao(), + scopeProvider = { credentialStore.getAccount().highlightScope() }, + scopeFlow = credentialStore.accountFlow.map { it.highlightScope() }, + onChanged = { SyncEnqueuer.enqueue(appContext) }, + ) val syncStateDao = db.syncStateDao() val readiumFactory = ReadiumFactory(appContext) val readerPreferencesStore = ReaderPreferencesStore(appContext) @@ -117,6 +124,10 @@ class AppContainer(context: Context) { progressDao = db.progressDao(), documentRepo = documentRepository, syncState = syncStateDao, + highlights = HighlightSyncer( + syncClient, db.highlightSyncDao(), syncStateDao, + sessionProvider = { credentialStore.getAccount().highlightSession() }, + ), ) val aiClient: AiClient = AiClient( diff --git a/app/src/main/java/io/theficos/ereader/di/HighlightAccountScope.kt b/app/src/main/java/io/theficos/ereader/di/HighlightAccountScope.kt new file mode 100644 index 00000000..68806b2b --- /dev/null +++ b/app/src/main/java/io/theficos/ereader/di/HighlightAccountScope.kt @@ -0,0 +1,36 @@ +package io.theficos.ereader.di + +import io.theficos.ereader.auth.AccountCredentials +import io.theficos.ereader.data.sync.HighlightSyncSession +import java.security.MessageDigest +import java.util.Base64 +import okhttp3.HttpUrl.Companion.toHttpUrl + +/** Password/token refresh preserves ownership; server, catalog, scheme or subject changes do not. */ +internal fun AccountCredentials?.highlightScope(): String { + val account = this ?: return "" + val server = account.quireServerUrlOrNull() + val parts = listOf( + account.scheme.name, + account.baseUrl.toHttpUrl().toString().trimEnd('/'), + server?.toHttpUrl()?.toString()?.trimEnd('/').orEmpty(), + account.subject, + ) + if (account is AccountCredentials.OpdsOnly) { + listOf((account.username != null).toString(), account.username.orEmpty()) + } else emptyList() + val input = parts.joinToString("") { "${it.length}:$it" } + return MessageDigest.getInstance("SHA-256").digest(input.toByteArray(Charsets.UTF_8)) + .joinToString("") { "%02x".format(it) } +} + +internal fun AccountCredentials?.highlightSession(): HighlightSyncSession? { + val account = this ?: return null + val base = account.quireServerUrlOrNull() ?: return null + val authorization = when (account) { + is AccountCredentials.Basic -> "Basic " + Base64.getEncoder() + .encodeToString("${account.username}:${account.password}".toByteArray(Charsets.UTF_8)) + is AccountCredentials.Bearer -> "Bearer ${account.token}" + is AccountCredentials.OpdsOnly -> return null + } + return HighlightSyncSession(account.highlightScope(), base, authorization) +} diff --git a/app/src/test/java/io/theficos/ereader/di/HighlightAccountScopeTest.kt b/app/src/test/java/io/theficos/ereader/di/HighlightAccountScopeTest.kt new file mode 100644 index 00000000..de652ff8 --- /dev/null +++ b/app/src/test/java/io/theficos/ereader/di/HighlightAccountScopeTest.kt @@ -0,0 +1,45 @@ +package io.theficos.ereader.di + +import com.google.common.truth.Truth.assertThat +import io.theficos.ereader.auth.AccountCredentials +import org.junit.Test + +class HighlightAccountScopeTest { + private val basic = AccountCredentials.Basic("https://catalog.example", "Reader", "secret", "https://sync.example") + + @Test fun `scope separates subjects servers catalogs and schemes while preserving reauthentication`() { + val original = basic.highlightScope() + assertThat(original).hasLength(64) + assertThat(basic.copy(password = "new password").highlightScope()).isEqualTo(original) // pragma: allowlist secret - dummy test credential + assertThat(basic.copy(username = "reader").highlightScope()).isEqualTo(original) + assertThat(basic.copy(baseUrl = "https://catalog.example/").highlightScope()).isEqualTo(original) + assertThat(basic.copy(username = "other").highlightScope()).isNotEqualTo(original) + assertThat(basic.copy(quireServerUrl = "https://other.example").highlightScope()).isNotEqualTo(original) + assertThat(basic.copy(baseUrl = "https://other-catalog.example").highlightScope()).isNotEqualTo(original) + val bearer = AccountCredentials.Bearer("https://sync.example", "reader", "token") + assertThat(bearer.highlightScope()).isNotEqualTo(original) + assertThat(bearer.copy(token = "new token").highlightScope()).isEqualTo(bearer.highlightScope()) + } + + @Test fun `generic OPDS gets a local account scope and logged out quotes remain unowned`() { + val opds = AccountCredentials.OpdsOnly("https://catalog.example/opds?api_key=secret") + assertThat(opds.highlightScope()).hasLength(64) + assertThat(opds.highlightScope()).isNotEqualTo(basic.highlightScope()) + assertThat(opds.highlightScope()).isNotEqualTo(opds.copy(baseUrl = "https://other.example/opds").highlightScope()) + assertThat(opds.highlightSession()).isNull() + val authenticated = opds.copy(username = "reader", password = "secret") + assertThat(authenticated.highlightScope()).isNotEqualTo(opds.highlightScope()) + assertThat(authenticated.copy(username = "other").highlightScope()).isNotEqualTo(authenticated.highlightScope()) + assertThat(authenticated.copy(password = "new secret").highlightScope()).isEqualTo(authenticated.highlightScope()) // pragma: allowlist secret - dummy test credential + assertThat((null as AccountCredentials?).highlightScope()).isEmpty() + assertThat((null as AccountCredentials?).highlightSession()).isNull() + } + + @Test fun `session freezes split server destination and authorization without logging credentials`() { + val session = basic.highlightSession()!! + assertThat(session.baseUrl).isEqualTo("https://sync.example") + assertThat(session.authorization).isEqualTo("Basic UmVhZGVyOnNlY3JldA==") + assertThat(session.toString()).doesNotContain("secret") + assertThat(session.toString()).doesNotContain(session.authorization) + } +} diff --git a/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightNavigationTest.kt b/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightNavigationTest.kt index cd2c409f..3b9890d2 100644 --- a/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightNavigationTest.kt +++ b/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightNavigationTest.kt @@ -149,11 +149,11 @@ class ReaderHighlightNavigationTest { private class Rows : HighlightDao { private val rows = MutableStateFlow>(emptyList()) override suspend fun insert(highlight: HighlightEntity) { rows.value += highlight } - override suspend fun findById(id: String) = rows.value.firstOrNull { it.id == id } - override fun observeActive(metadataId: String?, contentHash: String?): Flow> = rows.map { all -> + override suspend fun findById(id: String, ownerScope: String) = rows.value.firstOrNull { it.id == id } + override fun observeActive(metadataId: String?, contentHash: String?, ownerScope: String): Flow> = rows.map { all -> all.filter { it.deletedAt == null && it.contentHash == contentHash } } - override suspend fun setColour(id: String, colour: String, now: Long) {} - override suspend fun softDelete(id: String, now: Long) {} + override suspend fun setColour(id: String, colour: String, now: Long, ownerScope: String) {} + override suspend fun softDelete(id: String, now: Long, ownerScope: String) {} } } diff --git a/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightsTest.kt b/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightsTest.kt index 8bc0c71c..5695088a 100644 --- a/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightsTest.kt +++ b/app/src/test/java/io/theficos/ereader/ui/reader/ReaderHighlightsTest.kt @@ -186,18 +186,18 @@ class ReaderHighlightsTest { if (failWrites) throw IOException("test write failure") rows.value = rows.value + highlight } - override suspend fun findById(id: String) = rows.value.firstOrNull { it.id == id } - override fun observeActive(metadataId: String?, contentHash: String?): Flow> = if (failLoads) flow { + override suspend fun findById(id: String, ownerScope: String) = rows.value.firstOrNull { it.id == id } + override fun observeActive(metadataId: String?, contentHash: String?, ownerScope: String): Flow> = if (failLoads) flow { throw IOException("test load failure") } else rows.map { all -> all.filter { it.deletedAt == null && ((metadataId != null && it.metadataId == metadataId) || (contentHash != null && it.contentHash == contentHash)) } } - override suspend fun setColour(id: String, colour: String, now: Long) { + override suspend fun setColour(id: String, colour: String, now: Long, ownerScope: String) { if (failWrites) throw IOException("test write failure") rows.value = rows.value.map { if (it.id == id) it.copy(colour = colour, updatedAt = now) else it } } - override suspend fun softDelete(id: String, now: Long) { + override suspend fun softDelete(id: String, now: Long, ownerScope: String) { if (failWrites) throw IOException("test write failure") rows.value = rows.value.map { if (it.id == id) it.copy(deletedAt = now, updatedAt = now) else it } } diff --git a/data/local/schemas/io.theficos.ereader.data.local.db.EReaderDatabase/11.json b/data/local/schemas/io.theficos.ereader.data.local.db.EReaderDatabase/11.json new file mode 100644 index 00000000..f2757e26 --- /dev/null +++ b/data/local/schemas/io.theficos.ereader.data.local.db.EReaderDatabase/11.json @@ -0,0 +1,516 @@ +{ + "formatVersion": 1, + "database": { + "version": 11, + "identityHash": "7a79cc5759348ddf4ef967208027abfe", + "entities": [ + { + "tableName": "documents", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, `metadataId` TEXT, `contentHash` TEXT NOT NULL, `title` TEXT NOT NULL, `author` TEXT, `downloadUrl` TEXT NOT NULL, `localPath` TEXT NOT NULL, `coverPath` TEXT, `downloadedAt` INTEGER NOT NULL, `seriesName` TEXT, `seriesIndex` REAL, `librarySyncedAt` INTEGER, `identityHashVersion` INTEGER NOT NULL DEFAULT 1)", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "metadataId", + "columnName": "metadataId", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "contentHash", + "columnName": "contentHash", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "title", + "columnName": "title", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "author", + "columnName": "author", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "downloadUrl", + "columnName": "downloadUrl", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "localPath", + "columnName": "localPath", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "coverPath", + "columnName": "coverPath", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "downloadedAt", + "columnName": "downloadedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "seriesName", + "columnName": "seriesName", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "seriesIndex", + "columnName": "seriesIndex", + "affinity": "REAL", + "notNull": false + }, + { + "fieldPath": "librarySyncedAt", + "columnName": "librarySyncedAt", + "affinity": "INTEGER", + "notNull": false + }, + { + "fieldPath": "identityHashVersion", + "columnName": "identityHashVersion", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + } + ], + "primaryKey": { + "autoGenerate": true, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_documents_metadataId", + "unique": true, + "columnNames": [ + "metadataId" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_documents_metadataId` ON `${TABLE_NAME}` (`metadataId`)" + }, + { + "name": "index_documents_contentHash", + "unique": true, + "columnNames": [ + "contentHash" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_documents_contentHash` ON `${TABLE_NAME}` (`contentHash`)" + }, + { + "name": "index_documents_seriesName_seriesIndex", + "unique": false, + "columnNames": [ + "seriesName", + "seriesIndex" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_documents_seriesName_seriesIndex` ON `${TABLE_NAME}` (`seriesName`, `seriesIndex`)" + } + ], + "foreignKeys": [] + }, + { + "tableName": "progress", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, `documentId` INTEGER NOT NULL, `locator` TEXT NOT NULL, `percent` REAL NOT NULL, `updatedAt` INTEGER NOT NULL, `localUpdatedAt` INTEGER NOT NULL, `syncedAt` INTEGER NOT NULL, `finishedAt` INTEGER, `abandonedAt` INTEGER, FOREIGN KEY(`documentId`) REFERENCES `documents`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "documentId", + "columnName": "documentId", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "locator", + "columnName": "locator", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "percent", + "columnName": "percent", + "affinity": "REAL", + "notNull": true + }, + { + "fieldPath": "updatedAt", + "columnName": "updatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "localUpdatedAt", + "columnName": "localUpdatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "syncedAt", + "columnName": "syncedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "finishedAt", + "columnName": "finishedAt", + "affinity": "INTEGER", + "notNull": false + }, + { + "fieldPath": "abandonedAt", + "columnName": "abandonedAt", + "affinity": "INTEGER", + "notNull": false + } + ], + "primaryKey": { + "autoGenerate": true, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_progress_documentId", + "unique": true, + "columnNames": [ + "documentId" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_progress_documentId` ON `${TABLE_NAME}` (`documentId`)" + } + ], + "foreignKeys": [ + { + "table": "documents", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "documentId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "sync_state", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`tableName` TEXT NOT NULL, `lastPulledAt` INTEGER NOT NULL, PRIMARY KEY(`tableName`))", + "fields": [ + { + "fieldPath": "tableName", + "columnName": "tableName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "lastPulledAt", + "columnName": "lastPulledAt", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "tableName" + ] + }, + "indices": [], + "foreignKeys": [] + }, + { + "tableName": "book_insights", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`identityKey` TEXT NOT NULL, `metadataId` TEXT, `contentHash` TEXT, `modelId` TEXT NOT NULL, `promptVersion` TEXT NOT NULL, `tone` TEXT NOT NULL, `language` TEXT NOT NULL, `payloadJson` TEXT NOT NULL, `sourcesJson` TEXT NOT NULL, `schemaVersion` INTEGER NOT NULL, `serverId` INTEGER NOT NULL, `generatedAt` INTEGER NOT NULL, `syncedAt` INTEGER NOT NULL, `identityHashVersion` INTEGER NOT NULL DEFAULT 1, PRIMARY KEY(`identityKey`, `modelId`, `promptVersion`, `tone`, `language`))", + "fields": [ + { + "fieldPath": "identityKey", + "columnName": "identityKey", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "metadataId", + "columnName": "metadataId", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "contentHash", + "columnName": "contentHash", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "modelId", + "columnName": "modelId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "promptVersion", + "columnName": "promptVersion", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "tone", + "columnName": "tone", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "language", + "columnName": "language", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "payloadJson", + "columnName": "payloadJson", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sourcesJson", + "columnName": "sourcesJson", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "schemaVersion", + "columnName": "schemaVersion", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "serverId", + "columnName": "serverId", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "generatedAt", + "columnName": "generatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "syncedAt", + "columnName": "syncedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "identityHashVersion", + "columnName": "identityHashVersion", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "identityKey", + "modelId", + "promptVersion", + "tone", + "language" + ] + }, + "indices": [ + { + "name": "index_book_insights_syncedAt", + "unique": false, + "columnNames": [ + "syncedAt" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_book_insights_syncedAt` ON `${TABLE_NAME}` (`syncedAt`)" + }, + { + "name": "index_book_insights_metadataId", + "unique": false, + "columnNames": [ + "metadataId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_book_insights_metadataId` ON `${TABLE_NAME}` (`metadataId`)" + }, + { + "name": "index_book_insights_contentHash", + "unique": false, + "columnNames": [ + "contentHash" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_book_insights_contentHash` ON `${TABLE_NAME}` (`contentHash`)" + }, + { + "name": "index_book_insights_cursor", + "unique": false, + "columnNames": [ + "generatedAt", + "serverId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_book_insights_cursor` ON `${TABLE_NAME}` (`generatedAt`, `serverId`)" + } + ], + "foreignKeys": [] + }, + { + "tableName": "highlights", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `metadataId` TEXT, `contentHash` TEXT NOT NULL, `identityHashVersion` INTEGER NOT NULL DEFAULT 1, `locator` TEXT NOT NULL, `text` TEXT NOT NULL, `colour` TEXT NOT NULL, `bookTitle` TEXT NOT NULL, `bookAuthor` TEXT, `createdAt` INTEGER NOT NULL, `updatedAt` INTEGER NOT NULL, `deletedAt` INTEGER, `ownerScope` TEXT NOT NULL DEFAULT '', `syncedAt` INTEGER DEFAULT NULL, PRIMARY KEY(`ownerScope`, `id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "metadataId", + "columnName": "metadataId", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "contentHash", + "columnName": "contentHash", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "identityHashVersion", + "columnName": "identityHashVersion", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + }, + { + "fieldPath": "locator", + "columnName": "locator", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "text", + "columnName": "text", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "colour", + "columnName": "colour", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bookTitle", + "columnName": "bookTitle", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bookAuthor", + "columnName": "bookAuthor", + "affinity": "TEXT", + "notNull": false + }, + { + "fieldPath": "createdAt", + "columnName": "createdAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "updatedAt", + "columnName": "updatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "deletedAt", + "columnName": "deletedAt", + "affinity": "INTEGER", + "notNull": false + }, + { + "fieldPath": "ownerScope", + "columnName": "ownerScope", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "syncedAt", + "columnName": "syncedAt", + "affinity": "INTEGER", + "notNull": false, + "defaultValue": "NULL" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "ownerScope", + "id" + ] + }, + "indices": [ + { + "name": "index_highlights_metadataId_deletedAt", + "unique": false, + "columnNames": [ + "metadataId", + "deletedAt" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_highlights_metadataId_deletedAt` ON `${TABLE_NAME}` (`metadataId`, `deletedAt`)" + }, + { + "name": "index_highlights_contentHash_deletedAt", + "unique": false, + "columnNames": [ + "contentHash", + "deletedAt" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_highlights_contentHash_deletedAt` ON `${TABLE_NAME}` (`contentHash`, `deletedAt`)" + } + ], + "foreignKeys": [] + } + ], + "views": [], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '7a79cc5759348ddf4ef967208027abfe')" + ] + } +} diff --git a/data/local/src/main/java/io/theficos/ereader/data/local/HighlightRepository.kt b/data/local/src/main/java/io/theficos/ereader/data/local/HighlightRepository.kt index 5329fc3c..9f543cff 100644 --- a/data/local/src/main/java/io/theficos/ereader/data/local/HighlightRepository.kt +++ b/data/local/src/main/java/io/theficos/ereader/data/local/HighlightRepository.kt @@ -10,18 +10,26 @@ import io.theficos.ereader.data.local.db.HighlightEntity import java.util.UUID import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.map class HighlightRepository( private val dao: HighlightDao, private val nowMs: () -> Long = System::currentTimeMillis, private val newId: () -> String = { UUID.randomUUID().toString() }, + private val scopeProvider: () -> String = { "" }, + private val scopeFlow: Flow = flowOf(""), + private val onChanged: () -> Unit = {}, ) { + @OptIn(ExperimentalCoroutinesApi::class) fun observe(identity: DocumentIdentity): Flow> { val metadataId = normalizeMetadataId(identity.metadataId) val contentHash = identity.contentHash?.trim()?.takeIf { it.isNotEmpty() } if (metadataId == null && contentHash == null) return flowOf(emptyList()) - return dao.observeActive(metadataId, contentHash).map { rows -> rows.map { it.toDomain() } } + return scopeFlow.flatMapLatest { scope -> + dao.observeActive(metadataId, contentHash, scope).map { rows -> rows.map { it.toDomain() } } + } } suspend fun create( @@ -30,6 +38,7 @@ class HighlightRepository( text: String, colour: HighlightColour, ): Highlight { + val scope = scopeProvider() val contentHash = requireNotNull(document.identity.contentHash?.trim()?.takeIf { it.isNotEmpty() }) { "A highlight requires the downloaded EPUB's content hash" } @@ -48,16 +57,23 @@ class HighlightRepository( bookAuthor = document.author, createdAt = now, updatedAt = now, + ownerScope = scope, ) dao.insert(row) + onChanged() return row.toDomain() } - suspend fun setColour(id: String, colour: HighlightColour) = - dao.setColour(id, colour.name, nowMs()) + suspend fun setColour(id: String, colour: HighlightColour) { + dao.setColour(id, colour.name, nowMs(), scopeProvider()) + onChanged() + } /** Keep the tombstone so a later sync can propagate the removal. */ - suspend fun delete(id: String) = dao.softDelete(id, nowMs()) + suspend fun delete(id: String) { + dao.softDelete(id, nowMs(), scopeProvider()) + onChanged() + } private fun HighlightEntity.toDomain() = Highlight( id = id, diff --git a/data/local/src/main/java/io/theficos/ereader/data/local/db/EReaderDatabase.kt b/data/local/src/main/java/io/theficos/ereader/data/local/db/EReaderDatabase.kt index a0cf8a48..3004c38c 100644 --- a/data/local/src/main/java/io/theficos/ereader/data/local/db/EReaderDatabase.kt +++ b/data/local/src/main/java/io/theficos/ereader/data/local/db/EReaderDatabase.kt @@ -15,7 +15,7 @@ import androidx.sqlite.db.SupportSQLiteDatabase InsightEntity::class, HighlightEntity::class, ], - version = 10, + version = 11, exportSchema = true, ) abstract class EReaderDatabase : RoomDatabase() { @@ -24,6 +24,7 @@ abstract class EReaderDatabase : RoomDatabase() { abstract fun syncStateDao(): SyncStateDao abstract fun insightDao(): InsightDao abstract fun highlightDao(): HighlightDao + abstract fun highlightSyncDao(): HighlightSyncDao companion object { internal val MIGRATION_1_2 = object : Migration(1, 2) { @@ -197,6 +198,28 @@ abstract class EReaderDatabase : RoomDatabase() { } } + /** Keep existing offline annotations unowned until a successful authenticated sync. */ + internal val MIGRATION_10_11 = object : Migration(10, 11) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL(""" + CREATE TABLE highlights_scoped ( + id TEXT NOT NULL, metadataId TEXT, contentHash TEXT NOT NULL, + identityHashVersion INTEGER NOT NULL DEFAULT 1, + locator TEXT NOT NULL, text TEXT NOT NULL, colour TEXT NOT NULL, + bookTitle TEXT NOT NULL, bookAuthor TEXT, createdAt INTEGER NOT NULL, + updatedAt INTEGER NOT NULL, deletedAt INTEGER, + ownerScope TEXT NOT NULL DEFAULT '', syncedAt INTEGER DEFAULT NULL, + PRIMARY KEY(ownerScope, id) + ) + """.trimIndent()) + db.execSQL("INSERT INTO highlights_scoped (id, metadataId, contentHash, identityHashVersion, locator, text, colour, bookTitle, bookAuthor, createdAt, updatedAt, deletedAt) SELECT id, metadataId, contentHash, identityHashVersion, locator, text, colour, bookTitle, bookAuthor, createdAt, updatedAt, deletedAt FROM highlights") + db.execSQL("DROP TABLE highlights") + db.execSQL("ALTER TABLE highlights_scoped RENAME TO highlights") + db.execSQL("CREATE INDEX index_highlights_metadataId_deletedAt ON highlights(metadataId, deletedAt)") + db.execSQL("CREATE INDEX index_highlights_contentHash_deletedAt ON highlights(contentHash, deletedAt)") + } + } + fun build(context: Context): EReaderDatabase = Room.databaseBuilder(context, EReaderDatabase::class.java, "ereader.db") .addMigrations( @@ -209,6 +232,7 @@ abstract class EReaderDatabase : RoomDatabase() { MIGRATION_7_8, MIGRATION_8_9, MIGRATION_9_10, + MIGRATION_10_11, ) .build() } diff --git a/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightDao.kt b/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightDao.kt index 3d2153b7..1e14a02b 100644 --- a/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightDao.kt +++ b/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightDao.kt @@ -12,35 +12,35 @@ interface HighlightDao { @Insert(onConflict = OnConflictStrategy.ABORT) suspend fun insert(highlight: HighlightEntity) - @Query("SELECT * FROM highlights WHERE id = :id LIMIT 1") - suspend fun findById(id: String): HighlightEntity? + @Query("SELECT * FROM highlights WHERE id = :id AND ownerScope = :ownerScope LIMIT 1") + suspend fun findById(id: String, ownerScope: String = ""): HighlightEntity? @Query( """ SELECT * FROM highlights - WHERE deletedAt IS NULL + WHERE deletedAt IS NULL AND (ownerScope = :ownerScope OR ownerScope = '') AND ((:metadataId IS NOT NULL AND metadataId = :metadataId) OR (:contentHash IS NOT NULL AND contentHash = :contentHash)) ORDER BY createdAt, id """ ) - fun observeActive(metadataId: String?, contentHash: String?): Flow> + fun observeActive(metadataId: String?, contentHash: String?, ownerScope: String = ""): Flow> @Query( """ UPDATE highlights - SET colour = :colour, updatedAt = MAX(:now, updatedAt + 1) - WHERE id = :id AND deletedAt IS NULL AND colour != :colour + SET colour = :colour, syncedAt = NULL, updatedAt = MAX(:now, updatedAt + 1) + WHERE id = :id AND (ownerScope = :ownerScope OR ownerScope = '') AND deletedAt IS NULL AND colour != :colour """ ) - suspend fun setColour(id: String, colour: String, now: Long) + suspend fun setColour(id: String, colour: String, now: Long, ownerScope: String = "") @Query( """ UPDATE highlights - SET deletedAt = MAX(:now, updatedAt + 1), updatedAt = MAX(:now, updatedAt + 1) - WHERE id = :id AND deletedAt IS NULL + SET deletedAt = MAX(:now, updatedAt + 1), syncedAt = NULL, updatedAt = MAX(:now, updatedAt + 1) + WHERE id = :id AND (ownerScope = :ownerScope OR ownerScope = '') AND deletedAt IS NULL """ ) - suspend fun softDelete(id: String, now: Long) + suspend fun softDelete(id: String, now: Long, ownerScope: String = "") } diff --git a/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightEntity.kt b/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightEntity.kt index 760aedb4..ca598e6a 100644 --- a/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightEntity.kt +++ b/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightEntity.kt @@ -3,18 +3,18 @@ package io.theficos.ereader.data.local.db import androidx.room.ColumnInfo import androidx.room.Entity import androidx.room.Index -import androidx.room.PrimaryKey /** No documents FK: removing a download must not delete the reader's annotations. */ @Entity( tableName = "highlights", + primaryKeys = ["ownerScope", "id"], indices = [ Index(value = ["metadataId", "deletedAt"]), Index(value = ["contentHash", "deletedAt"]), ], ) data class HighlightEntity( - @PrimaryKey val id: String, + val id: String, val metadataId: String?, val contentHash: String, @ColumnInfo(defaultValue = "1") val identityHashVersion: Int = 1, @@ -26,4 +26,6 @@ data class HighlightEntity( val createdAt: Long, val updatedAt: Long, val deletedAt: Long? = null, + @ColumnInfo(defaultValue = "''") val ownerScope: String = "", + @ColumnInfo(defaultValue = "NULL") val syncedAt: Long? = null, ) diff --git a/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightSyncDao.kt b/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightSyncDao.kt new file mode 100644 index 00000000..064b82db --- /dev/null +++ b/data/local/src/main/java/io/theficos/ereader/data/local/db/HighlightSyncDao.kt @@ -0,0 +1,97 @@ +package io.theficos.ereader.data.local.db + +import androidx.room.Dao +import androidx.room.Insert +import androidx.room.OnConflictStrategy +import androidx.room.Query +import androidx.room.Transaction + +/** Account partitioning is part of the primary key, including on downloaded-file-free rows. */ +@Dao +abstract class HighlightSyncDao { + @Query("SELECT * FROM highlights WHERE ownerScope = :scope AND (syncedAt IS NULL OR syncedAt != updatedAt) ORDER BY id LIMIT 500") + abstract suspend fun dirty(scope: String): List + + @Query("SELECT * FROM highlights WHERE ownerScope = ''") + abstract suspend fun unowned(): List + + @Query("SELECT * FROM highlights WHERE ownerScope = :scope AND id = :id") + abstract suspend fun find(scope: String, id: String): HighlightEntity? + + @Insert(onConflict = OnConflictStrategy.REPLACE) + abstract suspend fun put(row: HighlightEntity) + + @Query("DELETE FROM highlights WHERE ownerScope = '' AND id = :id") + abstract suspend fun deleteUnowned(id: String) + + @Query("INSERT OR REPLACE INTO sync_state (tableName, lastPulledAt) VALUES (:key, :cursor)") + abstract suspend fun setCursor(key: String, cursor: Long) + + /** Called only after authenticated endpoint success. Never reassign an already owned row. */ + @Transaction + open suspend fun claimUnowned(scope: String) { + require(scope.isNotEmpty()) + for (row in unowned()) { + val existing = find(scope, row.id) + val version = maxOf(row.identityHashVersion, existing?.identityHashVersion ?: 1) + val candidate = row.copy(ownerScope = scope, identityHashVersion = version, syncedAt = null) + val normalizedExisting = existing?.copy(identityHashVersion = version) + if (normalizedExisting == null || compareHighlights(candidate, normalizedExisting) > 0) { + put(candidate) + } else if (existing.identityHashVersion != version) { + put(normalizedExisting.copy(syncedAt = null)) + } + deleteUnowned(row.id) + } + } + + /** + * Full authoritative rows acknowledge only their exact payload. A concurrent local edit + * with a greater version stays dirty; a deterministic server winner replaces it atomically. + */ + @Transaction + open suspend fun merge(rows: List) { + for (incoming in rows) { + val current = find(incoming.ownerScope, incoming.id) + val version = maxOf(incoming.identityHashVersion, current?.identityHashVersion ?: 1) + val normalizedIncoming = incoming.copy(identityHashVersion = version) + val normalizedCurrent = current?.copy(identityHashVersion = version) + if (normalizedCurrent == null || compareHighlights(normalizedIncoming, normalizedCurrent) >= 0) { + put(normalizedIncoming.copy(syncedAt = if (version == incoming.identityHashVersion) incoming.updatedAt else null)) + } else if (current.identityHashVersion != version) { + // The payload and version can come from different edits. Upload that composite winner. + put(normalizedCurrent.copy(syncedAt = null)) + } + } + } + + @Transaction + open suspend fun applyPage(rows: List, key: String, cursor: Long) { + merge(rows) + setCursor(key, cursor) + } +} + +/** Exact counterpart of the server's normalized wire comparator, with UTF-8 string ordering. */ +fun compareHighlights(a: HighlightEntity, b: HighlightEntity): Int { + fun text(left: String?, right: String?): Int { + val x = left.orEmpty().toByteArray(Charsets.UTF_8) + val y = right.orEmpty().toByteArray(Charsets.UTF_8) + for (i in 0 until minOf(x.size, y.size)) { + val order = (x[i].toInt() and 255).compareTo(y[i].toInt() and 255) + if (order != 0) return order + } + return x.size.compareTo(y.size) + } + val values = listOf( + a.updatedAt.compareTo(b.updatedAt), + (a.deletedAt != null).compareTo(b.deletedAt != null), + (a.deletedAt ?: 0).compareTo(b.deletedAt ?: 0), + text(a.metadataId, b.metadataId), text(a.contentHash, b.contentHash), + a.identityHashVersion.compareTo(b.identityHashVersion), + text(a.locator, b.locator), text(a.text, b.text), text(a.colour, b.colour), + text(a.bookTitle, b.bookTitle), (a.bookAuthor != null).compareTo(b.bookAuthor != null), + text(a.bookAuthor, b.bookAuthor), a.createdAt.compareTo(b.createdAt), + ) + return values.firstOrNull { it != 0 } ?: 0 +} diff --git a/data/local/src/test/java/io/theficos/ereader/data/local/HighlightRepositoryTest.kt b/data/local/src/test/java/io/theficos/ereader/data/local/HighlightRepositoryTest.kt index 8673c243..fe958cff 100644 --- a/data/local/src/test/java/io/theficos/ereader/data/local/HighlightRepositoryTest.kt +++ b/data/local/src/test/java/io/theficos/ereader/data/local/HighlightRepositoryTest.kt @@ -10,8 +10,10 @@ import io.theficos.ereader.core.model.HighlightColour import io.theficos.ereader.data.local.db.DocumentEntity import io.theficos.ereader.data.local.db.EReaderDatabase import java.util.UUID +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Before import org.junit.Test @@ -169,4 +171,70 @@ class HighlightRepositoryTest { } assertThat(repo.observe(document().identity).first()).isEmpty() } + @Test fun `open book observation follows account changes and edits never reach a foreign account`() = runTest { + val scope = MutableStateFlow("account-a") + repo = HighlightRepository(db.highlightDao(), nowMs = { now }, scopeProvider = { scope.value }, scopeFlow = scope) + val a = repo.create(document(), "locator", "A private quote", HighlightColour.YELLOW) + scope.value = "account-b" + val b = repo.create(document(), "locator", "B private quote", HighlightColour.GREEN) + scope.value = "account-a" + repo.observe(document().identity).test { + assertThat(awaitItem().map { it.id }).containsExactly(a.id) + scope.value = "account-b" + assertThat(awaitItem().map { it.id }).containsExactly(b.id) + repo.delete(a.id) + repo.setColour(a.id, HighlightColour.PINK) + assertThat(db.highlightDao().findById(a.id, "account-a")!!.deletedAt).isNull() + assertThat(db.highlightDao().findById(a.id, "account-a")!!.colour).isEqualTo("YELLOW") + scope.value = "" + assertThat(awaitItem()).isEmpty() + cancelAndIgnoreRemainingEvents() + } + } + + @Test fun `offline rows stay visible and editable until first authenticated account claims them`() = runTest { + val offline = repo.create(document(), "locator", "Offline quote", HighlightColour.YELLOW) + val scope = MutableStateFlow("account-a") + repo = HighlightRepository(db.highlightDao(), nowMs = { now }, scopeProvider = { scope.value }, scopeFlow = scope) + assertThat(repo.observe(document().identity).first().map { it.id }).containsExactly(offline.id) + repo.setColour(offline.id, HighlightColour.PINK) + assertThat(db.highlightDao().findById(offline.id)!!.colour).isEqualTo("PINK") + db.highlightSyncDao().claimUnowned("account-a") + scope.value = "account-b" + assertThat(repo.observe(document().identity).first()).isEmpty() + scope.value = "account-a" + assertThat(repo.observe(document().identity).first().map { it.id }).containsExactly(offline.id) + } + + @Test fun `OPDS account passages remain local and cannot be claimed by a later Quire account`() = runTest { + val scope = MutableStateFlow("local-opds-account") + repo = HighlightRepository(db.highlightDao(), nowMs = { now }, scopeProvider = { scope.value }, scopeFlow = scope) + val local = repo.create(document(), "locator", "Private OPDS quote", HighlightColour.GREEN) + scope.value = "quire-account" + db.highlightSyncDao().claimUnowned(scope.value) + assertThat(db.highlightSyncDao().dirty(scope.value)).isEmpty() + assertThat(repo.observe(document().identity).first()).isEmpty() + scope.value = "local-opds-account" + assertThat(repo.observe(document().identity).first().single().id).isEqualTo(local.id) + } + + @Test fun `offline creation crossing authenticated claim is retained for next run`() = runTest { + val scope = MutableStateFlow("") + repo = HighlightRepository( + db.highlightDao(), + scopeProvider = { scope.value }, scopeFlow = scope, + nowMs = { + // Creation already captured its offline scope, then authentication/claim completes. + scope.value = "account-a" + runBlocking { db.highlightSyncDao().claimUnowned("account-a") } + 100L + }, + ) + val pending = repo.create(document(), "locator", "Created during login", HighlightColour.YELLOW) + assertThat(db.highlightSyncDao().unowned().single().id).isEqualTo(pending.id) + db.highlightSyncDao().claimUnowned("account-a") + assertThat(db.highlightSyncDao().unowned()).isEmpty() + assertThat(db.highlightSyncDao().dirty("account-a").single().id).isEqualTo(pending.id) + } + } diff --git a/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightDaoTest.kt b/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightDaoTest.kt index c562c18a..3d168275 100644 --- a/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightDaoTest.kt +++ b/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightDaoTest.kt @@ -55,4 +55,76 @@ class HighlightDaoTest { assertThat(dao.findById("same")).isEqualTo(tombstone) assertThat(dao.observeActive("metadata", "hash").first()).isEmpty() } + @Test fun `atomic scoped writes cannot edit or delete another account's annotation`() = runTest { + dao.insert(row("same").copy(ownerScope = "account-a")) + dao.insert(row("same").copy(ownerScope = "account-b", text = "B quote")) + dao.setColour("same", "PINK", 20, "account-b") + dao.softDelete("same", 21, "account-b") + assertThat(dao.findById("same", "account-a")!!.colour).isEqualTo("YELLOW") + assertThat(dao.findById("same", "account-a")!!.deletedAt).isNull() + assertThat(dao.findById("same", "account-b")!!.deletedAt).isEqualTo(21) + assertThat(dao.observeActive("metadata", "hash", "account-a").first().map { it.text }).containsExactly("quote") + } + + @Test fun `claim resolves UUID collision deterministically without rebinding foreign rows`() = runTest { + dao.insert(row("same").copy(updatedAt = 20, deletedAt = 20)) + dao.insert(row("same").copy(ownerScope = "account-a", identityHashVersion = 2)) + dao.insert(row("same").copy(ownerScope = "account-b", text = "B quote")) + db.highlightSyncDao().claimUnowned("account-a") + assertThat(db.highlightSyncDao().unowned()).isEmpty() + assertThat(dao.findById("same", "account-a")!!.deletedAt).isEqualTo(20) + assertThat(dao.findById("same", "account-a")!!.identityHashVersion).isEqualTo(2) + assertThat(dao.findById("same", "account-b")!!.text).isEqualTo("B quote") + assertThat(db.highlightSyncDao().dirty("account-a")).hasSize(1) + } + + @Test fun `zero timestamp snapshots are dirty until acknowledged`() = runTest { + val zero = row("zero").copy(ownerScope = "account-a", createdAt = 0, updatedAt = 0) + dao.insert(zero) + assertThat(db.highlightSyncDao().dirty("account-a")).hasSize(1) + db.highlightSyncDao().merge(listOf(zero)) + assertThat(db.highlightSyncDao().dirty("account-a")).isEmpty() + } + + @Test fun `UTF8 comparator agrees with Unicode codepoint order beyond BMP and preserves null author tie`() { + val supplementary = row("same").copy(text = "\uD800\uDC00") + val bmp = row("same").copy(text = "\uE000") + assertThat(compareHighlights(supplementary, bmp)).isGreaterThan(0) + assertThat(compareHighlights(row("same").copy(bookAuthor = ""), row("same"))).isGreaterThan(0) + } + + @Test fun `hash version never regresses and composite payload winners remain dirty`() = runTest { + val newer = row("same").copy(ownerScope = "account-a", updatedAt = 20, text = "Local edit", syncedAt = 20) + dao.insert(newer) + val stale = newer.copy(updatedAt = 10, text = "Stale remote", identityHashVersion = 3) + db.highlightSyncDao().merge(listOf(stale)) + val merged = db.highlightSyncDao().find("account-a", "same")!! + assertThat(merged.text).isEqualTo("Local edit") + assertThat(merged.identityHashVersion).isEqualTo(3) + assertThat(db.highlightSyncDao().dirty("account-a")).hasSize(1) + db.highlightSyncDao().merge(listOf(merged.copy(identityHashVersion = 1, text = "New remote", updatedAt = 30))) + val latest = db.highlightSyncDao().find("account-a", "same")!! + assertThat(latest.identityHashVersion).isEqualTo(3) + assertThat(latest.text).isEqualTo("New remote") + assertThat(db.highlightSyncDao().dirty("account-a")).hasSize(1) + db.highlightSyncDao().merge(listOf(latest)) + assertThat(db.highlightSyncDao().dirty("account-a")).isEmpty() + } + + @Test fun `equal clock payload comparison normalizes both hash versions before deciding winner`() = runTest { + val green = row("same").copy(ownerScope = "account-a", identityHashVersion = 2, colour = "GREEN", syncedAt = 10) + val yellow = green.copy(identityHashVersion = 1, colour = "YELLOW") + dao.insert(green) + db.highlightSyncDao().merge(listOf(yellow)) + val winner = db.highlightSyncDao().find("account-a", "same")!! + assertThat(winner.colour).isEqualTo("YELLOW") + assertThat(winner.identityHashVersion).isEqualTo(2) + assertThat(db.highlightSyncDao().dirty("account-a")).hasSize(1) + db.highlightSyncDao().put(yellow) + db.highlightSyncDao().merge(listOf(green)) + val reversed = db.highlightSyncDao().find("account-a", "same")!! + assertThat(reversed.colour).isEqualTo(winner.colour) + assertThat(reversed.identityHashVersion).isEqualTo(winner.identityHashVersion) + } + } diff --git a/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightMigrationTest.kt b/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightMigrationTest.kt index 0b137bb5..17f78810 100644 --- a/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightMigrationTest.kt +++ b/data/local/src/test/java/io/theficos/ereader/data/local/db/HighlightMigrationTest.kt @@ -116,4 +116,32 @@ class HighlightMigrationTest { } } } + @Test fun `migration 10 to 11 keeps snapshots tombstones and progress and permits account UUID reuse`() { + helper.createDatabase("highlights-10-11.db", 10).use { db -> + db.execSQL("INSERT INTO highlights (id, metadataId, contentHash, locator, text, colour, bookTitle, bookAuthor, createdAt, updatedAt, deletedAt) VALUES ('saved', 'book', 'hash', ' exact locator ', 'quote', 'PINK', 'Title', 'Author', 10, 20, 20)") + db.execSQL("INSERT INTO sync_state (tableName, lastPulledAt) VALUES ('progress', 106)") + } + helper.runMigrationsAndValidate("highlights-10-11.db", 11, true, EReaderDatabase.MIGRATION_10_11).use { db -> + db.query("SELECT ownerScope, syncedAt, locator, text, bookAuthor, deletedAt FROM highlights WHERE id='saved'").use { c -> + assertThat(c.moveToFirst()).isTrue() + assertThat(c.getString(0)).isEmpty() + assertThat(c.isNull(1)).isTrue() + assertThat(c.getString(2)).isEqualTo(" exact locator ") + assertThat(c.getString(3)).isEqualTo("quote") + assertThat(c.getString(4)).isEqualTo("Author") + assertThat(c.getLong(5)).isEqualTo(20) + } + db.execSQL("INSERT INTO highlights (ownerScope, id, contentHash, locator, text, colour, bookTitle, createdAt, updatedAt) VALUES ('account-a', 'saved', 'hash', 'locator', 'A quote', 'GREEN', 'Title', 10, 10)") + db.query("SELECT COUNT(*) FROM highlights WHERE id='saved'").use { c -> + c.moveToFirst() + assertThat(c.getInt(0)).isEqualTo(2) + } + db.query("SELECT lastPulledAt FROM sync_state WHERE tableName='progress'").use { c -> + c.moveToFirst() + assertThat(c.getLong(0)).isEqualTo(106) + } + db.query("PRAGMA foreign_key_list('highlights')").use { c -> assertThat(c.count).isEqualTo(0) } + } + } + } diff --git a/data/sync/build.gradle.kts b/data/sync/build.gradle.kts index 5ab7ca5b..1c3d44c1 100644 --- a/data/sync/build.gradle.kts +++ b/data/sync/build.gradle.kts @@ -17,6 +17,7 @@ android { } kotlinOptions { jvmTarget = "21" } testOptions { unitTests.isIncludeAndroidResources = true } + sourceSets.getByName("test").resources.srcDir("../../server/tests/fixtures") } dependencies { diff --git a/data/sync/src/main/java/io/theficos/ereader/data/sync/HighlightDtos.kt b/data/sync/src/main/java/io/theficos/ereader/data/sync/HighlightDtos.kt new file mode 100644 index 00000000..016270fa --- /dev/null +++ b/data/sync/src/main/java/io/theficos/ereader/data/sync/HighlightDtos.kt @@ -0,0 +1,53 @@ +package io.theficos.ereader.data.sync + +import io.theficos.ereader.core.model.HighlightColour +import io.theficos.ereader.data.local.db.HighlightEntity +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable +import java.util.UUID + +@Serializable +data class HighlightItemDto( + val id: String, + val document: DocumentIdDto, + val locator: String, + val text: String, + val colour: String, + @SerialName("book_title") val bookTitle: String, + @SerialName("book_author") val bookAuthor: String? = null, + @SerialName("created_at") val createdAt: Long, + @SerialName("updated_at") val updatedAt: Long, + @SerialName("deleted_at") val deletedAt: Long? = null, +) { + fun toEntity(scope: String): HighlightEntity { + require(UUID.fromString(id).toString() == id) { "Invalid highlight ID" } + require(document.contentHash.isNotBlank() && locator.isNotBlank() && text.isNotBlank()) + require(document.identityHashVersion > 0) + HighlightColour.valueOf(colour) + require(createdAt >= 0 && updatedAt >= createdAt && (deletedAt == null || deletedAt in 0..updatedAt)) + return HighlightEntity( + id, document.metadataId, document.contentHash, document.identityHashVersion, + locator, text, colour, bookTitle, bookAuthor, createdAt, updatedAt, deletedAt, + ownerScope = scope, syncedAt = updatedAt, + ) + } +} + +fun HighlightEntity.toSyncDto() = HighlightItemDto( + id, DocumentIdDto(metadataId, contentHash, identityHashVersion), locator, text, colour, + bookTitle, bookAuthor, createdAt, updatedAt, deletedAt, +) + +@Serializable data class HighlightPushBody(val items: List) +@Serializable data class HighlightPushResult(val status: String, val item: HighlightItemDto) +@Serializable data class HighlightPushResponse(val results: List) +@Serializable data class HighlightPullResponse( + val items: List, + @SerialName("next_cursor") val nextCursor: String, + @SerialName("has_more") val hasMore: Boolean, +) + +/** A run freezes both credentials and origin; it must never consult a live auth interceptor. */ +class HighlightSyncSession(val scope: String, val baseUrl: String, val authorization: String) { + override fun toString() = "HighlightSyncSession(scope=$scope)" +} diff --git a/data/sync/src/main/java/io/theficos/ereader/data/sync/HighlightSyncer.kt b/data/sync/src/main/java/io/theficos/ereader/data/sync/HighlightSyncer.kt new file mode 100644 index 00000000..b1ba1bb8 --- /dev/null +++ b/data/sync/src/main/java/io/theficos/ereader/data/sync/HighlightSyncer.kt @@ -0,0 +1,106 @@ +package io.theficos.ereader.data.sync + +import io.theficos.ereader.data.local.db.HighlightSyncDao +import io.theficos.ereader.data.local.db.SyncStateDao +import io.theficos.ereader.data.local.db.compareHighlights +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock + +/** Reuses the progress worker/manual-sync entry point; older servers remain fully usable. */ +class HighlightSyncer( + private val client: SyncClient, + private val dao: HighlightSyncDao, + private val state: SyncStateDao, + private val sessionProvider: () -> HighlightSyncSession?, +) { + private val mutex = Mutex() + + suspend fun runOnce(): SyncResult = mutex.withLock { + val session = sessionProvider() ?: return@withLock SyncResult.Success(Unit) + val key = "highlights:${session.scope}" + try { + var cursor = state.lastPulled(key) ?: 0L + var page = client.pullHighlights(session, cursor) + if (page is SyncResult.HttpFailure && page.code == 409) { + cursor = 0 + page = client.pullHighlights(session, cursor) + } + // Old servers: no ownership claim, no lost dirty rows, progress still succeeds. + if (page is SyncResult.HttpFailure && page.code in listOf(404, 405)) { + return@withLock SyncResult.Success(Unit) + } + if (page !is SyncResult.Success) return@withLock page.asUnit() + validatePage(page.value, session.scope, cursor) + if (!isCurrent(session)) return@withLock SyncResult.Success(Unit) + // Only a successful authenticated endpoint response can bind never-owned quotes. + dao.claimUnowned(session.scope) + + while (true) { + if (!isCurrent(session)) return@withLock SyncResult.Success(Unit) + val value = (page as? SyncResult.Success)?.value ?: return@withLock page.asUnit() + val rows = validatePage(value, session.scope, cursor) + cursor = value.nextCursor.toLong() + dao.applyPage(rows, key, cursor) + if (!value.hasMore) break + page = client.pullHighlights(session, cursor) + } + + var pushedAny = false + while (isCurrent(session)) { + val dirty = dao.dirty(session.scope) + if (dirty.isEmpty()) break + val pushed = client.pushHighlights(session, dirty.map { it.toSyncDto() }) + if (pushed !is SyncResult.Success) return@withLock pushed.asUnit() + val results = pushed.value.results + require(results.size == dirty.size && results.map { it.item.id }.toSet() == dirty.map { it.id }.toSet()) { + "Incomplete highlight acknowledgement" + } + val byId = dirty.associateBy { it.id } + val winners = results.map { result -> + require(result.status == "accepted" || result.status == "stale") + result.item.toEntity(session.scope).also { winner -> + val sent = requireNotNull(byId[winner.id]) + require(winner.identityHashVersion >= sent.identityHashVersion && + compareHighlights(winner, sent.copy(identityHashVersion = winner.identityHashVersion)) >= 0) { + "Invalid highlight winner" + } + } + } + // Comparing the complete authoritative payload prevents an in-flight edit from + // being acknowledged merely because it shares the same UUID/timestamp. + dao.merge(winners) + pushedAny = true + } + if (pushedAny) { + while (true) { + if (!isCurrent(session)) return@withLock SyncResult.Success(Unit) + val pulled = client.pullHighlights(session, cursor) + if (pulled !is SyncResult.Success) return@withLock pulled.asUnit() + val rows = validatePage(pulled.value, session.scope, cursor) + cursor = pulled.value.nextCursor.toLong() + dao.applyPage(rows, key, cursor) + if (!pulled.value.hasMore) break + } + } + SyncResult.Success(Unit) + } catch (_: IllegalArgumentException) { + SyncResult.HttpFailure(0, "Invalid highlight sync response") + } + } + + private fun isCurrent(session: HighlightSyncSession) = sessionProvider()?.scope == session.scope + + private fun validatePage(page: HighlightPullResponse, scope: String, previous: Long) = + page.items.map { it.toEntity(scope) }.also { + val next = page.nextCursor.toLongOrNull() + require(next != null && next >= previous && (!page.hasMore || next > previous)) + require(page.items.size <= 500 && page.items.map { row -> row.id }.toSet().size == page.items.size) + } +} + +private fun SyncResult.asUnit(): SyncResult = when (this) { + is SyncResult.Success -> SyncResult.Success(Unit) + is SyncResult.Unauthorized -> this + is SyncResult.HttpFailure -> this + is SyncResult.NetworkFailure -> this +} diff --git a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncApi.kt b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncApi.kt index 75a7fd30..11221dc4 100644 --- a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncApi.kt +++ b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncApi.kt @@ -3,5 +3,6 @@ package io.theficos.ereader.data.sync object SyncApi { const val PATH_PROGRESS_PUSH = "/sync/v1/progress" const val PATH_PROGRESS_PULL = "/sync/v1/progress" + const val PATH_HIGHLIGHTS = "/sync/v1/highlights" const val PATH_HEALTHZ = "/sync/v1/healthz" } diff --git a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncClient.kt b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncClient.kt index 18d7917f..a92ebfd0 100644 --- a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncClient.kt +++ b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncClient.kt @@ -31,6 +31,26 @@ class SyncClient( execute(req, ProgressPullResponse.serializer()) } + // Redirects can replay a private POST body; this endpoint never follows them. + private val highlightHttp = okHttp.newBuilder().followRedirects(false).followSslRedirects(false).build() + + suspend fun pullHighlights(session: HighlightSyncSession, cursor: Long): SyncResult = + withContext(Dispatchers.IO) { + val url = (session.baseUrl.trimEnd('/') + SyncApi.PATH_HIGHLIGHTS).toHttpUrl() + .newBuilder().addQueryParameter("since", cursor.toString()).addQueryParameter("limit", "500").build() + execute(Request.Builder().url(url).header("Authorization", session.authorization).get().build(), + HighlightPullResponse.serializer(), highlightHttp) + } + + suspend fun pushHighlights(session: HighlightSyncSession, items: List): SyncResult = + withContext(Dispatchers.IO) { + val body = json.encodeToString(HighlightPushBody.serializer(), HighlightPushBody(items)) + .toRequestBody("application/json".toMediaType()) + val request = Request.Builder().url(session.baseUrl.trimEnd('/') + SyncApi.PATH_HIGHLIGHTS) + .header("Authorization", session.authorization).post(body).build() + execute(request, HighlightPushResponse.serializer(), highlightHttp) + } + private fun post( baseUrl: String, path: String, @@ -47,8 +67,8 @@ class SyncClient( return execute(req, respSerializer) } - private fun execute(req: Request, serializer: KSerializer): SyncResult = try { - okHttp.newCall(req).execute().use { resp -> + private fun execute(req: Request, serializer: KSerializer, http: OkHttpClient = okHttp): SyncResult = try { + http.newCall(req).execute().use { resp -> val raw = resp.body?.string().orEmpty() when { resp.code == 401 -> SyncResult.Unauthorized diff --git a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncOrchestrator.kt b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncOrchestrator.kt index cde95179..80057da3 100644 --- a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncOrchestrator.kt +++ b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncOrchestrator.kt @@ -7,15 +7,26 @@ import io.theficos.ereader.data.local.db.ProgressEntity import io.theficos.ereader.data.local.db.SyncStateDao import io.theficos.ereader.data.local.db.SyncStateEntity import java.time.Instant +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock class SyncOrchestrator( private val client: SyncClient, private val progressDao: ProgressDao, private val documentRepo: DocumentRepository, private val syncState: SyncStateDao, + private val highlights: HighlightSyncer? = null, ) { - suspend fun runOnce(): SyncResult { + private val runMutex = Mutex() + + suspend fun runOnce(): SyncResult = runMutex.withLock { + val progress = runProgress() + if (progress !is SyncResult.Success) return@withLock progress + highlights?.runOnce() ?: progress + } + + private suspend fun runProgress(): SyncResult { // 1) PUSH dirty rows val dirty = progressDao.dirty() if (dirty.isNotEmpty()) { diff --git a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncWorker.kt b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncWorker.kt index c50aed2b..c3c1b5f3 100644 --- a/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncWorker.kt +++ b/data/sync/src/main/java/io/theficos/ereader/data/sync/SyncWorker.kt @@ -57,7 +57,7 @@ class SyncWorker( } Log.i(TAG, "doWork: starting sync") val outcome = deps.orchestrator.runOnce() - Log.i(TAG, "doWork: outcome=$outcome") + Log.i(TAG, "doWork: outcome=${outcome.javaClass.simpleName}") return when (outcome) { is SyncResult.Success -> Result.success() is SyncResult.NetworkFailure -> Result.retry() diff --git a/data/sync/src/test/java/io/theficos/ereader/data/sync/HighlightSyncerTest.kt b/data/sync/src/test/java/io/theficos/ereader/data/sync/HighlightSyncerTest.kt new file mode 100644 index 00000000..9283aa74 --- /dev/null +++ b/data/sync/src/test/java/io/theficos/ereader/data/sync/HighlightSyncerTest.kt @@ -0,0 +1,219 @@ +package io.theficos.ereader.data.sync + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import io.theficos.ereader.data.local.db.EReaderDatabase +import io.theficos.ereader.data.local.db.HighlightEntity +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.jsonObject +import kotlinx.serialization.json.jsonArray +import kotlinx.serialization.json.jsonPrimitive +import kotlinx.serialization.json.int +import io.theficos.ereader.data.local.db.compareHighlights +import okhttp3.OkHttpClient +import okhttp3.mockwebserver.Dispatcher +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.RecordedRequest +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) +class HighlightSyncerTest { + private lateinit var db: EReaderDatabase + private lateinit var server: MockWebServer + private lateinit var client: SyncClient + private var scope = "account-a" + private val json = Json { ignoreUnknownKeys = true } + private val id = "14300000-0000-4000-8000-000000000001" + + @Before fun setUp() { + db = Room.inMemoryDatabaseBuilder(ApplicationProvider.getApplicationContext(), EReaderDatabase::class.java) + .allowMainThreadQueries().build() + db.openHelper.writableDatabase + server = MockWebServer().also { it.start() } + client = SyncClient({ server.url("/").toString() }, OkHttpClient()) + } + + @After fun tearDown() { db.close(); server.shutdown() } + + private fun syncer() = HighlightSyncer(client, db.highlightSyncDao(), db.syncStateDao()) { + HighlightSyncSession(scope, server.url("/").toString(), "Bearer $scope-secret") + } + + private fun row(owner: String = scope) = HighlightEntity( + id = id, metadataId = "book", contentHash = "hash", locator = " {\"href\":\"a.xhtml\"} ", + text = "Café ☺", colour = "YELLOW", bookTitle = "Retained book", bookAuthor = null, + createdAt = 10, updatedAt = 10, ownerScope = owner, + ) + + private fun page(items: List = emptyList(), cursor: String = "0", more: Boolean = false) = + MockResponse().setBody(json.encodeToString(HighlightPullResponse.serializer(), HighlightPullResponse(items, cursor, more))) + + private fun ack(items: List) = MockResponse().setBody(json.encodeToString( + HighlightPushResponse.serializer(), HighlightPushResponse(items.map { HighlightPushResult("accepted", it) }), + )) + + @Test fun `shared server fixture preserves opaque locator quote metadata and tombstone without document`() = runTest { + val fixture = json.parseToJsonElement(requireNotNull(javaClass.getResource("/highlight-sync.json")).readText()).jsonObject + val expected = json.decodeFromJsonElement(HighlightPullResponse.serializer(), fixture.getValue("pull")) + server.enqueue(MockResponse().setBody(fixture.getValue("pull").toString())) + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.Success::class.java) + val stored = db.highlightSyncDao().find(scope, id)!! + assertThat(stored.toSyncDto()).isEqualTo(expected.items.single()) + assertThat(db.highlightSyncDao().dirty(scope)).isEmpty() + assertThat(db.syncStateDao().lastPulled("highlights:$scope")).isEqualTo(2) + assertThat(server.takeRequest().getHeader("Authorization")).isEqualTo("Bearer account-a-secret") + } + + @Test fun `successful endpoint claims and uploads offline rows but never another account's rows`() = runTest { + val offline = row("") + db.highlightDao().insert(offline) + db.highlightDao().insert(row("other-account").copy(id = "14300000-0000-4000-8000-000000000002")) + server.enqueue(page()) + server.enqueue(ack(listOf(offline.toSyncDto()))) + server.enqueue(page(cursor = "1")) + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.Success::class.java) + assertThat(db.highlightSyncDao().unowned()).isEmpty() + assertThat(db.highlightSyncDao().dirty(scope)).isEmpty() + server.takeRequest() + val pushed = json.decodeFromString(HighlightPushBody.serializer(), server.takeRequest().body.readUtf8()) + assertThat(pushed.items.map { it.id }).containsExactly(id) + } + + @Test fun `unsupported or unauthorized endpoint does not claim offline rows or acknowledge dirty rows`() = runTest { + for (code in listOf(404, 405, 401)) { + db.highlightSyncDao().put(row("")) + server.enqueue(MockResponse().setResponseCode(code)) + val result = syncer().runOnce() + assertThat(result is SyncResult.Success).isEqualTo(code != 401) + assertThat(db.highlightSyncDao().unowned()).hasSize(1) + assertThat(db.syncStateDao().lastPulled("highlights:$scope")).isNull() + } + } + + @Test fun `same UUID lives independently per account and cursors do not skip returning account`() = runTest { + val original = row().copy(syncedAt = 10) + db.highlightDao().insert(original) + scope = "account-b" + val incoming = row().copy(text = "B's private quote") + server.enqueue(page(listOf(incoming.toSyncDto()), "4")) + syncer().runOnce() + assertThat(db.highlightSyncDao().find("account-a", id)).isEqualTo(original) + assertThat(db.highlightSyncDao().find("account-b", id)!!.text).isEqualTo("B's private quote") + assertThat(server.takeRequest().requestUrl!!.queryParameter("since")).isEqualTo("0") + scope = "account-a" + server.enqueue(page(cursor = "0")) + syncer().runOnce() + assertThat(server.takeRequest().requestUrl!!.queryParameter("since")).isEqualTo("0") + } + + @Test fun `switching account while authenticated preflight is in flight cannot claim or push`() = runTest { + db.highlightDao().insert(row("")) + server.dispatcher = object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse { + scope = "account-b" + assertThat(request.getHeader("Authorization")).isEqualTo("Bearer account-a-secret") + return page() + } + } + syncer().runOnce() + assertThat(db.highlightSyncDao().unowned()).hasSize(1) + assertThat(server.requestCount).isEqualTo(1) + } + + @Test fun `acknowledgement arriving after an edit cannot mark the new version synced`() = runTest { + val sent = row() + db.highlightDao().insert(sent) + var posts = 0 + server.dispatcher = object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse = when (request.method) { + "GET" -> page() + else -> { + posts++ + if (posts == 1) { + runBlocking { db.highlightDao().setColour(id, "PINK", 10, scope) } + ack(listOf(sent.toSyncDto())) + } else MockResponse().setResponseCode(500) + } + } + } + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.HttpFailure::class.java) + val dirty = db.highlightSyncDao().dirty(scope).single() + assertThat(dirty.updatedAt).isEqualTo(11) + assertThat(dirty.colour).isEqualTo("PINK") + assertThat(dirty.syncedAt).isNull() + } + + @Test fun `incomplete acknowledgement is rejected and dirty snapshot retained`() = runTest { + db.highlightDao().insert(row()) + server.enqueue(page()) + server.enqueue(ack(emptyList())) + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.HttpFailure::class.java) + assertThat(db.highlightSyncDao().dirty(scope)).hasSize(1) + } + + @Test fun `cursor reset after server restore and paged pulls commit one page at a time`() = runTest { + db.highlightSyncDao().setCursor("highlights:$scope", 100) + server.enqueue(MockResponse().setResponseCode(409)) + server.enqueue(page(listOf(row().toSyncDto()), "1", true)) + server.enqueue(MockResponse().setResponseCode(500)) + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.HttpFailure::class.java) + assertThat(db.syncStateDao().lastPulled("highlights:$scope")).isEqualTo(1) + assertThat(db.highlightSyncDao().find(scope, id)).isNotNull() + assertThat(server.takeRequest().requestUrl!!.queryParameter("since")).isEqualTo("100") + assertThat(server.takeRequest().requestUrl!!.queryParameter("since")).isEqualTo("0") + assertThat(server.takeRequest().requestUrl!!.queryParameter("since")).isEqualTo("1") + } + + @Test fun `equal timestamp tombstone beats dirty live row and stale live response cannot resurrect`() = runTest { + db.highlightDao().insert(row()) + val deleted = row().copy(deletedAt = 10) + server.enqueue(page(listOf(deleted.toSyncDto()), "1")) + syncer().runOnce() + assertThat(db.highlightSyncDao().find(scope, id)!!.deletedAt).isEqualTo(10) + server.enqueue(page(listOf(row().toSyncDto()), "2")) + syncer().runOnce() + assertThat(db.highlightSyncDao().find(scope, id)!!.deletedAt).isEqualTo(10) + assertThat(db.highlightSyncDao().dirty(scope)).isEmpty() + } + + @Test fun `remote edit committed during push is pulled in the same run`() = runTest { + val sent = row() + db.highlightDao().insert(sent) + server.enqueue(page()) + server.enqueue(ack(listOf(sent.toSyncDto()))) + val remote = sent.copy(text = "Concurrent remote edit", updatedAt = 20) + server.enqueue(page(listOf(remote.toSyncDto()), "2")) + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.Success::class.java) + assertThat(db.highlightSyncDao().find(scope, id)!!.text).isEqualTo(remote.text) + assertThat(db.syncStateDao().lastPulled("highlights:$scope")).isEqualTo(2) + } + + @Test fun `shared server conflict vectors use the same deterministic comparator`() { + val fixture = json.parseToJsonElement(requireNotNull(javaClass.getResource("/highlight-sync.json")).readText()).jsonObject + for (element in fixture.getValue("conflict_cases").jsonArray) { + val case = element.jsonObject + val left = json.decodeFromJsonElement(HighlightItemDto.serializer(), case.getValue("left")).toEntity(scope) + val right = json.decodeFromJsonElement(HighlightItemDto.serializer(), case.getValue("right")).toEntity(scope) + val actual = compareHighlights(left, right).compareTo(0) + assertWithMessage(case.getValue("name").jsonPrimitive.content).that(actual) + .isEqualTo(case.getValue("expected_order").jsonPrimitive.int) + } + } + + @Test fun `private highlight redirect is not followed`() = runTest { + server.enqueue(MockResponse().setResponseCode(307).addHeader("Location", server.url("/leaked"))) + assertThat(syncer().runOnce()).isInstanceOf(SyncResult.HttpFailure::class.java) + assertThat(server.requestCount).isEqualTo(1) + } +} diff --git a/docs/reader-highlights.md b/docs/reader-highlights.md index ec0aff3c..91f84f58 100644 --- a/docs/reader-highlights.md +++ b/docs/reader-highlights.md @@ -1,7 +1,8 @@ # Highlights and quotes Highlights work offline with local EPUBs, OPDS downloads and books from a Quire -Server. They are stored on this device; they do not sync through Quire Server yet. +Server. They are saved on the device immediately and sync through Quire Server +when an account and a compatible server are configured. Tap the centre of a reading page to show the controls, then choose **Highlight**. The title changes to **Highlight mode** and **Done** stays available. Long-press @@ -41,13 +42,40 @@ Downloading or importing the same book again restores them through its library identity. If an edition's chapter content changes, Quire keeps the saved quotation but does not guess a new location. A message explains when highlights cannot be shown in that edition. The list remains local to the current book; cross-book -search and syncing highlights are separate follow-up work. +search is separate follow-up work. + +## Sync across devices + +Highlights use the same Quire Server account and sync triggers as reading +progress, including **Sync now**. Saved passages, colours and removals travel +together. The server retains the quotation and its exact location even when +that book has not been downloaded on another device. Download the same edition +to draw its highlights in the reader. + +Existing highlights and passages saved before configuring an account join the +first Quire account that successfully authenticates to the highlight endpoint. +After that, their account binding is retained. Switching accounts or servers +shows that account's passages and does not transfer another account's highlights. +Returning to the original account restores access to its saved passages. +Passages saved while using a generic OPDS account remain bound to that catalog +and stay local when a Quire account is configured later. + +Offline edits remain pending until sync succeeds. Removing a highlight leaves a +deletion record so another device can remove its copy too. Concurrent edits use +the latest update timestamp; a deletion wins a tie. Device clocks therefore +affect which edit wins. Removing a download keeps the highlights and their +pending changes. + +Older servers without the highlight endpoint continue syncing reading progress; +highlights remain local and pending until the server is upgraded. Highlight +sync does not expose an agent API or send passages to an AI provider. ## Local storage and rendering Room schema 10 adds the `highlights` table. Its device-generated UUIDs, separate selected-text and locator columns, book metadata snapshots, update timestamps and -soft-deletion tombstones prepare it for a later sync protocol. It has no foreign +soft-deletion tombstones support the sync protocol. Room schema 11 adds account +ownership and sync acknowledgement state, preserving existing rows. It has no foreign key to `documents`: deleting a download cannot cascade into annotation deletion. Lookup uses the existing normalized metadata identifier or content hash. diff --git a/docs/sync-api.md b/docs/sync-api.md index 0677c6d4..2b0108f4 100644 --- a/docs/sync-api.md +++ b/docs/sync-api.md @@ -1,7 +1,7 @@ # Sync API REST surface of the `quire-server` server. Endpoints live under four -prefixes — `/sync/v1` (progress + planned bookmarks), `/library/v1` +prefixes — `/sync/v1` (progress, highlights + planned bookmarks), `/library/v1` (per-user library mirror, including the Phase 0 push-model `POST /library/v1/sync` bulk endpoint), `/ai/v1` (optional book insights and reader profile), and the unprefixed `/health` / `/readyz` operational @@ -134,6 +134,8 @@ the operator migrates. Plan cutover within the two-minor-release window. | `GET` | `/quire-admin` | admin | The status as an HTML page with a "Test AI connection" button (`POST /quire-admin/probe`) and an AI settings form (`POST /quire-admin/settings`) | | `POST` | `/sync/v1/progress` | yes | Push progress for one or more documents | | `GET` | `/sync/v1/progress` | yes | Pull progress deltas | +| `POST` | `/sync/v1/highlights` | yes | Push saved passages and tombstones | +| `GET` | `/sync/v1/highlights` | yes | Pull per-user highlights by revision cursor | | `POST` | `/sync/v1/documents/alias` | yes | Reconcile a hash-keyed record with a newly-known metadata-id (planned) | | `POST` | `/sync/v1/bookmarks` | yes | Push bookmark create/delete (planned) | | `GET` | `/sync/v1/bookmarks` | yes | Pull bookmark deltas (planned) | @@ -257,6 +259,80 @@ This unpaginated response costs O(the user's progress rows) on every pull; it favors reliable offline convergence at the current personal-library scale. A future delta protocol needs a commit-safe cursor before restoring filtering. +## Highlights + +These endpoints use the configured authentication backend and are available in +full and sync-only deployments (`QUIRE_SERVER_PROGRESS_ENABLED=true`). They are +not mounted in AI-only mode. Rows belong to the authenticated user; the client +cannot choose a user ID. Highlights have no dependency on a downloaded document. + +### `POST /sync/v1/highlights` + +Push up to 500 items in one transaction. Example: + +```json +{ + "items": [ + { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + } + ] +} +``` + +`id` is a canonical UUID. `document` follows the existing normalized identity +rules. `locator` is an opaque string, preserved byte for byte; `text` is the +selected quotation. Colours are `YELLOW`, `GREEN` or `PINK`. Timestamps are +nonnegative signed 64-bit epoch milliseconds; `created_at` and a non-null +`deleted_at` must not exceed `updated_at`. `book_author` and `metadata_id` may be +null. Quote text and locator must be nonblank. + +The response is `{"results":[{"status":"accepted","item":{...}}]}` in input +order. A losing mutation has status `stale`; both statuses include the complete +winning row. An identical replay is accepted without allocating a new revision. +Acknowledgement must match the exact sent local snapshot so an edit made during +the request remains pending. + +Before comparison, both candidates use the maximum of their +`identity_hash_version` values, so older clients cannot lower the learned version. +Conflicts then select the maximum tuple `(updated_at, deleted_at != null, +deleted_at or 0, metadata_id or "", content_hash, identity_hash_version, +locator, text, colour, book_title, book_author != null, book_author or "", +created_at)` after identity normalization. Strings compare by unsigned UTF-8 +order. This makes deletion win an equal-time tie and makes replay order +irrelevant. A version-only upgrade preserves the winning payload and allocates +a new revision even when the incoming edit is stale. Device clocks affect +conflict resolution. Tombstones are retained. + +### `GET /sync/v1/highlights?since=0&limit=500` + +`since` is a decimal integer revision cursor, initially `0`; `limit` is 1–500. +The response is `{"items":[...],"next_cursor":"2","has_more":false}`. +Continue with `next_cursor` while `has_more` is true. Each changed row receives +a distinct per-user revision under the same transaction lock used by pulls. +This prevents a later commit from appearing behind an already returned cursor. +Persist the cursor only after applying the complete page locally. Always pull +after pushing; a push response does not advance the pull cursor. + +`409` with detail `highlight_cursor_reset_required` means the server's revision +history is behind the supplied cursor: reset to `0` and pull again. An older +server returning `404` leaves local edits pending while progress sync continues. +The shared contract fixture is `server/tests/fixtures/highlight-sync.json`; +Python and Kotlin tests consume the same Unicode quote and opaque locator. + ## Documents (planned) ### `POST /sync/v1/documents/alias` diff --git a/server/README.md b/server/README.md index 097047ef..5196ea1b 100644 --- a/server/README.md +++ b/server/README.md @@ -76,6 +76,29 @@ uv run uvicorn quire_server.main:app --reload Tests require Docker (testcontainers spins up Postgres). +### Highlight sync + +`POST /sync/v1/highlights` pushes up to 500 private annotations; `GET +/sync/v1/highlights?since=0&limit=500` pulls revision-ordered pages including +deletion tombstones. Both use the existing calibre-web Basic authentication +(or configured native backend) and are mounted only in full and sync-only +deployments. UUIDs are scoped to the authenticated user. Quotes, opaque +locators, colours and book metadata are separate fields; no EPUB upload is +required and removing a download does not delete its highlights. + +Push results include each authoritative winning row. Equal client timestamps +use a deterministic complete-payload ordering, with tombstones winning over +live rows. Identity hash versions are promoted to the maximum learned value +independently of which payload wins. The decimal `next_cursor` belongs to one server/account; clients +save it after applying a page and continue while `has_more` is true. A 409 +`highlight_cursor_reset_required` requires restarting the pull at zero. +Per-user transaction locks order revision allocation, commit and reads, so +offline client clocks and in-flight writes cannot create cursor gaps. +Tombstones are retained indefinitely. Native account export and purge include +highlights and their cursor state. See [`../docs/sync-api.md`](../docs/sync-api.md) +for the wire format and tie ordering; the shared Android/Python contract +fixture is `tests/fixtures/highlight-sync.json`. + ## Self-hosting via docker-compose Two reference composes ship in this directory. Pick one: diff --git a/server/migrations/README.md b/server/migrations/README.md index bb754e5f..a4305057 100644 --- a/server/migrations/README.md +++ b/server/migrations/README.md @@ -28,12 +28,20 @@ The current AI head is `ai_009`, adding `ai_usage_monthly` for an optional generic authenticated HTTP policy. It follows `ai_008` without rewriting any deployed revision. Quotas are inert unless the operator enables that policy. +The current progress head is `progress_004`, following `progress_003` and +adding caller-scoped `highlights` plus `highlight_sync_state`. Highlights +have no document foreign key, so removing a downloaded book preserves +annotations. A unique `(user_id, revision)` index supports safe delta pages; +writers and readers serialize on the same per-user transaction lock before +allocating or observing revisions. Both tables are included in native +account export and purge. This migration is skipped in AI-only deployments. + From the split point onward, every new migration belongs to exactly one branch: | Branch | Label | Owners | Notes | | ---------- | ------------ | ------ | ------------------------------------------------------------------------------------------------ | | `core` | `"core"` | shared | Reserved; no migrations yet. | -| `progress` | `"progress"` | sync | First and current head: `progress_001_library_items` (PR1, 2026-05-16). Spliced from `0004`. | +| `progress` | `"progress"` | sync | `progress_001` → `progress_002` → `progress_003` → `progress_004` (private highlights). Spliced from `0004`. | | `ai` | `"ai"` | AI | Chain: `ai_001_generation_log` (PR-C) → `ai_002_insight_language` (PR4) → `ai_003_identity_aliases` (PR2, 2026-05-16) → `ai_004_themes` (PR3, 2026-05-17). | ### Adding the FIRST migration on a branch (splice) diff --git a/server/migrations/versions/progress_004_highlights.py b/server/migrations/versions/progress_004_highlights.py new file mode 100644 index 00000000..67f7084e --- /dev/null +++ b/server/migrations/versions/progress_004_highlights.py @@ -0,0 +1,46 @@ +"""Private highlights and per-user, transaction-ordered sync revisions. + +Revision ID: progress_004 +Revises: progress_003 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "progress_004" +down_revision = "progress_003" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "highlight_sync_state", + sa.Column("user_id", sa.String(), primary_key=True), + sa.Column("revision", sa.BigInteger(), nullable=False), + ) + op.create_table( + "highlights", + sa.Column("user_id", sa.String(), primary_key=True), + sa.Column("id", sa.Uuid(), primary_key=True), + sa.Column("metadata_id", sa.String(), nullable=True), + sa.Column("content_hash", sa.String(), nullable=False), + sa.Column("identity_hash_version", sa.Integer(), nullable=False), + sa.Column("locator", sa.String(), nullable=False), + sa.Column("text", sa.String(), nullable=False), + sa.Column("colour", sa.String(), nullable=False), + sa.Column("book_title", sa.String(), nullable=False), + sa.Column("book_author", sa.String(), nullable=True), + sa.Column("created_at", sa.BigInteger(), nullable=False), + sa.Column("updated_at", sa.BigInteger(), nullable=False), + sa.Column("deleted_at", sa.BigInteger(), nullable=True), + sa.Column("revision", sa.BigInteger(), nullable=False), + ) + op.create_index( + "ix_highlights_user_revision", "highlights", ["user_id", "revision"], unique=True + ) + + +def downgrade() -> None: + op.drop_table("highlights") + op.drop_table("highlight_sync_state") diff --git a/server/quire_server/api/account_lifecycle.py b/server/quire_server/api/account_lifecycle.py index 7f615f92..711b6fdd 100644 --- a/server/quire_server/api/account_lifecycle.py +++ b/server/quire_server/api/account_lifecycle.py @@ -12,6 +12,8 @@ router = APIRouter() PRIVATE = { + "highlights": "user_id", + "highlight_sync_state": "user_id", "documents": "user_id", "library_items": "user_id", "user_ai_preferences": "user_id", diff --git a/server/quire_server/api/highlights.py b/server/quire_server/api/highlights.py new file mode 100644 index 00000000..dfed730a --- /dev/null +++ b/server/quire_server/api/highlights.py @@ -0,0 +1,217 @@ +"""Private highlight sync with deterministic conflicts and commit-ordered deltas.""" + +from typing import Annotated, Literal +from uuid import UUID + +from fastapi import APIRouter, Depends, HTTPException, Query +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from sqlalchemy import select, text +from sqlalchemy.ext.asyncio import AsyncSession + +from quire_server.core.auth import current_user_id +from quire_server.core.identity import normalize_metadata_id +from quire_server.db.models import Highlight, HighlightSyncState +from quire_server.db.session import get_session + +router = APIRouter(tags=["highlights"]) +MAX_INT = 2**63 - 1 + + +class HighlightDocument(BaseModel): + metadata_id: str | None = None + content_hash: str = Field(min_length=1, max_length=1024) + identity_hash_version: int = Field(default=1, ge=1, le=2**31 - 1) + + @field_validator("metadata_id") + @classmethod + def normalize_metadata(cls, value: str | None) -> str | None: + return normalize_metadata_id(value) + + @field_validator("content_hash") + @classmethod + def normalize_hash(cls, value: str) -> str: + value = value.strip() + if not value: + raise ValueError("content_hash must not be blank") + return value + + +class HighlightItem(BaseModel): + model_config = ConfigDict(from_attributes=True) + + id: UUID + document: HighlightDocument + locator: str = Field(min_length=1, max_length=262144) + text: str = Field(min_length=1, max_length=262144) + colour: Literal["YELLOW", "GREEN", "PINK"] + book_title: str = Field(max_length=16384) + book_author: str | None = Field(default=None, max_length=16384) + created_at: int = Field(ge=0, le=MAX_INT) + updated_at: int = Field(ge=0, le=MAX_INT) + deleted_at: int | None = Field(default=None, ge=0, le=MAX_INT) + + @field_validator("locator", "text") + @classmethod + def nonblank(cls, value: str) -> str: + if not value.strip(): + raise ValueError("must not be blank") + return value # The locator is opaque: never parse or reserialize it. + + @model_validator(mode="after") + def timestamps(self): + if self.created_at > self.updated_at: + raise ValueError("created_at must be <= updated_at") + if self.deleted_at is not None and self.deleted_at > self.updated_at: + raise ValueError("deleted_at must be <= updated_at") + return self + + +def conflict_key(item: HighlightItem) -> tuple: + """Shared with Android; strings compare by unsigned UTF-8 lexical order.""" + return ( + item.updated_at, + item.deleted_at is not None, + item.deleted_at or 0, + item.document.metadata_id or "", + item.document.content_hash, + item.document.identity_hash_version, + item.locator, + item.text, + item.colour, + item.book_title, + item.book_author is not None, + item.book_author or "", + item.created_at, + ) + + +def wire(row: Highlight) -> HighlightItem: + return HighlightItem( + id=row.id, + document=HighlightDocument( + metadata_id=row.metadata_id, + content_hash=row.content_hash, + identity_hash_version=row.identity_hash_version, + ), + **{ + key: getattr(row, key) + for key in HighlightItem.model_fields + if key not in {"id", "document"} + }, + ) + + +class HighlightPushBody(BaseModel): + items: list[HighlightItem] = Field(max_length=500) + + +class HighlightPushResult(BaseModel): + status: Literal["accepted", "stale"] + item: HighlightItem + + +class HighlightPushResponse(BaseModel): + results: list[HighlightPushResult] + + +class HighlightPullResponse(BaseModel): + items: list[HighlightItem] + next_cursor: str + has_more: bool + + +async def lock_user(session: AsyncSession, user_id: str) -> None: + # A sequence alone is unsafe: a transaction can allocate a lower value and + # commit after the pull has advanced past it. Both writers AND readers use + # this transaction lock. Allocation, row changes and commit are serialized + # for each principal across processes, including creation of the first row. + await session.execute( + text("SELECT pg_advisory_xact_lock(hashtextextended(:key, 0))"), + {"key": "highlights:" + user_id}, + ) + + +@router.post("/highlights", response_model=HighlightPushResponse) +async def push_highlights( + body: HighlightPushBody, + user_id: Annotated[str, Depends(current_user_id)], + session: Annotated[AsyncSession, Depends(get_session)], +) -> HighlightPushResponse: + await lock_user(session, user_id) + state = await session.get(HighlightSyncState, user_id) + if state is None: + state = HighlightSyncState(user_id=user_id, revision=0) + session.add(state) + results = [] + for item in body.items: + row = await session.get(Highlight, (user_id, item.id)) + current = wire(row) if row is not None else None + if current is not None: + # Identity versions are monotonic metadata, independent of LWW. + # Normalize BOTH candidates before comparing so equal-clock + # conflicts converge regardless of which version arrived first. + version = max( + current.document.identity_hash_version, item.document.identity_hash_version + ) + current = current.model_copy( + update={ + "document": current.document.model_copy( + update={"identity_hash_version": version} + ) + } + ) + item = item.model_copy( + update={ + "document": item.document.model_copy(update={"identity_hash_version": version}) + } + ) + winner = item if current is None or conflict_key(item) > conflict_key(current) else current + if row is None or winner != wire(row): + state.revision += 1 + fields = winner.model_dump(exclude={"document"}) | winner.document.model_dump() + if row is None: + row = Highlight(user_id=user_id, revision=state.revision, **fields) + session.add(row) + else: + for name, value in fields.items(): + setattr(row, name, value) + row.revision = state.revision + await session.flush() + status = "accepted" if item == winner else "stale" + results.append(HighlightPushResult(status=status, item=wire(row))) + await session.commit() + return HighlightPushResponse(results=results) + + +@router.get("/highlights", response_model=HighlightPullResponse) +async def pull_highlights( + user_id: Annotated[str, Depends(current_user_id)], + session: Annotated[AsyncSession, Depends(get_session)], + since: Annotated[str, Query(pattern=r"^[0-9]{1,19}$")] = "0", + limit: Annotated[int, Query(ge=1, le=500)] = 500, +) -> HighlightPullResponse: + cursor = int(since) + if cursor > MAX_INT: + raise HTTPException(422, "invalid_cursor") + await lock_user(session, user_id) + state = await session.get(HighlightSyncState, user_id) + if cursor > (state.revision if state else 0): + # A cursor belongs to one server/account. Fail explicitly after a + # restore or account switch rather than silently suppressing changes. + raise HTTPException(409, "highlight_cursor_reset_required") + rows = ( + await session.scalars( + select(Highlight) + .where(Highlight.user_id == user_id, Highlight.revision > cursor) + .order_by(Highlight.revision) + .limit(limit + 1) + ) + ).all() + page = rows[:limit] + response = HighlightPullResponse( + items=[wire(row) for row in page], + next_cursor=str(page[-1].revision if page else cursor), + has_more=len(rows) > limit, + ) + await session.commit() + return response diff --git a/server/quire_server/db/models.py b/server/quire_server/db/models.py index 4591fea0..ea3c0989 100644 --- a/server/quire_server/db/models.py +++ b/server/quire_server/db/models.py @@ -1,5 +1,6 @@ from datetime import date, datetime from decimal import Decimal +from uuid import UUID from sqlalchemy import ( ARRAY, @@ -16,6 +17,7 @@ Numeric, String, UniqueConstraint, + Uuid, func, text, ) @@ -27,6 +29,35 @@ class Base(DeclarativeBase): pass +class HighlightSyncState(Base): + __tablename__ = "highlight_sync_state" + + user_id: Mapped[str] = mapped_column(String, primary_key=True) + revision: Mapped[int] = mapped_column(BigInteger, nullable=False, default=0) + + +class Highlight(Base): + """Private annotations survive removal of the downloaded document.""" + + __tablename__ = "highlights" + __table_args__ = (Index("ix_highlights_user_revision", "user_id", "revision", unique=True),) + + user_id: Mapped[str] = mapped_column(String, primary_key=True) + id: Mapped[UUID] = mapped_column(Uuid, primary_key=True) + metadata_id: Mapped[str | None] = mapped_column(String, nullable=True) + content_hash: Mapped[str] = mapped_column(String, nullable=False) + identity_hash_version: Mapped[int] = mapped_column(Integer, nullable=False, default=1) + locator: Mapped[str] = mapped_column(String, nullable=False) + text: Mapped[str] = mapped_column(String, nullable=False) + colour: Mapped[str] = mapped_column(String, nullable=False) + book_title: Mapped[str] = mapped_column(String, nullable=False) + book_author: Mapped[str | None] = mapped_column(String, nullable=True) + created_at: Mapped[int] = mapped_column(BigInteger, nullable=False) + updated_at: Mapped[int] = mapped_column(BigInteger, nullable=False) + deleted_at: Mapped[int | None] = mapped_column(BigInteger, nullable=True) + revision: Mapped[int] = mapped_column(BigInteger, nullable=False) + + class Document(Base): __tablename__ = "documents" __table_args__ = ( diff --git a/server/quire_server/main.py b/server/quire_server/main.py index 57402371..153d1941 100644 --- a/server/quire_server/main.py +++ b/server/quire_server/main.py @@ -394,10 +394,12 @@ async def closure_fence(request, call_next): # Lazy import: only pull progress + library routers when progress mode # is on. The `library_items` migration lives on the `progress` alembic # branch, so the gate must match for the table to exist. + from quire_server.api.highlights import router as highlights_router from quire_server.api.library import router as library_router from quire_server.api.progress import router as progress_router app.include_router(progress_router, prefix="/sync/v1") + app.include_router(highlights_router, prefix="/sync/v1") app.include_router(library_router, prefix="/library/v1") if settings.ai_enabled: diff --git a/server/tests/fixtures/highlight-sync.json b/server/tests/fixtures/highlight-sync.json new file mode 100644 index 00000000..31e94f8b --- /dev/null +++ b/server/tests/fixtures/highlight-sync.json @@ -0,0 +1,369 @@ +{ + "push": { + "items": [ + { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "PINK", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000200, + "deleted_at": 1700000000200 + } + ] + }, + "expected_results": { + "results": [ + { + "status": "accepted", + "item": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + } + }, + { + "status": "accepted", + "item": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "PINK", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000200, + "deleted_at": 1700000000200 + } + } + ] + }, + "pull": { + "items": [ + { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "PINK", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000200, + "deleted_at": 1700000000200 + } + ], + "next_cursor": "2", + "has_more": false + }, + "normalization_input": { + "metadata_id": " URN:ISBN:978-0-553-29335-7 ", + "content_hash": " abc123 ", + "identity_hash_version": 1 + }, + "conflict_cases": [ + { + "name": "later_clock", + "left": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "right": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000101, + "deleted_at": null + }, + "winner": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000101, + "deleted_at": null + }, + "expected_order": -1 + }, + { + "name": "equal_clock_deletion", + "left": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "right": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": 1700000000100 + }, + "winner": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": 1700000000100 + }, + "expected_order": -1 + }, + { + "name": "equal_clock_colour", + "left": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "GREEN", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "right": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "winner": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "expected_order": -1 + }, + { + "name": "nullable_author", + "left": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "right": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": "", + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "winner": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "“Café ☺ — 读书”", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": "", + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "expected_order": -1 + }, + { + "name": "unicode_codepoints", + "left": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "right": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "😀", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "winner": { + "id": "14300000-0000-4000-8000-000000000001", + "document": { + "metadata_id": "9780553293357", + "content_hash": "abc123", + "identity_hash_version": 1 + }, + "locator": " { \"href\": \"chapter\\\"1.xhtml\", \"text\": {\"highlight\":\"Café \\u263a\"} }\n", + "text": "😀", + "colour": "YELLOW", + "book_title": "Fóundation", + "book_author": null, + "created_at": 1700000000000, + "updated_at": 1700000000100, + "deleted_at": null + }, + "expected_order": -1 + } + ] +} diff --git a/server/tests/integration/conftest.py b/server/tests/integration/conftest.py index 993072aa..85f4656b 100644 --- a/server/tests/integration/conftest.py +++ b/server/tests/integration/conftest.py @@ -44,6 +44,7 @@ async def _truncate_ai_tables_between_tests(request, engine: AsyncEngine): "external_source_cache, ai_usage_daily, " "insight_identity_aliases, library_items, " "reader_profiles, progress, documents, " + "highlights, highlight_sync_state, " # Phase 0, task S-1: NativeAuth tables. Listed even when the # test fixture defaults to CalibreWeb backend so opt-in # NativeAuth tests start from a clean slate. diff --git a/server/tests/integration/test_admin_status.py b/server/tests/integration/test_admin_status.py index 6f77bfd1..ae89eca6 100644 --- a/server/tests/integration/test_admin_status.py +++ b/server/tests/integration/test_admin_status.py @@ -132,7 +132,7 @@ async def test_status_reports_what_the_server_runs_with(admin_client, alice): assert isinstance(body["modes"], list) assert isinstance(body["warnings"], list) assert body["migrations"]["missing"] == [] - assert body["migrations"]["applied"] == ["ai_009", "auth_003", "progress_003"] + assert body["migrations"]["applied"] == ["ai_009", "auth_003", "progress_004"] rows = {row["name"]: row for row in body["settings"]} assert rows["QUIRE_SERVER_ADMIN_USERS"] == { "name": "QUIRE_SERVER_ADMIN_USERS", diff --git a/server/tests/integration/test_browser_identity.py b/server/tests/integration/test_browser_identity.py index 6037b4fa..89db37ea 100644 --- a/server/tests/integration/test_browser_identity.py +++ b/server/tests/integration/test_browser_identity.py @@ -1,5 +1,7 @@ import asyncio +import json import time +from pathlib import Path from urllib.parse import parse_qs, urlsplit import httpx @@ -8,7 +10,7 @@ from cryptography.hazmat.primitives.asymmetric import rsa from sqlalchemy import select, text -from quire_server.db.models import LibraryItem, NativeUser +from quire_server.db.models import Highlight, HighlightSyncState, LibraryItem, NativeUser ORIGIN = "https://reader.example.test" SERVICE = "synthetic-account-service-token-00000000" @@ -251,10 +253,33 @@ async def test_export_close_purge_isolation_and_shared_cache( assert ( await client.post("/ai/v1/insights/lookup", headers=auth, json=payload) ).status_code == 200 + highlights = json.loads( + (Path(__file__).parents[1] / "fixtures/highlight-sync.json").read_text() + )["push"] + assert ( + await client.post("/sync/v1/highlights", headers=auth, json=highlights) + ).status_code == 200 + bob_token = ( + await client.post( + "/auth/v1/login", + json={"email": "synthetic-bob@example.test", "password": "synthetic-password"}, + ) + ).json()["token"] + assert ( + await client.post( + "/sync/v1/highlights", + headers={"Authorization": "Bearer " + bob_token}, + json=highlights, + ) + ).status_code == 200 exported_response = await client.get("/auth/v1/account/export", headers=auth) assert exported_response.headers["Cache-Control"] == "no-store" exported = exported_response.json() assert [r["title"] for r in exported["library_items"]] == ["Alice private"] + assert len(exported["highlights"]) == 1 + assert exported["highlights"][0]["user_id"] == account + assert exported["highlights"][0]["deleted_at"] is not None + assert exported["highlight_sync_state"] == [{"user_id": account, "revision": 2}] assert "password_hash" not in str(exported) and "token_hash" not in str(exported) path = "/internal/v1/accounts/" + account assert (await client.post(path + "/close")).status_code == 403 @@ -267,12 +292,21 @@ async def test_export_close_purge_isolation_and_shared_cache( assert ( await session.execute(select(LibraryItem).where(LibraryItem.user_id == f"native:{bob}")) ).scalar_one().title == "Bob private" - bob_token = ( - await client.post( - "/auth/v1/login", - json={"email": "synthetic-bob@example.test", "password": "synthetic-password"}, + assert await session.get(HighlightSyncState, account) is None + assert ( + await session.scalars(select(Highlight).where(Highlight.user_id == account)) + ).all() == [] + assert ( + len( + ( + await session.scalars( + select(Highlight).where(Highlight.user_id == f"native:{bob}") + ) + ).all() ) - ).json()["token"] + == 1 + ) + assert ( await client.post( "/ai/v1/insights/get", diff --git a/server/tests/integration/test_health.py b/server/tests/integration/test_health.py index 793a166f..9872206d 100644 --- a/server/tests/integration/test_health.py +++ b/server/tests/integration/test_health.py @@ -39,7 +39,7 @@ async def test_readyz_returns_200_when_db_reachable_and_heads_applied(app_under_ # `scripts/migrate.py` and `health._required_heads`) # * progress_003 (F-1, added `identity_hash_version` to # `documents` + `library_items`) - assert body["heads_applied"] == ["ai_009", "auth_003", "progress_003"] + assert body["heads_applied"] == ["ai_009", "auth_003", "progress_004"] async def test_old_sync_health_path_is_404(app_under_test): diff --git a/server/tests/integration/test_highlights.py b/server/tests/integration/test_highlights.py new file mode 100644 index 00000000..0771bb6a --- /dev/null +++ b/server/tests/integration/test_highlights.py @@ -0,0 +1,232 @@ +"""Real PostgreSQL regressions for the private highlight sync contract.""" + +import asyncio +import copy +import json +from pathlib import Path +from uuid import uuid4 + +import pytest +from httpx import ASGITransport, AsyncClient +from sqlalchemy import delete, select +from sqlalchemy.ext.asyncio import async_sessionmaker + +from quire_server.api.highlights import HighlightItem, conflict_key, lock_user +from quire_server.db.models import Document, Highlight, HighlightSyncState + +pytestmark = pytest.mark.requires_progress +FIXTURE = json.loads((Path(__file__).parents[1] / "fixtures/highlight-sync.json").read_text()) +PATH = "/sync/v1/highlights" + + +@pytest.mark.parametrize("case", FIXTURE["conflict_cases"], ids=lambda case: case["name"]) +def test_shared_conflict_order(case): + left, right = HighlightItem(**case["left"]), HighlightItem(**case["right"]) + assert max([left, right], key=conflict_key).model_dump(mode="json") == case["winner"] + assert max([right, left], key=conflict_key).model_dump(mode="json") == case["winner"] + + +@pytest.fixture +async def client(app_under_test, session): + async with AsyncClient( + transport=ASGITransport(app=app_under_test), base_url="http://test" + ) as c: + yield c + + +def headers(user="alice"): + import base64 + + return {"Authorization": "Basic " + base64.b64encode(f"{user}:{user}pass".encode()).decode()} + + +def item(**updates): + return copy.deepcopy(FIXTURE["push"]["items"][0]) | updates + + +async def push(client, rows, user="alice"): + response = await client.post(PATH, headers=headers(user), json={"items": rows}) + assert response.status_code == 200, response.text + return response.json()["results"] + + +async def pull(client, since="0", limit=500, user="alice"): + response = await client.get( + PATH, headers=headers(user), params={"since": since, "limit": limit} + ) + assert response.status_code == 200, response.text + return response.json() + + +async def test_shared_fixture_opaque_locator_normalization_and_replay(client): + response = await client.post(PATH, headers=headers(), json=FIXTURE["push"]) + assert response.status_code == 200, response.text + assert response.json() == FIXTURE["expected_results"] + assert await pull(client) == FIXTURE["pull"] + tombstone = FIXTURE["pull"]["items"][0] + assert (await push(client, [tombstone]))[0]["status"] == "accepted" + assert await pull(client, "2") == {"items": [], "next_cursor": "2", "has_more": False} + normalized = item(id=str(uuid4()), document=FIXTURE["normalization_input"]) + winner = (await push(client, [normalized]))[0]["item"] + assert winner["document"] == FIXTURE["push"]["items"][0]["document"] + assert winner["locator"] == normalized["locator"] + + +async def test_auth_and_same_uuid_tenant_isolation(client, session): + assert (await client.get(PATH)).status_code == 401 + assert (await client.post(PATH, json=FIXTURE["push"])).status_code == 401 + assert (await client.get(PATH, headers=headers("wrong"))).status_code == 401 + alice, bob = item(), item(text="Bob private quote") + await push(client, [alice]) + assert (await pull(client, user="bob"))["items"] == [] + await push(client, [bob], user="bob") + assert (await pull(client))["items"] == [alice] + assert (await pull(client, user="bob"))["items"] == [bob] + assert len((await session.scalars(select(Highlight))).all()) == 2 + + +async def test_equal_timestamp_conflicts_converge_and_tombstone_blocks_replay(client): + # Exercise every distinguishing field, including supplementary Unicode + # versus a BMP character (different UTF-16 and UTF-8 lexical order). + low = item(text="\ue000") + high = item(text="😀") + assert conflict_key(HighlightItem(**high)) > conflict_key(HighlightItem(**low)) + for order in ([low, high], [high, low]): + ident = str(uuid4()) + await push(client, [dict(row, id=ident) for row in order]) + winner = (await push(client, [dict(low, id=ident)]))[0] + assert winner == {"status": "stale", "item": dict(high, id=ident)} + live = item(id=str(uuid4()), book_author="") + tombstone = dict(live, deleted_at=live["updated_at"]) + await push(client, [tombstone]) + assert (await push(client, [live]))[0] == {"status": "stale", "item": tombstone} + older = dict(live, updated_at=live["updated_at"] - 1) + assert (await push(client, [older]))[0]["item"] == tombstone + + +async def test_identity_version_never_downgrades_even_with_newer_payload(client): + original = item() + original["document"]["identity_hash_version"] = 2 + await push(client, [original]) + edit = item(colour="PINK", updated_at=original["updated_at"] + 1) + expected = copy.deepcopy(edit) + expected["document"]["identity_hash_version"] = 2 + assert (await push(client, [edit]))[0] == {"status": "accepted", "item": expected} + # A stale payload can still teach us a newer algorithm version; it must + # produce a delta without reverting the winning quote, colour or clock. + stale = item() + stale["document"]["identity_hash_version"] = 3 + expected["document"]["identity_hash_version"] = 3 + assert (await push(client, [stale]))[0] == {"status": "stale", "item": expected} + assert (await pull(client, "2"))["items"] == [expected] + + +async def test_equal_clock_version_upgrade_does_not_make_conflicts_order_dependent(client): + higher_version = item(colour="GREEN") + higher_version["document"]["identity_hash_version"] = 2 + higher_colour = item(colour="YELLOW") + for rows in ([higher_version, higher_colour], [higher_colour, higher_version]): + ident = str(uuid4()) + await push(client, [dict(row, id=ident) for row in rows]) + winner = (await push(client, [dict(higher_colour, id=ident)]))[0]["item"] + assert winner["colour"] == "YELLOW" + assert winner["document"]["identity_hash_version"] == 2 + + +async def test_delta_pages_offline_timestamps_mutations_and_download_removal(client, session): + rows = [item(id=str(uuid4()), updated_at=1700000000100) for _ in range(3)] + await push(client, rows) + first = await pull(client, limit=1) + assert first["items"] == rows[:1] and first["has_more"] + # A row already delivered mutates after page 1; its new revision must + # appear again after the other rows even though the client clock is old. + changed = dict(rows[0], colour="PINK", updated_at=rows[0]["updated_at"] + 1) + await push(client, [changed]) + seen = [] + cursor = first["next_cursor"] + while True: + page = await pull(client, cursor, limit=1) + seen.extend(page["items"]) + cursor = page["next_cursor"] + if not page["has_more"]: + break + assert seen == rows[1:] + [changed] + assert cursor == "4" + offline = item(id=str(uuid4()), created_at=1, updated_at=2) + await push(client, [offline]) + assert (await pull(client, cursor))["items"] == [offline] + await session.execute(delete(Document).where(Document.user_id == "alice")) + await session.commit() + assert len((await pull(client))["items"]) == 4 + bad = await client.get(PATH, headers=headers(), params={"since": "999"}) + assert bad.status_code == 409 + assert bad.json()["detail"] == "highlight_cursor_reset_required" + + +async def test_transaction_lock_prevents_uncommitted_revision_holes(client, engine): + factory = async_sessionmaker(engine, expire_on_commit=False) + first = item() + async with factory() as pending: + await lock_user(pending, "alice") + state = HighlightSyncState(user_id="alice", revision=1) + row = Highlight( + user_id="alice", + revision=1, + **HighlightItem(**first).model_dump(exclude={"document"}), + **first["document"], + ) + pending.add_all([state, row]) + await pending.flush() # revision allocated but transaction not yet committed + reading = asyncio.create_task(pull(client, limit=1)) + writing = asyncio.create_task(push(client, [item(id=str(uuid4()), text="Second")])) + for task in (reading, writing): + with pytest.raises(TimeoutError): + await asyncio.wait_for(asyncio.shield(task), 0.1) + await pending.commit() + page, _ = await asyncio.gather(reading, writing) + assert page["items"] == [first] + assert page["next_cursor"] == "1" + assert (await pull(client, "1"))["items"][0]["text"] == "Second" + # Rollback cannot leave an allocated revision visible to a pull. + async with factory() as pending: + await lock_user(pending, "alice") + state = await pending.get(HighlightSyncState, "alice") + state.revision += 1 + await pending.flush() + await pending.rollback() + await push(client, [item(id=str(uuid4()), text="Third")]) + assert (await pull(client, "2"))["next_cursor"] == "3" + + +async def test_concurrent_same_uuid_writes_have_one_deterministic_winner(client): + rows = [item(colour=c) for c in ["YELLOW", "PINK", "GREEN", "YELLOW"]] + await asyncio.gather(*(push(client, [row]) for row in rows)) + assert (await pull(client))["items"] == [ + max(rows, key=lambda r: conflict_key(HighlightItem(**r))) + ] + + +@pytest.mark.parametrize( + "change", + [ + {"id": "not-a-uuid"}, + {"locator": " "}, + {"text": ""}, + {"colour": "RED"}, + {"created_at": -1}, + {"updated_at": 1}, + {"deleted_at": 2**63 - 1}, + {"document": {"content_hash": " "}}, + ], +) +async def test_invalid_highlight_rejects_entire_batch(client, change): + response = await client.post(PATH, headers=headers(), json={"items": [item(), item(**change)]}) + assert response.status_code == 422 + assert (await pull(client))["items"] == [] + + +@pytest.mark.parametrize( + "params", [{"since": "-1"}, {"since": "x"}, {"since": str(2**63)}, {"limit": 0}, {"limit": 501}] +) +async def test_invalid_cursor_and_page_size(client, params): + assert (await client.get(PATH, headers=headers(), params=params)).status_code == 422 diff --git a/server/tests/integration/test_migrate_script.py b/server/tests/integration/test_migrate_script.py index 0ef33453..5d86cb21 100644 --- a/server/tests/integration/test_migrate_script.py +++ b/server/tests/integration/test_migrate_script.py @@ -71,7 +71,7 @@ async def test_default_state_upgrades_backbone_then_ai_branch(postgres_url: str) # * auth_003 — S-1 added native_users + native_sessions (always-on # `auth` branch, materialized regardless of mode flags) # * progress_003 — F-1 added the column to `documents` + `library_items` - assert versions == {"ai_009", "auth_003", "progress_003"} + assert versions == {"ai_009", "auth_003", "progress_004"} async def test_idempotent_second_run(postgres_url: str): @@ -84,7 +84,7 @@ async def test_idempotent_second_run(postgres_url: str): eng = create_async_engine(postgres_url, future=True) versions = await _alembic_versions(eng) await eng.dispose() - assert versions == {"ai_009", "auth_003", "progress_003"} + assert versions == {"ai_009", "auth_003", "progress_004"} async def test_synthetic_ai_branch_upgrades_when_enabled(postgres_url: str, tmp_path: Path): @@ -196,7 +196,7 @@ def downgrade() -> None: assert "ai_001" not in versions # progress branch advanced (progress_enabled=True). The backbone itself # is no longer a head once progress_003 sits on top of 0004. - assert "progress_003" in versions + assert "progress_004" in versions # The always-on `auth` branch advances regardless of mode flags. assert "auth_003" in versions diff --git a/server/tests/integration/test_modes.py b/server/tests/integration/test_modes.py index b7520e76..7b0b9c5a 100644 --- a/server/tests/integration/test_modes.py +++ b/server/tests/integration/test_modes.py @@ -50,6 +50,8 @@ async def test_full_mode_mounts_everything(monkeypatch, postgres_url: str, alemb stats = await c.get("/library/v1/stats", headers=_basic_header("alice")) assert stats.status_code != 404 + assert (await c.get("/sync/v1/highlights")).status_code != 404 + # AI router mounted: any response other than 404 confirms routing. ai = await c.get("/ai/v1/config", headers=_basic_header("alice")) assert ai.status_code != 404 @@ -62,6 +64,8 @@ async def test_sync_only_mode_excludes_ai(monkeypatch, postgres_url: str, alembi assert h.status_code == 200 assert h.json()["modes"] == ["progress"] + assert (await c.get("/sync/v1/highlights")).status_code != 404 + # AI namespace entirely unmounted. ai = await c.get("/ai/v1/config", headers=_basic_header("alice")) assert ai.status_code == 404 @@ -78,6 +82,8 @@ async def test_ai_only_mode_excludes_progress(monkeypatch, postgres_url: str, al prog = await c.get("/sync/v1/progress", headers=_basic_header("alice")) assert prog.status_code == 404 + assert (await c.get("/sync/v1/highlights")).status_code == 404 + # Library namespace unmounted in AI-only mode. lib = await c.get("/library/v1/items", headers=_basic_header("alice")) assert lib.status_code == 404 @@ -97,6 +103,7 @@ async def test_neither_mode_only_mounts_health(monkeypatch, postgres_url: str, a assert r.status_code == 200 # All mode-gated namespaces unmounted. + assert (await c.get("/sync/v1/highlights")).status_code == 404 prog = await c.get("/sync/v1/progress", headers=_basic_header("alice")) assert prog.status_code == 404 lib = await c.get("/library/v1/items", headers=_basic_header("alice")) diff --git a/server/tests/integration/test_readyz_migration_state.py b/server/tests/integration/test_readyz_migration_state.py index a9157608..4f26db72 100644 --- a/server/tests/integration/test_readyz_migration_state.py +++ b/server/tests/integration/test_readyz_migration_state.py @@ -82,7 +82,7 @@ async def test_readyz_200_when_at_ai_head(monkeypatch, postgres_url, alembic_upg assert body["ready"] is True # Phase 0, task S-1 added the always-materialized `auth` branch: # `auth_003` joins `ai_009` / `progress_003` in `heads_applied`. - assert body["heads_applied"] == ["ai_009", "auth_003", "progress_003"] + assert body["heads_applied"] == ["ai_009", "auth_003", "progress_004"] async def test_readyz_503_when_db_below_backbone(