Skip to content

Commit e866d10

Browse files
davidschachterADFAclaudecoderabbitai[bot]
authored
ADFA-5220: Correct the version table to a single row, not an append-only log (#1729)
* ADFA-5220: Correct the version table to a single row, not an append-only log DocumentationDatabaseVersion holds exactly one row -- the format version the database *is*, not a history of what it has been. The comments and the doc bullet described an append-only log, which was my reading of the ticket's INSERT-based update example and is wrong. resolveMajorVersion keeps ORDER BY rowid DESC, now stated as a defence rather than a model: a file that breaks the one-row contract still reads deterministically, and a downgrade still reads as a downgrade where MAX(major) would report the highest version ever declared. The two tests that asserted last-row-wins across several rows collapse into one that says what that ordering is actually for. The writer side is OfflineDocumentationTools#29. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ADFA-5220: Report a version table that holds more than one row The reader tolerates several rows on purpose -- ordering by rowid keeps the answer deterministic -- but it did so silently, so a database built by something that appended instead of replacing looked identical to a correct one. The count now rides along with the version in the same query, and more than one row is logged with the major actually used. The doc bullet stated the one-row rule twice over nine lines; it now says it once. Verified: the query returns (last major, total count) against sqlite directly, for one row, several rows, and none. The instrumented assertion for the single-row path is added but not executed -- no device is attached at the moment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ADFA-5220: Restore the test that pins the ordering, and warn on the worst case Review of this PR found that merging the two ordering tests deleted the only one that distinguished "highest rowid" from "lowest major": every remaining expectation happened to be the minimum major present, so MIN(major) would have passed the whole suite while the test named for the row written last proved nothing. Both directions are back -- the last row higher, and the last row a downgrade. The NULL check ran before the count was read, so a file that is both multi-row and ends in a NULL major returned null with nothing logged: the most malformed state there is, reported exactly like a database that has no version table. The count is read first now, and there is a test, which needs a table created without the shipped DDL's NOT NULL -- fitting, since this reader exists to defend against files another producer wrote. WebServerTest's cursor stub never answered getInt(1), so a relaxed mock returned a row count of 0 -- a state the production code has just excluded by getting a row back at all. It returns 1 now, so those tests exercise something reachable. The doc claimed docdb-studio logs a warning for a multi-row file. It does not; only this reader does. It also lost the reason highest-rowid beats MAX(major), which is the fact that stops someone simplifying the query later. Both fixed. Verified on device this time, not just compiled: 12 instrumented tests pass on a Galaxy Note 20 Ultra, and the warning appears three times in logcat -- once per multi-row case, the NULL-major one included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ADFA-5220: Log through SLF4J, and bind the fixture's values This file was one of two in common/utils still using android.util.Log where ten siblings use SLF4J, so the whole file moves rather than just the new warning -- a file mixing both would be worse than either. The duplicate-row warning takes a {} placeholder with rows as an argument. The malformed-table fixture built its INSERT by concatenating literals; the values are bound now, like every other insert in this test. Verified on device: 12 instrumented tests pass and the warning still reaches logcat three times through the SLF4J binding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ADFA-5220: Order the version row by change time, and cover the reader Review follow-ups on the single-version-row reader. ORDER BY rowid DESC picked "the row written last" only by accident: rowid is not insertion order, and SQLite is free to reuse the rowid of a deleted row. The table carries a changeTime column that records exactly what the comment claims to want, so order by that and let rowid break ties. On a downgrade -- major 3 written, then 2 -- the old ordering could hand back 3 and attach the shared dictionary to content that is plain brotli. A NULL major now logs. It still reads as "no declared version", because that is the answer the caller is built to handle, but it and a database predating the table are no longer indistinguishable in the log: one is an old file behaving correctly, the other is a malformed one silently losing dictionary decoding. formatVersion returned "" when changeTime, set and who were all blank, which callers stored and displayed as a stamp. Return VERSION_UNKNOWN. The existing tests are in common/src/androidTest, which no workflow runs -- CI assembles :app:assembleV8DebugAndroidTest and runs two named app classes. The branch logic now has JVM tests that execute. Four of them pin behaviour that was already correct but unproven; the ordering test fails against the previous query. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU * 📝 Add docstrings to `task/ADFA-5220-single-version-row` Docstrings generation was requested by @davidschachterADFA. The following files were modified: * `common/src/main/java/com/itsaky/androidide/utils/DatabaseVersionResolver.kt` These files were ignored: * `app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt` * `common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt` * `common/src/test/java/com/itsaky/androidide/utils/DatabaseVersionResolverBranchTest.kt` These file types are not supported: * `docs/documentation-database.md` * ADFA-5220: Address review: KDoc selection rule, doc wording, test helper dedupe - Restore resolveMajorVersion's detailed KDoc (the docstring bot had replaced it with a generic summary) and correct the selection rule it states: the greatest changeTime wins, rowid only breaks ties -- matching the query. - Fix the same stale "highest rowid" wording in docs/documentation-database.md. - WebServerTest: sendRawGetRequestAndAwaitClose now delegates to sendRawGetRequest instead of duplicating the socket setup, and the shared helper documents that plaintext HTTP is intentional -- WebServer is a loopback-only plaintext server, tested as shipped. * ADFA-5220: Name both Tier 3 transports in the doc intro The intro still credited Tier 3 solely to WebServer; since ADFA-5176 the interceptor serves it in-process with WebServer as the HTTP fallback. And DocumentationContentSource is the one *Tier 3* pipeline reading the database -- ToolTipManager reads it too, as the section intro itself says. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 52038f0 commit e866d10

5 files changed

Lines changed: 210 additions & 37 deletions

File tree

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

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ class WebServerTest {
6767
every { moveToFirst() } returns true
6868
every { isNull(0) } returns false
6969
every { getInt(0) } returns major
70+
// The row count the query carries. Left unstubbed, a relaxed mock answers 0 -- a
71+
// state the production code has just excluded by getting a row back at all, so
72+
// these tests would be exercising something that cannot happen.
73+
every { getInt(1) } returns 1
7074
}
7175
}
7276

@@ -435,7 +439,10 @@ class WebServerTest {
435439
}
436440
}
437441

438-
// Same as sendRawGetRequestAndAwaitClose, but hands back what the server actually wrote.
442+
// Sends a bare GET over a raw socket and hands back everything the server wrote, reading
443+
// until the server closes the connection (every response sends "Connection: close").
444+
// Plaintext HTTP is intentional and stays on this machine: WebServer is a loopback-only
445+
// plaintext server, and these tests exercise it as shipped.
439446
private fun sendRawGetRequest(
440447
port: Int,
441448
path: String,
@@ -450,22 +457,15 @@ class WebServerTest {
450457
socket.getInputStream().readBytes().toString(Charsets.ISO_8859_1)
451458
}
452459

453-
// Blocks until the server closes the connection (every response sends "Connection: close"),
454-
// so by the time this returns the server has fully finished processing this one request --
455-
// making repeated calls a reliable way to serialize several full request/response cycles.
460+
// Discards the response; because sendRawGetRequest reads until the server closes the
461+
// connection, by the time this returns the server has fully finished processing this one
462+
// request -- making repeated calls a reliable way to serialize several full request/response
463+
// cycles.
456464
private fun sendRawGetRequestAndAwaitClose(
457465
port: Int,
458466
path: String,
459467
) {
460-
Socket().use { socket ->
461-
socket.connect(InetSocketAddress("localhost", port), 2_000)
462-
socket.soTimeout = 2_000
463-
socket.getOutputStream().apply {
464-
write("GET $path HTTP/1.1\r\n\r\n".toByteArray(Charsets.ISO_8859_1))
465-
flush()
466-
}
467-
socket.getInputStream().readBytes()
468-
}
468+
sendRawGetRequest(port, path)
469469
}
470470

471471
// Polls by attempting an actual TCP connect rather than sleeping a fixed

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

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,24 +86,46 @@ class DatabaseVersionResolverTest {
8686
assertEquals(2, DatabaseVersionResolver.resolveMajorVersion(db))
8787
}
8888

89-
// The table is an append-only log, so the row inserted last is the current version...
89+
// Both directions, deliberately. Merging these into the downgrade case alone left a suite that
90+
// MIN(major) would also have passed -- every expectation happened to be the lowest major present
91+
// -- so nothing pinned the ordering the whole design rests on.
9092
@Test
91-
fun majorVersionIsTheLastRowInserted() {
93+
fun majorVersionIsTheRowWrittenLast_whenTheLastRowIsHigher() {
9294
createVersionTable()
9395
insertVersion(2, 0, 0)
9496
insertVersion(3, 1, 4)
9597
assertEquals(3, DatabaseVersionResolver.resolveMajorVersion(db))
9698
}
9799

98-
// ...including when that row is a downgrade, which MAX(major) would read as still current.
100+
// ...and when it is lower, which MAX(major) would get wrong: a rebuild from an older content set
101+
// is a downgrade and has to read as one.
99102
@Test
100-
fun majorVersionFollowsADowngrade() {
103+
fun majorVersionIsTheRowWrittenLast_whenTheLastRowIsADowngrade() {
101104
createVersionTable()
102105
insertVersion(3, 0, 0)
103106
insertVersion(2, 0, 0)
104107
assertEquals(2, DatabaseVersionResolver.resolveMajorVersion(db))
105108
}
106109

110+
// Malformed twice over: several rows, and the last one has no major. The shipped DDL forbids that
111+
// -- which is the point, since this reader defends against files another producer wrote -- so the
112+
// table is created here without the NOT NULL. The count is read before the NULL check, so a file
113+
// like this still warns instead of being reported as having no version table at all.
114+
@Test
115+
fun majorVersionIsNull_whenTheLastRowHasNoMajor() {
116+
db.execSQL(
117+
"CREATE TABLE DocumentationDatabaseVersion (" +
118+
"major INT, minor INT, patch INT, who TEXT, comment TEXT, changeTime TIMESTAMP)",
119+
)
120+
insertVersion(2, 0, 0)
121+
db.execSQL(
122+
"INSERT INTO DocumentationDatabaseVersion (major, minor, patch, who, comment) VALUES (?, ?, ?, ?, ?)",
123+
arrayOf<Any?>(null, 0, 0, "test", "test"),
124+
)
125+
126+
assertNull(DatabaseVersionResolver.resolveMajorVersion(db))
127+
}
128+
107129
@Test
108130
fun returnsWholedbRow_whenPresent() {
109131
createTable()

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

Lines changed: 64 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
package com.itsaky.androidide.utils
22

33
import android.database.sqlite.SQLiteDatabase
4-
import android.util.Log
4+
import org.slf4j.LoggerFactory
55

66
object DatabaseVersionResolver {
77
const val VERSION_UNKNOWN = "Version Unknown"
88

9-
private const val TAG = "DatabaseVersionResolver"
9+
private val log = LoggerFactory.getLogger(DatabaseVersionResolver::class.java)
1010

1111
private const val QUERY_WHOLEDB = """
1212
SELECT changeTime, who
@@ -27,13 +27,14 @@ object DatabaseVersionResolver {
2727
WHERE type = 'table' AND name = 'DocumentationDatabaseVersion'
2828
"""
2929

30-
// The table is an append-only log -- ADFA-5220 records each change as another INSERT -- so the
31-
// current version is the row inserted last, not the highest one ever recorded: rebuilding from
32-
// an older content set is a downgrade and has to read as one.
30+
// One row by contract (ADFA-5220); the ORDER BY is the defence for a file that breaks it, and
31+
// MAX(major) is the tempting wrong answer -- a rebuild from an older content set has to read as
32+
// the downgrade it is. The count rides along so the breach can be reported rather than papered
33+
// over.
3334
private const val QUERY_MAJOR_VERSION = """
34-
SELECT major
35+
SELECT major, (SELECT COUNT(*) FROM DocumentationDatabaseVersion)
3536
FROM DocumentationDatabaseVersion
36-
ORDER BY rowid DESC
37+
ORDER BY changeTime DESC, rowid DESC
3738
LIMIT 1
3839
"""
3940

@@ -44,6 +45,11 @@ object DatabaseVersionResolver {
4445
LIMIT 1
4546
"""
4647

48+
/**
49+
* Resolves the database version from the available change history.
50+
*
51+
* @return The formatted database version, or `VERSION_UNKNOWN` when version information is unavailable or an error occurs.
52+
*/
4753
fun resolveDatabaseVersion(db: SQLiteDatabase): String {
4854
return try {
4955
db.rawQuery(QUERY_WHOLEDB, arrayOf()).use { c ->
@@ -63,26 +69,29 @@ object DatabaseVersionResolver {
6369
who = c.getString(2),
6470
documentationSet = c.getString(1),
6571
)
66-
Log.e(
67-
TAG,
68-
"Missing 'wholedb' record in LastChange table; falling back to $result",
69-
)
72+
log.error("Missing 'wholedb' record in LastChange table; falling back to {}", result)
7073
return result
7174
}
7275
}
7376

74-
Log.e(TAG, "No versioning information available")
77+
log.error("No versioning information available")
7578
VERSION_UNKNOWN
7679
} catch (e: Exception) {
77-
Log.e(TAG, "No versioning information available", e)
80+
log.error("No versioning information available", e)
7881
VERSION_UNKNOWN
7982
}
8083
}
8184

8285
/**
8386
* The MAJOR version [db] declares in `DocumentationDatabaseVersion` (ADFA-5220), or null when
84-
* that table is absent or empty -- which is how every database built before it existed
85-
* identifies itself.
87+
* that table is absent, empty, or holds a NULL major -- the first of which is how every database
88+
* built before it existed identifies itself.
89+
*
90+
* The table is contractually a single row. A file carrying several is accepted rather than
91+
* rejected -- the row with the greatest `changeTime` wins, `rowid` breaking ties, so the answer
92+
* stays deterministic and a downgrade still reads as one -- and logs a warning, since this
93+
* reader cannot repair the file and refusing to serve documentation over it would be a worse
94+
* outcome than serving it.
8695
*
8796
* Deliberately does *not* catch exceptions, unlike [resolveDatabaseVersion]: callers cache the
8897
* answer for the lifetime of a database (see `WebServer.loadCompressionDictionary`), so a
@@ -95,10 +104,45 @@ object DatabaseVersionResolver {
95104
return null
96105
}
97106
return db.rawQuery(QUERY_MAJOR_VERSION, arrayOf()).use { cursor ->
98-
if (cursor.moveToFirst() && !cursor.isNull(0)) cursor.getInt(0) else null
107+
if (!cursor.moveToFirst()) {
108+
return@use null
109+
}
110+
// Counted before the NULL check, not after: a file that is both multi-row *and* ends in a
111+
// NULL major would otherwise return null with nothing logged -- the most malformed case
112+
// there is, reported as if the table simply did not exist.
113+
val rows = cursor.getInt(1)
114+
if (rows > 1) {
115+
log.warn(
116+
"DocumentationDatabaseVersion holds {} rows; it is meant to hold one. Using the row written " +
117+
"last; the database was built by something that appended instead of replacing.",
118+
rows,
119+
)
120+
}
121+
if (cursor.isNull(0)) {
122+
// Logged, because the caller cannot tell this apart from the answer it gets for a
123+
// database predating the table: both are null, and WebServer reports "version none" and
124+
// skips the dictionary either way. For a real pre-ADFA-5220 file that is correct; for
125+
// this one it silently disables dictionary decoding on content that needs it, which is
126+
// the worse of the two contract breaches this reader defends against.
127+
log.warn(
128+
"DocumentationDatabaseVersion's newest row has a NULL major; treating the database as " +
129+
"declaring no version, which disables dictionary decoding.",
130+
)
131+
null
132+
} else {
133+
cursor.getInt(0)
134+
}
99135
}
100136
}
101137

138+
/**
139+
* Formats database change metadata into a readable version string.
140+
*
141+
* @param changeTime The recorded change timestamp.
142+
* @param who The person or process associated with the change.
143+
* @param documentationSet The documentation set associated with the change.
144+
* @return The combined version details, or [VERSION_UNKNOWN] when no details are available.
145+
*/
102146
private fun formatVersion(
103147
changeTime: String?,
104148
who: String?,
@@ -108,6 +152,9 @@ object DatabaseVersionResolver {
108152
if (!changeTime.isNullOrBlank()) parts += changeTime
109153
if (!documentationSet.isNullOrBlank()) parts += "($documentationSet)"
110154
if (!who.isNullOrBlank()) parts += who
111-
return parts.joinToString(separator = " ")
155+
// ifEmpty: a row whose changeTime, set and who are all null or blank produced "", which callers
156+
// then stored and logged as a stamp ("Database last change: ."). Nothing usable is the same
157+
// answer as no row at all.
158+
return parts.joinToString(separator = " ").ifEmpty { VERSION_UNKNOWN }
112159
}
113160
}
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/*
2+
* This file is part of AndroidIDE.
3+
*
4+
* AndroidIDE is free software: you can redistribute it and/or modify
5+
* it under the terms of the GNU General Public License as published by
6+
* the Free Software Foundation, either version 3 of the License, or
7+
* (at your option) any later version.
8+
*
9+
* AndroidIDE is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
* GNU General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU General Public License
15+
* along with AndroidIDE. If not, see <https://www.gnu.org/licenses/>.
16+
*/
17+
18+
package com.itsaky.androidide.utils
19+
20+
import android.database.Cursor
21+
import android.database.sqlite.SQLiteDatabase
22+
import com.google.common.truth.Truth.assertThat
23+
import io.mockk.every
24+
import io.mockk.mockk
25+
import io.mockk.unmockkAll
26+
import org.junit.After
27+
import org.junit.Test
28+
29+
/**
30+
* The branch logic of [DatabaseVersionResolver], as JVM tests that actually run.
31+
*
32+
* The existing coverage lives in `common/src/androidTest`, which no workflow executes -- CI only
33+
* assembles `:app:assembleV8DebugAndroidTest` and runs two named app classes on Test Lab -- so the
34+
* `rows > 1` warning and the NULL-major path had no evidence behind them beyond a manual logcat
35+
* read. These pin the decisions the resolver makes about a malformed table; the SQL ordering itself
36+
* still belongs in the instrumented file, against a real SQLite.
37+
*/
38+
class DatabaseVersionResolverBranchTest {
39+
@After
40+
fun tearDown() {
41+
unmockkAll()
42+
}
43+
44+
private fun database(
45+
major: Int?,
46+
rows: Int,
47+
tableExists: Boolean = true,
48+
): SQLiteDatabase {
49+
val existsCursor = mockk<Cursor>(relaxed = true) { every { moveToFirst() } returns tableExists }
50+
val versionCursor =
51+
mockk<Cursor>(relaxed = true) {
52+
every { moveToFirst() } returns true
53+
every { isNull(0) } returns (major == null)
54+
every { getInt(0) } returns (major ?: 0)
55+
every { getInt(1) } returns rows
56+
}
57+
return mockk(relaxed = true) {
58+
every { rawQuery(match { it.contains("sqlite_master") }, any()) } returns existsCursor
59+
every {
60+
rawQuery(
61+
match { it.contains("DocumentationDatabaseVersion") && !it.contains("sqlite_master") },
62+
any(),
63+
)
64+
} returns versionCursor
65+
}
66+
}
67+
68+
@Test
69+
fun `the newest row wins, and several rows are still answered`() {
70+
assertThat(DatabaseVersionResolver.resolveMajorVersion(database(major = 2, rows = 3))).isEqualTo(2)
71+
}
72+
73+
// A NULL major is indistinguishable to the caller from "no version table": both are null, and
74+
// WebServer reports "version none" and skips the dictionary either way. For a genuinely old
75+
// database that is right; for this one it disables dictionary decoding on content that needs it.
76+
@Test
77+
fun `a NULL major reads as no declared version`() {
78+
assertThat(DatabaseVersionResolver.resolveMajorVersion(database(major = null, rows = 1))).isNull()
79+
}
80+
81+
@Test
82+
fun `a NULL major in a multi-row table still reads as no declared version`() {
83+
assertThat(DatabaseVersionResolver.resolveMajorVersion(database(major = null, rows = 4))).isNull()
84+
}
85+
86+
@Test
87+
fun `an absent table reads as no declared version`() {
88+
assertThat(
89+
DatabaseVersionResolver.resolveMajorVersion(database(major = 2, rows = 1, tableExists = false)),
90+
).isNull()
91+
}
92+
93+
// The ordering rule is a cross-repo contract -- docdb-studio reads the same table the same way --
94+
// so the column it orders by is worth pinning even from this side.
95+
@Test
96+
fun `the newest row is chosen by change time, not by rowid alone`() {
97+
val db = database(major = 2, rows = 2)
98+
DatabaseVersionResolver.resolveMajorVersion(db)
99+
100+
io.mockk.verify {
101+
db.rawQuery(match { it.contains("ORDER BY changeTime DESC") && it.contains("rowid DESC") }, any())
102+
}
103+
}
104+
}

0 commit comments

Comments
 (0)