From 00493e12e7179bb8f031133ba1c83529b319ac03 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 14:33:45 -0700 Subject: [PATCH 01/11] ADFA-5405: Add a Pebble loader that reads the Templates table Pebble's StringLoader treats the name it is handed as the template body, so a cross-reference resolves to itself: {% include "nav.peb" %} renders the literal text "nav.peb", with no exception and no log line. That caps the web server at one self-contained template per page. This loader resolves a name against Templates.name instead, so extends, include, import and embed all work. A name with no row throws LoaderException naming it. Not wired up yet -- the next commit switches the engine over to it. --- .../documentation/DatabaseTemplateLoader.kt | 83 +++++++++++++++++++ .../DatabaseTemplateLoaderTest.kt | 76 +++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt create mode 100644 common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt new file mode 100644 index 0000000000..183ef44dbc --- /dev/null +++ b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt @@ -0,0 +1,83 @@ +/* + * This file is part of Code on the Go. + * + * 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.documentation + +import android.database.sqlite.SQLiteDatabase +import io.pebbletemplates.pebble.error.LoaderException +import io.pebbletemplates.pebble.loader.Loader +import java.io.Reader +import java.io.StringReader + +/** + * Resolves Pebble template names against the `Templates` table, so a template can pull in another + * one with `extends`, `include`, `import` or `embed` (ADFA-5405). + * + * This replaces Pebble's `StringLoader`, which treats the name it is handed *as* the template body. + * That works for one self-contained template and silently breaks every cross-reference: + * `{% include "nav.peb" %}` asks the loader for "nav.peb", `StringLoader` hands back those eight + * characters as a template, and the page renders the literal text instead of the partial -- no + * exception, no log line. + * + * Names are `Templates.name` values, matched exactly: the table is a flat namespace with no + * directories, so there is no prefix, suffix or relative path to apply. + * + * @param database Supplies the database to read, or null when none is open. Called on every + * resolution rather than captured, because the source swaps the handle when a newer database + * appears; callers resolve under the read lock that a swap excludes, so the handle cannot change + * mid-render. + */ +internal class DatabaseTemplateLoader( + private val database: () -> SQLiteDatabase?, +) : Loader { + override fun getReader(name: String): Reader { + val database = database() ?: throw LoaderException(null, "No documentation database is open, for template '$name'") + + return database.rawQuery(TEMPLATE_QUERY, arrayOf(name)).use { cursor -> + if (!cursor.moveToFirst()) { + throw LoaderException(null, "Template '$name' not found in the database") + } + StringReader(cursor.getBlob(0).toString(Charsets.UTF_8)) + } + } + + override fun resourceExists(name: String): Boolean { + val database = database() ?: return false + + return database.rawQuery(TEMPLATE_QUERY, arrayOf(name)).use { it.moveToFirst() } + } + + override fun createCacheKey(name: String): String = name + + /** The table is a flat namespace, so a reference resolves to itself -- as with Pebble's own `MemoryLoader`. */ + override fun resolveRelativePath( + relativePath: String, + anchorPath: String, + ): String = relativePath + + /** Template bodies are stored as UTF-8 blobs, so the engine's charset setting does not apply. */ + override fun setCharset(charset: String) = Unit + + /** Names are exact `Templates.name` values; decorating them would stop them matching. */ + override fun setPrefix(prefix: String) = Unit + + override fun setSuffix(suffix: String) = Unit + + private companion object { + private const val TEMPLATE_QUERY = "SELECT content FROM Templates WHERE name = ?" + } +} diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt new file mode 100644 index 0000000000..b975792087 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt @@ -0,0 +1,76 @@ +package com.itsaky.androidide.documentation + +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.pebbletemplates.pebble.error.LoaderException +import org.junit.Assert.assertThrows +import org.junit.Test + +/** + * Covers the loader that lets one template reference another (ADFA-5405): a name resolves to the + * `Templates` row that carries it, and a name with no row fails loudly instead of resolving to + * itself the way Pebble's `StringLoader` did. + */ +class DatabaseTemplateLoaderTest { + private fun database(vararg templates: Pair): SQLiteDatabase = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } answers + { + val name = (secondArg>())[0] + val body = templates.toMap()[name] + mockk(relaxed = true) { + every { moveToFirst() } returns (body != null) + if (body != null) every { getBlob(0) } returns body.toByteArray() + } + } + } + + private fun loader(database: SQLiteDatabase?) = DatabaseTemplateLoader { database } + + @Test + fun `a name resolves to its template row`() { + val reader = loader(database("nav.peb" to "[nav]")).getReader("nav.peb") + + assertThat(reader.readText()).isEqualTo("[nav]") + } + + @Test + fun `a name with no row fails, rather than resolving to itself`() { + val loader = loader(database("nav.peb" to "[nav]")) + + val thrown = assertThrows(LoaderException::class.java) { loader.getReader("missing.peb") } + + assertThat(thrown).hasMessageThat().contains("missing.peb") + } + + @Test + fun `a resolution with no database open fails`() { + val loader = loader(null) + + assertThrows(LoaderException::class.java) { loader.getReader("nav.peb") } + assertThat(loader.resourceExists("nav.peb")).isFalse() + } + + @Test + fun `existence follows the table`() { + val loader = loader(database("nav.peb" to "[nav]")) + + assertThat(loader.resourceExists("nav.peb")).isTrue() + assertThat(loader.resourceExists("missing.peb")).isFalse() + } + + @Test + fun `names are used verbatim, since the table is a flat namespace`() { + val loader = loader(database("nav.peb" to "[nav]")) + loader.setPrefix("templates/") + loader.setSuffix(".peb") + loader.setCharset("ISO-8859-1") + + assertThat(loader.createCacheKey("nav.peb")).isEqualTo("nav.peb") + assertThat(loader.resolveRelativePath("nav.peb", "k/html/page.peb")).isEqualTo("nav.peb") + assertThat(loader.getReader("nav.peb").readText()).isEqualTo("[nav]") + } +} From e9050931b1f712537bc6f3dfbdd5784b1c4de547 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 14:34:46 -0700 Subject: [PATCH 02/11] ADFA-5405: Load templates by name, so they can reference each other The engine now loads through DatabaseTemplateLoader instead of StringLoader, so an author can build a page out of several Templates rows -- a layout to extend, a nav partial to include -- instead of one self-contained file. Content rows still name their outermost template by id, so render() resolves the id to a name and lets the engine load and cache from there. That drops our own compiled-template map: the engine already caches by name, which is the better key anyway, since a partial shared by many pages is then compiled once. Both caches are dropped on a database swap, the engine's included -- it caches by name, so a template edited under the same name would otherwise survive one. A reference to a name with no row now fails the request. It used to render the name as text. --- .../DocumentationContentSource.kt | 44 ++++---- .../DocumentationContentSourceTest.kt | 106 ++++++++++++------ docs/documentation-database.md | 4 +- 3 files changed, 98 insertions(+), 56 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 79d5c574b3..45954e82c6 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -26,8 +26,6 @@ import com.google.gson.ToNumberPolicy import com.google.gson.reflect.TypeToken import com.itsaky.androidide.utils.DatabaseVersionResolver import io.pebbletemplates.pebble.PebbleEngine -import io.pebbletemplates.pebble.loader.StringLoader -import io.pebbletemplates.pebble.template.PebbleTemplate import org.slf4j.LoggerFactory import java.io.ByteArrayInputStream import java.io.Closeable @@ -203,10 +201,14 @@ class DocumentationContentSource( private var compressionDictionary: ByteBuffer? = null private var compressionDictionaryStale = true - private val pebbleEngine = PebbleEngine.Builder().loader(StringLoader()).build() + // The loader reads Templates rows, so a template can reference another one (ADFA-5405). It also + // makes the engine's own cache the compiled-template cache, keyed by name: a partial pulled in + // by several pages is compiled once, and dropping a database means invalidating that cache too. + private val pebbleEngine = PebbleEngine.Builder().loader(DatabaseTemplateLoader { database }).build() - // Compiled templates for the active database, cleared when it is swapped. - private val templateCache = ConcurrentHashMap() + // Template names by id, for the active database. Content rows reference a template by id; every + // reference between templates is by name, which is what the loader and the engine cache use. + private val templateNames = ConcurrentHashMap() private val gson: Gson = GsonBuilder() @@ -335,10 +337,13 @@ class DocumentationContentSource( ): ByteArray = withDatabase { database -> render(database, templateId, contextJson, path) } /** - * Clears all cached compiled templates. + * Clears all cached templates, compiled and by name. */ fun clearTemplateCache() { - templateCache.clear() + templateNames.clear() + // The engine caches by name, so a template edited under the same name would otherwise + // survive here even though its row has changed. + pebbleEngine.templateCache.invalidateAll() } /** The last-modified time of [file], or -1 when it does not exist. */ @@ -444,11 +449,12 @@ class DocumentationContentSource( contextJson: ByteArray, path: String, ): ByteArray { - val template = - templateCache.getOrPut(templateId) { - if (log.isDebugEnabled) log.debug("Template cache miss for id {}, path '{}'.", templateId, path) - compileTemplate(database, templateId, path) + val name = + templateNames.getOrPut(templateId) { + if (log.isDebugEnabled) log.debug("Template name cache miss for id {}, path '{}'.", templateId, path) + templateName(database, templateId, path) } + val template = pebbleEngine.getTemplate(name) val contextString = contextJson.toString(Charsets.UTF_8) if (contextString.isBlank() || contextString.trim() == "null") { @@ -460,19 +466,19 @@ class DocumentationContentSource( } /** - * Compiles the template identified by the given ID. + * Resolves a template id to the name the engine loads it by. * * @param templateId The database identifier of the template. * @param path The content path associated with the template. - * @return The compiled template. + * @return The template's name. * @throws IllegalStateException If the template is missing or has multiple database rows. */ - private fun compileTemplate( + private fun templateName( database: SQLiteDatabase, templateId: Int, path: String, - ): PebbleTemplate = - database.rawQuery("SELECT content FROM Templates WHERE id = ?", arrayOf(templateId.toString())).use { cursor -> + ): String = + database.rawQuery("SELECT name FROM Templates WHERE id = ?", arrayOf(templateId.toString())).use { cursor -> when { cursor.count > 1 -> { throw IllegalStateException("Template ID $templateId is shared by more than one template") @@ -483,9 +489,7 @@ class DocumentationContentSource( } else -> { - val body = cursor.getBlob(0) - if (log.isDebugEnabled) log.debug("Compiling template {}, {} bytes.", templateId, body.size) - pebbleEngine.getTemplate(body.toString(Charsets.UTF_8)) + cursor.getString(0) } } } @@ -739,7 +743,7 @@ class DocumentationContentSource( activeDatabasePath = path databaseTimestamp = timestamp compressionDictionaryStale = true - templateCache.clear() + clearTemplateCache() generation++ try { diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt index fb8bfe09e8..77b1f9bdc0 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt @@ -285,17 +285,7 @@ class DocumentationContentSourceTest { @Test fun `a templated row comes back rendered, so no caller needs the template engine`() { - val database = - mockk(relaxed = true) { - every { rawQuery(match { it.contains("FROM Content") }, any()) } returns - contentCursor(bytes = """{"who": "Kotlin"}""".toByteArray(), templateId = 7) - every { rawQuery(match { it.contains("FROM Templates") }, arrayOf("7")) } returns - mockk(relaxed = true) { - every { count } returns 1 - every { moveToFirst() } returns true - every { getBlob(0) } returns "Hello {{ who }}!".toByteArray() - } - } + val database = templatedDatabase("page.peb" to "Hello {{ who }}!") every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database val lookup = source().lookup("k/html/basic-syntax.html") @@ -305,24 +295,55 @@ class DocumentationContentSourceTest { } @Test - fun `a template is compiled once and reused for the next page that needs it`() { + fun `a template pulls in another one by name, so pages can share partials`() { val database = - mockk(relaxed = true) { - every { rawQuery(match { it.contains("FROM Content") }, any()) } returns - contentCursor(bytes = """{"who": "Kotlin"}""".toByteArray(), templateId = 7) - every { rawQuery(match { it.contains("FROM Templates") }, arrayOf("7")) } returns - mockk(relaxed = true) { - every { count } returns 1 - every { moveToFirst() } returns true - every { getBlob(0) } returns "Hello {{ who }}!".toByteArray() - } - } + templatedDatabase( + "page.peb" to """Hello {{ who }}! {% include "nav.peb" %}""", + "nav.peb" to "[nav]", + ) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + val lookup = source().lookup("k/html/basic-syntax.html") + + assertThat((lookup as DocumentationLookup.Found).content.bytes.toString(Charsets.UTF_8)) + .isEqualTo("Hello Kotlin! [nav]") + } + + @Test + fun `a template inherits a layout, filling in its blocks`() { + val database = + templatedDatabase( + "page.peb" to """{% extends "layout.pebble" %}{% block body %}Hello {{ who }}!{% endblock %}""", + "layout.pebble" to "
{% block body %}{% endblock %}
", + ) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + val lookup = source().lookup("k/html/basic-syntax.html") + + assertThat((lookup as DocumentationLookup.Found).content.bytes.toString(Charsets.UTF_8)) + .isEqualTo("
Hello Kotlin!
") + } + + // The ADFA-5405 regression: with Pebble's StringLoader the reference resolved to itself, so the + // page rendered the literal text "nav.peb" and nothing said the partial was missing. + @Test + fun `a reference to a template that is not in the database fails the lookup`() { + val database = templatedDatabase("page.peb" to """Hello! {% include "nav.peb" %}""") + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + assertThat(source().lookup("k/html/basic-syntax.html")).isInstanceOf(DocumentationLookup.Failed::class.java) + } + + @Test + fun `a template is compiled once and reused for the next page that needs it`() { + val database = templatedDatabase("page.peb" to "Hello {{ who }}!") every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database val source = source() repeat(3) { source.lookup("k/html/basic-syntax.html") } - verify(exactly = 1) { database.rawQuery(match { it.contains("FROM Templates") }, arrayOf("7")) } + verify(exactly = 1) { database.rawQuery(match { it.contains("WHERE id = ?") }, arrayOf("7")) } + verify(exactly = 1) { database.rawQuery(match { it.contains("WHERE name = ?") }, arrayOf("page.peb")) } } @Test @@ -331,11 +352,7 @@ class DocumentationContentSourceTest { mockk(relaxed = true) { every { rawQuery(match { it.contains("FROM Content") }, any()) } returns contentCursor(bytes = "{}".toByteArray(), templateId = 7) - every { rawQuery(match { it.contains("FROM Templates") }, arrayOf("7")) } returns - mockk(relaxed = true) { - every { count } returns 0 - every { moveToFirst() } returns false - } + every { rawQuery(match { it.contains("WHERE id = ?") }, arrayOf("7")) } returns missingRowCursor() } every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database @@ -367,8 +384,8 @@ class DocumentationContentSourceTest { @Test fun `a debug-database swap drops the compiled templates, so pages render from the new database`() { - val installed = templatedDatabase(template = "Hello {{ who }}!") - val debug = templatedDatabase(template = "Goodbye {{ who }}!") + val installed = templatedDatabase("page.peb" to "Hello {{ who }}!") + val debug = templatedDatabase("page.peb" to "Goodbye {{ who }}!") every { SQLiteDatabase.openDatabase(installedFile.absolutePath, isNull(), any()) } returns installed every { SQLiteDatabase.openDatabase(debugFile.absolutePath, isNull(), any()) } returns debug @@ -386,19 +403,40 @@ class DocumentationContentSourceTest { .isEqualTo("Goodbye Kotlin!") } - /** A database whose single Content row is templated with id 7, and whose template is [template]. */ - private fun templatedDatabase(template: String): SQLiteDatabase = + /** + * A database whose single Content row is templated with id 7, and whose `Templates` rows are + * [templates] as name-to-body pairs. Template id 7 is the first pair; the rest are reachable + * only by name, which is how one template references another. + */ + private fun templatedDatabase(vararg templates: Pair): SQLiteDatabase = mockk(relaxed = true) { every { rawQuery(match { it.contains("FROM Content") }, any()) } returns contentCursor(bytes = """{"who": "Kotlin"}""".toByteArray(), templateId = 7) - every { rawQuery(match { it.contains("FROM Templates") }, arrayOf("7")) } returns + every { rawQuery(match { it.contains("WHERE id = ?") }, arrayOf("7")) } returns mockk(relaxed = true) { every { count } returns 1 every { moveToFirst() } returns true - every { getBlob(0) } returns template.toByteArray() + every { getString(0) } returns templates.first().first + } + every { rawQuery(match { it.contains("WHERE name = ?") }, any()) } answers + { + val name = (secondArg>())[0] + val body = templates.toMap()[name] ?: return@answers missingRowCursor() + mockk(relaxed = true) { + every { count } returns 1 + every { moveToFirst() } returns true + every { getBlob(0) } returns body.toByteArray() + } } } + /** A cursor over no rows, for a `Templates` name or id that the database does not have. */ + private fun missingRowCursor() = + mockk(relaxed = true) { + every { count } returns 0 + every { moveToFirst() } returns false + } + @Test fun `a debug database that will not open leaves the installed one serving`() { val installed = database(contentCursor(bytes = "installed".toByteArray())) diff --git a/docs/documentation-database.md b/docs/documentation-database.md index 14360add71..cc8f9e204b 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -64,7 +64,7 @@ CREATE TABLE Tooltips ( - **`DocumentationDatabaseVersion(major, minor, patch, who, comment, changeTime)`** — the database's own semver (ADFA-5220), replacing the heuristics that used to infer the format from which tables happened to exist. **Exactly one row**: the version the file *is*, which `populate_db.py` replaces rather than appends. Nothing in the DDL enforces that, so both readers (`DatabaseVersionResolver.resolveMajorVersion` here, `database_major_version` in docdb-studio) take the newest row rather than the one with the highest `major` (the app orders by `changeTime DESC, rowid DESC`, so the greatest `changeTime` wins and `rowid` only breaks ties) — a rebuild from an older content set is a downgrade and has to read as one, which `MAX(major)` would get wrong. The app's reader additionally logs a warning when it finds more than one row; docdb-studio's does not. `populate_db.py` collapses a file that somehow accumulated several back to one, and `resolveMajorVersion` returns null for a database predating the table. `MAJOR >= 2` is what tells the app its brotli `Content` rows are dictionary-compressed; below that, `DocumentationContentSource` neither reads nor attaches `CompressionDictionary`. Gating on the declared version rather than on the table's presence matters in both directions: a database can carry the dictionary table while its content is still plain brotli (every row would then pay a failed dictionary decode before its plain one, on every request), and a migrated database that lost the table fails loudly instead of quietly. - **`CompressionDictionary(id, data)`** — single-row table (`id INTEGER PRIMARY KEY CHECK (id = 1)`) holding the raw Brotli dictionary every ADFA-5153-migrated `compression = 'brotli'` `Content` row is compressed against. Trained once, from a representative sample across the whole `Content` table, by `OfflineDocumentationTools`' `migrate_content_to_dictionary_brotli.py` / `populate_db.py` (never retrained after that — a dictionary-compressed row is only decodable against the exact dictionary it was compressed with, so replacing it would silently orphan every already-migrated row). Shipping the dictionary inside `documentation.db` itself, rather than as a separate bundled asset, keeps it version-locked to the content compressed against it. `DocumentationContentSource` loads it lazily -- not merely from opening or swapping databases, but on the first content fetch that needs it after the active database changes, and only when `DocumentationDatabaseVersion` declares `MAJOR >= 2` (see above) -- and caches it from then on, reloading again only on the next database change (a swap can bring in a database with a different dictionary or none, so it can't stay cached across one). Per row, it tries decoding with the dictionary attached first via brotli4j's `attachDictionary`, falling back to a plain decode on failure — needed both for a database predating this migration (no `CompressionDictionary` table at all) and for plugin-contributed rows within an otherwise-migrated database (see `PluginDocumentationManager` below). -- **`Templates(id, name, content)`** — Pebble template source, keyed by id (and by `name` for well-known templates like `bookshelf`). Referenced by `Content.templateId`. +- **`Templates(id, name, content)`** — Pebble template source. `Content.templateId` names a page's outermost template by id; every reference *between* templates is by `name` (ADFA-5405), which is also how `WebServer` reaches well-known templates like `bookshelf`. So a template can `extends`, `include`, `import` or `embed` another one by writing its `Templates.name` verbatim (`{% include "nav.peb" %}`) — the table is a flat namespace, with no directories, prefixes or suffixes applied. A reference to a name with no row fails the request rather than rendering the name; before ADFA-5405 the latter is exactly what happened, silently, because the engine's loader treated the name it was handed as the template body. - **`Bookshelf(contentID, bookCategoryID, title, description)`** / **`BookCategories(id, category, description)`** — the Dynamic Bookshelf: one row per "book" (PDF or similar), linked to its Tier 3 page via `contentID` -> `Content.id`. Two DB triggers keep `Bookshelf` in sync when a PDF row is inserted/deleted from `Content`; `title`/`description` don't come from those triggers and must be set by hand. Non-PDF books need a separate ingestion path (plugin-provided, e.g. via `PluginDocumentationManager`). - **`LastChange(documentationSet, changeTime, who)`** — audit trail for edits made through `docdb-studio`; not shown to end users. `DatabaseVersionResolver` reads the `documentationSet = 'wholedb'` row to report the DB's build/edit stamp in debug logging, falling back to the most recent row of any set if `'wholedb'` is missing. - Misc `ide_tooltip_table` and `PUCC` tables are historical/example artifacts — not part of the live lookup paths above. @@ -73,7 +73,7 @@ CREATE TABLE Tooltips ( Of the five sites below, only `DocumentationContentSource` and `ToolTipManager` open the file, with `SQLiteDatabase.openDatabase(..., OPEN_READONLY)` — `WebServer` and the interceptor read through the content source, and `PluginDocumentationManager` is the one write path, opening `OPEN_READWRITE` to install plugin content (see ADR 0001 for why raw SQLite is justified here instead of Room). -- **`common/.../documentation/DocumentationContentSource.kt`** — the one Tier 3 pipeline that reads this database: row lookup (a request target is matched raw first, since stored `Content.path` values are percent-encoded, with a percent-decoded fallback on a miss — shared by both transports so they agree on which pages exist), chunked-row reassembly, dictionary-aware Brotli decode gated on the declared documentation version, and the database swaps — to a newer sdcard debug database, and reopening the installed file when an asset install rewrites it in place under the cached handle — under a read/write lock so several threads can read while a swap cannot close the handle under them. It also renders the rows that are Pebble template contexts (`templateId > 0`, the Kotlin doc set's pages), so both transports below serve finished pages and neither needs the template engine itself. All of that logic exists once (ADFA-5176). +- **`common/.../documentation/DocumentationContentSource.kt`** — the one Tier 3 pipeline that reads this database: row lookup (a request target is matched raw first, since stored `Content.path` values are percent-encoded, with a percent-decoded fallback on a miss — shared by both transports so they agree on which pages exist), chunked-row reassembly, dictionary-aware Brotli decode gated on the declared documentation version, and the database swaps — to a newer sdcard debug database, and reopening the installed file when an asset install rewrites it in place under the cached handle — under a read/write lock so several threads can read while a swap cannot close the handle under them. It also renders the rows that are Pebble template contexts (`templateId > 0`, the Kotlin doc set's pages) — resolving each template's references to other templates against `Templates.name` as it goes — so both transports below serve finished pages and neither needs the template engine itself. All of that logic exists once (ADFA-5176). - **`common/.../documentation/DocumentationRequestInterceptor.kt`** — serves Tier 3 *in-process* for the app's WebViews (`HelpActivity`, the tooltip fragment, `FAQActivity`), through `WebViewClient.shouldInterceptRequest`, so a page's assets cost a database read instead of a TCP connection each (ADFA-5176). It matches the same `http://localhost:6174/...` URL space, so the strings.xml entries, `ToolTipManager`'s link builder and the `DocumentationExtension` contract need no changes; anything it declines — a `/pr/` endpoint, an unknown path, a failed read — falls through to `WebServer` unchanged. Both transports get their `Content-Type` charset from the same place, `ContentTypeHeaders` (ADFA-5241), so a row does not describe itself differently depending on which one served it. Set `/sdcard/Download/CodeOnTheGo.nointercept` to force documentation back onto the server. - **`app/.../localWebServer/WebServer.kt`** — serves Tier 3 over HTTP on port 6174, for WebViews that are not wired to the interceptor above and for the `/pr/` developer endpoints. It reads through `DocumentationContentSource`, so it holds no database, template engine or decode logic of its own; what remains here is HTTP: request parsing, the `/pr/` pages, error responses, and the CSS/asset shortcuts. Also serves a Dynamic Bookshelf JSON payload (joining `Content`/`Bookshelf`/`BookCategories`, rendered through the `bookshelf` template) and debug-only HTML dumps at `/pr/db` (`LastChange`, last 20 rows) and `/pr/pr` (recent projects, from a *different* database). - **`idetooltips/.../ToolTipManager.kt`** — serves Tier 1/2. Looks up `Tooltips` joined to `TooltipCategories` by `(category, tag)`, then `TooltipButtons` for the Tier 3 links shown at the bottom. From e3628225d602d503f4df694880edc1cb0e9445fa Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 14:40:26 -0700 Subject: [PATCH 03/11] ADFA-5405: Render the bookshelf by name, dropping its id cache Templates resolve by name now, so the bookshelf endpoint no longer has to look its id up and hold on to it: renderNamedTemplate takes the name straight. That removes the whole cache-coherency mechanism the cached id needed -- the volatile field, its lock, the generation it was tagged with, and the pre-serve refresh that existed only to make the generation check land on the right side of a swap. Nothing outside the content source caches per database any more, and the source applies a pending swap inside lookup()/withDatabase() itself. isCursorOneRow went with it; the id lookup was its only caller. --- .../androidide/localWebServer/WebServer.kt | 73 +------------------ .../localWebServer/WebServerTest.kt | 10 +-- .../DocumentationContentSource.kt | 46 ++++++++++-- .../DocumentationContentSourceTest.kt | 21 ++++++ 4 files changed, 67 insertions(+), 83 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 d5dc83eabd..039bb8b343 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -1,6 +1,5 @@ package com.itsaky.androidide.localWebServer -import android.database.Cursor import android.database.sqlite.SQLiteDatabase import android.net.TrafficStats import android.os.Environment.getExternalStorageDirectory @@ -148,18 +147,6 @@ class WebServer( .serializeNulls() .create() - // -1 means "not fetched yet". Volatile because the WebView transport shares this server's - // process, and the interceptor's reads can run on WebView threads while the accept loop writes. - @Volatile - private var bookshelfTemplateId: Int = -1 - - private val cacheLock = Any() - - // Which of the source's databases bookshelfTemplateId was filled from. The compiled templates - // themselves live in the source and are dropped by its own swap. - @Volatile - private var cachedDatabaseGeneration = 0L - // Long enough to stop a descriptor-exhaustion spin starving the connections whose closing would // fix it; short enough to be invisible to a user, and never paid on a successful accept. private val initialAcceptBackoffMs = 50L @@ -551,27 +538,6 @@ class WebServer( serveRequest(writer, output, path) } - /** - * Invalidates the cached bookshelf template identifier when the documentation database changes. - */ - private fun discardCachesIfDatabaseChanged() { - // Apply any pending swap first. The source swaps inside lookup()/withDatabase(), so checking - // the generation before those runs reads the generation from before the swap: on the very - // request that swaps, this would leave bookshelfTemplateId pointing at the previous - // database's template row -- rendering the old bookshelf, or 500ing if that id is absent. - contentSource.refreshDatabase() - - if (contentSource.generation == cachedDatabaseGeneration) return - - synchronized(cacheLock) { - val generation = contentSource.generation - if (generation == cachedDatabaseGeneration) return - - bookshelfTemplateId = -1 - cachedDatabaseGeneration = generation - } - } - /** * Serves a parsed request using the appropriate diagnostic endpoint or documentation content. * @@ -584,8 +550,6 @@ class WebServer( output: java.io.OutputStream, path: String, ) { - discardCachesIfDatabaseChanged() - // Handle the special "pr" endpoint with highest priority if (path.startsWith("pr/", false)) { if (debugEnabled) log.debug("Found a pr/ path, '{}'.", path) @@ -877,24 +841,9 @@ class WebServer( val jsonText = contentSource.withDatabase { database -> try { - val json = bookshelfJson(database) - if (debugEnabled) log.debug("json content = '{}'.", String(json, Charsets.UTF_8)) - if (debugEnabled) log.debug("before fetch bookshelf template ID = '{}'", bookshelfTemplateId) - - // Have we already fetched the template - if (bookshelfTemplateId == -1) { - database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()).use { cursor -> - if (!isCursorOneRow(cursor, writer, output)) { - return@withDatabase null - } - - cursor.moveToFirst() - bookshelfTemplateId = cursor.getInt(0) - if (debugEnabled) log.debug("after the fetch bookshelf template ID = '{}'", bookshelfTemplateId) - } + bookshelfJson(database).also { + if (debugEnabled) log.debug("json content = '{}'.", String(it, Charsets.UTF_8)) } - - json } catch (e: Exception) { log.error("Error processing request: {}", e.message) sendError(writer, output, httpInternalServerError, "Internal Server Error", e.message ?: "") @@ -902,7 +851,7 @@ class WebServer( } } ?: return false - val result = contentSource.renderTemplate(bookshelfTemplateId, jsonText, "/bookshelf") + val result = contentSource.renderNamedTemplate("bookshelf", jsonText, "/bookshelf") if (debugEnabled) log.debug("Bookshelf result is '{}'.", String(result)) @@ -1070,22 +1019,6 @@ ORDER BY BC.category, ) } - private fun isCursorOneRow( - cursor: Cursor, - writer: PrintWriter, - output: java.io.OutputStream, - ): Boolean { - if (cursor.count == 1) { - return true - } - if (cursor.count == 0) { - sendError(writer, output, httpNotFound, "Corrupt database, no rows found, expected one.") - } else { - sendError(writer, output, httpInternalServerError, "Corrupt database - found ${cursor.count} rows when 1 was expected.") - } - return false - } - /** * Builds an HTML table of recent projects from the provided project database and writes it to the client. * 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 1dc43258f0..907bbfe1f1 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -363,15 +363,9 @@ class WebServerTest { // The bookshelf join matches nothing: a cursor whose moveToNext() is immediately false. every { db.rawQuery(match { it.contains("FROM Content AS C") }, any()) } returns mockk(relaxed = true) { every { moveToNext() } returns false } - // The bookshelf template: its id lookup, then its body -- a Pebble expression over the JSON - // context, so the assertion proves the empty-shelf payload actually reached the render. + // The bookshelf template's body, fetched by name (ADFA-5405) -- a Pebble expression over the + // JSON context, so the assertion proves the empty-shelf payload actually reached the render. every { db.rawQuery(match { it.contains("FROM Templates WHERE name") }, any()) } returns - mockk(relaxed = true) { - every { count } returns 1 - every { moveToFirst() } returns true - every { getInt(0) } returns 7 - } - every { db.rawQuery(match { it.contains("FROM Templates WHERE id") }, any()) } returns mockk(relaxed = true) { every { count } returns 1 every { moveToFirst() } returns true diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 45954e82c6..be200e6b5f 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -173,8 +173,9 @@ class DocumentationContentSource( private var activeDatabasePath: String? = null /** - * Bumped on every swap, so a caller can tell that anything it cached from this source -- - * a compiled template, a looked-up template id -- belongs to a database that is gone. + * Bumped on every swap, so a caller can tell that anything it derived from this source belongs + * to a database that is gone. Nothing outside caches per database now that templates resolve by + * name and the caches for them live here; this stays as the observable that a swap happened. */ @Volatile var generation: Long = 0 @@ -336,6 +337,23 @@ class DocumentationContentSource( path: String, ): ByteArray = withDatabase { database -> render(database, templateId, contextJson, path) } + /** + * Renders the named template using the supplied JSON context. + * + * For a caller that knows a well-known template by name -- the bookshelf, say -- rather than + * through a `Content` row's `templateId`. + * + * @param name The template's `Templates.name`. + * @param contextJson The JSON object used as the template context. + * @param path The path associated with the rendering request for diagnostics. + * @return The rendered content encoded as UTF-8 bytes. + */ + fun renderNamedTemplate( + name: String, + contextJson: ByteArray, + path: String, + ): ByteArray = withDatabase { renderNamed(name, contextJson, path) } + /** * Clears all cached templates, compiled and by name. */ @@ -454,15 +472,33 @@ class DocumentationContentSource( if (log.isDebugEnabled) log.debug("Template name cache miss for id {}, path '{}'.", templateId, path) templateName(database, templateId, path) } - val template = pebbleEngine.getTemplate(name) + return renderNamed(name, contextJson, path) + } + + /** + * Renders the named template using the provided JSON context. + * + * Callers hold the read lock, since the loader the engine resolves through reads the active + * database -- for this template and for every one it references. + * + * @param name The template's `Templates.name`. + * @param contextJson The JSON-encoded context supplied to the template. + * @param path The content path associated with the rendering request. + * @return The rendered template content encoded as UTF-8 bytes. + */ + private fun renderNamed( + name: String, + contextJson: ByteArray, + path: String, + ): ByteArray { val contextString = contextJson.toString(Charsets.UTF_8) if (contextString.isBlank() || contextString.trim() == "null") { - throw IllegalStateException("Template ID $templateId has empty or null JSON context") + throw IllegalStateException("Template '$name' has empty or null JSON context, for path '$path'") } val context: Map = gson.fromJson(contextString, templateContextType) - return StringWriter().also { template.evaluate(it, context) }.toString().toByteArray() + return StringWriter().also { pebbleEngine.getTemplate(name).evaluate(it, context) }.toString().toByteArray() } /** diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt index 77b1f9bdc0..d527b2ff5b 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt @@ -334,6 +334,27 @@ class DocumentationContentSourceTest { assertThat(source().lookup("k/html/basic-syntax.html")).isInstanceOf(DocumentationLookup.Failed::class.java) } + @Test + fun `a well-known template renders by name, with no Content row of its own`() { + val database = templatedDatabase("bookshelf" to "Books for {{ who }}") + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + val rendered = source().renderNamedTemplate("bookshelf", """{"who": "Kotlin"}""".toByteArray(), "/bookshelf") + + assertThat(rendered.toString(Charsets.UTF_8)).isEqualTo("Books for Kotlin") + } + + @Test + fun `a template context that carries nothing is rejected rather than rendered`() { + val database = templatedDatabase("bookshelf" to "Books for {{ who }}") + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + val source = source() + + assertThrows(IllegalStateException::class.java) { + source.renderNamedTemplate("bookshelf", "null".toByteArray(), "/bookshelf") + } + } + @Test fun `a template is compiled once and reused for the next page that needs it`() { val database = templatedDatabase("page.peb" to "Hello {{ who }}!") From ccc34a1d8bb50a9358e0e9d687b98b616853afdc Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 15:03:04 -0700 Subject: [PATCH 04/11] ADFA-5405: Name the unresolvable template in the /pr/bs error handleBsEndpoint replaced the caught exception's message with generic text, so a bookshelf template the loader cannot resolve -- the row itself, or anything it references -- 500ed with nothing to identify it. The name is the whole point of failing loudly. The generic text stays as the fallback for an exception with no message. Found in review of #1779. --- .../androidide/localWebServer/WebServer.kt | 12 ++++++++- .../localWebServer/WebServerTest.kt | 27 +++++++++++++++++++ 2 files changed, 38 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 039bb8b343..3233e63b58 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -768,7 +768,17 @@ class WebServer( outputStarted = realHandleBsEndpoint(writer, output) { outputStarted = true } } catch (e: Exception) { log.error("Error handling /pr/bs endpoint: {}", e.message) - sendError(writer, output, httpInternalServerError, "Internal Server Error 6", "Error generating bookshelf HTML.", outputStarted) + // The message, not just the generic text: a template this endpoint cannot resolve -- the + // bookshelf row itself, or anything it references -- is named by the loader's throw, and + // that name is the whole diagnostic (ADFA-5405). + sendError( + writer, + output, + httpInternalServerError, + "Internal Server Error 6", + e.message ?: "Error generating bookshelf HTML.", + outputStarted, + ) } if (debugEnabled) log.debug("Leaving handleBsEndpoint().") 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 907bbfe1f1..0fc06bded1 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -386,6 +386,33 @@ class WebServerTest { } } + // ADFA-5405: a template the endpoint cannot resolve -- the bookshelf row, or anything it + // references -- is named by the loader's throw, and handleBsEndpoint has to pass that name on + // rather than replace it with its generic text. The name is the whole diagnostic. + @Test + fun `a bookshelf template that is not in the database answers 500 naming it`() { + val port = freePort() + val db = mockk(relaxed = true) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns db + every { db.rawQuery(match { it.contains("FROM Content AS C") }, any()) } returns + mockk(relaxed = true) { every { moveToNext() } returns false } + // No bookshelf row: a relaxed cursor's moveToFirst() is already false. + every { db.rawQuery(match { it.contains("FROM Templates WHERE name") }, any()) } returns mockk(relaxed = true) + + val server = WebServer(testConfig(port)) + val serverThread = Thread { server.start() }.apply { isDaemon = true } + serverThread.start() + try { + awaitPortBound(port) + val response = sendRawGetRequest(port, "/pr/bs") + assertTrue("Expected a 500 status line, got:\n$response", response.startsWith("HTTP/1.1 500")) + assertTrue("Expected the missing template to be named, got:\n$response", response.contains("bookshelf")) + } finally { + server.stop() + serverThread.join(2_000) + } + } + // ADFA-5241: the two transports have to answer the same way about what a response says, and // only a real response proves what this one sends. The decision itself lives in // ContentTypeHeaders, shared with DocumentationRequestInterceptor. From 6e6d865c69fd9de5f682a4e0d809c2f05bcf40e8 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 15:42:54 -0700 Subject: [PATCH 05/11] ADFA-5405: Handle a reference cycle, and clean up the render diagnostics Review follow-ups on #1779. A cycle between two templates is reachable now that a reference resolves to another row, and Pebble has no cycle detection: it recurses until the stack ends. StackOverflowError is an Error, so every catch on the serving path passes it through and the client gets a closed socket with no status line. Raised as an IllegalStateException naming the template instead. The listener already survived this (the accept loop catches Throwable), so this is about the response. PebbleException formats getMessage() as " (:)" and the loader throws with both null, so /pr/bs was answering "... not found in the database (?:?)". Translated to getPebbleMessage() in the content source, which also keeps Pebble out of both transports. clearTemplateCache() left the tag cache populated, so a template's {% cache %} blocks would outlive the database swap the rest of the method exists to handle. Dropped renderTemplate(): the id-keyed entry point had one caller, which the previous commit moved to renderNamedTemplate, and no test uses it. Said plainly in the KDoc that generation and refreshDatabase have no production reader. The regression test from the previous commit asserted the body contained "bookshelf", which the generic fallback text does too -- it passed with or without the fix. It now asserts the loader's own sentence, and fails without it. --- .../localWebServer/WebServerTest.kt | 9 +++- .../DocumentationContentSource.kt | 46 ++++++++++--------- .../DocumentationContentSourceTest.kt | 33 ++++++++++++- 3 files changed, 65 insertions(+), 23 deletions(-) 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 0fc06bded1..f86d131d10 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -406,7 +406,14 @@ class WebServerTest { awaitPortBound(port) val response = sendRawGetRequest(port, "/pr/bs") assertTrue("Expected a 500 status line, got:\n$response", response.startsWith("HTTP/1.1 500")) - assertTrue("Expected the missing template to be named, got:\n$response", response.contains("bookshelf")) + // The loader's own text, not just the template name: the generic fallback on the same + // sendError call is "Error generating bookshelf HTML.", which contains "bookshelf" too, + // so asserting on the name alone passes with or without the fix. + assertTrue( + "Expected the loader's diagnostic, got:\n$response", + response.contains("Template 'bookshelf' not found in the database"), + ) + assertFalse("Expected no Pebble placeholder padding, got:\n$response", response.contains("(?:?)")) } finally { server.stop() serverThread.join(2_000) diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index be200e6b5f..5d2eda60b3 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -26,6 +26,7 @@ import com.google.gson.ToNumberPolicy import com.google.gson.reflect.TypeToken import com.itsaky.androidide.utils.DatabaseVersionResolver import io.pebbletemplates.pebble.PebbleEngine +import io.pebbletemplates.pebble.error.PebbleException import org.slf4j.LoggerFactory import java.io.ByteArrayInputStream import java.io.Closeable @@ -173,9 +174,9 @@ class DocumentationContentSource( private var activeDatabasePath: String? = null /** - * Bumped on every swap, so a caller can tell that anything it derived from this source belongs - * to a database that is gone. Nothing outside caches per database now that templates resolve by - * name and the caches for them live here; this stays as the observable that a swap happened. + * Bumped on every swap. Nothing outside caches per database now that templates resolve by name + * and the caches for them live here, so this has no production reader: it stays as the + * observable a test asserts a swap happened on. */ @Volatile var generation: Long = 0 @@ -297,7 +298,9 @@ class DocumentationContentSource( /** * Ensures the documentation database is open and applies any pending database changes. * - * Does nothing when the source is closed or the database cannot be opened. + * Does nothing when the source is closed or the database cannot be opened. No production caller: + * [lookup] and [withDatabase] apply a pending swap themselves, so this is the seam a test uses to + * drive one directly. */ fun refreshDatabase() { if (!openIfNeeded()) return @@ -323,20 +326,6 @@ class DocumentationContentSource( } } - /** - * Renders a template using the supplied JSON context. - * - * @param templateId The identifier of the template to render. - * @param contextJson The JSON object used as the template context. - * @param path The path associated with the rendering request for diagnostics. - * @return The rendered content encoded as UTF-8 bytes. - */ - fun renderTemplate( - templateId: Int, - contextJson: ByteArray, - path: String, - ): ByteArray = withDatabase { database -> render(database, templateId, contextJson, path) } - /** * Renders the named template using the supplied JSON context. * @@ -359,9 +348,11 @@ class DocumentationContentSource( */ fun clearTemplateCache() { templateNames.clear() - // The engine caches by name, so a template edited under the same name would otherwise - // survive here even though its row has changed. + // Both engine caches are keyed by name, so a template edited under the same name would + // otherwise survive here even though its row has changed -- the tag cache included, which + // holds whatever a template's {% cache %} blocks rendered from the previous database. pebbleEngine.templateCache.invalidateAll() + pebbleEngine.tagCache.invalidateAll() } /** The last-modified time of [file], or -1 when it does not exist. */ @@ -498,7 +489,20 @@ class DocumentationContentSource( } val context: Map = gson.fromJson(contextString, templateContextType) - return StringWriter().also { pebbleEngine.getTemplate(name).evaluate(it, context) }.toString().toByteArray() + return try { + StringWriter().also { pebbleEngine.getTemplate(name).evaluate(it, context) }.toString().toByteArray() + } catch (e: PebbleException) { + // getPebbleMessage(), not message: PebbleException formats the latter as + // " (:)" and the loader throws with both null, so a caller that puts + // message in a response body gets a trailing "(?:?)". + throw IllegalStateException(e.pebbleMessage ?: e.message, e) + } catch (e: StackOverflowError) { + // Templates can reference each other now (ADFA-5405), so they can also reference each + // other in a cycle, which Pebble resolves by recursing until the stack runs out. Raised + // here as an exception because an Error passes through every catch on this path: the + // client would get a closed socket with no status line and nothing naming the template. + throw IllegalStateException("Rendering template '$name' overflowed the stack; check for a reference cycle between templates", e) + } } /** diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt index d527b2ff5b..aa003111f3 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt @@ -19,7 +19,7 @@ import java.io.File /** * Covers the pipeline both documentation transports read through (ADFA-5176): what a lookup reports, * how chunked rows are reassembled, and what a debug-database swap does to the handle and to the - * generation counter callers use to drop their per-database caches. + * generation counter these tests read a swap off. * * The database itself is a mock. These tests are about this class's decisions, and a real * SQLiteDatabase needs a device; the on-device behavior is covered by WebServerTest and by the @@ -355,6 +355,37 @@ class DocumentationContentSourceTest { } } + // ADFA-5405 made cycles reachable: before it, a reference resolved to itself and never recursed. + // Pebble has no cycle detection and resolves one by recursing until the stack ends, and a + // StackOverflowError is an Error -- it passes through every catch on the serving path, so the + // client would get a closed socket with no status line. + @Test + fun `a reference cycle between templates fails the lookup instead of unwinding the stack`() { + val database = + templatedDatabase( + "page.peb" to """{% include "other.peb" %}""", + "other.peb" to """{% include "page.peb" %}""", + ) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + val lookup = source().lookup("k/html/basic-syntax.html") + + assertThat(lookup).isInstanceOf(DocumentationLookup.Failed::class.java) + assertThat((lookup as DocumentationLookup.Failed).cause).hasMessageThat().contains("reference cycle") + } + + // PebbleException formats its message as " (:)" and the loader throws with both + // null, so passing message straight to a response body appends "(?:?)". + @Test + fun `a failed render reports the engine's text without its placeholder padding`() { + val database = templatedDatabase("page.peb" to """Hello! {% include "nav.peb" %}""") + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + val lookup = source().lookup("k/html/basic-syntax.html") as DocumentationLookup.Failed + + assertThat(lookup.cause).hasMessageThat().isEqualTo("Template 'nav.peb' not found in the database") + } + @Test fun `a template is compiled once and reused for the next page that needs it`() { val database = templatedDatabase("page.peb" to "Hello {{ who }}!") From 59d7328b7166a9386d12c602b3258d2bee1ab6b1 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 15:44:24 -0700 Subject: [PATCH 06/11] ADFA-5405: Correct the stale swap comment on serveRequest The call it described, discardCachesIfDatabaseChanged(), was deleted two commits ago. The swap is applied by lookup()/withDatabase() inside the content source now, so a request reaching neither does not poll for one -- the opposite of what the comment led a reader to expect. --- .../java/com/itsaky/androidide/localWebServer/WebServer.kt | 3 ++- 1 file changed, 2 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 3233e63b58..171cdd4dcb 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -534,7 +534,8 @@ class WebServer( return sendError(writer, output, 501, "Not Implemented") } - // serveRequest applies any pending sdcard debug-database swap via the content source. + // The content source applies a pending sdcard debug-database swap inside lookup()/withDatabase(), + // so a request reaching neither -- an unknown /pr/ target -- does not poll for one. serveRequest(writer, output, path) } From cdaf2da9dd8e220543d2878e6de46977f8fd403d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 16:13:11 -0700 Subject: [PATCH 07/11] ADFA-5405: Keep a parse error's file and line, and bound a runaway render Second review pass on #1779. The getPebbleMessage() change in 6e6d865c6 traded one diagnostic loss for another: it strips PebbleException's "(:)" suffix from every exception, not just the loader's null/null ones, so a syntax error in a template said what was wrong but not which template or line. Now conditional on the exception actually carrying neither. The new test fails without it with 'Unexpected token "EXECUTE_END"' and no template name. maxRenderedSize bounds output that grows without end -- a runaway loop stays at one frame, so the StackOverflowError guard never sees it and OutOfMemoryError is an Error every catch on this path misses. Pebble raises a PebbleException at the limit instead. Not covered by a test: exercising it means rendering 16M chars. clearTemplateCache() now takes the write lock. The sentinel and the interceptor call it with no lock, so clearing three caches piecemeal under a concurrent render could hand it a template from before the clear and a tag-cache miss from after -- the mixed state the sentinel is pressed to escape. resourceExists() no longer copies the whole template blob into a CursorWindow to answer a boolean, and generation/refreshDatabase() are marked @VisibleForTesting rather than described as unused in prose. The Templates DDL is now in documentation-database.md. Two reviews read the bare column list there and concluded name has no UNIQUE constraint; it does -- SQLite resolves the single-quoted UNIQUE('name') to the column. --- .../documentation/DatabaseTemplateLoader.kt | 6 ++- .../DocumentationContentSource.kt | 50 ++++++++++++++----- .../DocumentationContentSourceTest.kt | 14 ++++++ docs/documentation-database.md | 2 +- 4 files changed, 57 insertions(+), 15 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt index 183ef44dbc..cb14690bd0 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt @@ -58,7 +58,10 @@ internal class DatabaseTemplateLoader( override fun resourceExists(name: String): Boolean { val database = database() ?: return false - return database.rawQuery(TEMPLATE_QUERY, arrayOf(name)).use { it.moveToFirst() } + // Not TEMPLATE_QUERY: that copies the whole template blob into a CursorWindow to answer a + // boolean. Pebble reaches this only through the delegating and servlet loaders, neither of + // which is wired here, so the cost would be invisible -- which is the reason to get it right. + return database.rawQuery(EXISTS_QUERY, arrayOf(name)).use { it.moveToFirst() } } override fun createCacheKey(name: String): String = name @@ -79,5 +82,6 @@ internal class DatabaseTemplateLoader( private companion object { private const val TEMPLATE_QUERY = "SELECT content FROM Templates WHERE name = ?" + private const val EXISTS_QUERY = "SELECT 1 FROM Templates WHERE name = ? LIMIT 1" } } diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 5d2eda60b3..53b7daa49e 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.documentation import android.database.sqlite.SQLiteDatabase +import androidx.annotation.VisibleForTesting import com.aayushatharva.brotli4j.Brotli4jLoader import com.aayushatharva.brotli4j.decoder.BrotliInputStream import com.google.gson.Gson @@ -178,6 +179,7 @@ class DocumentationContentSource( * and the caches for them live here, so this has no production reader: it stays as the * observable a test asserts a swap happened on. */ + @VisibleForTesting @Volatile var generation: Long = 0 private set @@ -206,7 +208,12 @@ class DocumentationContentSource( // The loader reads Templates rows, so a template can reference another one (ADFA-5405). It also // makes the engine's own cache the compiled-template cache, keyed by name: a partial pulled in // by several pages is compiled once, and dropping a database means invalidating that cache too. - private val pebbleEngine = PebbleEngine.Builder().loader(DatabaseTemplateLoader { database }).build() + private val pebbleEngine = + PebbleEngine + .Builder() + .loader(DatabaseTemplateLoader { database }) + .maxRenderedSize(MAX_RENDERED_CHARS) + .build() // Template names by id, for the active database. Content rows reference a template by id; every // reference between templates is by name, which is what the loader and the engine cache use. @@ -302,6 +309,7 @@ class DocumentationContentSource( * [lookup] and [withDatabase] apply a pending swap themselves, so this is the seam a test uses to * drive one directly. */ + @VisibleForTesting fun refreshDatabase() { if (!openIfNeeded()) return swapDatabaseIfChanged() @@ -346,14 +354,18 @@ class DocumentationContentSource( /** * Clears all cached templates, compiled and by name. */ - fun clearTemplateCache() { - templateNames.clear() - // Both engine caches are keyed by name, so a template edited under the same name would - // otherwise survive here even though its row has changed -- the tag cache included, which - // holds whatever a template's {% cache %} blocks rendered from the previous database. - pebbleEngine.templateCache.invalidateAll() - pebbleEngine.tagCache.invalidateAll() - } + fun clearTemplateCache() = + // The write lock, which a render's read lock excludes: clearing the three caches piecemeal + // under a concurrent render can hand it a template from before the clear and a tag-cache miss + // from after it. Reentrant, so switchToDatabase can keep calling this while holding it. + databaseLock.write { + templateNames.clear() + // Both engine caches are keyed by name, so a template edited under the same name would + // otherwise survive here even though its row has changed -- the tag cache included, which + // holds whatever a template's {% cache %} blocks rendered from the previous database. + pebbleEngine.templateCache.invalidateAll() + pebbleEngine.tagCache.invalidateAll() + } /** The last-modified time of [file], or -1 when it does not exist. */ private fun timestampOf( @@ -492,10 +504,12 @@ class DocumentationContentSource( return try { StringWriter().also { pebbleEngine.getTemplate(name).evaluate(it, context) }.toString().toByteArray() } catch (e: PebbleException) { - // getPebbleMessage(), not message: PebbleException formats the latter as - // " (:)" and the loader throws with both null, so a caller that puts - // message in a response body gets a trailing "(?:?)". - throw IllegalStateException(e.pebbleMessage ?: e.message, e) + // PebbleException formats getMessage() as " (:)". When it carries + // neither -- the loader's throws, and the rendered-size limit -- that suffix is a bare + // "(?:?)" in the response body; when it carries both, as a parse error in a template + // does, it is the diagnostic that says which template and line to go fix. + val message = if (e.fileName == null && e.lineNumber == null) e.pebbleMessage else e.message + throw IllegalStateException(message, e) } catch (e: StackOverflowError) { // Templates can reference each other now (ADFA-5405), so they can also reference each // other in a cycle, which Pebble resolves by recursing until the stack runs out. Raised @@ -796,6 +810,16 @@ class DocumentationContentSource( companion object { const val CONTENT_CHUNK_SIZE = 1024 * 1024 + // Bounds a render whose output grows without end -- a runaway loop, say. The reference-cycle + // guard below does not cover that case: a cycle recurses, so it reaches the stack's end first, + // while a loop stays at one frame and grows the writer until the heap gives out, and + // OutOfMemoryError is an Error every catch on this path misses. Pebble raises a PebbleException + // at this limit instead, which the caller turns into a 500. + // + // Headroom over the largest context this database holds (171 KB of compressed JSON), not a + // measured ceiling on rendered output; tighten it if the real maximum is known. + private const val MAX_RENDERED_CHARS = 16 * 1024 * 1024 + private const val CONTENT_QUERY = """ SELECT C.content, CT.value, CT.compression, C.templateId FROM Content C, ContentTypes CT diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt index aa003111f3..dca59ae132 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt @@ -386,6 +386,20 @@ class DocumentationContentSourceTest { assertThat(lookup.cause).hasMessageThat().isEqualTo("Template 'nav.peb' not found in the database") } + // The other half of the padding fix: getPebbleMessage() drops the "(:)" suffix, which + // is what a parse error uses to say which template and line to go and fix. Only the loader's own + // throws, which carry neither, should lose it. + @Test + fun `a broken template keeps the file and line the engine reports`() { + val database = templatedDatabase("page.peb" to "Hello\n{% if %}") + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database + + val lookup = source().lookup("k/html/basic-syntax.html") as DocumentationLookup.Failed + + assertThat(lookup.cause).hasMessageThat().contains("page.peb") + assertThat(lookup.cause).hasMessageThat().contains("2") + } + @Test fun `a template is compiled once and reused for the next page that needs it`() { val database = templatedDatabase("page.peb" to "Hello {{ who }}!") diff --git a/docs/documentation-database.md b/docs/documentation-database.md index cc8f9e204b..73fa21984b 100644 --- a/docs/documentation-database.md +++ b/docs/documentation-database.md @@ -64,7 +64,7 @@ CREATE TABLE Tooltips ( - **`DocumentationDatabaseVersion(major, minor, patch, who, comment, changeTime)`** — the database's own semver (ADFA-5220), replacing the heuristics that used to infer the format from which tables happened to exist. **Exactly one row**: the version the file *is*, which `populate_db.py` replaces rather than appends. Nothing in the DDL enforces that, so both readers (`DatabaseVersionResolver.resolveMajorVersion` here, `database_major_version` in docdb-studio) take the newest row rather than the one with the highest `major` (the app orders by `changeTime DESC, rowid DESC`, so the greatest `changeTime` wins and `rowid` only breaks ties) — a rebuild from an older content set is a downgrade and has to read as one, which `MAX(major)` would get wrong. The app's reader additionally logs a warning when it finds more than one row; docdb-studio's does not. `populate_db.py` collapses a file that somehow accumulated several back to one, and `resolveMajorVersion` returns null for a database predating the table. `MAJOR >= 2` is what tells the app its brotli `Content` rows are dictionary-compressed; below that, `DocumentationContentSource` neither reads nor attaches `CompressionDictionary`. Gating on the declared version rather than on the table's presence matters in both directions: a database can carry the dictionary table while its content is still plain brotli (every row would then pay a failed dictionary decode before its plain one, on every request), and a migrated database that lost the table fails loudly instead of quietly. - **`CompressionDictionary(id, data)`** — single-row table (`id INTEGER PRIMARY KEY CHECK (id = 1)`) holding the raw Brotli dictionary every ADFA-5153-migrated `compression = 'brotli'` `Content` row is compressed against. Trained once, from a representative sample across the whole `Content` table, by `OfflineDocumentationTools`' `migrate_content_to_dictionary_brotli.py` / `populate_db.py` (never retrained after that — a dictionary-compressed row is only decodable against the exact dictionary it was compressed with, so replacing it would silently orphan every already-migrated row). Shipping the dictionary inside `documentation.db` itself, rather than as a separate bundled asset, keeps it version-locked to the content compressed against it. `DocumentationContentSource` loads it lazily -- not merely from opening or swapping databases, but on the first content fetch that needs it after the active database changes, and only when `DocumentationDatabaseVersion` declares `MAJOR >= 2` (see above) -- and caches it from then on, reloading again only on the next database change (a swap can bring in a database with a different dictionary or none, so it can't stay cached across one). Per row, it tries decoding with the dictionary attached first via brotli4j's `attachDictionary`, falling back to a plain decode on failure — needed both for a database predating this migration (no `CompressionDictionary` table at all) and for plugin-contributed rows within an otherwise-migrated database (see `PluginDocumentationManager` below). -- **`Templates(id, name, content)`** — Pebble template source. `Content.templateId` names a page's outermost template by id; every reference *between* templates is by `name` (ADFA-5405), which is also how `WebServer` reaches well-known templates like `bookshelf`. So a template can `extends`, `include`, `import` or `embed` another one by writing its `Templates.name` verbatim (`{% include "nav.peb" %}`) — the table is a flat namespace, with no directories, prefixes or suffixes applied. A reference to a name with no row fails the request rather than rendering the name; before ADFA-5405 the latter is exactly what happened, silently, because the engine's loader treated the name it was handed as the template body. +- **`Templates(id, name, content)`** — Pebble template source. The DDL is `id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL, content BLOB NOT NULL, UNIQUE('name')` — worth stating, because the single-quoted `UNIQUE('name')` reads like a constant rather than a column and has twice been taken for an absent constraint. SQLite resolves the quoted string to the column, so a duplicate name, a null name and a null body are all rejected by the database, and readers here do not re-check them. `Content.templateId` names a page's outermost template by id; every reference *between* templates is by `name` (ADFA-5405), which is also how `WebServer` reaches well-known templates like `bookshelf`. So a template can `extends`, `include`, `import` or `embed` another one by writing its `Templates.name` verbatim (`{% include "nav.peb" %}`) — the table is a flat namespace, with no directories, prefixes or suffixes applied. A reference to a name with no row fails the request rather than rendering the name; before ADFA-5405 the latter is exactly what happened, silently, because the engine's loader treated the name it was handed as the template body. - **`Bookshelf(contentID, bookCategoryID, title, description)`** / **`BookCategories(id, category, description)`** — the Dynamic Bookshelf: one row per "book" (PDF or similar), linked to its Tier 3 page via `contentID` -> `Content.id`. Two DB triggers keep `Bookshelf` in sync when a PDF row is inserted/deleted from `Content`; `title`/`description` don't come from those triggers and must be set by hand. Non-PDF books need a separate ingestion path (plugin-provided, e.g. via `PluginDocumentationManager`). - **`LastChange(documentationSet, changeTime, who)`** — audit trail for edits made through `docdb-studio`; not shown to end users. `DatabaseVersionResolver` reads the `documentationSet = 'wholedb'` row to report the DB's build/edit stamp in debug logging, falling back to the most recent row of any set if `'wholedb'` is missing. - Misc `ide_tooltip_table` and `PUCC` tables are historical/example artifacts — not part of the live lookup paths above. From 4e2e61984ff3a68d779b9ea5c9e9274d3b07db01 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 08:23:16 -0700 Subject: [PATCH 08/11] ADFA-5405: Address the review, and close the leak one layer deeper All three findings hold. The first one is not fully fixed by what it suggested, which is the interesting part. The echoed message is narrowed, and there was a second site. The suggestion was to give render failures a distinct type and echo only those; TemplateRenderException does that, extending IllegalStateException so callers that only care the render failed are unaffected. But narrowing handleBsEndpoint's catch does not stop the leak: realHandleBsEndpoint has its own catch around bookshelfJson which calls sendError with e.message and returns null, so the outer catch never sees the exception. That inner site is where a SQLiteException's SQL and withDatabase's check() failure -- which names the database file -- were actually reaching the client. Both are closed now. The regression test fails against the first fix alone, which is how the second site turned up. MAX_RENDERED_CHARS drops from 16 MiB to 1 MiB. The arithmetic in the finding is right: Pebble counts characters, so 16 Mi chars is a 33.5 MB char[], the doubling that reaches it holds 33.5 MB and 67 MB at once, and toString() copies another 33.5 MB. Against a 192-256 MB heap the runaway loop OOMs long before the guard fires -- the one case it exists for. The old number was headroom over the largest context in the database, which is an unrelated quantity, as its own comment conceded. 1 MiB still sits well above any legitimate rendered page here. clearTemplateCache carries swapDatabaseIfChanged's warning. It takes the write lock, withDatabase runs its block under the read lock, and ReentrantReadWriteLock does not upgrade -- so withDatabase { clearTemplateCache() } deadlocks permanently. Latent: all four current callers are outside the read lock. The note is what keeps the next one out. Separately, and NOT fixed here: sendError echoes e.message on two general request paths too (WebServer.kt around the request-processing catch, and the DocumentationLookup.Failed branch). Same exposure, any content path rather than just /pr/bs, and pre-existing rather than introduced by this ticket -- so it wants its own change, not a quiet widening of this one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../androidide/localWebServer/WebServer.kt | 28 ++++++++--- .../localWebServer/WebServerTest.kt | 40 ++++++++++++++++ .../DocumentationContentSource.kt | 47 ++++++++++++++++--- 3 files changed, 103 insertions(+), 12 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 171cdd4dcb..701d3b3a6a 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -10,6 +10,7 @@ import com.itsaky.androidide.documentation.DocumentationContent import com.itsaky.androidide.documentation.DocumentationContentSource import com.itsaky.androidide.documentation.DocumentationLookup import com.itsaky.androidide.documentation.DocumentationRequestInterceptor +import com.itsaky.androidide.documentation.TemplateRenderException import com.itsaky.androidide.utils.ContentTypeHeaders import com.itsaky.androidide.utils.DatabaseVersionResolver import org.slf4j.LoggerFactory @@ -769,15 +770,24 @@ class WebServer( outputStarted = realHandleBsEndpoint(writer, output) { outputStarted = true } } catch (e: Exception) { log.error("Error handling /pr/bs endpoint: {}", e.message) - // The message, not just the generic text: a template this endpoint cannot resolve -- the - // bookshelf row itself, or anything it references -- is named by the loader's throw, and - // that name is the whole diagnostic (ADFA-5405). + // The message is echoed ONLY for a template failure. That one names a template -- the + // bookshelf row itself, or anything it references -- and the name is the whole diagnostic + // (ADFA-5405). Everything else keeps the generic text, because this catch spans the whole + // of realHandleBsEndpoint: a SQLiteException carries SQL, and withDatabase's + // check(openIfNeeded()) carries the database's filesystem path. Any app on the device can + // GET this port, so echoing those was handing out internals for the sake of one + // diagnostic. + val detail = + when (e) { + is TemplateRenderException -> e.message ?: "Error generating bookshelf HTML." + else -> "Error generating bookshelf HTML." + } sendError( writer, output, httpInternalServerError, "Internal Server Error 6", - e.message ?: "Error generating bookshelf HTML.", + detail, outputStarted, ) } @@ -856,8 +866,14 @@ class WebServer( if (debugEnabled) log.debug("json content = '{}'.", String(it, Charsets.UTF_8)) } } catch (e: Exception) { - log.error("Error processing request: {}", e.message) - sendError(writer, output, httpInternalServerError, "Internal Server Error", e.message ?: "") + log.error("Error building the bookshelf JSON: {}", e.message, e) + // The message stays in the log and out of the response. Everything reachable here is + // a database failure -- bookshelfJson is the SQL join -- so the message carries SQL + // text, or the database's filesystem path when withDatabase's check() is what threw. + // Any app on the device can GET this port. Narrowing handleBsEndpoint's own catch is + // not enough on its own: this one answers and returns null, so the outer catch never + // sees the exception at all. + sendError(writer, output, httpInternalServerError, "Internal Server Error", "Error generating bookshelf HTML.") null } } ?: return false 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 f86d131d10..65a9c561d8 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -420,6 +420,46 @@ class WebServerTest { } } + // The other half of the ADFA-5405 diagnostic: handleBsEndpoint's catch spans the whole handler, + // so echoing e.message put anything thrown in there into the response body -- a SQLiteException's + // SQL, or withDatabase's check() failure naming the database file. Any app on the device can GET + // this port. Only a template failure is echoed now; this pins that the rest is not. + @Test + fun `a failure that is not a template failure answers 500 without leaking internals`() { + val port = freePort() + val db = mockk(relaxed = true) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns db + // Thrown from inside the handler rather than from openDatabase, which would fail start() + // before the port is bound. The type matters more than the origin: this is the same + // IllegalStateException that withDatabase's check() raises, which used to be + // indistinguishable from a template diagnostic and so was echoed verbatim -- and its message + // carries both a filesystem path and SQL, the two things worth not sending. + every { db.rawQuery(match { it.contains("FROM Content AS C") }, any()) } throws + IllegalStateException( + "unable to open database file /data/user/0/com.itsaky.androidide/databases/documentation.db " + + "(while compiling: SELECT C.content FROM Content AS C JOIN ContentTypes)", + ) + + val server = WebServer(testConfig(port)) + val serverThread = Thread { server.start() }.apply { isDaemon = true } + serverThread.start() + try { + awaitPortBound(port) + val response = sendRawGetRequest(port, "/pr/bs") + assertTrue("Expected a 500 status line, got:\n$response", response.startsWith("HTTP/1.1 500")) + assertTrue( + "Expected the generic text, got:\n$response", + response.contains("Error generating bookshelf HTML."), + ) + assertFalse("Leaked a filesystem path:\n$response", response.contains("/data/user/0/")) + assertFalse("Leaked a database filename:\n$response", response.contains("documentation.db")) + assertFalse("Leaked SQL text:\n$response", response.contains("SELECT C.content")) + } finally { + server.stop() + serverThread.join(2_000) + } + } + // ADFA-5241: the two transports have to answer the same way about what a response says, and // only a real response proves what this one sends. The decision itself lives in // ContentTypeHeaders, shared with DocumentationRequestInterceptor. diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 53b7daa49e..21df4538d6 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -125,6 +125,24 @@ sealed interface DocumentationLookup { ) : DocumentationLookup } +/** + * A template could not be loaded, parsed or rendered. + * + * Exists so a caller can tell a template diagnostic apart from every other failure on the serving + * path. That matters because one caller puts the message in an HTTP response body: a template + * failure names a template, which is the whole point of the ADFA-5405 diagnostic and safe to send, + * while the [IllegalStateException] a closed or unopenable database raises carries the database's + * filesystem path, and a `SQLiteException` carries SQL text. Both of those were reaching the + * response because they share [IllegalStateException] with the diagnostics. + * + * Extends [IllegalStateException] rather than replacing it, so callers that only care that the + * render failed are unaffected. + */ +class TemplateRenderException( + message: String?, + cause: Throwable? = null, +) : IllegalStateException(message, cause) + /** * What [DocumentationContentSource.lookupRequestPath] found, plus the path form that produced it -- * so a transport reporting a miss or a corrupt row can quote the string that was actually queried. @@ -353,6 +371,12 @@ class DocumentationContentSource( /** * Clears all cached templates, compiled and by name. + * + * Takes the write lock, so like [swapDatabaseIfChanged] this must not be called while holding the + * read lock -- which is what [withDatabase] runs its block under. `ReentrantReadWriteLock` does + * not upgrade a read hold to a write hold, so `withDatabase { clearTemplateCache() }` (or the same + * shape through `DocumentationRequestInterceptor.clearTemplateCache()`) deadlocks that thread + * permanently. No caller does this today; the note is here to keep the next one out of it. */ fun clearTemplateCache() = // The write lock, which a render's read lock excludes: clearing the three caches piecemeal @@ -497,7 +521,7 @@ class DocumentationContentSource( ): ByteArray { val contextString = contextJson.toString(Charsets.UTF_8) if (contextString.isBlank() || contextString.trim() == "null") { - throw IllegalStateException("Template '$name' has empty or null JSON context, for path '$path'") + throw TemplateRenderException("Template '$name' has empty or null JSON context, for path '$path'") } val context: Map = gson.fromJson(contextString, templateContextType) @@ -509,13 +533,16 @@ class DocumentationContentSource( // "(?:?)" in the response body; when it carries both, as a parse error in a template // does, it is the diagnostic that says which template and line to go fix. val message = if (e.fileName == null && e.lineNumber == null) e.pebbleMessage else e.message - throw IllegalStateException(message, e) + throw TemplateRenderException(message, e) } catch (e: StackOverflowError) { // Templates can reference each other now (ADFA-5405), so they can also reference each // other in a cycle, which Pebble resolves by recursing until the stack runs out. Raised // here as an exception because an Error passes through every catch on this path: the // client would get a closed socket with no status line and nothing naming the template. - throw IllegalStateException("Rendering template '$name' overflowed the stack; check for a reference cycle between templates", e) + throw TemplateRenderException( + "Rendering template '$name' overflowed the stack; check for a reference cycle between templates", + e, + ) } } @@ -816,9 +843,17 @@ class DocumentationContentSource( // OutOfMemoryError is an Error every catch on this path misses. Pebble raises a PebbleException // at this limit instead, which the caller turns into a 500. // - // Headroom over the largest context this database holds (171 KB of compressed JSON), not a - // measured ceiling on rendered output; tighten it if the real maximum is known. - private const val MAX_RENDERED_CHARS = 16 * 1024 * 1024 + // Sized so it actually fires first, which 16 MiB did not. Pebble counts CHARACTERS, in a + // LimitedSizeWriter wrapping a StringWriter, so 16 Mi chars means a 33.5 MB char[] -- and the + // doubling step that reaches it holds the old 33.5 MB array and the new 67 MB one at once, + // then toString() copies another 33.5 MB. Against a 192-256 MB Android heap the runaway loop + // OOMs long before the guard trips, which is the one scenario it exists for. The old number + // was headroom over the largest CONTEXT in the database (171 KB of compressed JSON) -- an + // unrelated quantity, as its own comment admitted. + // + // 1 MiB of characters is ~2 MB of char[] and still roughly 6x the largest legitimate rendered + // page here, so it bounds the runaway without being reachable by real content. + private const val MAX_RENDERED_CHARS = 1024 * 1024 private const val CONTENT_QUERY = """ SELECT C.content, CT.value, CT.compression, C.templateId From 1db0714d7aacd8ddc939c91d497e791ac77f525d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 23:18:25 -0700 Subject: [PATCH 09/11] ADFA-5405: sweep the leak to the other transport, and close the swap window Review of #1779 found the error-message leak this PR fixed for /pr/bs was left in place on serveRequest, which answers every documentation URL on the same port any app on the device can GET. Same rule applies there now: only a TemplateRenderException's message reaches the client, everything else gets generic text and the detail goes to the log. The bookshelf handler built its payload under one withDatabase and rendered the template under a second, so a debug-database swap landing between them rendered the new database's template against the old one's payload -- the pairing the bookshelfTemplateId/generation machinery this PR deleted used to keep. renderNamedTemplate now takes a payload builder and spans both under one acquisition. Nesting was not an option: withDatabase takes the write lock to check for a swap before it takes the read lock, so a nested call deadlocks. That also removes the inner try/catch around the payload build, which is why `a failure that is not a template failure answers 500 without leaking internals` could not fail before: the inner catch answered and returned, so the classification the test names never ran. It runs now, and reverting the classification fails it. DatabaseTemplateLoader: every failure leaves as a LoaderException, including SQLite's. Pebble does not wrap what a loader throws, so a raw SQLiteException escaped renderNamed's PebbleException catch carrying SQL text and reached the branch above as "not a template failure". Also rejects a duplicated name instead of taking row 0 by scan order (the sibling check templateName already made for the id path), and a NULL content column by name instead of an NPE with no message. templateName's two diagnostics are TemplateRenderException now, so the id half of the render path classifies the same way as the name half, and its database failures are wrapped rather than escaping with SQL text. MAX_RENDERED_CHARS keeps its value and loses the claim that it is "6x the largest legitimate rendered page here" -- unmeasured, and unmeasurable from this repo, since documentation.db is fetched rather than checked in. The comment now says what the number is for and what to do if a real page ever trips it. Not done, filed as ADFA-5626: replacing the StackOverflowError catch with an in-flight template-name set. It is the better mechanism, but Pebble resolves {% include %} at evaluate time against its own compiled-template cache, so the loader is not consulted and there is no seam to track names through without a custom extension. The catch does produce a correct 500 naming the template. Also not done: removing refreshDatabase() and generation as dead production code. Both are test-only, but the two refreshDatabase tests pin a real past regression (it used to fall through to the swap check after close()), and I would rather keep that coverage than the tidiness. Noted in ADFA-5626. Tests: 109 green across the documentation and web server suites, three new loader tests, one new WebServerTest for the swept sibling. Both new guards were mutated and fail without their fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/localWebServer/WebServer.kt | 43 ++++------ .../localWebServer/WebServerTest.kt | 34 ++++++++ .../documentation/DatabaseTemplateLoader.kt | 40 +++++++-- .../DocumentationContentSource.kt | 86 ++++++++++--------- .../DatabaseTemplateLoaderTest.kt | 58 +++++++++++++ .../DocumentationContentSourceTest.kt | 5 +- 6 files changed, 191 insertions(+), 75 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 701d3b3a6a..f18a54e657 100644 --- a/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt +++ b/app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt @@ -589,7 +589,13 @@ class WebServer( } is DocumentationLookup.Failed -> { - sendError(writer, output, httpInternalServerError, "Internal Server Error", lookup.cause.message ?: "") + log.error("Cannot serve the documentation request", lookup.cause) + // Same rule as /pr/bs, and for the same reason: only a template failure names a + // template, and only its message is safe to send. A SQLiteException carries SQL text + // and withDatabase's check() carries the database's filesystem path, and any app on + // the device can GET this port. This is the sibling the first pass missed. + val detail = (lookup.cause as? TemplateRenderException)?.message ?: "Internal Server Error" + sendError(writer, output, httpInternalServerError, "Internal Server Error", detail) } } } @@ -777,11 +783,7 @@ class WebServer( // check(openIfNeeded()) carries the database's filesystem path. Any app on the device can // GET this port, so echoing those was handing out internals for the sake of one // diagnostic. - val detail = - when (e) { - is TemplateRenderException -> e.message ?: "Error generating bookshelf HTML." - else -> "Error generating bookshelf HTML." - } + val detail = (e as? TemplateRenderException)?.message ?: "Error generating bookshelf HTML." sendError( writer, output, @@ -858,27 +860,16 @@ class WebServer( ): Boolean { if (debugEnabled) log.debug("Entering realHandleBsEndpoint().") - // Null means an error response has already been sent, so there is nothing left to write. - val jsonText = - contentSource.withDatabase { database -> - try { - bookshelfJson(database).also { - if (debugEnabled) log.debug("json content = '{}'.", String(it, Charsets.UTF_8)) - } - } catch (e: Exception) { - log.error("Error building the bookshelf JSON: {}", e.message, e) - // The message stays in the log and out of the response. Everything reachable here is - // a database failure -- bookshelfJson is the SQL join -- so the message carries SQL - // text, or the database's filesystem path when withDatabase's check() is what threw. - // Any app on the device can GET this port. Narrowing handleBsEndpoint's own catch is - // not enough on its own: this one answers and returns null, so the outer catch never - // sees the exception at all. - sendError(writer, output, httpInternalServerError, "Internal Server Error", "Error generating bookshelf HTML.") - null + // The payload and the template are built under one database acquisition, so a swap cannot + // land between them. Nothing is caught here: handleBsEndpoint's catch is the single place + // that decides what reaches the client, and an inner catch that answered and returned made + // that decision unreachable for everything raised inside this block. + val result = + contentSource.renderNamedTemplate("bookshelf", "/bookshelf") { database -> + bookshelfJson(database).also { + if (debugEnabled) log.debug("json content = '{}'.", String(it, Charsets.UTF_8)) } - } ?: return false - - val result = contentSource.renderNamedTemplate("bookshelf", jsonText, "/bookshelf") + } if (debugEnabled) log.debug("Bookshelf result is '{}'.", String(result)) 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 65a9c561d8..1fb9c9f0f2 100644 --- a/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt +++ b/app/src/test/java/com/itsaky/androidide/localWebServer/WebServerTest.kt @@ -424,6 +424,10 @@ class WebServerTest { // so echoing e.message put anything thrown in there into the response body -- a SQLiteException's // SQL, or withDatabase's check() failure naming the database file. Any app on the device can GET // this port. Only a template failure is echoed now; this pins that the rest is not. + // + // This throw has to reach that catch to pin anything. It did not until the inner try/catch + // around the payload build was removed: that one answered and returned, so the classification + // under test never ran and this test passed against the unfixed code. @Test fun `a failure that is not a template failure answers 500 without leaking internals`() { val port = freePort() @@ -460,6 +464,36 @@ class WebServerTest { } } + // The sibling handleBsEndpoint's fix missed: serveRequest answers every documentation URL, and + // its Failed branch sent lookup.cause.message verbatim. Same port, same reachable-by-any-app + // exposure, and ADFA-5405's loader made database failures reachable from more places. + @Test + fun `a documentation request that fails answers 500 without leaking internals`() { + val port = freePort() + val db = mockk(relaxed = true) + every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns db + every { db.rawQuery(match { it.contains("FROM Content") }, any()) } throws + IllegalStateException( + "unable to open database file /data/user/0/com.itsaky.androidide/databases/documentation.db " + + "(while compiling: SELECT C.content FROM Content C)", + ) + + val server = WebServer(testConfig(port)) + val serverThread = Thread { server.start() }.apply { isDaemon = true } + serverThread.start() + try { + awaitPortBound(port) + val response = sendRawGetRequest(port, "/k/html/basic-syntax.html") + assertTrue("Expected a 500 status line, got:\n$response", response.startsWith("HTTP/1.1 500")) + assertFalse("Leaked a filesystem path:\n$response", response.contains("/data/user/0/")) + assertFalse("Leaked a database filename:\n$response", response.contains("documentation.db")) + assertFalse("Leaked SQL text:\n$response", response.contains("SELECT C.content")) + } finally { + server.stop() + serverThread.join(2_000) + } + } + // ADFA-5241: the two transports have to answer the same way about what a response says, and // only a real response proves what this one sends. The decision itself lives in // ContentTypeHeaders, shared with DocumentationRequestInterceptor. diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt index cb14690bd0..62d5c57a74 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt @@ -47,12 +47,42 @@ internal class DatabaseTemplateLoader( override fun getReader(name: String): Reader { val database = database() ?: throw LoaderException(null, "No documentation database is open, for template '$name'") - return database.rawQuery(TEMPLATE_QUERY, arrayOf(name)).use { cursor -> - if (!cursor.moveToFirst()) { - throw LoaderException(null, "Template '$name' not found in the database") + // Every failure leaves here as a LoaderException, including the ones SQLite raises. Pebble + // does not wrap what a loader throws -- getTemplate has no catch around its cache's + // computeIfAbsent -- so a raw SQLiteException would escape the render's PebbleException + // catch carrying SQL text, and reach a caller that classifies it as "not a template + // failure" and cannot name the template. + val body = + try { + database.rawQuery(TEMPLATE_QUERY, arrayOf(name)).use { cursor -> + when { + // The DDL declares UNIQUE('name'), but the database that is open may be a + // debug one dropped on the sdcard, which is under no obligation to honour + // it. Picking row 0 by scan order would render the wrong partial silently, + // which is the failure ADFA-5405 exists to remove, not to relocate. + cursor.count > 1 -> { + throw LoaderException(null, "Template '$name' is shared by more than one database row") + } + + !cursor.moveToFirst() -> { + throw LoaderException(null, "Template '$name' not found in the database") + } + + // getBlob returns a platform type: a NULL content column yields null and + // the decode below would NPE with no message and no template name. + else -> { + cursor.getBlob(0) + ?: throw LoaderException(null, "Template '$name' has no body") + } + } + } + } catch (e: LoaderException) { + throw e + } catch (e: RuntimeException) { + throw LoaderException(e, "Cannot read template '$name' from the database") } - StringReader(cursor.getBlob(0).toString(Charsets.UTF_8)) - } + + return StringReader(body.toString(Charsets.UTF_8)) } override fun resourceExists(name: String): Boolean { diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 197f8c98af..937692235f 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -341,35 +341,35 @@ class DocumentationContentSource( * For a caller that knows a well-known template by name -- the bookshelf, say -- rather than * through a `Content` row's `templateId`. * + * [contextJson] builds the payload from the same database the template is then loaded from, + * under one acquisition. Building it through a separate [withDatabase] and passing the bytes in + * would let a debug-database swap land between the two, rendering the new database's template + * against the old one's payload. Nesting is not the alternative: [withDatabase] takes the write + * lock to check for a swap before it takes the read lock, so a nested call deadlocks. + * * @param name The template's `Templates.name`. - * @param contextJson The JSON object used as the template context. * @param path The path associated with the rendering request for diagnostics. + * @param contextJson Builds the JSON object used as the template context. * @return The rendered content encoded as UTF-8 bytes. */ fun renderNamedTemplate( name: String, - contextJson: ByteArray, path: String, - ): ByteArray = withDatabase { renderNamed(name, contextJson, path) } + contextJson: (SQLiteDatabase) -> ByteArray, + ): ByteArray = withDatabase { database -> renderNamed(name, contextJson(database), path) } /** * Clears all cached templates, compiled and by name. * - * Takes the write lock, so like [swapDatabaseIfChanged] this must not be called while holding the - * read lock -- which is what [withDatabase] runs its block under. `ReentrantReadWriteLock` does - * not upgrade a read hold to a write hold, so `withDatabase { clearTemplateCache() }` (or the same - * shape through `DocumentationRequestInterceptor.clearTemplateCache()`) deadlocks that thread - * permanently. No caller does this today; the note is here to keep the next one out of it. + * Takes the write lock, which `ReentrantReadWriteLock` will not upgrade to from a read hold, so + * `withDatabase { clearTemplateCache() }` deadlocks that thread permanently. */ fun clearTemplateCache() = - // The write lock, which a render's read lock excludes: clearing the three caches piecemeal - // under a concurrent render can hand it a template from before the clear and a tag-cache miss - // from after it. Reentrant, so switchToDatabase can keep calling this while holding it. + // All three under one write lock: clearing them piecemeal under a concurrent render can hand + // it a template from before the clear and a tag cache from after it. The engine's two caches + // are keyed by name, so a template edited under the same name survives without this. databaseLock.write { templateNames.clear() - // Both engine caches are keyed by name, so a template edited under the same name would - // otherwise survive here even though its row has changed -- the tag cache included, which - // holds whatever a template's {% cache %} blocks rendered from the previous database. pebbleEngine.templateCache.invalidateAll() pebbleEngine.tagCache.invalidateAll() } @@ -535,27 +535,36 @@ class DocumentationContentSource( * @param templateId The database identifier of the template. * @param path The content path associated with the template. * @return The template's name. - * @throws IllegalStateException If the template is missing or has multiple database rows. + * @throws TemplateRenderException If the template is missing, has multiple database rows, or + * cannot be read. */ private fun templateName( database: SQLiteDatabase, templateId: Int, path: String, ): String = - database.rawQuery("SELECT name FROM Templates WHERE id = ?", arrayOf(templateId.toString())).use { cursor -> - when { - cursor.count > 1 -> { - throw IllegalStateException("Template ID $templateId is shared by more than one template") - } - - !cursor.moveToFirst() -> { - throw IllegalStateException("Template ID $templateId not found in the database, for path '$path'") - } - - else -> { - cursor.getString(0) + try { + database.rawQuery("SELECT name FROM Templates WHERE id = ?", arrayOf(templateId.toString())).use { cursor -> + when { + cursor.count > 1 -> { + throw TemplateRenderException("Template ID $templateId is shared by more than one template") + } + + !cursor.moveToFirst() -> { + throw TemplateRenderException("Template ID $templateId not found in the database, for path '$path'") + } + + else -> { + cursor.getString(0) + } } } + } catch (e: TemplateRenderException) { + throw e + } catch (e: RuntimeException) { + // Not the raw exception: a SQLiteException's message carries SQL text and one caller + // puts a TemplateRenderException's message in an HTTP response body. + throw TemplateRenderException("Cannot read the template for ID $templateId", e) } /** @@ -749,22 +758,15 @@ class DocumentationContentSource( companion object { const val CONTENT_CHUNK_SIZE = 1024 * 1024 - // Bounds a render whose output grows without end -- a runaway loop, say. The reference-cycle - // guard below does not cover that case: a cycle recurses, so it reaches the stack's end first, - // while a loop stays at one frame and grows the writer until the heap gives out, and - // OutOfMemoryError is an Error every catch on this path misses. Pebble raises a PebbleException - // at this limit instead, which the caller turns into a 500. - // - // Sized so it actually fires first, which 16 MiB did not. Pebble counts CHARACTERS, in a - // LimitedSizeWriter wrapping a StringWriter, so 16 Mi chars means a 33.5 MB char[] -- and the - // doubling step that reaches it holds the old 33.5 MB array and the new 67 MB one at once, - // then toString() copies another 33.5 MB. Against a 192-256 MB Android heap the runaway loop - // OOMs long before the guard trips, which is the one scenario it exists for. The old number - // was headroom over the largest CONTEXT in the database (171 KB of compressed JSON) -- an - // unrelated quantity, as its own comment admitted. + // Bounds a render whose output grows without end -- a runaway {% for %}, say -- which would + // otherwise raise OutOfMemoryError, an Error every catch on this path misses. Pebble counts + // characters, so this is ~2 MB of char[]; it has to stay well under the 192-256 MB Android + // heap or it never fires, which is why 16 MiB (33.5 MB of char[], doubled during growth) + // did not. Pebble's own default is unbounded. // - // 1 MiB of characters is ~2 MB of char[] and still roughly 6x the largest legitimate rendered - // page here, so it bounds the runaway without being reachable by real content. + // Not calibrated against real page sizes: the shipped documentation.db is fetched, not + // checked in, so the largest legitimate rendered page is not measurable from this repo. If + // a real page ever trips this, raise it -- the number is a heap guard, not a content limit. private const val MAX_RENDERED_CHARS = 1024 * 1024 private const val CONTENT_QUERY = """ diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt index b975792087..cd39ee62a5 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt @@ -2,6 +2,7 @@ package com.itsaky.androidide.documentation import android.database.Cursor import android.database.sqlite.SQLiteDatabase +import android.database.sqlite.SQLiteException import com.google.common.truth.Truth.assertThat import io.mockk.every import io.mockk.mockk @@ -73,4 +74,61 @@ class DatabaseTemplateLoaderTest { assertThat(loader.resolveRelativePath("nav.peb", "k/html/page.peb")).isEqualTo("nav.peb") assertThat(loader.getReader("nav.peb").readText()).isEqualTo("[nav]") } + + @Test + fun `a duplicated name fails rather than picking a row by scan order`() { + // The DDL declares UNIQUE('name'), but the open database may be a debug one dropped on the + // sdcard, which is under no obligation to honour it. Taking row 0 would render the wrong + // partial with no error and no log line. + val database = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } returns + mockk(relaxed = true) { + every { count } returns 2 + every { moveToFirst() } returns true + every { getBlob(0) } returns "[nav]".toByteArray() + } + } + + val thrown = assertThrows(LoaderException::class.java) { loader(database).getReader("nav.peb") } + + assertThat(thrown).hasMessageThat().contains("nav.peb") + assertThat(thrown).hasMessageThat().contains("more than one") + } + + @Test + fun `a row with no body fails by name, rather than throwing NullPointerException`() { + // getBlob returns a platform type: a NULL content column yields null, and decoding it would + // raise an NPE carrying neither a message nor the template name. + val database = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } returns + mockk(relaxed = true) { + every { count } returns 1 + every { moveToFirst() } returns true + every { getBlob(0) } returns null + } + } + + val thrown = assertThrows(LoaderException::class.java) { loader(database).getReader("nav.peb") } + + assertThat(thrown).hasMessageThat().contains("nav.peb") + } + + @Test + fun `a database failure arrives as a loader failure, without the SQL`() { + // Pebble does not wrap what a loader throws, so a raw SQLiteException would escape the + // render's PebbleException catch carrying SQL text -- and one caller puts that message in an + // HTTP response body on a port any app on the device can reach. + val database = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } throws + SQLiteException("no such table: Templates (code 1): , while compiling: SELECT content FROM Templates") + } + + val thrown = assertThrows(LoaderException::class.java) { loader(database).getReader("nav.peb") } + + assertThat(thrown).hasMessageThat().contains("nav.peb") + assertThat(thrown).hasMessageThat().doesNotContain("SELECT") + } } diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt index dca59ae132..f5cad65a29 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DocumentationContentSourceTest.kt @@ -339,7 +339,8 @@ class DocumentationContentSourceTest { val database = templatedDatabase("bookshelf" to "Books for {{ who }}") every { SQLiteDatabase.openDatabase(any(), isNull(), any()) } returns database - val rendered = source().renderNamedTemplate("bookshelf", """{"who": "Kotlin"}""".toByteArray(), "/bookshelf") + val rendered = + source().renderNamedTemplate("bookshelf", "/bookshelf") { """{"who": "Kotlin"}""".toByteArray() } assertThat(rendered.toString(Charsets.UTF_8)).isEqualTo("Books for Kotlin") } @@ -351,7 +352,7 @@ class DocumentationContentSourceTest { val source = source() assertThrows(IllegalStateException::class.java) { - source.renderNamedTemplate("bookshelf", "null".toByteArray(), "/bookshelf") + source.renderNamedTemplate("bookshelf", "/bookshelf") { "null".toByteArray() } } } From 8bce99405e76e91495eadbbfd3a68b3610f38930 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 01:41:43 -0700 Subject: [PATCH 10/11] ADFA-5405: sweep resourceExists, and widen the render cap resourceExists had none of the error handling getReader was given, so a SQLiteException escaped it raw with its SQL text -- and because it returns a Boolean rather than a Reader, the caller above could not classify the throw as a template failure at all. Same wrapping as getReader now. The KDoc's reason for not caring was that Pebble only reaches this through loaders that are not wired here, which is a fact about today's wiring rather than about the loader's contract. MAX_RENDERED_CHARS goes from 1 MiB to 4 Mi chars. The old number was uncalibrated by my own admission, and a cap where Pebble's default is unbounded can turn a page that served into a 500. 4 Mi chars is an 8 MB char[] with a ~32 MB transient through the doubling step and toString, against a 192-256 MB heap -- still comfortably ahead of the OOM it exists to catch, since unbounded growth passes any finite number, but with a wide margin over real content rather than a tight one. Review argued the multi-megabyte rows readChunks exists for prove content that large reaches the writer. They do not: render() runs only for templateId > 0 and those rows are the bundled PDFs, which have no template. The comment now says so, since that was the reasoning the old number lacked. Tests: 110 green, one new for the swept sibling. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../documentation/DatabaseTemplateLoader.kt | 10 ++++++++- .../DocumentationContentSource.kt | 21 ++++++++++++------- .../DatabaseTemplateLoaderTest.kt | 16 ++++++++++++++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt index 62d5c57a74..de805557f1 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt @@ -91,7 +91,15 @@ internal class DatabaseTemplateLoader( // Not TEMPLATE_QUERY: that copies the whole template blob into a CursorWindow to answer a // boolean. Pebble reaches this only through the delegating and servlet loaders, neither of // which is wired here, so the cost would be invisible -- which is the reason to get it right. - return database.rawQuery(EXISTS_QUERY, arrayOf(name)).use { it.moveToFirst() } + // + // Wrapped like getReader, and for the same reason: a SQLiteException carries SQL text, and + // this returns a Boolean so a raw throw would not even be classifiable as a template + // failure. Which loaders reach this today is a fact about the wiring, not the contract. + return try { + database.rawQuery(EXISTS_QUERY, arrayOf(name)).use { it.moveToFirst() } + } catch (e: RuntimeException) { + throw LoaderException(e, "Cannot look up template '$name' in the database") + } } override fun createCacheKey(name: String): String = name diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 937692235f..02b263a115 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -759,15 +759,20 @@ class DocumentationContentSource( const val CONTENT_CHUNK_SIZE = 1024 * 1024 // Bounds a render whose output grows without end -- a runaway {% for %}, say -- which would - // otherwise raise OutOfMemoryError, an Error every catch on this path misses. Pebble counts - // characters, so this is ~2 MB of char[]; it has to stay well under the 192-256 MB Android - // heap or it never fires, which is why 16 MiB (33.5 MB of char[], doubled during growth) - // did not. Pebble's own default is unbounded. + // otherwise raise OutOfMemoryError, an Error every catch on this path misses. Pebble's own + // default is unbounded, so this is a cap where there was none: it has to be high enough + // that no real page reaches it and low enough that it fires before the heap does. // - // Not calibrated against real page sizes: the shipped documentation.db is fetched, not - // checked in, so the largest legitimate rendered page is not measurable from this repo. If - // a real page ever trips this, raise it -- the number is a heap guard, not a content limit. - private const val MAX_RENDERED_CHARS = 1024 * 1024 + // Pebble counts characters, so 4 Mi chars is an 8 MB char[], and the doubling step that + // reaches it holds the old 8 MB and the new 16 MB at once, then toString() copies another + // 8 MB -- ~32 MB transient against a 192-256 MB heap. 16 MiB failed that test, which is + // why it never fired. The largest rendered page is not measurable from this repo, so the + // margin above it is deliberately wide rather than tight: the only thing this has to + // catch is unbounded growth, and unbounded growth passes any finite number. + // + // Untemplated content is irrelevant to it. render() runs only for templateId > 0, so the + // multi-megabyte rows readChunks exists for -- the bundled PDFs -- never reach the writer. + private const val MAX_RENDERED_CHARS = 4 * 1024 * 1024 private const val CONTENT_QUERY = """ SELECT C.content, CT.value, CT.compression, C.templateId diff --git a/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt index cd39ee62a5..71d6abc891 100644 --- a/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt +++ b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt @@ -131,4 +131,20 @@ class DatabaseTemplateLoaderTest { assertThat(thrown).hasMessageThat().contains("nav.peb") assertThat(thrown).hasMessageThat().doesNotContain("SELECT") } + + @Test + fun `an existence check that fails arrives as a loader failure, without the SQL`() { + // getReader's guarantee applies here too: this answers a Boolean, so a raw SQLiteException + // would not even be classifiable as a template failure by the caller above it. + val database = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } throws + SQLiteException("no such table: Templates (code 1): , while compiling: SELECT 1 FROM Templates") + } + + val thrown = assertThrows(LoaderException::class.java) { loader(database).resourceExists("nav.peb") } + + assertThat(thrown).hasMessageThat().contains("nav.peb") + assertThat(thrown).hasMessageThat().doesNotContain("SELECT") + } } From bc81f55789ee033da98fc08df48998a210d2dd06 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 9 Sep 2026 06:59:20 -0700 Subject: [PATCH 11/11] ADFA-5405: guard the other NULL column templateName() read cursor.getString(0) into a non-null String. getString returns a platform type, so a NULL Templates.name yielded null and the implicit check threw a bare NPE, which the RuntimeException catch below rewrapped as "Cannot read the template for ID N" -- losing which column was null and, unlike the loader's path, never naming the template. The loader guards exactly this for getBlob, with a test. This was the second of the two sites and the sweep missed it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/documentation/DocumentationContentSource.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt index 02b263a115..173d95a84e 100644 --- a/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt +++ b/common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt @@ -554,8 +554,13 @@ class DocumentationContentSource( throw TemplateRenderException("Template ID $templateId not found in the database, for path '$path'") } + // The same guard the loader applies to getBlob. getString returns a platform + // type, so a NULL name column yields null and the implicit null check throws a + // bare NPE -- rewrapped below as the generic message, losing both the column + // and, unlike the loader's path, the template's identity. else -> { cursor.getString(0) + ?: throw TemplateRenderException("Template ID $templateId has no name, for path '$path'") } } }