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

@@ -67,6 +67,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
 			}
 	}
 
@@ -435,7 +439,10 @@ class WebServerTest {
 		}
 	}
 
-	// Same as sendRawGetRequestAndAwaitClose, but hands back what the server actually wrote.
+	// Sends a bare GET over a raw socket and hands back everything the server wrote, reading
+	// until the server closes the connection (every response sends "Connection: close").
+	// Plaintext HTTP is intentional and stays on this machine: WebServer is a loopback-only
+	// plaintext server, and these tests exercise it as shipped.
 	private fun sendRawGetRequest(
 		port: Int,
 		path: String,
@@ -450,22 +457,15 @@ class WebServerTest {
 			socket.getInputStream().readBytes().toString(Charsets.ISO_8859_1)
 		}
 
-	// Blocks until the server closes the connection (every response sends "Connection: close"),
-	// so by the time this returns the server has fully finished processing this one request --
-	// making repeated calls a reliable way to serialize several full request/response cycles.
+	// Discards the response; because sendRawGetRequest reads until the server closes the
+	// connection, by the time this returns the server has fully finished processing this one
+	// request -- making repeated calls a reliable way to serialize several full request/response
+	// cycles.
 	private fun sendRawGetRequestAndAwaitClose(
 		port: Int,
 		path: String,
 	) {
-		Socket().use { socket ->
-			socket.connect(InetSocketAddress("localhost", port), 2_000)
-			socket.soTimeout = 2_000
-			socket.getOutputStream().apply {
-				write("GET $path HTTP/1.1\r\n\r\n".toByteArray(Charsets.ISO_8859_1))
-				flush()
-			}
-			socket.getInputStream().readBytes()
-		}
+		sendRawGetRequest(port, path)
 	}
 
 	// Polls by attempting an actual TCP connect rather than sleeping a fixed

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
 	"""
 
@@ -44,6 +45,11 @@ object DatabaseVersionResolver {
 		LIMIT  1
 	"""
 
+	/**
+	 * Resolves the database version from the available change history.
+	 *
+	 * @return The formatted database version, or `VERSION_UNKNOWN` when version information is unavailable or an error occurs.
+	 */
 	fun resolveDatabaseVersion(db: SQLiteDatabase): String {
 		return try {
 			db.rawQuery(QUERY_WHOLEDB, arrayOf()).use { c ->
@@ -63,26 +69,29 @@ 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 greatest `changeTime` wins, `rowid` breaking ties, 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,10 +104,45 @@ 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)
+			}
 		}
 	}
 
+	/**
+	 * Formats database change metadata into a readable version string.
+	 *
+	 * @param changeTime The recorded change timestamp.
+	 * @param who The person or process associated with the change.
+	 * @param documentationSet The documentation set associated with the change.
+	 * @return The combined version details, or [VERSION_UNKNOWN] when no details are available.
+	 */
 	private fun formatVersion(
 		changeTime: String?,
 		who: String?,
@@ -108,6 +152,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())
+		}
+	}
+}