margelo / margelo/react-native-nitro-sqlite

loadFile masks statement errors by rolling back twice

Open
#308 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C
Stars
565
Forks
53
Avg merge
1d 21h
Merged PRs (30d)
18

Description

Summary

loadFile() rolls back twice after a statement failure. The second ROLLBACK fails with cannot rollback - no transaction is active, masking the actual import failure and the library's intended CouldNotLoadFile error.

Verified on main at ad8b835 (v9.7.0).

Code evidence

In importSqlFile.cpp#L20-L41:

  1. A per-line NitroSQLiteException catch executes ROLLBACK at line 28.
  2. It throws CouldNotLoadFile at line 30.
  3. The outer catch (...) catches that exception and executes ROLLBACK again at line 40.
  4. SQLite reports cannot rollback - no transaction is active; that new SqlExecutionError escapes before the intended UnknownError at line 41 can be thrown.

The same outer catch also replaces useful BEGIN/COMMIT failures with a generic message, and a rollback failure can replace any primary error.

Smallest reproducer

Create a SQL file with one valid command followed by invalid SQL:

CREATE TABLE imported(id INTEGER PRIMARY KEY);
THIS IS INVALID SQL;

Then:

const db = open({ name: 'load-file.sqlite' })

await expect(db.loadFileAsync(path)).rejects.toThrow()

Expected:

  • The transaction is rolled back exactly once.
  • The error identifies loadFile, the source file, and ideally the failing command/line.
  • imported does not exist.

Observed from the current control flow:

  • The transaction is rolled back.
  • The intended CouldNotLoadFile(..., "Transaction was rolled back") is caught internally.
  • A second rollback fails and surfaces cannot rollback - no transaction is active, masking the import error.

The rollback behavior itself is reproducible with BEGIN; ROLLBACK; ROLLBACK;, where SQLite rejects the second rollback.

Impact

Import failures lose their actionable error and source context. Applications cannot reliably distinguish malformed import files from transaction-state errors, making migrations and recovery diagnostics substantially harder.

Acceptance criteria

  • Give one scope sole ownership of transaction finalization; execute at most one rollback per failed import transaction.
  • Track whether BEGIN succeeded and whether the transaction is still active before attempting rollback.
  • Preserve the primary statement/BEGIN/COMMIT error, including file and failing line/command context.
  • If rollback also fails, retain both errors without masking the primary cause.
  • Close the input file through RAII rather than repeated manual close paths.

Regression-test target

A native or Harness test that imports a file containing a valid command followed by invalid SQL and asserts:

  1. The promise rejects with the original import/SQL context, not cannot rollback - no transaction is active.
  2. The valid command was rolled back.
  3. The same connection remains usable afterward.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in packages/react-native-nitro-sqlite/cpp/importSqlFile.cpp, especially the lines and catch blocks identified in the issue. Reproduce the failure with a valid SQL command followed by invalid SQL, then add a native or Harness regression test. Done means one rollback, preserved import/SQL context, no imported table, and a connection that remains usable.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, react-native, sqlite
Domain
databases, mobile
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.