Extract BookClient interface + storage adapter design docs - #71
Closed
QuirkyTurtle94 wants to merge 19 commits into
Closed
Extract BookClient interface + storage adapter design docs#71QuirkyTurtle94 wants to merge 19 commits into
QuirkyTurtle94 wants to merge 19 commits into
Conversation
Introduce a BookClient abstraction as the seam between UI code and the storage backend so a Postgres adapter (issue #48) can slot in without touching domain or UI code. OpfsBookClient wraps the existing GnuCashWorkerClient unchanged; dashboard-context now consumes only BookClient via the createBookClient factory. No behaviour change for OPFS users. The architectural plan (docs/architecture/storage-adapters.md) and adversarial security review land alongside so subsequent PRs can reference them. Refs #48.
Two build targets from one codebase, selected via BUILD_MODE env var:
- BUILD_MODE=local → static export, OPFS-only (today's behaviour).
Deploys to Cloudflare Pages, nginx, etc.
- BUILD_MODE=server → Next.js server, will host API routes for the
Postgres adapter (coming in follow-up commits).
npm run build now aliases to build:local so the existing Dockerfile and
Cloudflare deploy keep working without change. build:server is a new
opt-in entrypoint.
bin/build-local.mjs wraps `next build` so that when src/app/api/ exists
(future PRs) it is stashed out of the route tree during the static build
and restored in a finally block, making failed builds safe to retry.
NEXT_PUBLIC_SERVER_MODE is exposed to client code so the BookClient
factory can tree-shake server-only modules out of the static bundle.
Refs #48.
Mirrors the schema emitted by GnuCash's own libdbi backend, taken
1:1 from gnucash-source/libgnucash/backend/dbi/gnc-*-sql.cpp. Table
names, column names, column order, NOT NULL constraints, and per-table
version numbers all match what desktop GnuCash writes when using its
PostgreSQL backend.
Type mapping follows gnc-dbisqlconnection.cpp:
CT_GUID → varchar(32)
CT_STRING → varchar(N)
CT_TIME → timestamp
CT_GDATE → date
CT_BOOLEAN → integer (0/1, preserving GnuCash convention)
CT_NUMERIC → pair of int8 {field}_num / {field}_denom
CT_ADDRESS → 8-column expansion
CT_OWNERREF → 2-column expansion
Includes business tables (customers, vendors, employees, invoices,
entries, orders, jobs, billterms, taxtables, taxtable_entries) as
pass-through so round-trip import/export preserves files from users
who do use GnuCash's invoicing features.
No foreign key constraints — GnuCash enforces referential integrity
at the application level; we preserve that convention.
Phase 1 uses a single fixed schema 'gnudash_book'. Phase 2 will
clone this structure per book.
Refs #48.
The adversarial security review flagged a naive ?->$N rewrite as the
load-bearing design risk for the Postgres adapter (see
storage-adapters-security-review.md §0). A global string replace
corrupts ?-characters that sit inside string literals, identifiers,
comments, or dollar-quoted blocks -- silently breaking queries, or
producing parameter bindings an attacker can shift with a crafted
literal.
rewritePlaceholders is a context-aware tokenizer that recognises:
- single-quoted strings with '' escape
- E-strings with backslash escapes and '' escape, respecting word
boundary so `employee'x'` is identifier + string, not E-string
- double-quoted identifiers with "" escape
- dollar-quoted strings with balanced tags, including $$...$$
- line comments (-- ... newline)
- block comments with nesting (Postgres nests these)
Only ? characters outside all of those are rewritten.
39 vitest cases cover basic rewriting, each quote/comment variant, the
word-boundary rule for E-strings, realistic multi-line domain queries,
and the adversarial fixtures flagged in the review (attacker-controlled
literals containing ??, injected --, /*, $$ sequences).
Refs #48.
The plan originally called for Postgres UDFs (julianday, strftime) to polyfill SQLite functions. Auditing the domain code showed every SQLite-ism is funnelled through three helpers in shared/dates.ts: sqlMonth, sqlYear, sqlMonthNum. Making those helpers dialect-aware is cleaner than UDFs — no polyfill, no shim maintenance, and Postgres gets native to_char() which handles timestamp + null propagation correctly. - DbAdapter gains a readonly `dialect: "sqlite" | "postgres"` field. - All existing adapters (createWasmAdapter, createWritableWasmAdapter, createWritableConnection, createWritableMemoryDb) set "sqlite". - openAndValidate now wraps better-sqlite3 (was a bare cast) so it can satisfy the interface properly. - ParseContext mirrors adapter.dialect so domain functions can read it. - sqlMonth/sqlYear/sqlMonthNum accept dialect and emit native to_char(col, 'YYYY-MM' | 'YYYY' | 'MM') on Postgres, keeping the existing CASE/strftime/substr dual-format dance for SQLite (where dates come in as either compact YYYYMMDDHHMMSS or ISO TEXT). - 8 domain call sites updated to pass ctx.dialect. - 7 new vitest cases covering both emissions. No UDF migration needed for Phase 1. If future domain SQL reaches for strftime/julianday directly, add UDFs at that point; this commit makes that unnecessary for the code we have today. Refs #48.
Postgres has no sync driver, so the adapter interface can't stay sync if we want the same domain code to run against SQLite WASM (browser) and pg (server). PreparedQuery.all()/.get() → Promise<T> WritableDbAdapter.run()/.exec() → Promise<T> .transaction(fn) → fn is async, result is Promise SQLite-backed adapters (WASM, better-sqlite3) wrap their sync returns in Promise.resolve so there's no real cost on the browser path. Domain code, engine ops, engine builders, validation, context-builder, the Web Worker dispatcher, and tests all add awaits at call sites. Function signatures change from `T` to `Promise<T>` and `() => T` callbacks to `async () => T` where needed. Pattern at cast sites: `(await db.prepare(sql).all()) as T[]` — the parentheses matter because `as` binds tighter than `await`. No behavioural change intended — the Promise.resolve wrapping is a microtask hop, invisible in practice. The full test suite still passes bar the pre-existing parseGnuCashFile snapshot drift that's been present since before this branch. This is the last invasive change in Phase 1. The Postgres adapter, API routes, and import/export endpoints can now slot in on top of an interface that actually supports them. Refs #48.
Progress-bar drill-down was setting manualDrillPath correctly, but the render path then clobbered drillPath back to [] whenever selectedCategory was null. Effectively the only way to drill was through the pie chart. Drop the reset branch: manualDrillPath now drives drill-down on its own; selectedCategory only overrides it when present. Pre-existing regression, surfaced during Phase 1 testing.
Phase 1 Postgres wiring for issue #48. Three layers: pg-pool.ts singleton pg.Pool; enforces TLS in production, refuses rejectUnauthorized:false without a CA; sets statement_timeout / idle_in_transaction_session_timeout / lock_timeout at connection level. writable-pg-adapter implements WritableDbAdapter against a checked-out PoolClient. prepare() runs every SQL through the ?→$N tokenizer before binding. close() is a no-op — client lifecycle belongs to withBookClient. with-book-client the one sanctioned way to execute queries. Validates schema name against strict regex, calls set_config('search_path', $1, true) so the identifier value is parameterised, runs the callback, RESET ALL on finally, destroys the client on error. Also schema-name.ts with 8 vitest cases covering valid Phase 1 / Phase 2 patterns and rejecting every injection shape the security review flagged. All server-side files import server-only so misuse from client code fails at build time. Refs #48.
Phase 1 authentication (single shared passphrase). Operator sets
APP_PASSPHRASE_HASH via one-shot Argon2id hash generation; login verifies
with a decoy-hash fallback so an empty or wrong passphrase takes the
same time as a correct one (security review §1.5).
Components:
lib/server/auth.ts always runs exactly one argon2 verify,
against APP_PASSPHRASE_HASH or a decoy.
lib/server/session.ts iron-session with __Host- cookie prefix,
httpOnly, Secure (prod), SameSite=Strict.
Versioned payload so Phase 2 can extend it.
lib/server/rate-limit.ts in-memory 5/15min bucket keyed by IP;
IP resolution honours TRUSTED_PROXY_HOPS
(security review §1.6).
/api/auth/login POST; Content-Type guard, Origin check vs
APP_ORIGIN, zod strictObject validation.
/api/auth/logout POST; destroys session cookie.
/api/health GET; does not touch Postgres.
middleware.ts protects /api/* except auth + health by
requiring a session cookie; route handlers
do the authoritative verification.
Adds @node-rs/argon2, iron-session, zod to dependencies.
Refs #48. Part of Phase 1.
Adds POST /api/book/import and GET /api/book/export for the Phase 1
single-book flow. Both routes sit behind the iron-session gate.
lib/server/import-export/tables.ts per-table descriptors + timestamp /
date transforms between compact
GnuCash TEXT and Postgres native
timestamp/date. Pass-through
business tables declared but full
row-copy deferred (dashboard doesn't
read them; opaque copy without
column metadata risks silent data
loss).
import.ts in-memory better-sqlite3 (binary
only for now), TRUNCATE-then-insert
inside one Postgres transaction,
refuses non-empty target unless
?overwrite=true (409).
export.ts in-memory SQLite with the project's
reduced DDL, row-by-row copy from
Postgres, returns serialised bytes
as a Blob.
/api/book/import POST, 100 MB cap, Origin-pinned,
session-gated.
/api/book/export GET, session-gated, streams
application/x-gnucash.
Known gaps tracked for follow-up:
- pass-through business tables not yet copied;
- reduced DDL in export means desktop GnuCash may not open the file
(columns like reconcile_date, commodity.quote_* etc. missing);
- gzipped XML uploads not yet supported server-side.
Refs #48.
Adds the read-side of the server-mode plumbing:
lib/server/book-dispatch.ts whitelisted DomainFunction → handler map.
Each handler receives a ParseContext built
fresh for the request from the scoped pg
adapter. Adding a new function requires a
code change here (security review §1.1).
/api/book/query POST, session-gated, Origin-pinned,
zod-validated. Dispatches via withBookClient
+ book-dispatch.
lib/client/api-book-client BookClient impl that fetches /api/book/...
Reads work end-to-end (full dashboard data
over HTTP). Mutations stub with a clear
error pointing at the follow-up PR.
lib/client/factory extended to accept backend: "api" via a
dynamic require, gated on NEXT_PUBLIC_SERVER_MODE.
Local builds tree-shake the ApiBookClient
module entirely.
Mutation dispatcher + setCurrency over HTTP are scaffolded-but-not-
implemented; tracked for the next PR on this branch.
Refs #48.
Two-image strategy for the two deploy shapes:
Dockerfile existing — static export, nginx serves /out.
Unchanged; covers Cloudflare Pages / GitHub
Pages / nginx deploys.
Dockerfile.server new — Next.js server, USER node (non-root),
--cap-drop ALL friendly, read-only rootfs
friendly. Builds with BUILD_MODE=server,
prunes dev deps, healthcheck on /api/health
(does not touch Postgres so DB outages don't
loop the container).
docker/docker-compose.example.yml
reference compose for self-hosters including
a Coolify-style Postgres service with volume
mount for the CA cert and init SQL.
docker/init.sql.example two-role Postgres bootstrap (gnudash_migrator
for DDL, gnudash_app for everyday queries).
docs/deployment-server.md
full operator runbook — Postgres role setup,
TLS / CA cert posture, Docker single-container
walkthrough, Coolify walkthrough, backup
(with age encryption), rotation and incident
response. Refers back to the plan and the
security review for threat model.
Install scripts disabled during build (npm ci --ignore-scripts); only
@node-rs/argon2 and better-sqlite3 get their native bindings rebuilt
explicitly by name. Closes off the transitive post-install vector
flagged in the security review §1.14.
Refs #48.
Three-job workflow matching the plan (task 8):
lint-typecheck eslint + tsc --noEmit on every push/PR.
test-sqlite existing vitest suite against SQLite WASM / better-sqlite3.
test-postgres Postgres 16 service container. Bootstraps the two roles
from deployment-server.md, applies
db/migrations/0001_gnucash_schema.sql as the migrator,
grants the app role day-to-day access. Runs integration
tests under engine/db/pg/__tests__/integration/ (empty
today; populated in a follow-up).
The round-trip-gnucash-cli job (task 14) is commented-in as a plan-of-record
rather than active CI — enabling it requires a real GnuCash 3.x fixture in
the repo and gnucash-cli installed in the runner, both follow-ups.
Refs #48.
The snapshot was frozen before these two fields landed in DashboardData:
prices — added alongside the transaction-linked prices
work in commit 6796ba6 (the prices table).
orphanedPriceGuids — same commit; surfaces prices whose linked
transaction no longer exists.
The code was producing both correctly; the snapshot just hadn't been
regenerated. No behavioural change.
Full suite now: 186/186 passing.
Two compose flavours plus the reference file from earlier:
docker-compose.dev.yml Postgres + Adminer only. App runs
natively via `npm run dev` for hot
reload. Postgres comes up with roles,
schema, and grants pre-applied via
docker-entrypoint-initdb.d scripts, so
`up -d` gives you a ready-to-use DB
with no manual psql. `down -v` wipes
the volume for a fresh start.
docker-compose.prod.yml full-stack: builds Dockerfile.server
and runs the app container against the
same dev Postgres setup. For smoke-
testing the image before a PR, not
daily iteration. Reads secrets from
.env.prod (gitignored).
docker-compose.example.yml unchanged — reference for real
self-hosted deploys.
docker/dev-init/01-roles.sql creates gnudash_app role, per-role
timeouts, locks down public schema.
docker/dev-init/03-grants.sql runs after the schema migration
(02-schema.sql mounted from
app/db/migrations/0001_gnucash_schema.sql),
grants CRUD to the app role and sets
default privileges so future migrations
don't need manual re-grants.
docker/README.md usage walkthrough for each compose file
including Adminer credentials and the
app command to pair with the dev DB.
.gitignore ignores docker/.env.prod.
Dev passwords (testpass / testapppass) are hard-coded in the init
scripts — documented-only-for-dev, docs/deployment-server.md covers
real-deploy secret generation.
Refs #48.
Server-mode builds now let the user pick where a book lives from the
upload page: "Browser" (OPFS, default, local-mode parity) or "Server"
(Postgres via ApiBookClient). Choice persists to sessionStorage.
Local-mode builds stay OPFS-only — the selector hides, the login form
is unreachable, the factory tree-shakes ApiBookClient out of the bundle
as before.
New components:
components/upload/backend-selector.tsx two-panel radio.
components/upload/login-form.tsx passphrase entry, posts to
/api/auth/login via the
dashboard context.
New endpoint:
/api/auth/me session probe. UI uses this
on mount to tell "not logged
in" apart from "logged in
but no book yet".
Context changes (lib/dashboard-context.tsx):
- backend state + setBackend() disposes the current client and
rebuilds against the new backend, clearing per-book caches.
- needsLogin / login() / logout() for the api path.
- serverModeAvailable exposes the compile-time NEXT_PUBLIC_SERVER_MODE
flag so components can decide whether to render the selector/login.
- uploadFile and other methods surface 401 by bouncing back to the
login form.
- API mode is always writable once authed; toggleWritable is a no-op
there.
FileUpload component:
- if serverMode + backend=api + needsLogin → renders LoginForm.
- otherwise renders the existing upload UI with the BackendSelector
on top (server mode only).
- footer copy swaps between the "data stays in browser" and
"stored on server in Postgres" messages.
186/186 tests pass; both build modes compile cleanly.
Refs #48.
Two related bugs surfaced when exercising the server-mode path end-to-end: 1. POST /api/book/import crashed with "db.deserialize is not a function". better-sqlite3 opens from file paths, not from a buffer. Fix: persist the uploaded bytes to a randomly-named file under os.tmpdir() at mode 0600, open it read-only, and clean up in a finally. Container deploys already mount a writable tmpfs at /tmp (see Dockerfile.server), so the fs posture doesn't change. 2. POST /api/book/query returned 500 at mount time on an empty Postgres schema. ApiBookClient.restoreSession was calling computeCurrentNetWorth to probe whether a book existed — on an empty schema that hits buildParseContext's "Could not find root account" branch. The client caught the throw but the server still logged a 500, which was both noise and a bad signal to condition UI flow on. Fix: add a lightweight /api/book/exists route that counts rows in `books` (GnuCash's internal per-file metadata table; always exactly one row in a valid book, zero when empty). ApiBookClient.restoreSession now calls exists instead of a domain query, so mount-time flow on an empty schema returns a clean true/false with no 500 in the log. Refs #48.
withBookClient was calling SELECT set_config('search_path', $1, true) —
transaction-local. The problem: the callers don't always wrap their work
in BEGIN/COMMIT (reads often don't), and Postgres silently ignores a
transaction-local set_config outside a transaction block. So
search_path stayed at the default, unqualified `accounts` / `books`
/ etc. failed with "relation does not exist" even though the schema
migration had populated gnudash_book correctly.
Fix: flip is_local to false. Session-level set persists for the life
of the client; RESET ALL in withBookClient's finally still cleans up
before the client returns to the pool, so tenant bleed-between is
still impossible. Destroy-on-error stays in place as belt-and-braces
for the case where RESET ALL itself can't run.
Refs #48.
GRANT ON ALL TABLES only covers tables existing at grant time. Operators who ran the grants before applying the migration — or re-applied the migration after granting — ended up with newer tables (versions, etc.) that gnudash_app couldn't read, surfacing as 'permission denied for table versions' at import time. ALTER DEFAULT PRIVILEGES makes future tables in gnudash_book inherit the same grants automatically. Idempotent, safe to re-run.
Owner
Author
|
Closing — approach abandoned, starting from a different direction. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First step toward issue #48 (pluggable storage backends). Introduces the
BookClientabstraction so a Postgres adapter can slot in without touching UI or domain code, and lands the architectural plan for review.BookClientinterface (app/src/lib/client/book-client.ts) — the seam between UI and storage backend.OpfsBookClientwraps the existingGnuCashWorkerClientunchanged. No behaviour change for OPFS users.createBookClientfactory picks the backend — only OPFS wired today;ApiBookClientlands in a follow-up PR.dashboard-context.tsxnow depends only onBookClient.docs/architecture/storage-adapters.md— full plan: two build modes (local/server), schema-per-book Postgres, GnuCash 1:1 compatibility, Phase 1 / Phase 2 split.docs/architecture/storage-adapters-security-review.md— adversarial review of the plan with P0/P1/P2 tiering.Scope
In: interface extraction, factory, docs.
Out: Postgres adapter, API routes,
BUILD_MODEsplit, auth — each lands as its own PR per the build order in the plan (§13).Test plan
npx tsc --noEmit— cleannpm run lint— no new errors in the changed files (existing warnings/errors on main unchanged)npx vitest run— all tests that pass on main still pass. One pre-existing snapshot failure insrc/lib/gnucash/__tests__/index.test.ts(parseGnuCashFilesnapshot drift from recent prices-table changes) is unaffected by this refactor and not introduced here.Refs #48.