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 714219e460..d374f351e4 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.ContentTypeHeaders import com.itsaky.androidide.utils.DatabaseVersionResolver @@ -60,6 +61,41 @@ data class ServerConfig( val projectDatabasePath: String = "/data/data/com.itsaky.androidide/databases/RecentProject_database", ) +/** + * 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( + @SerializedName("result") val result: List, +) + +internal data class BookshelfCategory( + @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 +// BookshelfCategory's once every row has been read. +private class CategoryGroup( + val description: String?, + val books: MutableList = mutableListOf(), +) + +internal data class BookshelfBook( + @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. */ + @SerializedName("pdf") val pdf: Int, +) + data class JavaExecutionResult( val compileOutput: String, val runOutput: String, @@ -156,14 +192,21 @@ 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 + private var bookshelfTemplateId: Int = -1 private val httpInternalServerError = 500 private val httpNotFound = 404 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( @@ -989,67 +1032,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 = bookshelfJson(database) 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") @@ -1064,6 +1068,162 @@ ORDER BY BC.category, 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. + // "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. + // 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) + } + + /** + * 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. + * + * 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. + * + * 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 + * 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 + * 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 { + // 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 BC.category, + BC.description, + B.title, + 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 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, + COALESCE(B.title, C.path) COLLATE NOCASE + """.trimIndent() + + // LinkedHashMap: the query's ORDER BY decides the order categories and books appear in, and + // the template renders them in that order. + // + // 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. + // 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) { + // 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. + // 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 + // 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) + + categories + .getOrPut(category) { CategoryGroup(cursor.getString(1)) } + .books + .add( + BookshelfBook( + // 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. + pdf = if (path.endsWith(".pdf", ignoreCase = true)) 1 else 0, + ), + ) + } + } + + return Bookshelf( + categories.map { (category, group) -> + BookshelfCategory( + category = category ?: uncategorizedLabel, + description = group.description, + // 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(), + ) + }, + ) + } + 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..6c7e56730b --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfPayloadTest.kt @@ -0,0 +1,231 @@ +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 io.mockk.unmockkAll +import org.junit.After +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 { + // 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(testServerConfig()) + + /** + * 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 + * `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 = + mockk(relaxed = true) { + every { moveToNext() } answers { ++index < rows.size } + // 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) { + 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 `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") + } + + // 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 = + server().readBookshelf( + database(arrayOf("General", "", null, "", "i/index.html")), + ) + + assertThat( + bookshelf.result + .single() + .books + .single() + .title, + ).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( + // 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"), + ), + ) + + 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 + // 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 = + String( + server().bookshelfJson( + database(arrayOf("General", null, "A guide", null, "d/guide.pdf")), + ), + Charsets.UTF_8, + ) + + assertThat(json).isEqualTo( + """{"result":[{"category":"General","description":null,""" + + """"books":[{"title":"A guide","description":null,"link":"d/guide.pdf","pdf":1}]}]}""", + ) + } +} 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..407a126eda --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/BookshelfQueryTest.kt @@ -0,0 +1,200 @@ +/* + * 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() + } + + // 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) + + 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 3bbb403556..0515bf05fb 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -47,17 +47,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 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.