fix(sqlite): reject promise on getAll query failure (#180)#353
fix(sqlite): reject promise on getAll query failure (#180)#353JacobiusMakes wants to merge 1 commit intolinagora:devfrom
Conversation
When a SQLite query fails during statement preparation (e.g., invalid table name or malformed SQL), the promise returned by _get() and _getMinMax() would hang indefinitely because db.prepare() could fail without calling the stmt.all() callback. This change uses the prepare() callback to catch preparation errors and properly reject the promise. A try-catch wrapper is also added as a safety net for synchronous throws. Fixes: linagora#180 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@JacobiusMakes thank you again for that changes suggestion! A note for the future though.. We provide a PR template that aims to help you write about this: ## What
<!-- A concise description of what this PR changes. One paragraph is enough. -->
## Why
<!-- Why is this change needed? Link the issue it addresses. -->
<!-- If any related issues -->
Closes #
## How
<!-- Non-obvious implementation decisions, trade-offs, or anything a reviewer should know before reading the diff. Skip if the why + diff are self-explanatory. -->I wish you a happy contribution! |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected -t check |
❌ Failed | 5s | View ↗ |
nx affected -t test |
✅ Succeeded | 3m 10s | View ↗ |
nx affected -t build |
✅ Succeeded | 17s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-30 15:44:02 UTC
There was a problem hiding this comment.
LGTM -- please rebase dev on your branch and check the conflicts to proceed forward with more test runs :)
Worth noting (side notes to self):
A few issues exist in the surrounding code (double-rejection risk, stmt reference timing, silent finalize errors after resolve) but these all predate this PR and are out of scope here. This change is a net improvement: surfacing prepare() failures was a real gap and the added error handling is solid.
The broader issues are worth addressing properly as part of a larger DB module overhaul rather than piecemeal...
pm-McFly
added
bug
pm-McFly
force-pushed
the
dev
branch
2 times, most recently
from
5950f13 to
fbcc854
Compare
2 participants
What
Add proper error handling for db.prepare() failures in SQLite _get() and _getMinMax() methods. When a SQL statement fails to prepare, the promise now rejects instead of hanging forever.
Why
When db.prepare(query) fails silently, stmt.all() callback is never called, leaving the promise unresolved indefinitely. This causes the application to hang with no error output, making debugging extremely difficult.
Fixes #180
How
Changed db.prepare(query) to use the error callback so preparation errors trigger a proper rejection. Added try-catch wrapper as safety net. Applied to both the shared db package and the identity server db.