From 0b741d066d26af524f789baa131b77bc2be26f05 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 19 Aug 2026 13:57:43 -0700 Subject: [PATCH 01/10] ADFA-5179: Build the bookshelf payload in Kotlin, not with SQLite's JSON1 /pr/bs returned HTTP 500 on a Galaxy Note 20 Ultra (Android 13): "no such function: JSON_OBJECT". The query was fine -- it runs against the same documentation.db under desktop sqlite3 3.44 -- but that device's system SQLite has no JSON1 extension, so the bookshelf could not be opened at all. Nothing catches this before real hardware, since every desktop test passes. The JSON is now assembled from a plain relational query and gson, which work everywhere. Same keys, same nesting, same explicit nulls (gson gets serializeNulls, because JSON_OBJECT emitted "description": null and the bookshelf template was written against that), and the same 1/0 pdf flag rather than a boolean. Two behavior differences, both improvements, neither reachable in the data seen so far: - An empty bookshelf now renders as an empty page instead of failing. The old query turned it into a 500: group_concat over no rows is NULL, so the concatenated JSON was NULL and reading it as a blob threw. That case is not hypothetical -- the sdcard documentation.db copy on the test device has a NULL bookCategoryID on all 15 Bookshelf rows, so the join yields nothing and the endpoint would have failed there even with JSON1 present. - A path ending .PDF is flagged as a PDF. The old SUBSTR comparison was case-sensitive; the 15 PDFs in the database are all lowercase, so this changes nothing today. Grouping also collapses a NULL category into an existing "General" rather than producing two sections with the same name, since it groups by the coalesced label instead of the raw column. readBookshelf() takes the database as a parameter so the payload is testable without starting a server. Four tests cover the grouping and order, the pdf flag including the case difference, the empty bookshelf, and the exact JSON the template receives. Not yet verified on device: the phone was disconnected before this could be installed. What needs checking is that /pr/bs returns 200 both against the sdcard copy (expect an empty bookshelf, given its data) and against the installed asset database with the sdcard copy moved aside (expect real content). --- .../androidide/localWebServer/WebServer.kt | 149 ++++++++++++------ .../localWebServer/BookshelfPayloadTest.kt | 113 +++++++++++++ 2 files changed, 214 insertions(+), 48 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 0b76b64d2d..4a49f13592 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -54,6 +54,28 @@ data class ServerConfig( val projectDatabasePath: String = "/data/data/com.itsaky.androidide/databases/RecentProject_database", ) +/** + * The `bookshelf` template's JSON context. Field names are the JSON keys, so they match what the + * template reads -- and what SQLite's JSON1 functions used to emit before ADFA-5179. + */ +data class Bookshelf( + val result: List, +) + +data class BookshelfCategory( + val category: String, + val description: String?, + val books: List, +) + +data class BookshelfBook( + val title: String, + val description: String?, + val link: String, + /** 1 or 0, not a boolean: the shape the template already expects. */ + val pdf: Int, +) + data class JavaExecutionResult( val compileOutput: String, val runOutput: String, @@ -92,8 +114,15 @@ class WebServer( private val gson: Gson = GsonBuilder() .setObjectToNumberStrategy(ToNumberPolicy.LONG_OR_DOUBLE) + // JSON_OBJECT emitted "description": null for a null column, and the bookshelf template + // was written against that; gson would drop the key entirely by default. + .serializeNulls() .create() private val dbContextType = object : TypeToken>() {}.type + + /** The configured gson, so a test can assert the exact JSON the template receives. */ + internal val gsonForTest: Gson + get() = gson private var bookshelfTemplateId: Int = -1 private val httpInternalServerError = 500 private val httpNotFound = 404 @@ -730,67 +759,28 @@ class WebServer( ): Boolean { if (debugEnabled) log.debug("Entering realHandleBsEndpoint().") - // Database fetch - val sqlQuery = -""" -SELECT '{"result" : [' || group_concat(Item) || ']}' FROM ( -SELECT - JSON_OBJECT( - 'category', IFNULL(BC.category, 'General'), - 'description', BC.description, - 'books', JSON_GROUP_ARRAY(JSON_OBJECT( - 'title', IFNULL(B.title, C.path), - 'description', B.description, - 'link', C.path, - 'pdf', IIF(SUBSTR(C.path, -4) == '.pdf', 1, 0) ) - ) - ) AS Item -FROM Content AS C, - Bookshelf AS B, - BookCategories AS BC -WHERE C.id = B.contentID -AND B.bookCategoryID = BC.id -GROUP BY BC.category -ORDER BY BC.category, - B.title -); -""".trimIndent() - - var cursor = database.rawQuery(sqlQuery, arrayOf()) - lateinit var jsonText: ByteArray + val jsonText: ByteArray - // Process database fetch try { - if (!isCursorOneRow(cursor, writer, output)) { - return false - } - - // get the JSON from the bookshelf table - cursor.moveToFirst() - jsonText = cursor.getBlob(0) + jsonText = gson.toJson(readBookshelf(database)).toByteArray(Charsets.UTF_8) if (debugEnabled) log.debug("json content = '${String(jsonText)}'.") if (debugEnabled) log.debug("before fetch bookshelf template ID = '$bookshelfTemplateId'") - // Have we already fetched the template if (bookshelfTemplateId == -1) { - // safety first, close the cursor - cursor.close() - cursor = database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()) + database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()).use { cursor -> + if (!isCursorOneRow(cursor, writer, output)) { + return false + } - if (!isCursorOneRow(cursor, writer, output)) { - return false + cursor.moveToFirst() + bookshelfTemplateId = cursor.getInt(0) + if (debugEnabled) log.debug("after the fetch bookshelf template ID = '$bookshelfTemplateId'") } - - cursor.moveToFirst() - bookshelfTemplateId = cursor.getInt(0) - if (debugEnabled) log.debug("after the fetch bookshelf template ID = '$bookshelfTemplateId'") } } catch (e: Exception) { log.error("Error processing request: {}", e.message) sendError(writer, output, httpInternalServerError, "Internal Server Error", e.message ?: "") return false - } finally { - cursor.close() } val result = instantiatePebbleTemplate(bookshelfTemplateId, jsonText, "/bookshelf", "application/json", "none") @@ -805,6 +795,69 @@ ORDER BY BC.category, return true } + /** + * The bookshelf, grouped into categories, for the `bookshelf` template's JSON context. + * + * Assembled here rather than by SQLite's JSON1 functions (ADFA-5179): `JSON_OBJECT` and + * `JSON_GROUP_ARRAY` are absent from the system SQLite on some devices -- a Galaxy Note 20 Ultra + * on Android 13 among them -- where the old query failed at runtime with `no such function: + * JSON_OBJECT` and the bookshelf could not be opened at all. A plain relational query and gson + * work everywhere, and the payload is identical. + * + * An empty bookshelf comes back as an empty list, which the template renders as an empty page. + * The old query turned that case into an HTTP 500: `group_concat` over no rows is NULL, so the + * concatenated JSON was NULL and reading it as a blob threw. Worth knowing, because the rows in + * at least one `documentation.db` copy have a NULL `bookCategoryID` and so join to nothing. + */ + internal fun readBookshelf(database: SQLiteDatabase): Bookshelf { + val query = + """ +SELECT IFNULL(BC.category, 'General'), + BC.description, + IFNULL(B.title, C.path), + B.description, + C.path +FROM Content AS C, + Bookshelf AS B, + BookCategories AS BC +WHERE C.id = B.contentID +AND B.bookCategoryID = BC.id +ORDER BY BC.category, + B.title + """.trimIndent() + + // LinkedHashMap: the query's ORDER BY decides the order categories and books appear in, and + // the template renders them in that order. + val categories = LinkedHashMap>() + val descriptions = LinkedHashMap() + + database.rawQuery(query, arrayOf()).use { cursor -> + while (cursor.moveToNext()) { + val category = cursor.getString(0) + val path = cursor.getString(4) + + descriptions.putIfAbsent(category, cursor.getString(1)) + categories + .getOrPut(category) { mutableListOf() } + .add( + BookshelfBook( + title = cursor.getString(2), + description = cursor.getString(3), + link = path, + // 1/0 rather than a boolean: what the template has always received. + pdf = if (path.endsWith(".pdf", ignoreCase = true)) 1 else 0, + ), + ) + } + } + + return Bookshelf( + categories.map { (category, books) -> + BookshelfCategory(category = category, description = descriptions[category], books = books) + }, + ) + } + private fun isCursorOneRow( cursor: Cursor, writer: PrintWriter, diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt new file mode 100644 index 0000000000..b5c9eaf49b --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -0,0 +1,113 @@ +package com.itsaky.androidide.localWebServer + +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 org.junit.Test + +/** + * Covers the bookshelf payload now that it is assembled in Kotlin rather than by SQLite's JSON1 + * functions (ADFA-5179), which are missing from the system SQLite on some devices. + * + * The shape matters as much as the content: the `bookshelf` Pebble template was written against what + * `JSON_OBJECT`/`JSON_GROUP_ARRAY` emitted, so the keys, the nesting, the explicit nulls and the 1/0 + * `pdf` flag all have to survive the change. + */ +class BookshelfPayloadTest { + private fun server() = + WebServer( + ServerConfig( + port = 0, + databasePath = "/nonexistent/test.db", + fileDirPath = "/tmp", + debugDatabasePath = "/nonexistent/debug.db", + debugEnablePath = "/nonexistent/debug-flag", + experimentsEnablePath = "/nonexistent/exp-flag", + clearCacheEnablePath = "/nonexistent/cs0-flag", + projectDatabasePath = "/nonexistent/recent-projects.db", + ), + ) + + /** One joined row: category, category description, title, book description, path. */ + private fun database(vararg rows: Array): SQLiteDatabase { + var index = -1 + val cursor = + mockk(relaxed = true) { + every { moveToNext() } answers { ++index < rows.size } + every { getString(any()) } answers { rows[index][firstArg()] } + } + + return mockk(relaxed = true) { + every { rawQuery(match { it.contains("FROM Content AS C") }, any()) } returns cursor + } + } + + @Test + fun `books are grouped into the categories the query ordered them by`() { + val bookshelf = + server().readBookshelf( + database( + arrayOf("Java", "Books about Java", "Effective Java", "A classic", "j/effective.html"), + arrayOf("Java", "Books about Java", "Java Concurrency", "Also good", "j/concurrency.html"), + arrayOf("Kotlin", "Books about Kotlin", "Kotlin in Action", "Recommended", "k/in-action.html"), + ), + ) + + assertThat(bookshelf.result.map { it.category }).containsExactly("Java", "Kotlin").inOrder() + assertThat(bookshelf.result[0].description).isEqualTo("Books about Java") + assertThat(bookshelf.result[0].books.map { it.title }) + .containsExactly("Effective Java", "Java Concurrency") + .inOrder() + assertThat( + bookshelf.result[1] + .books + .single() + .link, + ).isEqualTo("k/in-action.html") + } + + @Test + fun `a pdf link is flagged with 1, anything else with 0`() { + val bookshelf = + server().readBookshelf( + database( + arrayOf("General", "", "A guide", "", "d/guide.pdf"), + arrayOf("General", "", "Shouty guide", "", "d/GUIDE.PDF"), + arrayOf("General", "", "A page", "", "i/index.html"), + ), + ) + + assertThat( + bookshelf.result + .single() + .books + .map { it.pdf }, + ).containsExactly(1, 1, 0).inOrder() + } + + @Test + fun `an empty bookshelf is an empty list, not a failure`() { + // The old query made this an HTTP 500: group_concat over no rows is NULL, and reading that + // as a blob threw. At least one documentation.db copy joins to nothing, so it is reachable. + val bookshelf = server().readBookshelf(database()) + + assertThat(bookshelf.result).isEmpty() + } + + @Test + fun `the JSON keeps the keys, nesting and explicit nulls the template was written against`() { + val json = + server().gsonForTest.toJson( + server().readBookshelf( + database(arrayOf("General", null, "A guide", null, "d/guide.pdf")), + ), + ) + + assertThat(json).isEqualTo( + """{"result":[{"category":"General","description":null,""" + + """"books":[{"title":"A guide","description":null,"link":"d/guide.pdf","pdf":1}]}]}""", + ) + } +} From 786f086626c0d2594fdfda488a0ba19b77637340 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 19 Aug 2026 14:21:35 -0700 Subject: [PATCH 02/10] ADFA-5179: Unmock in the bookshelf test's teardown The new test class left mockk's instrumentation installed for the rest of the JVM, and the next test to run in it -- BrotliDictionaryDecodeTest -- then failed in @BeforeClass with 'Failed to load Brotli native library'. Reproduced both ways: the full app suite passes with this class excluded and fails with it included, deterministically. Every other mockk-using test in this module already unmocks in teardown; this one just missed it. --- .../androidide/localWebServer/BookshelfPayloadTest.kt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt index b5c9eaf49b..20cfb69418 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -5,6 +5,8 @@ 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 /** @@ -16,6 +18,13 @@ import org.junit.Test * `pdf` flag all have to survive the change. */ class BookshelfPayloadTest { + // Without this, mockk's instrumentation outlives the class and breaks a later test in the same + // JVM: BrotliDictionaryDecodeTest's @BeforeClass then fails to load the brotli native library. + @After + fun tearDown() { + unmockkAll() + } + private fun server() = WebServer( ServerConfig( From 22911bab8218f486bfa44850e6297af834c67dd7 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 19 Aug 2026 18:36:52 -0700 Subject: [PATCH 03/10] ADFA-5179: Move the bookshelf query's fallbacks into Kotlin Review pointed at IFNULL(BC.category, 'General') and read it as dead code, on the grounds that the join already drops a book whose bookCategoryID is NULL. The join does drop those -- deliberately, since that is what the query this PR replaced did -- but the IFNULL is not about them: BookCategories.category has no NOT NULL constraint, so a book can be linked to a category row that has no label, and that is the case it covers. Both of the query's fallbacks are now expressed in Kotlin instead, which makes which case each one handles visible at the point it applies, and lets a test pin them: a category row with no label files its books under General, and a book with no title of its own shows its path. Neither was covered before, because the SQL is not exercised by unit tests -- the cursor is mocked. Behavior is unchanged in every case. --- .../androidide/localWebServer/WebServer.kt | 18 +++++++--- .../localWebServer/BookshelfPayloadTest.kt | 35 +++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 4a49f13592..399b8160bc 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -129,6 +129,9 @@ class WebServer( private val contentChunkSize = 1024 * 1024 + /** Where a book whose category row has no label is filed (see [readBookshelf]). */ + private val uncategorizedLabel = "General" + // function to obtain the last modified date of a documentation.db database // this is used to see if there is a newer version of the database on the sdcard fun getDatabaseTimestamp( @@ -810,11 +813,13 @@ class WebServer( * at least one `documentation.db` copy have a NULL `bookCategoryID` and so join to nothing. */ internal fun readBookshelf(database: SQLiteDatabase): Bookshelf { + // The two fallbacks the old query expressed as IFNULL live in Kotlin now (see below): they + // are easier to see there, and a unit test can cover them. val query = """ -SELECT IFNULL(BC.category, 'General'), +SELECT BC.category, BC.description, - IFNULL(B.title, C.path), + B.title, B.description, C.path FROM Content AS C, @@ -833,15 +838,20 @@ ORDER BY BC.category, database.rawQuery(query, arrayOf()).use { cursor -> while (cursor.moveToNext()) { - val category = cursor.getString(0) val path = cursor.getString(4) + // BookCategories.category is nullable, so a book can be linked to a category row that + // has no label; it lands under "General", as the old query's IFNULL had it. This is + // *not* about a book with no category at all -- the join drops those, exactly as the + // query this replaced did. + val category = cursor.getString(0) ?: uncategorizedLabel descriptions.putIfAbsent(category, cursor.getString(1)) categories .getOrPut(category) { mutableListOf() } .add( BookshelfBook( - title = cursor.getString(2), + // A book with no title of its own shows its path, again as before. + title = cursor.getString(2) ?: path, description = cursor.getString(3), link = path, // 1/0 rather than a boolean: what the template has always received. diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt index 20cfb69418..a52c90ba46 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -96,6 +96,41 @@ class BookshelfPayloadTest { ).containsExactly(1, 1, 0).inOrder() } + @Test + fun `a category row with no label files its books under General`() { + // BookCategories.category is nullable, so this is reachable; a book with no category at all + // is a different case, dropped by the join exactly as the query this replaced dropped it. + val bookshelf = + server().readBookshelf( + database(arrayOf(null, "No label", "A guide", "", "d/guide.pdf")), + ) + + assertThat(bookshelf.result.single().category).isEqualTo("General") + assertThat( + bookshelf.result + .single() + .books + .single() + .title, + ).isEqualTo("A guide") + } + + @Test + fun `a book with no title of its own shows its path`() { + val bookshelf = + server().readBookshelf( + database(arrayOf("General", "", null, "", "i/index.html")), + ) + + assertThat( + bookshelf.result + .single() + .books + .single() + .title, + ).isEqualTo("i/index.html") + } + @Test fun `an empty bookshelf is an empty list, not a failure`() { // The old query made this an HTTP 500: group_concat over no rows is NULL, and reading that From 16cdd1e62f094ecbf73409568c860823e536345c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 21 Aug 2026 23:11:10 -0700 Subject: [PATCH 04/10] ADFA-5179: Keep an unlabelled category separate from a literal "General" The review is right. The old JSON1 query grouped by BC.category and applied IFNULL(BC.category, 'General') only when building the payload, so a category row with no label and a row labelled "General" were two groups that both rendered as "General", each with its own description. Coalescing before grouping merged them and kept whichever description came first. The maps are now keyed by the raw nullable category and the label is applied when the BookshelfCategory is constructed. This PR is a port whose whole claim is that the payload is identical, so a behaviour change here -- even a defensible one -- does not belong in it. Latent rather than live: no BookCategories row in the current database has a NULL category. The column is nullable, so the case is reachable. Co-Authored-By: Claude Opus 5 --- .../androidide/localWebServer/WebServer.kt | 23 +++++++++++++------ .../localWebServer/BookshelfPayloadTest.kt | 22 ++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 399b8160bc..3620758bf7 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -833,17 +833,22 @@ ORDER BY BC.category, // LinkedHashMap: the query's ORDER BY decides the order categories and books appear in, and // the template renders them in that order. - val categories = LinkedHashMap>() - val descriptions = LinkedHashMap() + // + // Keyed by the *raw* category, null included. The query this replaced grouped by BC.category, + // where NULL and a literal "General" are two groups that both render as "General"; coalescing + // before grouping merges them and keeps only the first description. This port is meant to + // change nothing, so the label is applied at construction instead. + val categories = LinkedHashMap>() + val descriptions = LinkedHashMap() database.rawQuery(query, arrayOf()).use { cursor -> while (cursor.moveToNext()) { val path = cursor.getString(4) // BookCategories.category is nullable, so a book can be linked to a category row that - // has no label; it lands under "General", as the old query's IFNULL had it. This is - // *not* about a book with no category at all -- the join drops those, exactly as the - // query this replaced did. - val category = cursor.getString(0) ?: uncategorizedLabel + // has no label; it is labelled "General" below, as the old query's IFNULL had it. This + // is *not* about a book with no category at all -- the join drops those, exactly as + // the query this replaced did. + val category = cursor.getString(0) descriptions.putIfAbsent(category, cursor.getString(1)) categories @@ -863,7 +868,11 @@ ORDER BY BC.category, return Bookshelf( categories.map { (category, books) -> - BookshelfCategory(category = category, description = descriptions[category], books = books) + BookshelfCategory( + category = category ?: uncategorizedLabel, + description = descriptions[category], + books = books, + ) }, ) } diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt index a52c90ba46..0e6e19498d 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -115,6 +115,28 @@ class BookshelfPayloadTest { ).isEqualTo("A guide") } + // The old query grouped by BC.category, so an unlabelled category row and a row labelled + // "General" were two groups that both rendered as "General" -- each with its own description. + // Coalescing before grouping merged them and dropped one description; the payload has to match. + @Test + fun `an unlabelled category and a literal General stay separate groups`() { + val bookshelf = + server().readBookshelf( + database( + arrayOf(null, "No label", "Unlabelled book", null, "u/book.pdf"), + arrayOf("General", "Books about computing", "General book", null, "g/book.pdf"), + ), + ) + + assertThat(bookshelf.result.map { it.category }).containsExactly("General", "General").inOrder() + assertThat(bookshelf.result.map { it.description }) + .containsExactly("No label", "Books about computing") + .inOrder() + assertThat(bookshelf.result.map { category -> category.books.single().title }) + .containsExactly("Unlabelled book", "General book") + .inOrder() + } + @Test fun `a book with no title of its own shows its path`() { val bookshelf = From b9f337b55f1892823bce0718bff3dadf6394cbe3 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 25 Aug 2026 18:31:42 -0700 Subject: [PATCH 05/10] ADFA-5179: Group the bookshelf in one map, and survive a row without a path Three findings from the review of this PR. putIfAbsent was the wrong tool for "the first description wins". java.util.Map counts a key mapped to null as absent -- HashMap.putVal only skips the write when oldValue != null -- so a category whose first row had a NULL description was overwritten by whatever the next row carried, which is the opposite of what the comment claimed and what the query this replaced did. The two parallel maps keyed by the same category are now one map of a small holder, so the description is read once, when the group is created, and nothing can write it again. That also removes the second lookup per row and the possibility of the two maps disagreeing. The holder's type has to stay non-null for this to hold: getOrPut treats a null value as absent too, so a map of nullable descriptions would reintroduce the same bug in a different shape. A NULL Content.path took the whole endpoint down. cursor.getString(4) is a platform type, so handing it to BookshelfBook(link: String) inserts an intrinsic null check -- an NPE caught upstairs as an HTTP 500 for every book on the shelf, where the old SQL degraded to "link": null for the one row. The schema says NOT NULL and the maintained database honours it, but this endpoint exists because a shipped copy had NULLs nobody expected. The row is skipped and logged. Bookshelf, BookshelfCategory and BookshelfBook are internal rather than public. They are one endpoint's payload shape, and common's classes ship in the plugin-api coordinate; three very generic names did not belong in that surface. internal, not private: a private top-level class cannot be the return type of an internal function, and the tests in this module read them. Three tests added. Two of them fail against the code as it stood -- "expected: null but was : Arrived late" and a NullPointerException -- for the reasons they are named for. 276 app tests pass. Found in review of PR #1700. --- .../androidide/localWebServer/WebServer.kt | 45 ++++++++++++---- .../localWebServer/BookshelfPayloadTest.kt | 54 +++++++++++++++++++ 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 7d587ba8cd..8d49dfae13 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -63,17 +63,24 @@ data class ServerConfig( * The `bookshelf` template's JSON context. Field names are the JSON keys, so they match what the * template reads -- and what SQLite's JSON1 functions used to emit before ADFA-5179. */ -data class Bookshelf( +internal data class Bookshelf( val result: List, ) -data class BookshelfCategory( +internal data class BookshelfCategory( val category: String, val description: String?, val books: List, ) -data class BookshelfBook( +// Not part of the JSON payload: the accumulator readBookshelf groups rows into. Its fields become +// BookshelfCategory's once every row has been read. +private class CategoryGroup( + val description: String?, + val books: MutableList = mutableListOf(), +) + +internal data class BookshelfBook( val title: String, val description: String?, val link: String, @@ -1072,21 +1079,39 @@ ORDER BY BC.category, // where NULL and a literal "General" are two groups that both render as "General"; coalescing // before grouping merges them and keeps only the first description. This port is meant to // change nothing, so the label is applied at construction instead. - val categories = LinkedHashMap>() - val descriptions = LinkedHashMap() + // One entry per category, holding the label's own description alongside its books. Two maps + // keyed by the same category would have to be kept in agreement by hand, and putIfAbsent is + // the wrong tool for that: java.util.Map treats a key mapped to null as absent, so a category + // whose first row had a NULL description was overwritten by the next row's -- the opposite of + // the "first one wins" this comment used to claim. getOrPut's lambda runs only when the key + // is genuinely missing, so the description is read once, at group creation, and there is no + // second write to get wrong. + // + // The value type has to stay non-null for that to hold: getOrPut treats a null *value* as + // absent too, so a LinkedHashMap of descriptions would reintroduce the bug + // in a different shape. + val categories = LinkedHashMap() database.rawQuery(query, arrayOf()).use { cursor -> while (cursor.moveToNext()) { + // Content.path is NOT NULL in the maintained schema, so this is unreachable there -- but + // this endpoint exists because a shipped documentation.db had NULLs nobody expected, and + // a platform-type null reaching BookshelfBook(link: String) is an NPE that costs the + // whole shelf rather than the one bad row. val path = cursor.getString(4) + if (path == null) { + log.warn("Bookshelf row for content id {} has no path; skipping it.", cursor.getString(2)) + continue + } // BookCategories.category is nullable, so a book can be linked to a category row that // has no label; it is labelled "General" below, as the old query's IFNULL had it. This // is *not* about a book with no category at all -- the join drops those, exactly as // the query this replaced did. val category = cursor.getString(0) - descriptions.putIfAbsent(category, cursor.getString(1)) categories - .getOrPut(category) { mutableListOf() } + .getOrPut(category) { CategoryGroup(cursor.getString(1)) } + .books .add( BookshelfBook( // A book with no title of its own shows its path, again as before. @@ -1101,11 +1126,11 @@ ORDER BY BC.category, } return Bookshelf( - categories.map { (category, books) -> + categories.map { (category, group) -> BookshelfCategory( category = category ?: uncategorizedLabel, - description = descriptions[category], - books = books, + description = group.description, + books = group.books, ) }, ) diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt index 0e6e19498d..a987822179 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -153,6 +153,60 @@ class BookshelfPayloadTest { ).isEqualTo("i/index.html") } + // The description belongs to the category label, so it is read from the first row of the group and + // the rest are the same category repeated. putIfAbsent got this wrong in the one case that has no + // visible symptom until it happens: java.util.Map counts a key mapped to null as absent, so a + // first row with no description was overwritten by whatever the second row carried. + @Test + fun `a category whose first row has no description keeps the null`() { + val bookshelf = + server().readBookshelf( + database( + arrayOf("Kotlin", null, "First", "", "a.pdf"), + arrayOf("Kotlin", "Arrived late", "Second", "", "b.pdf"), + ), + ) + + val category = bookshelf.result.single() + assertThat(category.description).isNull() + assertThat(category.books.map { it.title }).containsExactly("First", "Second").inOrder() + } + + // ...and the ordinary direction still holds: the first row's description wins over later ones. + @Test + fun `a category keeps the description from its first row`() { + val bookshelf = + server().readBookshelf( + database( + arrayOf("Kotlin", "The real one", "First", "", "a.pdf"), + arrayOf("Kotlin", "A later, different one", "Second", "", "b.pdf"), + ), + ) + + assertThat(bookshelf.result.single().description).isEqualTo("The real one") + } + + // Content.path is NOT NULL in the maintained schema, but this endpoint exists because a shipped + // copy had NULLs nobody expected. One unusable row must not cost the whole shelf: the null would + // otherwise reach BookshelfBook(link: String) as an intrinsic null check, i.e. an HTTP 500. + @Test + fun `a row with no path is skipped, not fatal`() { + val bookshelf = + server().readBookshelf( + database( + arrayOf("General", "", "Broken", "", null), + arrayOf("General", "", "Fine", "", "d/guide.pdf"), + ), + ) + + assertThat( + bookshelf.result + .single() + .books + .map { it.title }, + ).containsExactly("Fine") + } + @Test fun `an empty bookshelf is an empty list, not a failure`() { // The old query made this an HTTP 500: group_concat over no rows is NULL, and reading that From 15eba460dd297c606592f73af9e643381cebc8be Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 25 Aug 2026 19:14:27 -0700 Subject: [PATCH 06/10] ADFA-5179: Say what actually changed, and test the query that changed it Review findings from hal-eisen-adfa. The KDoc claimed the payload was identical. It is not, in two ways, both checked against the shipped database rather than reasoned about: Books within a category are now genuinely sorted by title. The old ORDER BY was inert for them -- it ordered the groups, while JSON_GROUP_ARRAY aggregated in scan order and B.title was a bare column under GROUP BY. Against the current documentation.db this reverses the two Java books, since a space sorts before a comma. Deterministic order is worth having; claiming nothing changed is not. The pdf flag is now case-insensitive, where SUBSTR(path, -4) == '.pdf' compared under BINARY collation. No shipped row spells the extension any other way -- GLOB '*.[Pp][Dd][Ff]' excluding '*.pdf' returns zero -- so nothing changes today, but the widening was undocumented. Nothing ran the new SQL. The existing tests hand readBookshelf canned cursor rows through a mockk matcher, so the column order, the joins and the ORDER BY were never executed -- on a ticket whose whole subject is a query that passed on a desktop and failed on a device. BookshelfQueryTest runs it against a real SQLite database. It earns its place: reordering the two columns both named `description` in the SELECT list, leaving the reader alone, fails that test and leaves all ten mock-based tests green. gsonForTest is gone. The test that pinned the payload's keys, nesting and explicit nulls asserted on a re-composed gson.toJson(readBookshelf(...)) -- the same expression as production, which is not the same thing as the production path, and could not fail if that line changed. Both now call bookshelfJson(). An empty shelf is logged. It is a 200 with an empty page either way, but it was indistinguishable from a working shelf in a bug report, and it is the state ADFA-5204 produced. The JSON keys are pinned with @SerializedName instead of being derived reflectively from field names. The template reads them literally, so a renamed field is a page of blanks with nothing failing. -dontobfuscate currently keeps them intact, but that is a global build flag two open tickets are changing, not a contract this payload should rest on. ServerConfig's eight fields were copied field-for-field between two test classes in the same package; both now call testServerConfig(). 281 app tests pass. Still open from the same review: the JSON round trip through text that instantiatePebbleTemplate immediately parses back into a map, which needs an overload taking a prepared context; and the unmockkAll coupling with BrotliDictionaryDecodeTest, which wants a shared rule rather than a copy per class. --- .../androidide/localWebServer/WebServer.kt | 69 ++++++-- .../localWebServer/BookshelfPayloadTest.kt | 29 ++-- .../localWebServer/BookshelfQueryTest.kt | 154 ++++++++++++++++++ .../localWebServer/TestServerConfig.kt | 20 +++ .../localWebServer/WebServerTest.kt | 12 +- 5 files changed, 241 insertions(+), 43 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt create mode 100644 app/src/test/java/com/itsaky/androidide/localWebServer/TestServerConfig.kt diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 8d49dfae13..c3930d7719 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -9,6 +9,7 @@ import com.aayushatharva.brotli4j.decoder.BrotliInputStream import com.google.gson.Gson import com.google.gson.GsonBuilder import com.google.gson.ToNumberPolicy +import com.google.gson.annotations.SerializedName import com.google.gson.reflect.TypeToken import com.itsaky.androidide.utils.DatabaseVersionResolver import io.pebbletemplates.pebble.PebbleEngine @@ -60,17 +61,23 @@ data class ServerConfig( ) /** - * The `bookshelf` template's JSON context. Field names are the JSON keys, so they match what the - * template reads -- and what SQLite's JSON1 functions used to emit before ADFA-5179. + * The `bookshelf` template's JSON context: the keys the template reads, and what SQLite's JSON1 + * functions used to emit before ADFA-5179. + * + * Every key is spelled out with [SerializedName] rather than left to gson's reflection over field + * names. The template reads these names literally -- `{{ item.category }}`, `book.pdf` -- and a + * renamed field would produce a page of blanks with nothing failing anywhere. Today `-dontobfuscate` + * happens to keep the field names intact in release builds, but that is a global build flag two + * tickets are actively changing, not a contract this payload can rely on. */ internal data class Bookshelf( - val result: List, + @SerializedName("result") val result: List, ) internal data class BookshelfCategory( - val category: String, - val description: String?, - val books: List, + @SerializedName("category") val category: String, + @SerializedName("description") val description: String?, + @SerializedName("books") val books: List, ) // Not part of the JSON payload: the accumulator readBookshelf groups rows into. Its fields become @@ -81,11 +88,11 @@ private class CategoryGroup( ) internal data class BookshelfBook( - val title: String, - val description: String?, - val link: String, + @SerializedName("title") val title: String, + @SerializedName("description") val description: String?, + @SerializedName("link") val link: String, /** 1 or 0, not a boolean: the shape the template already expects. */ - val pdf: Int, + @SerializedName("pdf") val pdf: Int, ) data class JavaExecutionResult( @@ -190,9 +197,6 @@ class WebServer( .create() private val dbContextType = object : TypeToken>() {}.type - /** The configured gson, so a test can assert the exact JSON the template receives. */ - internal val gsonForTest: Gson - get() = gson private var bookshelfTemplateId: Int = -1 private val httpInternalServerError = 500 private val httpNotFound = 404 @@ -1006,7 +1010,7 @@ class WebServer( val jsonText: ByteArray try { - jsonText = gson.toJson(readBookshelf(database)).toByteArray(Charsets.UTF_8) + jsonText = bookshelfJson(database) if (debugEnabled) log.debug("json content = '${String(jsonText)}'.") if (debugEnabled) log.debug("before fetch bookshelf template ID = '$bookshelfTemplateId'") @@ -1039,6 +1043,26 @@ class WebServer( return true } + /** + * The exact bytes the `bookshelf` template is rendered against. + * + * Extracted so the test that pins the payload's keys, nesting and explicit nulls can call the + * path production uses. Asserting on a re-composed `gson.toJson(readBookshelf(...))` looked + * equivalent but could not fail if this line changed -- a differently configured serializer here + * would drop every `"description": null` the template was written against and the test would + * still pass. + */ + internal fun bookshelfJson(database: SQLiteDatabase): ByteArray { + val bookshelf = readBookshelf(database) + if (bookshelf.result.isEmpty()) { + // Not an error -- the endpoint answers 200 with an empty shelf -- but it is indistinguishable + // from a working shelf in a bug report, and it is the state ADFA-5204 produced. The query + // this replaced surfaced it only by accident, as a 500 from reading a NULL blob. + log.info("Bookshelf query matched no rows; serving an empty shelf.") + } + return gson.toJson(bookshelf).toByteArray(Charsets.UTF_8) + } + /** * The bookshelf, grouped into categories, for the `bookshelf` template's JSON context. * @@ -1046,7 +1070,22 @@ class WebServer( * `JSON_GROUP_ARRAY` are absent from the system SQLite on some devices -- a Galaxy Note 20 Ultra * on Android 13 among them -- where the old query failed at runtime with `no such function: * JSON_OBJECT` and the bookshelf could not be opened at all. A plain relational query and gson - * work everywhere, and the payload is identical. + * work everywhere. + * + * The payload keeps its keys, nesting and explicit nulls, but two things about it do change, both + * deliberately: + * + * Books within a category are now genuinely sorted by title. The old `ORDER BY BC.category, + * B.title` was inert for them -- it ordered the *groups*, while `JSON_GROUP_ARRAY` aggregated + * rows in scan order, and `B.title` was a bare column under `GROUP BY BC.category`. Against the + * shipped database this reverses the two Java books: "Java, Java, Java" came first by insertion, + * and "Java Notes for Professionals" comes first by title (a space sorts before a comma). + * Deterministic order is worth having, but it is a visible change, not a no-op. + * + * The `pdf` flag is now case-insensitive. `SUBSTR(C.path, -4) == '.pdf'` compared under BINARY + * collation, so a row at `books/Guide.PDF` was flagged 0 and rendered as a web link. No shipped + * row spells the extension any other way -- checked with `GLOB '*.[Pp][Dd][Ff]'` -- so nothing + * changes today; a future upper-case path is simply treated as the PDF it is. * * An empty bookshelf comes back as an empty list, which the template renders as an empty page. * The old query turned that case into an HTTP 500: `group_concat` over no rows is NULL, so the diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt index a987822179..5a59014405 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -25,21 +25,15 @@ class BookshelfPayloadTest { unmockkAll() } - private fun server() = - WebServer( - ServerConfig( - port = 0, - databasePath = "/nonexistent/test.db", - fileDirPath = "/tmp", - debugDatabasePath = "/nonexistent/debug.db", - debugEnablePath = "/nonexistent/debug-flag", - experimentsEnablePath = "/nonexistent/exp-flag", - clearCacheEnablePath = "/nonexistent/cs0-flag", - projectDatabasePath = "/nonexistent/recent-projects.db", - ), - ) - - /** One joined row: category, category description, title, book description, path. */ + private fun server() = WebServer(testServerConfig()) + + /** + * One joined row: category, category description, title, book description, path. + * + * These are canned cursor rows, so nothing here runs the real SQL -- `BookshelfQueryTest` covers + * the query itself against a real SQLite database, including the two columns both named + * `description` that this mock's positional convention would happily keep in step with a bug. + */ private fun database(vararg rows: Array): SQLiteDatabase { var index = -1 val cursor = @@ -219,10 +213,11 @@ class BookshelfPayloadTest { @Test fun `the JSON keeps the keys, nesting and explicit nulls the template was written against`() { val json = - server().gsonForTest.toJson( - server().readBookshelf( + String( + server().bookshelfJson( database(arrayOf("General", null, "A guide", null, "d/guide.pdf")), ), + Charsets.UTF_8, ) assertThat(json).isEqualTo( diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt new file mode 100644 index 0000000000..6c68822e27 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt @@ -0,0 +1,154 @@ +/* + * 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 . + */ + +package com.itsaky.androidide.localWebServer + +import android.database.sqlite.SQLiteDatabase +import com.google.common.truth.Truth.assertThat +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Runs the real query against a real SQLite database. + * + * The sibling `BookshelfPayloadTest` hands `readBookshelf` canned cursor rows, so the SQL itself -- + * its column order, its joins, its ORDER BY -- is never executed there. On a ticket whose whole + * subject is a query that passed on a desktop and failed on a device, that gap is the one worth + * closing: the SELECT list has two columns both named `description` (`BC.description` at index 1, + * `B.description` at index 3) read by bare positional index, so inserting or reordering a column + * silently swaps category descriptions onto books, and a mock encoding the same convention shifts + * with it and keeps passing. + */ +@RunWith(RobolectricTestRunner::class) +class BookshelfQueryTest { + private lateinit var database: SQLiteDatabase + + @Before + fun setUp() { + database = SQLiteDatabase.create(null) + database.execSQL("CREATE TABLE ContentTypes (id INTEGER PRIMARY KEY, value TEXT, compression TEXT)") + database.execSQL( + "CREATE TABLE Content (id INTEGER PRIMARY KEY, path TEXT, languageID INTEGER, " + + "content BLOB, contentTypeID INTEGER, templateId INTEGER)", + ) + database.execSQL("CREATE TABLE BookCategories (id INTEGER PRIMARY KEY, category TEXT, description TEXT)") + database.execSQL( + "CREATE TABLE Bookshelf (contentID INTEGER, bookCategoryID INTEGER, title TEXT, description TEXT)", + ) + } + + @After + fun tearDown() { + database.close() + } + + private fun book( + id: Int, + path: String, + categoryId: Int?, + title: String, + bookDescription: String?, + ) { + database.execSQL("INSERT INTO Content (id, path) VALUES (?, ?)", arrayOf(id, path)) + database.execSQL( + "INSERT INTO Bookshelf (contentID, bookCategoryID, title, description) VALUES (?, ?, ?, ?)", + arrayOf(id, categoryId, title, bookDescription), + ) + } + + private fun category( + id: Int, + name: String?, + description: String?, + ) = database.execSQL( + "INSERT INTO BookCategories (id, category, description) VALUES (?, ?, ?)", + arrayOf(id, name, description), + ) + + // The two description columns are the thing this pins: index 1 is the category's, index 3 is the + // book's. Reading them the other way round is invisible to a mock that encodes the same order. + @Test + fun `the category description and the book description do not swap`() { + category(1, "Java", "Books about Java") + book(10, "d/notes.pdf", 1, "Java Notes", "Compiled from Stack Overflow") + + val shelf = WebServer(testServerConfig()).readBookshelf(database) + + val java = shelf.result.single() + assertThat(java.description).isEqualTo("Books about Java") + assertThat(java.books.single().description).isEqualTo("Compiled from Stack Overflow") + assertThat(java.books.single().link).isEqualTo("d/notes.pdf") + } + + // The join is inner, deliberately: a book with no category is not on the shelf, which is what the + // JSON1 query did. It is also why the template's General section is reachable only for a category + // row that exists but has no label. + @Test + fun `a book with no category is not on the shelf`() { + category(1, "Java", null) + book(10, "d/a.pdf", 1, "Has a category", null) + book(11, "d/b.pdf", null, "Has none", null) + + val shelf = WebServer(testServerConfig()).readBookshelf(database) + + assertThat( + shelf.result + .single() + .books + .map { it.title }, + ).containsExactly("Has a category") + } + + // A category row that exists but has no label is the case IFNULL(BC.category, 'General') covered. + @Test + fun `a category row with no label files its books under General`() { + category(1, null, null) + book(10, "d/a.pdf", 1, "Unlabelled", null) + + val shelf = WebServer(testServerConfig()).readBookshelf(database) + + assertThat(shelf.result.single().category).isEqualTo("General") + } + + // Books come back sorted by title, which the JSON1 version did not do -- see readBookshelf's KDoc. + @Test + fun `books within a category come back ordered by title`() { + category(1, "Java", null) + book(11, "d/b.pdf", 1, "Java, Java, Java", null) + book(10, "d/a.pdf", 1, "Java Notes", null) + + val shelf = WebServer(testServerConfig()).readBookshelf(database) + + assertThat( + shelf.result + .single() + .books + .map { it.title }, + ).containsExactly("Java Notes", "Java, Java, Java") + .inOrder() + } + + @Test + fun `an empty database yields an empty shelf rather than throwing`() { + val shelf = WebServer(testServerConfig()).readBookshelf(database) + + assertThat(shelf.result).isEmpty() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/TestServerConfig.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/TestServerConfig.kt new file mode 100644 index 0000000000..2aba66319c --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/TestServerConfig.kt @@ -0,0 +1,20 @@ +package com.itsaky.androidide.localWebServer + +/** + * The `ServerConfig` every test in this package uses. + * + * Every path is given explicitly: `ServerConfig`'s own defaults reach for external storage, which a + * JVM test has no stub for. Shared rather than copied per class, so a new required field is a + * one-line fix instead of a hunt, and two fixtures cannot drift into subtly different servers. + */ +internal fun testServerConfig(port: Int = 0) = + ServerConfig( + port = port, + databasePath = "/nonexistent/test.db", + fileDirPath = "/tmp", + debugDatabasePath = "/nonexistent/debug.db", + debugEnablePath = "/nonexistent/debug-flag", + experimentsEnablePath = "/nonexistent/exp-flag", + clearCacheEnablePath = "/nonexistent/cs0-flag", + projectDatabasePath = "/nonexistent/recent-projects.db", + ) diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt index e68b2e05e4..29c6356568 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -46,17 +46,7 @@ class WebServerTest { unmockkAll() } - private fun testConfig(port: Int) = - ServerConfig( - port = port, - databasePath = "/nonexistent/test.db", - fileDirPath = "/tmp", - debugDatabasePath = "/nonexistent/debug.db", - debugEnablePath = "/nonexistent/debug-flag", - experimentsEnablePath = "/nonexistent/exp-flag", - clearCacheEnablePath = "/nonexistent/cs0-flag", - projectDatabasePath = "/nonexistent/recent-projects.db", - ) + private fun testConfig(port: Int) = testServerConfig(port) // ADFA-5153/ADFA-5220: the dictionary is gated on the MAJOR version the database declares, so // every test that expects the dictionary to load has to declare one. A relaxed mock answers the From 9cbab8b81bb4b15dbb5039ef8d74339a32f633f4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 11:43:06 -0700 Subject: [PATCH 07/10] ADFA-5179: Name the row the skip warning is about The warning said "Bookshelf row for content id {}" and logged cursor.getString(2), which is B.title -- the SELECT had no C.id. It pointed a reader at the wrong column while diagnosing the malformed-data case the branch exists for, and in that branch the title is often null too, so it identified nothing while claiming to. C.id is appended to the SELECT, not inserted: every read here is by positional index, so a column added anywhere else silently re-points the four above it. The cursor mock now answers a missing index with null rather than throwing, so a test row need only give the columns its case cares about, the way a NULL column would read. 308 app tests pass. Found in review of PR #1700. --- .../com/itsaky/androidide/localWebServer/WebServer.kt | 9 +++++++-- .../androidide/localWebServer/BookshelfPayloadTest.kt | 11 +++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index c3930d7719..626b95c1a1 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -1101,7 +1101,10 @@ SELECT BC.category, BC.description, B.title, B.description, - C.path + C.path, + -- Only for the diagnostic below. Appended, not inserted: every read here is by positional + -- index, so a column added anywhere else silently re-points the four above it. + C.id FROM Content AS C, Bookshelf AS B, BookCategories AS BC @@ -1139,7 +1142,9 @@ ORDER BY BC.category, // whole shelf rather than the one bad row. val path = cursor.getString(4) if (path == null) { - log.warn("Bookshelf row for content id {} has no path; skipping it.", cursor.getString(2)) + // Index 5, C.id -- the title at index 2 is not an id, and in this branch it is + // often null too, so it identified nothing while claiming to. + log.warn("Bookshelf row for content id {} has no path; skipping it.", cursor.getString(5)) continue } // BookCategories.category is nullable, so a book can be linked to a category row that diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt index 5a59014405..6c7e56730b 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -28,7 +28,7 @@ class BookshelfPayloadTest { private fun server() = WebServer(testServerConfig()) /** - * One joined row: category, category description, title, book description, path. + * One joined row: category, category description, title, book description, path, content id. * * These are canned cursor rows, so nothing here runs the real SQL -- `BookshelfQueryTest` covers * the query itself against a real SQLite database, including the two columns both named @@ -39,7 +39,9 @@ class BookshelfPayloadTest { val cursor = mockk(relaxed = true) { every { moveToNext() } answers { ++index < rows.size } - every { getString(any()) } answers { rows[index][firstArg()] } + // getOrNull, not [] -- the query has six columns and a row here need only give the + // ones its test cares about; a short row reads as NULL, the way SQLite would. + every { getString(any()) } answers { rows[index].getOrNull(firstArg()) } } return mockk(relaxed = true) { @@ -188,8 +190,9 @@ class BookshelfPayloadTest { val bookshelf = server().readBookshelf( database( - arrayOf("General", "", "Broken", "", null), - arrayOf("General", "", "Fine", "", "d/guide.pdf"), + // Six columns, as the query returns: the last is C.id, which the skip warning names. + arrayOf("General", "", "Broken", "", null, "4071"), + arrayOf("General", "", "Fine", "", "d/guide.pdf", "4072"), ), ) From fe66bbc88a950605847fcd61808a9244c255a8f1 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 12:16:03 -0700 Subject: [PATCH 08/10] ADFA-5179: Do not say "no rows" when rows were skipped The empty-shelf log claimed the query matched no rows. A row whose Content.path is NULL is skipped a few lines below, so the query can return rows and still leave nothing to serve -- in which case the line said something false about the database while diagnosing the case it exists for. It reports no categories now, and each skipped row already logs its own warning, which is what distinguishes the two states. Found in review of PR #1700. --- .../java/com/itsaky/androidide/localWebServer/WebServer.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 626b95c1a1..62ebfdf6ef 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -1058,7 +1058,10 @@ class WebServer( // Not an error -- the endpoint answers 200 with an empty shelf -- but it is indistinguishable // from a working shelf in a bug report, and it is the state ADFA-5204 produced. The query // this replaced surfaced it only by accident, as a 500 from reading a NULL blob. - log.info("Bookshelf query matched no rows; serving an empty shelf.") + // "no categories", not "no rows": a row whose Content.path is NULL is skipped above, so + // the query can return rows and still leave nothing to serve. Each skip logs its own + // warning, which is what tells the two cases apart. + log.info("No bookshelf categories to serve; serving an empty shelf.") } return gson.toJson(bookshelf).toByteArray(Charsets.UTF_8) } From 84988dcf159866d433aaa4af60d70d4f227f8b52 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 12:50:00 -0700 Subject: [PATCH 09/10] ADFA-5179: Record the JSON1 constraint where SQL authors will see it The constraint this ticket exists for lived only in readBookshelf's KDoc. docs/documentation-database.md opens with "read this before writing/editing SQL against this database" and carries the list of gotchas found writing these scripts, and said nothing about it -- so the next person writes JSON_OBJECT, validates it under sqlite3 3.44 where JSON1 is built in, and ships a 500 that only appears on a device. The bullet says which functions, what the failure looks like, that the desktop will not warn you, and that every reader of this database is exposed -- a docdb-studio script, ToolTipManager, and PluginDocumentationManager as much as WebServer. Prompted by reproducing it on hardware while testing PR #1707: /pr/bs returned HTTP 500 with "no such function: JSON_OBJECT" on a Galaxy Note 20 Ultra, minutes after the same database served the same query fine under the desktop CLI. --- docs/documentation-database.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/documentation-database.md b/docs/documentation-database.md index 3055c955f0..76c6b8a3ed 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -98,6 +98,7 @@ Schema changes and data edits happen **outside this repo**, in `OfflineDocumenta Some tickets (e.g. ADFA-5088) ship a one-off `.sql` script under `docs/docdb/` for a `docdb-studio` maintainer to run against the real database, rather than editing it directly through the tool. Gotchas found writing those scripts: +- **The system SQLite has no JSON1 on some devices, and your desktop does.** `JSON_OBJECT`, `JSON_GROUP_ARRAY` and friends compile fine under the `sqlite3` CLI (3.44 ships JSON1 built in) and then fail at runtime with `no such function: JSON_OBJECT` on real hardware — reproduced on a Galaxy Note 20 Ultra, where the Dynamic Bookshelf was an HTTP 500 until the query was rewritten (ADFA-5179). Nothing on the desktop side of the fence will warn you. Write plain relational SQL and assemble the JSON in Kotlin, and treat any device-only 500 from a new query as this until proven otherwise. Applies to every reader of this database, not just `WebServer`: a `docdb-studio` script, a tooltip query in `ToolTipManager`, and `PluginDocumentationManager` are all equally exposed. - **Keep each `.system` line simple.** The sqlite3 CLI's `.system` dot-command can hit a content-dependent shell-parsing failure when a line chains multiple operators (`;`, `&&`, `||`, parentheses) — it reproduces for some input strings and not others, so it won't necessarily show up in a quick test. Stick to one plain `command | pipe > file` per `.system` line. - **`.bail on` is required for `BEGIN`/`COMMIT` to actually mean atomic.** Without it, a mid-script SQL error prints to stderr but the script *keeps going* — including reaching the final `COMMIT`, which then persists whatever succeeded before the error (verified empirically, not just documented behavior). `.bail` also can't see `.system` shell failures directly, so a failed or empty Brotli payload (which leaves its target file missing or zero-length) needs its own check: insert its `READFILE()` into a throwaway `CREATE TEMP TABLE` guarded by `NOT NULL CHECK (length(content) > 0)` immediately before the real `Content` insert, turning that failure into a real SQL error `.bail` will catch. See `docs/docdb/ADFA-5088-preference-tooltips.sql` for the working pattern. - **Don't write Brotli payloads to bare `/tmp/*.br` filenames.** A fixed, guessable name directly under world-writable `/tmp` lets another local user pre-plant a symlink or race the write/read pair between the `.system echo | brotli` write and the `READFILE()` read (CWE-377). Create an owner-only working directory instead — `rm -rf` it, then `mkdir -m 700` it (the mode is set atomically at creation, with no window where it's briefly world-accessible) — write every payload under that directory, and remove it again before `COMMIT`. See the same script for the working pattern. From e861dfa8f9eb255939271245131ae775b4ccdcda Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 14:45:03 -0700 Subject: [PATCH 10/10] ADFA-5179: Sort by what the page shows, and name the change that hides a category Review findings, all in the parts of this change that alter what a user sees. The books were sorted on B.title but displayed as B.title ?: C.path, and under BINARY collation with NULLs first that is a different order: an untitled book rendered as its path at the top of its section, and every capitalised title sorted ahead of every lower-case one. ORDER BY now uses COALESCE(B.title, C.path) COLLATE NOCASE, so the sort key is the string on the page. The KDoc claimed the books were "genuinely sorted by title" with no qualification; the new test fails against the old clause, putting d/zebra.pdf first. A category whose books all have a NULL Content.path now disappears entirely. The old query emitted the section with "link": null in it -- broken but present -- because it built JSON per row before any filtering; skipping the row here happens before its group exists. The skip is still right, but it was a fourth behaviour change absent from the KDoc's list of three. Now listed, and pinned by a test. Both new log lines are gated on debugEnabled, like everything else on this path. On the database this ticket exists for -- every Bookshelf row joining to nothing -- the empty shelf is the steady state, so the info line fired on every page load, and the per-row warning was unbounded. BookshelfCategory.books is handed a copy. It is typed as a List but was the accumulator's own MutableList, which a future caller that keeps the map could mutate afterwards. Also: a test now pins that an empty shelf serialises to {"result":[]} rather than something blank -- instantiatePebbleTemplate throws for a blank or "null" context, and that guard sits outside this endpoint's try/catch, so the empty-page promise depended on a property nothing checked. And a comment miscounted the columns preceding C.id as four. 310 app tests pass. Found in review of PR #1700. --- .../androidide/localWebServer/WebServer.kt | 26 ++++++++-- .../localWebServer/BookshelfQueryTest.kt | 48 ++++++++++++++++++- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt index 62ebfdf6ef..f63a4e815d 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -1061,7 +1061,10 @@ class WebServer( // "no categories", not "no rows": a row whose Content.path is NULL is skipped above, so // the query can return rows and still leave nothing to serve. Each skip logs its own // warning, which is what tells the two cases apart. - log.info("No bookshelf categories to serve; serving an empty shelf.") + // debugEnabled, like every other log on this path: on the database this ticket exists for, + // where every Bookshelf row joins to nothing, the empty shelf is the steady state and this + // would write a line on every page load. + if (debugEnabled) log.info("No bookshelf categories to serve; serving an empty shelf.") } return gson.toJson(bookshelf).toByteArray(Charsets.UTF_8) } @@ -1085,6 +1088,12 @@ class WebServer( * and "Java Notes for Professionals" comes first by title (a space sorts before a comma). * Deterministic order is worth having, but it is a visible change, not a no-op. * + * A category whose books all have a NULL `Content.path` disappears from the page. The old query + * emitted the section with `"link": null` in it -- visibly broken, but present -- because the JSON + * was built per row before any filtering. Here the row is skipped before its group is created, so + * an entire category can vanish with only a log line to say so. Skipping a row that cannot be + * linked is still right; the section going with it is the part worth knowing. + * * The `pdf` flag is now case-insensitive. `SUBSTR(C.path, -4) == '.pdf'` compared under BINARY * collation, so a row at `books/Guide.PDF` was flagged 0 and rendered as a web link. No shipped * row spells the extension any other way -- checked with `GLOB '*.[Pp][Dd][Ff]'` -- so nothing @@ -1106,15 +1115,18 @@ SELECT BC.category, B.description, C.path, -- Only for the diagnostic below. Appended, not inserted: every read here is by positional - -- index, so a column added anywhere else silently re-points the four above it. + -- index, so a column added anywhere else silently re-points the five above it. C.id FROM Content AS C, Bookshelf AS B, BookCategories AS BC WHERE C.id = B.contentID AND B.bookCategoryID = BC.id +-- COALESCE and NOCASE so the sort key is the string the page shows: the title falls back to the +-- path when it is NULL, and BINARY collation would otherwise put every capitalised title ahead of +-- every lower-case one and NULL titles ahead of everything. ORDER BY BC.category, - B.title + COALESCE(B.title, C.path) COLLATE NOCASE """.trimIndent() // LinkedHashMap: the query's ORDER BY decides the order categories and books appear in, and @@ -1147,7 +1159,9 @@ ORDER BY BC.category, if (path == null) { // Index 5, C.id -- the title at index 2 is not an id, and in this branch it is // often null too, so it identified nothing while claiming to. - log.warn("Bookshelf row for content id {} has no path; skipping it.", cursor.getString(5)) + // Also gated: one line per malformed row per request is unbounded, and the rows do not + // change between requests. + if (debugEnabled) log.warn("Bookshelf row for content id {} has no path; skipping it.", cursor.getString(5)) continue } // BookCategories.category is nullable, so a book can be linked to a category row that @@ -1177,7 +1191,9 @@ ORDER BY BC.category, BookshelfCategory( category = category ?: uncategorizedLabel, description = group.description, - books = group.books, + // toList(): BookshelfCategory.books is a List, and handing over the accumulator's own + // MutableList would let a future caller that keeps the map mutate it afterwards. + books = group.books.toList(), ) }, ) diff --git a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt index 6c68822e27..407a126eda 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt @@ -63,7 +63,7 @@ class BookshelfQueryTest { id: Int, path: String, categoryId: Int?, - title: String, + title: String?, bookDescription: String?, ) { database.execSQL("INSERT INTO Content (id, path) VALUES (?, ?)", arrayOf(id, path)) @@ -145,6 +145,52 @@ class BookshelfQueryTest { .inOrder() } + // The sort key has to be the string the page shows, not the raw column: a NULL title displays as + // its path, and SQLite's BINARY collation would otherwise file every capitalised title ahead of + // every lower-case one. + @Test + fun `books are ordered by what the page displays, case-insensitively`() { + category(1, "Mixed", null) + book(10, "d/zebra.pdf", 1, null, null) + book(11, "d/b.pdf", 1, "Android Basics", null) + book(12, "d/c.pdf", 1, "apple guide", null) + + val titles = + WebServer(testServerConfig()) + .readBookshelf(database) + .result + .single() + .books + .map { it.title } + + assertThat(titles).containsExactly("Android Basics", "apple guide", "d/zebra.pdf").inOrder() + } + + // A row with no path cannot be linked, so it is skipped -- and with it goes its category, if that + // was the only book in it. Pinned because it is a behaviour change the old query did not make. + @Test + fun `a category whose only book has no path disappears from the shelf`() { + category(1, "Reference", null) + book(10, "unused", 1, "Broken", null) + database.execSQL("UPDATE Content SET path = NULL WHERE id = 10") + + val shelf = WebServer(testServerConfig()).readBookshelf(database) + + assertThat(shelf.result).isEmpty() + } + + // The empty payload must stay renderable: instantiatePebbleTemplate throws for a blank or "null" + // context, and that guard sits outside realHandleBsEndpoint's try/catch, so a blank here would be + // a 500 rather than the empty page this ticket promises. + @Test + fun `an empty shelf serialises to a renderable context, not blank or null`() { + val json = String(WebServer(testServerConfig()).bookshelfJson(database), Charsets.UTF_8) + + assertThat(json).isEqualTo("""{"result":[]}""") + assertThat(json.isBlank()).isFalse() + assertThat(json.trim()).isNotEqualTo("null") + } + @Test fun `an empty database yields an empty shelf rather than throwing`() { val shelf = WebServer(testServerConfig()).readBookshelf(database)