Make the database portable and encryptable (#3848) - #5526
Open
shai-almog wants to merge 28 commits into
Open
Conversation
The database API was five unrelated implementations sharing an interface. Cursors counted from zero on some ports and one on others, iOS reported success on an empty result set and returned null for every blob, the simulator could not seek at all, and no port could encrypt anything. This lands the port-independent half: - package-info.java now carries the normative contract every port must satisfy: zero-based positions, first() lands on a row, execute() runs a whole script while the parameterized forms take exactly one statement, typed parameter binding, flat transactions, IOException with a chained cause, idempotent close. - AbstractDBCursor derives all navigation from two primitives, rewind() and stepForward(), so every port gets identical semantics rather than each reimplementing them. Seeks rewind and re-step, which is what Android's windowed cursor already does on a window miss; buffering rows instead would mean materializing every column of every row stepped past. - SQLStatementSplitter splits a script the way SQLite does, respecting string literals, quoted identifiers, comments and CREATE TRIGGER bodies. - DatabaseConfig, DatabaseEncryptionException and ManagedKeys add keyed opens. Managed keys are resolved in the core so every platform derives identical material from an alias, and a key that cannot be stored is fatal rather than a silent downgrade to plaintext. - db.legacy restores each platform's previous behaviour for the ten changes that alter a previously successful result. It is read lazily, because the generated stubs set it after Display.init. Blob parameters now raise IOException rather than RuntimeException, and the truncated javadoc samples in Database, Cursor and Row are replaced with complete ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The simulator was the weakest database implementation, which mattered more than it sounds: it is where people develop. Its cursor could not seek at all, because the JDBC driver only produces TYPE_FORWARD_ONLY result sets and first(), last(), prev() and position() each threw outright. execute() silently ran the first statement of a script and discarded the rest. rollbackTransaction() left the connection outside autocommit, so every following statement quietly joined a new implicit transaction. Every query leaked its PreparedStatement. - SECursor now extends AbstractDBCursor, rewinding by re-executing the statement. The simulator has working random access for the first time. - execute(String) splits the script and runs each statement, rather than trusting a driver to decide how much of it to run. - The parameterized forms reject a multi-statement script instead of dropping its tail. - Statements are closed on the success path, cursors are closed with the database, close() is idempotent and rollback restores autocommit. - getColumnName reports the result set label, matching getColumnIndex, so an aliased column can be found under the name it was found by. The shaded driver moves from org.xerial to io.github.willena, which is the same driver with SQLite3MC compiled in: same package, same config, verified identical on plaintext databases, plus the SQLCipher-compatible cipher the simulator needs to open a database written on a device. getV4Defaults() is required over getDefault() - the latter selects SQLite3MC's own variant, which real SQLCipher cannot read. That driver also stops being frozen. Freezing assumed the shaded content never changed; it now carries a crypto-bearing engine that has to track upstream security releases. SEDatabaseConformanceTest runs the portable contract against the real SEDatabase headlessly in about two seconds, including both the strict and legacy modes and the encrypt/decrypt round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iOS was the port the "radically different implementations" complaint is
really about, and it had real bugs behind the divergence:
- sqlDbClose called sqlite3_free on the connection handle. That never
closed it, leaked the file descriptor, skipped the WAL checkpoint and
handed the pointer to the wrong allocator. Now sqlite3_close_v2.
- sqlCursorValueAtColumnBlob was { return nil; }, so iOS could not read
a blob at all, in either direction.
- Opening a database called sqlite3_config(SQLITE_CONFIG_SERIALIZED) and,
on failure, sqlite3_shutdown(). That has to run before
sqlite3_initialize() to do anything, and calling shutdown with
connections open is undefined behaviour. Replaced with per-connection
SQLITE_OPEN_FULLMUTEX.
Behaviour now matches the portable contract:
- CursorImpl extends AbstractDBCursor, so last(), prev() and position()
work instead of throwing "Unsupported", and first() lands on a row and
reports false for an empty result set rather than reporting success and
leaving the statement unpositioned.
- Parameters bind by runtime type through new statement natives. They
used to be stringified, which stored an Integer as TEXT, and a comment
conceded it "will probably fail with blobs".
- Parameter count mismatches and multi-statement scripts in the
parameterized forms are rejected rather than silently mis-executed.
- Errors carry sqlite3_errmsg unconditionally; the dead XMLVM branches
that gated error reporting are gone.
- finalize() is removed from the database and cursor. Closing sqlite
handles from the GC thread is the "platform specific nuance" that
defeated ThreadSafeDatabase.
- Custom file:// database paths work, matching Android and the simulator.
Keying is a separate native that reports success rather than throwing, so
the Java side can tell a wrong key from a failure to open the file
without the native layer naming a core exception class.
isDatabaseEncryptionSupported() asks the linked engine via PRAGMA
cipher_version rather than assuming, so it reports honestly on a build
that does not bundle a cipher-capable SQLite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Android was already the most capable port, so this is mostly tightening rather than rebuilding: - A null element in a String[] now binds SQL NULL. bindString rejects null, so passing one used to fail the whole statement. - execute(sql, (Object[]) null) no longer dereferences a null array. - execute(String) runs a whole script. execSQL refuses anything after the first statement, so the script is split and run statement by statement. - executeQuery forces the window fill before returning, so malformed SQL is reported there rather than from the first next(). rawQuery is lazy. - Transactions use the shared flat-transaction guards, so a nested begin is rejected here as it already was everywhere else. - Exceptions carry their cause and are no longer printStackTrace'd on the way out. - Cursors are invalidated when the database closes, close() is idempotent, getRow() off a row throws, getColumnIndex is case insensitive, and wasNull() is false before any value has been read. - Blob query parameters work, bound through a cursor factory, which is the only supported route: rawQuery can carry text arguments only. This is what androidx.sqlite does for the same reason. Encryption lives in a new com/codename1/impl/android/cipher package built on net.zetetic:sqlcipher-android. It compiles against classes that are only on the classpath of app builds that use encryption, so it is excluded from the port's own javac and reached purely by reflection, letting the builder delete it for every app that never touches DatabaseConfig. That gating is why the package is a near copy of AndroidDB rather than a shared supertype: any shared type naming net.zetetic would have to live in the part of the port that must stay deletable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ports inherited the base openOrCreateDB, which returns null, so Database.openOrCreate() handed back null and calling code failed with a NullPointerException. They now have a full implementation that satisfies the same contract as every other port, encryption included. Neither runs a JVM, so JDBC was never an option; they needed a C binding. That is cheap because both are ParparVM C targets whose CMake project already compiles every .c in the source root. - The engine is SQLite3 Multiple Ciphers, bundled once in the translator and emitted only for applications that use com.codename1.db. iOS shares the same copy, so those three targets run one engine at one version, and the simulator's JDBC driver is built from the same upstream project. - The amalgamation is named .h deliberately. The iOS project generator lists .h but excludes it from the compile phase; CMake globs *.c for sources; and the ParparVM native symbol scanner reads only .c and .m. Named .c it would be compiled twice without its build options, named .inc it would ship inside the .ipa as 13MB of dead weight. - cn1_sqlite3.c is the single translation unit that compiles it, with the build options set immediately before the include so they cannot leak into unrelated sources. It is gated internally, so an emitted but disabled build produces an empty object rather than a link error. - The binding itself is shared. Both ports need identical code but mangle their entry points from different Java classes, so the logic lives once in cn1_db_sqlite_impl.h and each port's .c expands CN1_DB_DEFINE_NATIVES for its own prefix. Verified that every declared native has both its plain and its _R_ symbol in both ports. - iOS stops linking the system libsqlite3 when the bundled engine is used, rather than carrying two SQLite implementations in one process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JavaScript port sat on WebSQL, which Chrome removed in 119 and Firefox never implemented, so its database was dead on every current browser. What it did support was thin: transactions were printlns, getBlob threw, position(n) always returned the first row, close() did nothing, and the bridge busy-waited a CN1 thread on a lock. It now runs the same SQLite build the other ports use, compiled to WebAssembly, inside the application's own worker. Every call after the first is an ordinary synchronous call; only the initial load suspends, through the runtime's existing yield-on-promise support, so the lock and its 200ms poll are gone. Storage uses the opfs-sahpool VFS rather than the default OPFS one. The default needs crossOriginIsolated, which needs COOP/COEP response headers, which we cannot require of the arbitrary static hosting these bundles are deployed to. Browsers without synchronous OPFS access fall back to memory with a console warning, because silently losing every write on reload is not a failure anyone should discover in production. Gating, so nobody pays for what they do not use: - iOS emits the bundled engine, and drops the system libsqlite3, only for applications that reference DatabaseConfig. Everyone else keeps the system SQLite exactly as before. - Windows and Linux emit it for anything referencing com.codename1.db, since they have no system SQLite at all, and its cipher only when encryption is configured. - Android's SQLCipher package is deleted unless DatabaseConfig is referenced, and the AAR arrives through a new PlatformFeatureCatalog entry keyed on that same class. - The JavaScript builder prunes the 1.5MB engine from bundles that never open a database. The catalog entry is keyed on DatabaseConfig rather than the db package on purpose, and two new tests hold that line: every database application references com.codename1.db, so keying it there would bundle SQLCipher for all of them and push the minimum Android SDK from 19 to 23 for people who never asked for encryption. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract and the encryption are only real if they are checked, and the portability claim in particular is the kind that fails silently: a cipher misconfiguration produces files each platform reads perfectly well on its own and nothing else can touch. - Seven device tests run the shared conformance suite on every port through the existing screenshot harness. They are assertion only, so they take no screenshots and sit before the ordering-sensitive graphics baselines. Ports without a database self-skip, so a port turns green on its own once it has one. - Two of the seven run in legacy mode, which is what makes the compatibility promise testable rather than aspirational: they fail the moment a refactor changes what db.legacy restores. - Two Port Status features expose the results publicly, split so a threading regression cannot blank the whole database row. - scripts/ci/db-cipher-interop.sh checks our encrypted files against the stock sqlcipher client in both directions, with a raw key to isolate the cipher configuration and a passphrase leg to cover the key derivation. Wired into the pull request workflow. The developer guide's SQL section said the iOS SQLite "isn't threadsafe" and warned that the garbage collector closing a connection would crash the app. That was true, and this branch is what fixes it, so the section is rewritten and extended with encryption, key management, threading, cursor cost and the legacy compatibility table. ThreadSafeDatabase is un-deprecated. Its note blamed platform nuances; the nuance was the iOS finalizers, now gone. Its close() was fire and forget, so it returned before the database was closed and a following delete() raced it, which is fixed here too. The cursor inner classes are static: with an explicit owner field the implicit outer reference was dead weight, which SpotBugs flagged on iOS and would eventually have flagged everywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce77b834d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Companion PR with the build-side gating: codenameone/BuildDaemon#172 |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
- The Ant build for the JavaSE port links whichever sqlite-jdbc is pinned in cn1-binaries, which has no org.sqlite.mc, so importing the driver's config builder broke that build for everyone. JavaSEPort now writes the SQLCipher connection properties out literally, which needs no extra class at compile time, and reports isDatabaseEncryptionSupported() by probing for the cipher-capable driver rather than assuming it. The simulator therefore answers honestly under either build. - The Windows cross-compile failed to link. The sample application now uses com.codename1.db, but that integration test drives the translator directly rather than through the builder, so the engine was never emitted and the natives had no definitions. Two fixes: the shared binding header is always emitted and defines every entry point either way, as real bindings or as stubs that raise a clear IOException, so an application always links however the translator was invoked; and the integration tests ask for the engine explicitly, so those ports actually exercise the database instead of only ever self-skipping. Verified that both branches of the header export an identical symbol set. - The developer guide requires snippets to live in docs/demos and be included by tag. Migrated with the repository's own migration script. The snippet harness had no com.codename1.db import, which is why all three failed to compile once moved; added, since it is a core package the guide documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
The Maven build already excluded it, but the Ant target compiles every source in the port, so it tried to build the package against net.zetetic and failed for anyone building that way -- including BuildDaemon CI, which clones this repo and runs the Ant target. Mirrors the exclusion into both places the ARCore and AI packages already use: the javac in Ports/Android/build.xml and the excludes property in nbproject/project.properties. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d595bd94da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 12 screenshots: 12 matched. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Review findings, all eight real: - Database.encrypt() could never work on Android. The system SQLite has no cipher, so a plaintext database opened through it can never be re-keyed. Added openOrCreateDBForRekey(), which Android routes through SQLCipher (an empty key opens an unencrypted file, which can then be re-keyed). - A managed key resolves its keystore alias from the database name, and every port passed null when re-keying, so changeKey(managed()) raised a NullPointerException instead of encrypting. Each Database now retains the name it was opened under. - Two threads first-opening the same managed database could each see nothing stored, generate different keys and overwrite each other, leaving one of them holding data nobody could ever read. The read-generate-store sequence is now serialized. - isKeyHardwareBacked() inferred hardware backing from the API level, but emulators and plenty of real devices back AndroidKeyStore keys in software. It now asks the key itself, via KeyInfo. Applications are told they may use this to refuse to store sensitive data, so it has to be true. - checkEndTransaction() cleared the flag before the engine had ended the transaction, so a failed commit left the transaction open while the API believed it was closed, and the recovering rollback was rejected. Splitting out markTransactionEnded() means the flag drops only on success. A conformance check covers the failed-commit path. - An encrypted Android database opened by file:// URL had no toNativePath() conversion, so java.io.File treated the URL as a literal relative name. - Calling next() past the end repeatedly re-derived the row count each time, inflating it, after which last() would seek to a row that does not exist. Verified the new check fails against the old code (5 became 8). - PRAGMA rekey interpolated the key directly, so a passphrase containing a quote produced a different statement. Both Android and the simulator now go through one helper that quotes text and passes a raw key literal through untouched. CI failures: - Six SpotBugs findings in core-unittests, a module the earlier local runs had not covered: boxed constructors, a default-encoding String, and a Boolean-returning method that could return null. - The arm64 Linux and Windows cross-builds failed compiling the engine's ARM AES intrinsics. Where the compiler defines __ARM_FEATURE_CRYPTO the engine uses them directly, which is what Apple's toolchain does, so iOS is unaffected; otherwise it tags individual functions with __attribute__((target)), which the cross-compiling clang does not honour for these intrinsics. Rather than require ARM crypto extensions of every chip, that path now uses the software implementation. - DatabaseStatementLegacyTest failed on Android because the legacy expectation was wrong, not the code: only iOS ran a whole script before this branch, through sqlite3_exec. Android's execSQL and the simulator's PreparedStatement both dropped everything after the first statement. Corrected in the suite and in both places it is documented. - The migrated guide snippet fixture needed a copyright header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f2f2c70ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5767b05c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The compatibility switch was reachable only from code. Nothing read the db.legacy hint anywhere -- not the builders, not the simulator -- so the users most likely to need it, the ones on an Ant project that predates the portable contract, had no way to ask for it short of editing their application. Every builder now emits the switch into the generated stub, the simulator resolves the hint through JavaSEPort so it can be tested without a device, and the default follows the project type. The Maven plugin stamps its own version into the settings it hands over, and the Ant build client never has, so an Ant project is recognisable and is defaulted to the behaviour it was written against. A Maven project gets the portable contract. An explicit db.legacy overrides either way. The default only applies when the application actually references com.codename1.db, so it does not appear in the log of every Ant build. The decision lives on Executor, shared by the iOS, Android, Windows, Linux and JavaScript builders. The JavaScript launcher runs before Display exists, so it applies the switch through Database.setLegacyBehavior instead. The desktop wrapper carries an explicit hint only, since it is generated from a Maven project by definition. Also in here, because the fast gate found them: The simulator gate ran the conformance groups in strict mode only, so the compatibility promise was checked on devices and nowhere else. Running the legacy groups too immediately failed nine checks -- the suite keys its legacy expectations on the port, and with no Display up it could not tell which port it was, so it asserted the portable contract instead. Hence setPortKind, which lets a headless harness declare what it is testing. One over-stepping check then turned out to hardcode a position rather than adjust for the legacy base. Android's failed-commit recovery was still wrong. endTransaction() reports the failure and clears the wrapper's bookkeeping, but SQLite keeps the transaction open, so clearing the flag alone moved the failure from "a transaction is already in progress" to "cannot start a transaction within a transaction" one call later. It now rolls back through the engine rather than trusting the wrapper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e28281912b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Measured on API 34 rather than reasoned about, after the previous two
attempts at this both failed in CI.
SQLiteDatabase.endTransaction() pops its own transaction record before it
sends the COMMIT, so a failed COMMIT leaves the engine holding the
transaction while the wrapper believes there is none. In that state
inTransaction() reports false, the uncommitted row is still visible to every
later read, and the next beginTransaction() fails with "cannot start a
transaction within a transaction".
The wrapper cannot end it. The session layer classifies a statement by its
first three characters, so execSQL("ROLLBACK") never reaches SQLite: "ROL"
becomes its own endTransaction(), which throws because it thinks no
transaction is open. A second endTransaction() throws for the same reason,
and compileStatement and rawQuery route through the same classifier. A
statement that does not begin with those characters is passed to the engine
unexamined, so the rollback goes out behind a leading comment.
Probed against every candidate on a real emulator: no attempt, plain
ROLLBACK, a second endTransaction, and the comment-prefixed form. Only the
last recovers. The shipped sequence is verified end to end -- a later
transaction commits normally and the uncommitted row is gone rather than
lingering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 730a69a2aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Android encryption could never have worked. The reflective lookup asked for open(String, String) while the factory declares open(String, String, String), so every encrypted open raised NoSuchMethodException. What hid it is the more interesting half: the catch reported any failure as NOT_SUPPORTED, and the conformance suite treats an unsupported platform as a skip once the refusal is clean, which that satisfies. The bug produced a passing device run. A missing method on a class that is present is a broken build, not an unsupported platform, so the two are now separated and the second is loud. The same split applies to the re-key path, which used to fall back to the plaintext engine and would have turned a re-key into a silent no-op. Android bound every non-blob query argument as text, so a Long reached SQLite as TEXT and "SELECT ? = 42" and typeof(?) both answered wrongly. The cursor factory that already carried blobs carries every type, and is now the only strict-mode path; text coercion survives under db.legacy alone. The no-argument executeQuery overload skipped the parameter-count check on iOS, Linux, Windows and JavaScript, so a statement with placeholders ran with every slot left as NULL. Other fixes in this round: encrypt() and decrypt() created and converted an empty database when handed a name that does not exist, reporting success while the intended database sat untouched. ThreadSafeDatabase reported isInTransaction() false even directly after a begin, because the flag it read belongs to the wrapper and beginTransaction moves the underlying one; its CursorWrapper implemented only Cursor, so wrapping a database dropped CursorExt and count() answered -1 even on Android. wasNull() was the one shared cursor operation without a closed check. AndroidCipherFactory reported an unwritable directory or a full disk as WRONG_KEY, sending applications into prompting for a passphrase that cannot help; the simulator leaked the JDBC connection on that same rejection path because DatabaseEncryptionException is an IOException and missed the SQLException handler that owned the cleanup. The JavaScript bridge collapsed every open failure to the WRONG_KEY sentinel, including for opens with no key at all. Native delete discarded the result of remove(), so a read-only or locked file left delete() reporting success. The SLF4J finding on scripts/ci/db-cipher-interop.sh does not reproduce and is left alone: the script passes locally on JDK 8 with only the unshaded driver on the classpath, and CI has already run it green on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb0456a7c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The cipher package could not compile. AndroidCipherDB lives in com.codename1.impl.android.cipher and uses three package-private members of AndroidCursor - CloseListener, setCloseListener and invalidate - from another package. The package is excluded from the port jar by design and compiled inside the generated Gradle app, so any application referencing DatabaseConfig would have failed its compile. Those three are now public, with a comment saying why the seam crosses a package boundary at all. Nothing caught it because nothing built it. The builders gate the cipher payload on the application's own classes referencing DatabaseConfig, and the device test reached encryption only through DatabaseConformanceSuite, which lives in the core jar and is not scanned. So the CI app shipped without a cipher, isEncryptionSupported() answered false, and the whole group logged status=SKIPPED. That is also why the reflective arity bug fixed in the previous commit survived: the path was never built, let alone run. DatabaseEncryptionTest now references DatabaseConfig directly, which is what a real application does and what the gate is designed to detect. Also from a self-review of the previous commit: Routing every Android query through the typed cursor factory dropped blob support in legacy mode, because the legacy branch coerced arguments to text including byte[]. Blob query parameters used to throw on every port, so the compatibility switch deliberately does not cover them; a blob now takes the factory in both modes. This is what the Android suite went red on. SQLCipher reads page one during open to settle the page size, so a wrong key surfaces there and not only from the schema probe - the same place the simulator's driver reports it. Splitting the catch had reclassified that as a plain IOException, losing WRONG_KEY on the most common path of all. The JavaScript bridge tagged any probe failure as a key failure even when no key was supplied, so a corrupt plaintext file still reported WRONG_KEY. The tag is now conditional on a key being present, and a failure to close the pooled handle no longer replaces the tagged error. The parameter-count checks on iOS, Linux, Windows and JavaScript ignored legacy mode, so an Ant project - which now defaults to compatibility mode - would have started throwing on a query it used to run unbound, on three platforms but not the other two. executeQuery(String, String[]) on Android still rejected a null element rather than binding SQL NULL, so the same query behaved differently depending only on the declared type of the array. isInTransaction() reads the underlying flag directly rather than through the worker. It is the one Database method with no IOException, so callers treat it as a cheap accessor and poll it from the event thread; dispatching would block that thread behind whatever statement is running. Restored the 26 bare-LF lines in AndroidImplementation.java that later edits normalized again, taking that file back to 114 changed lines from 140. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6f57d42cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
With the encryption path finally being built and run, the device suite reached code nobody had executed, and two SQLCipher behaviours the port had assumed turned out to be wrong. Both were measured on an API 34 emulator rather than reasoned about. PRAGMA rekey cannot go through execSQL. It answers with a row, and execSQL rejects anything that returns one - "Queries can be performed using SQLiteDatabase query or rawQuery methods only" - so every re-key failed before it began. That is the error the suite reported. It goes through rawQuery now. More seriously, SQLCipher re-keys only between two encrypted states. On a plaintext database, and on a re-key to the empty key, it refuses outright: "PRAGMA rekey can only be run on an existing encrypted database. Use sqlcipher_export() and ATTACH to convert encrypted/plaintext databases." Those two cases are precisely Database.encrypt and Database.decrypt, so both were broken on Android. The SQLite3MC build the other ports carry does re-key all three directions in place, which is why only this port needs the other route. Those conversions now ATTACH the target with its key, run sqlcipher_export, and swap the finished file in. sqlcipher_export copies schema and rows but not the header pragmas, so user_version is carried across explicitly - an application using it for schema versioning would otherwise silently come back at zero. The converted database is built beside the original and only replaces it once complete, so an interruption leaves the original live. Verified on-device end to end: encrypt leaves ciphertext on disk that no longer opens without the key, decrypt leaves a "SQLite format 3" header, a round trip returns the file to plaintext, and the row and user_version survive all three. Documented the platform difference in the guide, including what a leftover .cn1migrate file next to a database means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 452c6cf58e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The first JavaScript suite run that reached the database tests wedged on DatabaseLifecycleTest and never finished, taking the whole 40 minute budget with it. The thread dump is unambiguous: the event thread was parked in "await" with every other thread queued behind it and nothing runnable. The engine bring-up is reached through the runtime's yield-on-promise bridge, which resumes the calling thread when the promise settles. Neither half of the bring-up is guaranteed to settle. Acquiring a synchronous OPFS access handle blocks while another context holds the same file, so a browser that never releases it leaves the promise pending indefinitely rather than rejecting, and a pending promise parks the event thread with nothing logged anywhere. A rejection was no better: the bridge resumes on resolution, so an importScripts or module-init failure hung in the same way. Both halves are now bounded by a timeout and the promise never rejects. A storage pool that will not open falls back to the in-memory VFS, which already existed for browsers without synchronous OPFS, so the database still works and the tests still run. An engine that will not load at all resolves to false, which the port already turns into a clean IOException and the suite reports as a skip with a reason. The same class of problem was in the open binding, which this also fixes: it had been changed to rethrow non-key failures so callers could tell storage errors from authentication errors, but an exception raised inside a native binding does not arrive in the translated code as a Java throwable - it unwinds the worker and hangs every thread waiting on the call. The classification now travels as a zero peer plus two accessors, so the caller still distinguishes a wrong key from a corrupt or unreadable database without anything being thrown across the bridge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 952f337007
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
…cked runLifecycle required the value from getDatabasePath to resolve through FileSystemStorage. That holds where databases are files, which is every port except JavaScript: there they live in a browser storage pool keyed by name, and the port says so by reporting no custom-path support and returning the name as an opaque handle. The check would have gone red the moment the JavaScript suite stopped hanging and actually reached it. The assertion is now gated on isCustomPathSupported(), with a note recorded for the ports where it does not apply, and the stale javadoc claiming getDatabasePath returns null on JavaScript is corrected to describe what it actually returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a70937adb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The migration swap could lose data. Deleting the original before renaming
the converted file in leaves an interval where the only copy is under a name
nothing looks for; the error path then recreated an empty database at the
live name, and the next conversion removed the stranded copy as stale
leftovers. It now renames the original aside instead of deleting it, so a
complete database exists under one of the two names at every instant, and
AndroidCipherFactory restores from the backup before opening if the process
died in the gap.
Both schema probes reported every failure as a wrong key. A malformed image
or a read error is not something a key repairs, so an application following
the error codes would prompt for a passphrase forever. Android now applies
the same isNotADatabase test the open path uses, and JavaScript gained its
counterpart: a wrong key looks like a file that is not a database, because
the plaintext it produces has no valid header, while SQLITE_CORRUPT is a
different thing entirely.
A non-null empty argument array took the no-parameter shortcut on all seven
implementations, so execute("INSERT ... VALUES (?)", new Object[0]) ran with
the slot unbound. Only a null array takes that path now; an empty one is a
parameterized call and is held to the count like any other.
The OPFS pool was installed at its six-file default, which is a hard ceiling
on databases and lower than it looks because journal files take slots too.
Raised to 64, where an unused slot is an empty file and costs nothing.
Android leaked a cursor whenever eager validation rejected a query: the
cursor was never handed out and never registered, so nothing would close it
and its query kept the database referenced until GC.
ThreadSafeDatabase read the underlying transaction flag with no
happens-before against the worker that writes it, so a polling thread could
see stale state indefinitely on a class whose whole purpose is sharing. The
read now takes the dispatch lock, which is what publishes the write, while
still not queueing work behind a running statement.
On the JavaScript builders the staging directory holds the framework's own
classes, so a scan of it reports database use for every application and
cannot be made precise. The launcher now reaches the compatibility switch
reflectively, which removes both consequences: nothing is pinned for the
optimizer, and a project built against a core predating setLegacyBehavior
still compiles rather than failing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62e22710b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The open binding was fixed last round and the reason documented there, but the rest of the bindings still let exceptions escape - so a syntax error, a constraint violation or any ordinary SQL failure unwound the worker and left every thread blocked on a call that never returned. An application would hang rather than see the error. This is the same defect the suite already hung on once, in a different binding. The whole SPI now reports failure by value. A shared error channel holds the message, and each fallible call answers with a sentinel the Java side turns back into the IOException this API promises: execScript, rekey and executeAndFinish return a boolean, prepare keeps its zero peer, and step became an int because a boolean had no room to say "failed". The rest are wrapped too, so nothing in the file can throw at all. Also in this round: The native open leaked its handle on failure. throwException longjmps, so the close after it never ran, and SQLite returns a usable handle even from an unsuccessful open. The message is now built and the handle closed before raising. A zero length blob came back as null on Linux and Windows, because SQLite may return a null pointer with a count of zero and that was read as SQL NULL - disagreeing with both the column type and wasNull(). sqlcipher_export does not carry application_id any more than it carries user_version, so the Android migration now restores both. The iOS key probe reported any non-OK result as a wrong key. It now returns the SQLite result, and only SQLITE_NOTADB - which is what a key that did not decrypt the file looks like - is reported as WRONG_KEY. The JavaScript builder scanned for database use only on the bundled-jar path, so a build from a source checkout never set the flag and neither the compatibility default nor the asset pruning applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a11dc9bae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+6563
to
+6566
| } catch (err) { | ||
| // Firefox before 111 and Safari before 15.2 have no createSyncAccessHandle. | ||
| // Degrade to memory, but say so: silently losing every write on reload is | ||
| // exactly the kind of failure that should not be discovered in production. |
There was a problem hiding this comment.
When OPFS initialization times out because another tab holds the synchronous access handles, or fails because of a temporary storage error, this catch treats the failure like permanent browser incompatibility and continues with a new in-memory database. An existing persistent database then appears empty, and any writes made in this session are silently lost when the page closes; only unsupported-API errors should select the memory fallback, while contention and storage failures should make the database open fail.
Useful? React with 👍 / 👎.
Comment on lines
+294
to
+296
| SQLiteDatabase closing = db; | ||
| db = null; | ||
| closing.close(); |
There was a problem hiding this comment.
When plaintext-to-encrypted or encrypted-to-plaintext changeKey() runs while a query cursor is still open, this bypasses the cursor cleanup performed by close(): the cursor's native SQLiteQuery keeps the old database connection alive while its wrapper remains marked open. After the file swap, that cursor can therefore read the stale backup or fail with an unchecked closed-pool error, and it retains the old connection until eventually closed; close and invalidate the tracked cursors before closing this connection.
Useful? React with 👍 / 👎.
ThreadSafeDatabase deadlocked on any unchecked exception. The dispatch callbacks caught only IOException, so anything else escaped into EasyThread, whose outer loop swallowed it without delivering a result - while the caller waited synchronously holding dispatchLock. One CursorIndexOutOfBoundsException from a bad column index wedged the database and everything queued behind it forever. Both callbacks now catch Throwable and carry it back, re-raised in kind: unchecked as itself, anything else wrapped in IOException. The value-returning overload uses a Failure holder rather than an instanceof test, since a query may legitimately return a Throwable as a value. Migration recovery only ran in AndroidCipherFactory, which a plaintext open never reaches, so a database interrupted mid-conversion could be opened by the ordinary path and replaced with an empty one. It moved to AndroidImplementation, which both paths go through - and which survives the build-time deletion of the cipher package, so a database left behind by a build that had encryption is still recoverable by one that does not. The migration backup was deleted without checking. It holds the database in its previous form, so after an encrypt it is a plaintext copy of what is now an encrypted database sitting at a predictable name. A failed delete left it there while encrypt() reported success, which defeats encryption at rest more quietly than a failed conversion would. It now raises rather than returning as though nothing were wrong. Also, the reflective compatibility-switch call from the round before was wrong in the other direction: ParparVM does not retain a member reached only reflectively, so it would have been culled and the failure silently left the application on the new behaviour. The launcher above it calls SVGRegistry.installGlobal directly for exactly that reason. It is a direct call again, emitted only when the staged core actually carries the switch, which DatabaseConfig's presence establishes. A core predating it has nothing to set and already behaves the old way, so emitting nothing there reaches the same result rather than compromising. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4d3c89764
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+11075
to
+11076
| if (backup.isFile() && !live.exists()) { | ||
| backup.renameTo(live); |
There was a problem hiding this comment.
When Android was killed between renaming the original database to .cn1backup and installing the converted file, renameTo() can return false because of a transient filesystem or permission error, but this result is ignored. The caller then continues into openOrCreateDatabase() with no live file and creates an empty database; a later migration can delete the backup containing the real data as stale. Check the rename result and fail the open instead of creating a replacement database.
Useful? React with 👍 / 👎.
| } | ||
| File live = new File(path); | ||
| File backup = new File(path + DATABASE_BACKUP_SUFFIX); | ||
| if (backup.isFile() && !live.exists()) { |
There was a problem hiding this comment.
When encryption is interrupted after the converted database has been installed but before AndroidCipherDB deletes .cn1backup, both files exist and this condition deliberately does nothing, leaving the old plaintext database indefinitely at a predictable path. Fresh evidence in the current tree is that the new common recovery helper still handles only !live.exists() and has no completed-swap cleanup path, so the previously reported encryption-at-rest exposure remains after the recovery changes.
Useful? React with 👍 / 👎.
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
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.
Resolves #3848.
The request was database encryption. Encryption is here, but the reason it took a
whole PR is that
com.codename1.dbwas not one API over SQLite -- it was fiveunrelated implementations that happened to share an interface, and there was no
sensible place to add a key to.
What was actually wrong
Verified in the source, not from memory:
openOrCreatenull, callers NPElast()/prev()/position()IOException("Unsupported")position(n)always gave row 0getPosition()basefirst()trueon an empty set, then reads unset memorygetBlob{ return nil; }execute(sql)multi-statementBEGINprintlnno-opsRuntimeExceptionon every portPlus three defects worth calling out on their own:
sqlDbClosecalledsqlite3_freeon asqlite3*, so no iOS connection was ever closed, the WAL wasnever checkpointed and the handle went to the wrong allocator;
SEDatabaseleakeda
PreparedStatementper query; andThreadSafeDatabase.close()was fire andforget, so a following
delete()raced it.And no device test touched
Databaseat all -- 142 test classes in the screenshotsuite, none of them about databases. That is why Windows and Linux were allowed to
ship with no implementation.
What this does
One contract.
com.codename1.db/package-info.javanow states what every portmust do, and
DatabaseConformanceSuitein the framework checks it. Seven devicetests run that suite on every port in CI; two of them run in legacy mode.
One cursor implementation.
AbstractDBCursorderives all navigation from twoprimitives,
rewind()andstepForward(), so ports stop re-deriving it. Seeksrewind and re-step rather than buffering:
sqlite3_column_*is only valid on thecurrent row, so buffering would mean copying every column of every row stepped
past, blobs included. This is what Android's windowed cursor already does on a
window miss.
Encryption, with a passphrase, a keystore-managed random key, or raw bytes.
Managed keys resolve in the core so every platform derives identical material from
an alias, and a key that cannot be stored is fatal rather than a silent downgrade
to plaintext.
Windows and Linux get a database at all.
JavaScript stops using WebSQL, which Chrome removed in 119 and Firefox never
implemented, in favour of the same SQLite compiled to WebAssembly.
Compatibility
Ten behaviours change in ways an application could depend on. All ten are restored
by the
db.legacybuild hint, per platform, and two device tests assert that itreally does restore them -- so the promise is testable rather than aspirational.
The table is in the developer guide.
The hint deliberately does not cover defects, or capabilities that used to throw
and now work. Nobody can depend on
getBlobreturning null.Cost, when unused
Nothing. iOS keeps the system SQLite unless the app references
DatabaseConfig;Android's SQLCipher package is deleted and its AAR never added; Windows and Linux
compile the engine to an empty object; the JavaScript builder prunes 1.5MB from
bundles that never open a database. Two catalog tests hold that line, because the
entry is keyed on
DatabaseConfigrather than the package -- keying it on thepackage would bundle SQLCipher for every database app and push Android's minimum
SDK from 19 to 23 for people who never asked for encryption.
Verification
SEDatabaseConformanceTestcases, all green.android,ios,codenameone-maven-pluginandByteCodeTranslator.scripts/ci/db-cipher-interop.sh, wired into PR CI, writes an encrypted databasewith our engine and reads it with the stock
sqlcipherclient, and vice versa,with both a raw key and a passphrase. This is the check that matters: a cipher
misconfiguration produces files each platform reads happily and nothing else can
touch, which no single-platform test would catch.
sqlcipher4.17.0 client and the realnet.zetetic:sqlcipher-androidAAR, not against assumed APIs.Three things the spikes caught
Worth recording, because each would have shipped broken:
sqlcipher_export()does not exist in SQLite3MC, so the ATTACH-basedmigration everyone writes would have failed.
PRAGMA rekeyworks, and alsopreserves
user_version, whichsqlcipher_exportdrops.getConnection()on the simulator but on first read onthe device ports, so both paths need handling.
SQLiteMCSqlCipherConfig.getDefault()really does produce files real SQLCiphercannot open;
getV4Defaults()is required. One line, and nothing but across-engine test would have found it.
Review rounds
Nineteen findings from the automated reviewers, all real, all fixed. The ones worth knowing about:
Database.encrypt()could never have worked on Android. The system SQLite has no cipher, so aplaintext database opened through it can never be re-keyed; there is now a platform hook that
routes the migration through SQLCipher.
nullwhen re-keying, so
changeKey(managed())raised aNullPointerExceptionrather than encrypting./,\,:and space all to_, socustomer/dbandcustomer_dbshared one key and forgetting either destroyed the other.
and
sqlite3_close_v2then leaves a zombie connection alive forever.isEncrypted()reported every plaintext JavaScript database as encrypted, because that port hasno readable path and a failed header read is indistinguishable from ciphertext.
PRAGMA rekeyinterpolated the key directly, so a passphrase containing a quote changed thestatement.
Two of the fixes are covered by new conformance checks, including one verified by reinstating the
old code and watching it fail: the exhausted-cursor count went 5 to 8 before the fix.
Two decisions worth a second opinion
maven/sqlite-jdbcis no longer frozen. It was pinned and excluded frompublication because a shade of a fixed driver never changed. It now carries the
engine used to read encrypted databases, so it has to track upstream security
releases. Costs ~13.5MB per release, which is what the freeze was avoiding.
compile. It ships a prebuilt amalgamation where SQLCipher would need its
configure script run per build, and it is what the simulator's JDBC driver is
already built from -- so iOS, Windows, Linux, JavaScript and the simulator all
run one engine at one version. Android still uses the SQLCipher AAR because it
cannot compile C in our build; both write the same format, which is the part
that matters.
Companion PR
The build-side gating is mirrored in codenameone/BuildDaemon#172, which is green.
🤖 Generated with Claude Code