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..aa83dd2355 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 @@ -11,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 @@ -148,18 +148,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 @@ -547,31 +535,11 @@ 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) } - /** - * 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 +552,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) @@ -623,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) } } } @@ -801,10 +773,25 @@ class WebServer( var outputStarted = false try { - outputStarted = realHandleBsEndpoint(writer, output) { outputStarted = true } + 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 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 = (e as? TemplateRenderException)?.message ?: "Error generating bookshelf HTML." + sendError( + writer, + output, + httpInternalServerError, + "Internal Server Error 6", + detail, + outputStarted, + ) } if (debugEnabled) log.debug("Leaving handleBsEndpoint().") @@ -864,45 +851,29 @@ class WebServer( /** * Generates the bookshelf page and sends it to the client. * - * @return `true` if a response was produced, `false` if processing failed or no response was produced. + * Returns nothing: [markOutputStarted] is how the caller learns the response has begun, and it + * fires at the moment it actually does. Returning the same fact as well meant two mechanisms + * for one piece of state -- and once the only early return went, the returned value was a + * constant. A later early return that updated one and not the other would leave the caller + * sending response headers onto a socket that already carries a body. */ private fun realHandleBsEndpoint( writer: PrintWriter, output: java.io.OutputStream, markOutputStarted: () -> Unit, - ): 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 { - 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) - } - } - - json - } catch (e: Exception) { - log.error("Error processing request: {}", e.message) - sendError(writer, output, httpInternalServerError, "Internal Server Error", e.message ?: "") - 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.renderTemplate(bookshelfTemplateId, jsonText, "/bookshelf") + } if (debugEnabled) log.debug("Bookshelf result is '{}'.", String(result)) @@ -910,8 +881,6 @@ class WebServer( writeNormalToClient(writer, output, String(result)) if (debugEnabled) log.debug("Leaving realHandleBsEndpoint().") - - return true } /** @@ -1070,22 +1039,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..1fb9c9f0f2 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 @@ -392,6 +386,114 @@ 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")) + // 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) + } + } + + // 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. + // + // 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() + 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) + } + } + + // 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 new file mode 100644 index 0000000000..19e80caa5d --- /dev/null +++ b/common/src/main/java/com/itsaky/androidide/documentation/DatabaseTemplateLoader.kt @@ -0,0 +1,138 @@ +/* + * 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 org.slf4j.LoggerFactory +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'") + + // 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") + } + + return StringReader(body.toString(Charsets.UTF_8)) + } + + override fun resourceExists(name: String): Boolean { + val database = database() ?: return false + + // 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. + // + // False rather than a throw, on both a database error and a duplicated name. This is + // Pebble's existence predicate, which a DelegatingLoader uses to decide whether to fall + // through to the next loader; a throw aborts resolution where a miss would fall back. An + // earlier version threw here to keep SQL text out of the response, which kept the SQL out + // but broke the contract to do it -- logging keeps both. + // + // A duplicated name answers false because [getReader] refuses to load one: a predicate that + // said yes to something the reader then rejects is worse than one that says no. + return try { + database.rawQuery(COUNT_QUERY, arrayOf(name)).use { cursor -> + cursor.moveToFirst() && cursor.getInt(0) == 1 + } + } catch (e: RuntimeException) { + log.warn("Cannot look up template '{}' in the database", name, e) + false + } + } + + 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 val log = LoggerFactory.getLogger(DatabaseTemplateLoader::class.java) + + private const val TEMPLATE_QUERY = "SELECT content FROM Templates WHERE name = ?" + + /** Counts rather than existence-checks, so a duplicated name can be told from a single one. */ + private const val COUNT_QUERY = "SELECT COUNT(*) FROM Templates WHERE 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 b63a347017..173d95a84e 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.google.gson.Gson import com.google.gson.GsonBuilder import com.google.gson.ToNumberPolicy @@ -25,8 +26,7 @@ import com.google.gson.reflect.TypeToken import com.itsaky.androidide.utils.BrotliDictionaryCodec import com.itsaky.androidide.utils.loadCompressionDictionary import io.pebbletemplates.pebble.PebbleEngine -import io.pebbletemplates.pebble.loader.StringLoader -import io.pebbletemplates.pebble.template.PebbleTemplate +import io.pebbletemplates.pebble.error.PebbleException import org.slf4j.LoggerFactory import java.io.ByteArrayInputStream import java.io.Closeable @@ -104,6 +104,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. @@ -156,9 +174,11 @@ 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. 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. */ + @VisibleForTesting @Volatile var generation: Long = 0 private set @@ -186,10 +206,19 @@ class DocumentationContentSource( private var codec: BrotliDictionaryCodec? = null private var codecStale = 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 }) + .maxRenderedSize(MAX_RENDERED_CHARS) + .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() @@ -277,8 +306,11 @@ 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. */ + @VisibleForTesting fun refreshDatabase() { if (!openIfNeeded()) return swapDatabaseIfChanged() @@ -304,25 +336,43 @@ class DocumentationContentSource( } /** - * Renders a template using the supplied JSON context. + * Renders the named 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. + * 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 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 renderTemplate( - templateId: Int, - contextJson: ByteArray, + fun renderNamedTemplate( + name: String, path: String, - ): ByteArray = withDatabase { database -> render(database, templateId, contextJson, path) } + contextJson: (SQLiteDatabase) -> ByteArray, + ): ByteArray = withDatabase { database -> renderNamed(name, contextJson(database), path) } /** - * Clears all cached compiled templates. + * Clears all cached templates, compiled and by name. + * + * Takes the write lock, which `ReentrantReadWriteLock` will not upgrade to from a read hold, so + * `withDatabase { clearTemplateCache() }` deadlocks that thread permanently. */ - fun clearTemplateCache() { - templateCache.clear() - } + fun clearTemplateCache() = + // 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() + pebbleEngine.templateCache.invalidateAll() + pebbleEngine.tagCache.invalidateAll() + } /** The last-modified time of [file], or -1 when it does not exist. */ private fun timestampOf( @@ -427,50 +477,99 @@ 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) } + 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 TemplateRenderException("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 try { + StringWriter().also { pebbleEngine.getTemplate(name).evaluate(it, context) }.toString().toByteArray() + } catch (e: PebbleException) { + // 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 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 TemplateRenderException( + "Rendering template '$name' overflowed the stack; check for a reference cycle between templates", + e, + ) + } } /** - * 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. - * @throws IllegalStateException If the template is missing or has multiple database rows. + * @return The template's name. + * @throws TemplateRenderException If the template is missing, has multiple database rows, or + * cannot be read. */ - 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 -> - 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 -> { - val body = cursor.getBlob(0) - if (log.isDebugEnabled) log.debug("Compiling template {}, {} bytes.", templateId, body.size) - pebbleEngine.getTemplate(body.toString(Charsets.UTF_8)) + ): String = + 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'") + } + + // 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'") + } } } + } 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) } /** @@ -651,7 +750,7 @@ class DocumentationContentSource( // reused. codec = null codecStale = true - templateCache.clear() + clearTemplateCache() generation++ try { @@ -664,6 +763,22 @@ class DocumentationContentSource( companion object { 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'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. + // + // 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 FROM Content C, ContentTypes CT 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..a44445357c --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/documentation/DatabaseTemplateLoaderTest.kt @@ -0,0 +1,170 @@ +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 +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] + // The two queries answer differently: a count always has a row, a template + // lookup has one only when the template is there. + val counting = firstArg().contains("COUNT") + mockk(relaxed = true) { + every { moveToFirst() } returns (counting || body != null) + every { getInt(0) } returns if (body != null) 1 else 0 + every { count } returns if (body != null) 1 else 0 + 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]") + } + + @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") + } + + @Test + fun `an existence check that fails answers false rather than throwing`() { + // Pebble's existence predicate, which a DelegatingLoader uses to decide whether to fall + // through to the next loader: a throw aborts resolution where a miss would fall back. An + // earlier version threw to keep SQL text out of the response, which broke the contract to + // do it. + val database = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } throws + SQLiteException("no such table: Templates (code 1): , while compiling: SELECT 1 FROM Templates") + } + + assertThat(loader(database).resourceExists("nav.peb")).isFalse() + } + + @Test + fun `a duplicated name does not exist, since it cannot be loaded`() { + // getReader refuses a name with more than one row, so a predicate that said yes to it would + // promise something the reader then rejects. + val database = + mockk(relaxed = true) { + every { rawQuery(any(), any()) } returns + mockk(relaxed = true) { + every { moveToFirst() } returns true + every { getInt(0) } returns 2 + } + } + + assertThat(loader(database).resourceExists("nav.peb")).isFalse() + } +} 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..f5cad65a29 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 @@ -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,122 @@ 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 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", "/bookshelf") { """{"who": "Kotlin"}""".toByteArray() } + + 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", "/bookshelf") { "null".toByteArray() } + } + } + + // 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") + } + + // 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 }}!") 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 +419,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 +451,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 +470,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 158c348d67..8fb2303bcc 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 brotli row would then be decoded with a dictionary attached and fail outright, since ADFA-5240 removed the plain-decode retry that used to mask this), 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. Both the reader and the writer load it through `loadCompressionDictionary` in `:common`, so they cannot disagree about whether a given database's brotli rows carry a dictionary — a row compressed against one the reader won't attach is undecodable, and vice versa. `DocumentationContentSource` loads it lazily -- not at database open or swap time, but on the first content lookup after the active database changes, whatever that row's own compression, 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). Each row is then decoded exactly once, with the dictionary attached or not according to that version; there is no retry, because there is no longer a second way a row might have been written. -- **`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. 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. @@ -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.