Repository navigation
Commit 5a26dfa
ADFA-5405: Let templates reference each other (#1779)
* 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.
* 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.
* 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.
* 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.
* 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 "<text> (<file>:<line>)" 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.
* 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.
* ADFA-5405: Keep a parse error's file and line, and bound a runaway render
Second review pass on #1779.
The getPebbleMessage() change in 6e6d865 traded one diagnostic loss for
another: it strips PebbleException's "(<file>:<line>)" 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.
* 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr
* 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
* 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
* 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
* ADFA-5405: honour the loader's existence contract, and stop encoding one fact twice
resourceExists is Pebble's existence predicate -- what a DelegatingLoader
uses to decide whether to fall through to the next loader -- so a throw
there aborts resolution where a miss would fall back. The previous commit
made it throw, to keep SQL text out of the response. That kept the SQL
out and broke the contract to do it; logging keeps both.
It also disagreed with getReader about a duplicated name: the predicate
said the template existed, the reader refused to load it. It counts now
rather than existence-checking, so a name with more than one row answers
false, which is what the reader will do with it anyway.
realHandleBsEndpoint returns Unit. Removing the isCursorOneRow path took
its only `return false` with it, so the Boolean had become a constant
that the caller assigned to the same variable markOutputStarted already
set -- two mechanisms for one piece of state, and a later early return
that updated only one would leave the caller sending response headers
onto a socket that already carries a body.
Tests: 113 green across the documentation and web server suites, two new
for the predicate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent 424e8d8 commit 5a26dfa
7 files changed
Lines changed: 762 additions & 179 deletions
File tree
- app/src
- main/java/com/itsaky/androidide/localWebServer
- test/java/com/itsaky/androidide/localWebServer
- common/src
- main/java/com/itsaky/androidide/documentation
- test/java/com/itsaky/androidide/documentation
- docs
Lines changed: 42 additions & 89 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
4 | 3 | | |
5 | 4 | | |
6 | 5 | | |
| |||
11 | 10 | | |
12 | 11 | | |
13 | 12 | | |
| 13 | + | |
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
| |||
148 | 148 | | |
149 | 149 | | |
150 | 150 | | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
155 | | - | |
156 | | - | |
157 | | - | |
158 | | - | |
159 | | - | |
160 | | - | |
161 | | - | |
162 | | - | |
163 | 151 | | |
164 | 152 | | |
165 | 153 | | |
| |||
547 | 535 | | |
548 | 536 | | |
549 | 537 | | |
550 | | - | |
| 538 | + | |
| 539 | + | |
551 | 540 | | |
552 | 541 | | |
553 | 542 | | |
554 | | - | |
555 | | - | |
556 | | - | |
557 | | - | |
558 | | - | |
559 | | - | |
560 | | - | |
561 | | - | |
562 | | - | |
563 | | - | |
564 | | - | |
565 | | - | |
566 | | - | |
567 | | - | |
568 | | - | |
569 | | - | |
570 | | - | |
571 | | - | |
572 | | - | |
573 | | - | |
574 | | - | |
575 | 543 | | |
576 | 544 | | |
577 | 545 | | |
| |||
584 | 552 | | |
585 | 553 | | |
586 | 554 | | |
587 | | - | |
588 | | - | |
589 | 555 | | |
590 | 556 | | |
591 | 557 | | |
| |||
623 | 589 | | |
624 | 590 | | |
625 | 591 | | |
626 | | - | |
| 592 | + | |
| 593 | + | |
| 594 | + | |
| 595 | + | |
| 596 | + | |
| 597 | + | |
| 598 | + | |
627 | 599 | | |
628 | 600 | | |
629 | 601 | | |
| |||
801 | 773 | | |
802 | 774 | | |
803 | 775 | | |
804 | | - | |
| 776 | + | |
805 | 777 | | |
806 | 778 | | |
807 | | - | |
| 779 | + | |
| 780 | + | |
| 781 | + | |
| 782 | + | |
| 783 | + | |
| 784 | + | |
| 785 | + | |
| 786 | + | |
| 787 | + | |
| 788 | + | |
| 789 | + | |
| 790 | + | |
| 791 | + | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
808 | 795 | | |
809 | 796 | | |
810 | 797 | | |
| |||
864 | 851 | | |
865 | 852 | | |
866 | 853 | | |
867 | | - | |
| 854 | + | |
| 855 | + | |
| 856 | + | |
| 857 | + | |
| 858 | + | |
868 | 859 | | |
869 | 860 | | |
870 | 861 | | |
871 | 862 | | |
872 | 863 | | |
873 | | - | |
| 864 | + | |
874 | 865 | | |
875 | 866 | | |
876 | | - | |
877 | | - | |
878 | | - | |
879 | | - | |
880 | | - | |
881 | | - | |
882 | | - | |
883 | | - | |
884 | | - | |
885 | | - | |
886 | | - | |
887 | | - | |
888 | | - | |
889 | | - | |
890 | | - | |
891 | | - | |
892 | | - | |
893 | | - | |
894 | | - | |
895 | | - | |
896 | | - | |
897 | | - | |
898 | | - | |
899 | | - | |
900 | | - | |
901 | | - | |
| 867 | + | |
| 868 | + | |
| 869 | + | |
| 870 | + | |
| 871 | + | |
| 872 | + | |
| 873 | + | |
| 874 | + | |
902 | 875 | | |
903 | | - | |
904 | | - | |
905 | | - | |
| 876 | + | |
906 | 877 | | |
907 | 878 | | |
908 | 879 | | |
909 | 880 | | |
910 | 881 | | |
911 | 882 | | |
912 | 883 | | |
913 | | - | |
914 | | - | |
915 | 884 | | |
916 | 885 | | |
917 | 886 | | |
| |||
1070 | 1039 | | |
1071 | 1040 | | |
1072 | 1041 | | |
1073 | | - | |
1074 | | - | |
1075 | | - | |
1076 | | - | |
1077 | | - | |
1078 | | - | |
1079 | | - | |
1080 | | - | |
1081 | | - | |
1082 | | - | |
1083 | | - | |
1084 | | - | |
1085 | | - | |
1086 | | - | |
1087 | | - | |
1088 | | - | |
1089 | 1042 | | |
1090 | 1043 | | |
1091 | 1044 | | |
| |||
Lines changed: 110 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
363 | 363 | | |
364 | 364 | | |
365 | 365 | | |
366 | | - | |
367 | | - | |
| 366 | + | |
| 367 | + | |
368 | 368 | | |
369 | | - | |
370 | | - | |
371 | | - | |
372 | | - | |
373 | | - | |
374 | | - | |
375 | 369 | | |
376 | 370 | | |
377 | 371 | | |
| |||
392 | 386 | | |
393 | 387 | | |
394 | 388 | | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
395 | 497 | | |
396 | 498 | | |
397 | 499 | | |
| |||
0 commit comments