ADFA-5220: Correct the version table to a single row, not an append-only log

app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt

@@ -77,6 +77,10 @@ class WebServerTest {
 					every { moveToFirst() } returns true
 					every { isNull(0) } returns false
 					every { getInt(0) } returns major
+					// The row count the query carries. Left unstubbed, a relaxed mock answers 0 -- a
+					// state the production code has just excluded by getting a row back at all, so
+					// these tests would be exercising something that cannot happen.
+					every { getInt(1) } returns 1
 				}
 		}
 	}

common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt

@@ -86,24 +86,46 @@ class DatabaseVersionResolverTest {
 		assertEquals(2, DatabaseVersionResolver.resolveMajorVersion(db))
 	}
 
-	// The table is an append-only log, so the row inserted last is the current version...
+	// Both directions, deliberately. Merging these into the downgrade case alone left a suite that
+	// MIN(major) would also have passed -- every expectation happened to be the lowest major present
+	// -- so nothing pinned the ordering the whole design rests on.
 	@Test
-	fun majorVersionIsTheLastRowInserted() {
+	fun majorVersionIsTheRowWrittenLast_whenTheLastRowIsHigher() {
 		createVersionTable()
 		insertVersion(2, 0, 0)
 		insertVersion(3, 1, 4)
 		assertEquals(3, DatabaseVersionResolver.resolveMajorVersion(db))
 	}
 
-	// ...including when that row is a downgrade, which MAX(major) would read as still current.
+	// ...and when it is lower, which MAX(major) would get wrong: a rebuild from an older content set
+	// is a downgrade and has to read as one.
 	@Test
-	fun majorVersionFollowsADowngrade() {
+	fun majorVersionIsTheRowWrittenLast_whenTheLastRowIsADowngrade() {
 		createVersionTable()
 		insertVersion(3, 0, 0)
 		insertVersion(2, 0, 0)
 		assertEquals(2, DatabaseVersionResolver.resolveMajorVersion(db))
 	}
 
+	// Malformed twice over: several rows, and the last one has no major. The shipped DDL forbids that
+	// -- which is the point, since this reader defends against files another producer wrote -- so the
+	// table is created here without the NOT NULL. The count is read before the NULL check, so a file
+	// like this still warns instead of being reported as having no version table at all.
+	@Test
+	fun majorVersionIsNull_whenTheLastRowHasNoMajor() {
+		db.execSQL(
+			"CREATE TABLE DocumentationDatabaseVersion (" +
+				"major INT, minor INT, patch INT, who TEXT, comment TEXT, changeTime TIMESTAMP)",
+		)
+		insertVersion(2, 0, 0)
+		db.execSQL(
+			"INSERT INTO DocumentationDatabaseVersion (major, minor, patch, who, comment) VALUES (?, ?, ?, ?, ?)",
+			arrayOf<Any?>(null, 0, 0, "test", "test"),
+		)
+
+		assertNull(DatabaseVersionResolver.resolveMajorVersion(db))
+	}
+
 	@Test
 	fun returnsWholedbRow_whenPresent() {
 		createTable()

common/src/main/java/com/itsaky/androidide/utils/DatabaseVersionResolver.kt

@@ -1,12 +1,12 @@
 package com.itsaky.androidide.utils
 
 import android.database.sqlite.SQLiteDatabase
-import android.util.Log
+import org.slf4j.LoggerFactory
 
 object DatabaseVersionResolver {
 	const val VERSION_UNKNOWN = "Version Unknown"
 
-	private const val TAG = "DatabaseVersionResolver"
+	private val log = LoggerFactory.getLogger(DatabaseVersionResolver::class.java)
 
 	private const val QUERY_WHOLEDB = """
 		SELECT changeTime, who
@@ -27,13 +27,14 @@ object DatabaseVersionResolver {
 		WHERE  type = 'table' AND name = 'DocumentationDatabaseVersion'
 	"""
 
-	// The table is an append-only log -- ADFA-5220 records each change as another INSERT -- so the
-	// current version is the row inserted last, not the highest one ever recorded: rebuilding from
-	// an older content set is a downgrade and has to read as one.
+	// One row by contract (ADFA-5220); the ORDER BY is the defence for a file that breaks it, and
+	// MAX(major) is the tempting wrong answer -- a rebuild from an older content set has to read as
+	// the downgrade it is. The count rides along so the breach can be reported rather than papered
+	// over.
 	private const val QUERY_MAJOR_VERSION = """
-		SELECT major
+		SELECT major, (SELECT COUNT(*) FROM DocumentationDatabaseVersion)
 		FROM   DocumentationDatabaseVersion
-		ORDER BY rowid DESC
+		ORDER BY changeTime DESC, rowid DESC
 		LIMIT  1
 	"""
 
@@ -63,26 +64,28 @@ object DatabaseVersionResolver {
 							who = c.getString(2),
 							documentationSet = c.getString(1),
 						)
-					Log.e(
-						TAG,
-						"Missing 'wholedb' record in LastChange table; falling back to $result",
-					)
+					log.error("Missing 'wholedb' record in LastChange table; falling back to {}", result)
 					return result
 				}
 			}
 
-			Log.e(TAG, "No versioning information available")
+			log.error("No versioning information available")
 			VERSION_UNKNOWN
 		} catch (e: Exception) {
-			Log.e(TAG, "No versioning information available", e)
+			log.error("No versioning information available", e)
 			VERSION_UNKNOWN
 		}
 	}
 
 	/**
 	 * The MAJOR version [db] declares in `DocumentationDatabaseVersion` (ADFA-5220), or null when
-	 * that table is absent or empty -- which is how every database built before it existed
-	 * identifies itself.
+	 * that table is absent, empty, or holds a NULL major -- the first of which is how every database
+	 * built before it existed identifies itself.
+	 *
+	 * The table is contractually a single row. A file carrying several is accepted rather than
+	 * rejected -- the row with the highest `rowid` wins, so the answer stays deterministic and a
+	 * downgrade still reads as one -- and logs a warning, since this reader cannot repair the file
+	 * and refusing to serve documentation over it would be a worse outcome than serving it.
 	 *
 	 * Deliberately does *not* catch exceptions, unlike [resolveDatabaseVersion]: callers cache the
 	 * answer for the lifetime of a database (see `WebServer.loadCompressionDictionary`), so a
@@ -95,7 +98,34 @@ object DatabaseVersionResolver {
 			return null
 		}
 		return db.rawQuery(QUERY_MAJOR_VERSION, arrayOf()).use { cursor ->
-			if (cursor.moveToFirst() && !cursor.isNull(0)) cursor.getInt(0) else null
+			if (!cursor.moveToFirst()) {
+				return@use null
+			}
+			// Counted before the NULL check, not after: a file that is both multi-row *and* ends in a
+			// NULL major would otherwise return null with nothing logged -- the most malformed case
+			// there is, reported as if the table simply did not exist.
+			val rows = cursor.getInt(1)
+			if (rows > 1) {
+				log.warn(
+					"DocumentationDatabaseVersion holds {} rows; it is meant to hold one. Using the row written " +
+						"last; the database was built by something that appended instead of replacing.",
+					rows,
+				)
+			}
+			if (cursor.isNull(0)) {
+				// Logged, because the caller cannot tell this apart from the answer it gets for a
+				// database predating the table: both are null, and WebServer reports "version none" and
+				// skips the dictionary either way. For a real pre-ADFA-5220 file that is correct; for
+				// this one it silently disables dictionary decoding on content that needs it, which is
+				// the worse of the two contract breaches this reader defends against.
+				log.warn(
+					"DocumentationDatabaseVersion's newest row has a NULL major; treating the database as " +
+						"declaring no version, which disables dictionary decoding.",
+				)
+				null
+			} else {
+				cursor.getInt(0)
+			}
 		}
 	}
 
@@ -108,6 +138,9 @@ object DatabaseVersionResolver {
 		if (!changeTime.isNullOrBlank()) parts += changeTime
 		if (!documentationSet.isNullOrBlank()) parts += "($documentationSet)"
 		if (!who.isNullOrBlank()) parts += who
-		return parts.joinToString(separator = " ")
+		// ifEmpty: a row whose changeTime, set and who are all null or blank produced "", which callers
+		// then stored and logged as a stamp ("Database last change: ."). Nothing usable is the same
+		// answer as no row at all.
+		return parts.joinToString(separator = " ").ifEmpty { VERSION_UNKNOWN }
 	}
 }

common/src/test/java/com/itsaky/androidide/utils/DatabaseVersionResolverBranchTest.kt

@@ -0,0 +1,104 @@
+/*
+ *  This file is part of AndroidIDE.
+ *
+ *  AndroidIDE is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  AndroidIDE is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *   along with AndroidIDE.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package com.itsaky.androidide.utils
+
+import android.database.Cursor
+import android.database.sqlite.SQLiteDatabase
+import com.google.common.truth.Truth.assertThat
+import io.mockk.every
+import io.mockk.mockk
+import io.mockk.unmockkAll
+import org.junit.After
+import org.junit.Test
+
+/**
+ * The branch logic of [DatabaseVersionResolver], as JVM tests that actually run.
+ *
+ * The existing coverage lives in `common/src/androidTest`, which no workflow executes -- CI only
+ * assembles `:app:assembleV8DebugAndroidTest` and runs two named app classes on Test Lab -- so the
+ * `rows > 1` warning and the NULL-major path had no evidence behind them beyond a manual logcat
+ * read. These pin the decisions the resolver makes about a malformed table; the SQL ordering itself
+ * still belongs in the instrumented file, against a real SQLite.
+ */
+class DatabaseVersionResolverBranchTest {
+	@After
+	fun tearDown() {
+		unmockkAll()
+	}
+
+	private fun database(
+		major: Int?,
+		rows: Int,
+		tableExists: Boolean = true,
+	): SQLiteDatabase {
+		val existsCursor = mockk<Cursor>(relaxed = true) { every { moveToFirst() } returns tableExists }
+		val versionCursor =
+			mockk<Cursor>(relaxed = true) {
+				every { moveToFirst() } returns true
+				every { isNull(0) } returns (major == null)
+				every { getInt(0) } returns (major ?: 0)
+				every { getInt(1) } returns rows
+			}
+		return mockk(relaxed = true) {
+			every { rawQuery(match { it.contains("sqlite_master") }, any()) } returns existsCursor
+			every {
+				rawQuery(
+					match { it.contains("DocumentationDatabaseVersion") && !it.contains("sqlite_master") },
+					any(),
+				)
+			} returns versionCursor
+		}
+	}
+
+	@Test
+	fun `the newest row wins, and several rows are still answered`() {
+		assertThat(DatabaseVersionResolver.resolveMajorVersion(database(major = 2, rows = 3))).isEqualTo(2)
+	}
+
+	// A NULL major is indistinguishable to the caller from "no version table": both are null, and
+	// WebServer reports "version none" and skips the dictionary either way. For a genuinely old
+	// database that is right; for this one it disables dictionary decoding on content that needs it.
+	@Test
+	fun `a NULL major reads as no declared version`() {
+		assertThat(DatabaseVersionResolver.resolveMajorVersion(database(major = null, rows = 1))).isNull()
+	}
+
+	@Test
+	fun `a NULL major in a multi-row table still reads as no declared version`() {
+		assertThat(DatabaseVersionResolver.resolveMajorVersion(database(major = null, rows = 4))).isNull()
+	}
+
+	@Test
+	fun `an absent table reads as no declared version`() {
+		assertThat(
+			DatabaseVersionResolver.resolveMajorVersion(database(major = 2, rows = 1, tableExists = false)),
+		).isNull()
+	}
+
+	// The ordering rule is a cross-repo contract -- docdb-studio reads the same table the same way --
+	// so the column it orders by is worth pinning even from this side.
+	@Test
+	fun `the newest row is chosen by change time, not by rowid alone`() {
+		val db = database(major = 2, rows = 2)
+		DatabaseVersionResolver.resolveMajorVersion(db)
+
+		io.mockk.verify {
+			db.rawQuery(match { it.contains("ORDER BY changeTime DESC") && it.contains("rowid DESC") }, any())
+		}
+	}
+}

docs/documentation-database.md

@@ -62,7 +62,7 @@ CREATE TABLE Tooltips (
 
 ### Supporting tables
 
-- **`DocumentationDatabaseVersion(major, minor, patch, who, comment, changeTime)`** — the database's own semver (ADFA-5220), replacing the heuristics that used to infer the format from which tables happened to exist. Append-only: each change is another `INSERT`, so the **row inserted last** is the current version, not the highest one ever recorded — a rebuild from an older content set is a downgrade and has to read as one (`DatabaseVersionResolver.resolveMajorVersion`, which returns null for a database predating the table). `MAJOR >= 2` is what tells the app its brotli `Content` rows are dictionary-compressed; below that, `WebServer` neither reads nor attaches `CompressionDictionary`. Gating on the declared version rather than on the table's presence matters in both directions: a database can carry the dictionary table while its content is still plain brotli (every row would then pay a failed dictionary decode before its plain one, on every request), and a migrated database that lost the table fails loudly instead of quietly.
+- **`DocumentationDatabaseVersion(major, minor, patch, who, comment, changeTime)`** — the database's own semver (ADFA-5220), replacing the heuristics that used to infer the format from which tables happened to exist. **Exactly one row**: the version the file *is*, which `populate_db.py` replaces rather than appends. Nothing in the DDL enforces that, so both readers (`DatabaseVersionResolver.resolveMajorVersion` here, `database_major_version` in docdb-studio) take the row with the highest `rowid` rather than the highest `major` — a rebuild from an older content set is a downgrade and has to read as one, which `MAX(major)` would get wrong. The app's reader additionally logs a warning when it finds more than one row; docdb-studio's does not. `populate_db.py` collapses a file that somehow accumulated several back to one, and `resolveMajorVersion` returns null for a database predating the table. `MAJOR >= 2` is what tells the app its brotli `Content` rows are dictionary-compressed; below that, `WebServer` neither reads nor attaches `CompressionDictionary`. Gating on the declared version rather than on the table's presence matters in both directions: a database can carry the dictionary table while its content is still plain brotli (every row would then pay a failed dictionary decode before its plain one, on every request), and a migrated database that lost the table fails loudly instead of quietly.
 - **`CompressionDictionary(id, data)`** — single-row table (`id INTEGER PRIMARY KEY CHECK (id = 1)`) holding the raw Brotli dictionary every ADFA-5153-migrated `compression = 'brotli'` `Content` row is compressed against. Trained once, from a representative sample across the whole `Content` table, by `OfflineDocumentationTools`' `migrate_content_to_dictionary_brotli.py` / `populate_db.py` (never retrained after that — a dictionary-compressed row is only decodable against the exact dictionary it was compressed with, so replacing it would silently orphan every already-migrated row). Shipping the dictionary inside `documentation.db` itself, rather than as a separate bundled asset, keeps it version-locked to the content compressed against it. `WebServer` loads it lazily -- not merely from starting the server or swapping databases, but on the first content fetch that needs it after `database` changes, and only when `DocumentationDatabaseVersion` declares `MAJOR >= 2` (see above) -- and caches it from then on, reloading again only on the next database change (a swap can bring in a database with a different dictionary or none, so it can't stay cached across one). Per row, it tries decoding with the dictionary attached first via brotli4j's `attachDictionary`, falling back to a plain decode on failure — needed both for a database predating this migration (no `CompressionDictionary` table at all) and for plugin-contributed rows within an otherwise-migrated database (see `PluginDocumentationManager` below).
 - **`Templates(id, name, content)`** — Pebble template source, keyed by id (and by `name` for well-known templates like `bookshelf`). Referenced by `Content.templateId`.
 - **`Bookshelf(contentID, bookCategoryID, title, description)`** / **`BookCategories(id, category, description)`** — the Dynamic Bookshelf: one row per "book" (PDF or similar), linked to its Tier 3 page via `contentID` -> `Content.id`. Two DB triggers keep `Bookshelf` in sync when a PDF row is inserted/deleted from `Content`; `title`/`description` don't come from those triggers and must be set by hand. Non-PDF books need a separate ingestion path (plugin-provided, e.g. via `PluginDocumentationManager`).