Skip to content

fix(database): surface typeorm errors in ConnectionService.update so UI can show editfailed#2046

Open
calebcgates wants to merge 1 commit into
emqx:mainfrom
calebcgates:fix/connection-service-update-surface-db-errors
Open

fix(database): surface typeorm errors in ConnectionService.update so UI can show editfailed#2046
calebcgates wants to merge 1 commit into
emqx:mainfrom
calebcgates:fix/connection-service-update-surface-db-errors

Conversation

@calebcgates
Copy link
Copy Markdown

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

When ConnectionService.update(id, data) is called from the connection edit form (src/views/connections/ConnectionForm.vue:739) or the connection settings dialog (src/views/connections/ConnectionInfo.vue:192), and the underlying typeorm connectionRepository.save() (or the cascading willService.save()) throws — for example because:

  • the SQLite database file is locked by another renderer instance,
  • a Will Message field exceeds a CHECK constraint after a recent migration,
  • the disk is full / read-only / on a flaky network share,
  • a foreign-key constraint fails because the parent collection was concurrently deleted,

— the QueryFailedError propagates past the Vue async handler unhandled. The UI's existing failure path at ConnectionForm.vue:757:

if (!(res && res.id)) {
  const msgError = this.oper === 'create' ? this.$tc('common.createfailed') : this.$tc('common.editfailed')
  this.$message.error(msgError)
  this.$log.error(msgError)
  return
}

never fires (because res was never assigned — the await rejected). The user clicks Save, nothing visibly happens, and the error only appears in the renderer devtools console (which most users don't open).

Issue Number

No existing issue — happy to file one if preferred.

What is the new behavior?

ConnectionService.update() now wraps its body in a try { ... } catch (error) { console.error(...); return undefined } block, mirroring the existing pattern in src/database/services/DashboardService.ts:159-174 (updateOrders) and src/database/services/WidgetService.ts:120-128 (updateOrders).

When the typeorm save() throws, update() now returns undefined. The existing if (!(res && res.id)) check in ConnectionForm.vue:757 then fires the common.editfailed toast (already wired up, already i18n'd in all supported locales). The error is also logged to console.error for renderer devtools / log file capture.

ConnectionService.importOneConnection at line 213 (the third caller of update(), used by the bulk-import flow) previously relied on update() throwing to halt the import via import()'s try/catch wrapper (line 270-293). Now that update() returns undefined on failure instead, the import path needs an explicit guard to preserve that signal — otherwise import() would silently mark a partially-failed import as 'ok' and ImportData.vue:403-414 would show a misleading success toast. Added a 3-line if (!updated) throw new Error(...) guard at line 213 to keep the import-path failure semantics unchanged.

Does this PR introduce a breaking change?

  • Yes
  • No

update()'s return type was already Promise<ConnectionModel | undefined> by inference (because this.get(id) returns ConnectionModel | undefined). All call sites already handle undefined. The PR makes the return type explicit and adds the undefined path on caught error — no caller behaviour changes for the happy path.

Specific Instructions

  • No automated test added. The codebase doesn't currently have any service-layer unit tests (existing tests/unit/ covers Vue components and utils/). Adding one would require introducing test scaffolding for typedi + typeorm import resolution that webpack 4 currently can't compile (typeorm pulls in mongodb whose modern-JS doesn't parse). Happy to add a test in a follow-up PR if you can point at how you'd like service-layer tests structured.
  • Manual reproduction: in dev mode (yarn run electron:serve), open the renderer devtools, then in ConnectionService.update's body, temporarily inject throw new Error('test') before the save() call. Without the fix: clicking Save in the connection edit form fails silently with only a console error. With the fix: the existing common.editfailed toast appears at the top of the window.
  • Scope: doesn't touch create() (line 374 — same shape of issue, separate follow-up PR), the migrations folder, or the other service files (CollectionService, MessageService, etc. have similar issues — also follow-up scope per "one violation site per PR").
  • Pattern matches DashboardService.updateOrders and WidgetService.updateOrders exactly: same try/catch shape, same console.error('Error <verb>:', error) style, same falsy-return convention. The same in-repo neighbours are the strongest argument that this is the project's preferred handling pattern for database/services/ files.

Other information

The explicit return-type annotation (Promise<ConnectionModel | undefined>) is a no-op for callers — this.get(id) already returns ConnectionModel | undefined, so the inferred type was the same. Making it explicit communicates intent: "this can fail, you must check," matching the explicit return type on ConnectionService.create() at line 374.

@ysfscream ysfscream requested review from Copilot and ysfscream May 22, 2026 02:42
@ysfscream ysfscream added enhancement New feature or request fix Fix bug or issues desktop MQTTX Desktop labels May 22, 2026
@ysfscream ysfscream moved this to In Progress in MQTTX May 22, 2026
@ysfscream ysfscream added this to the v1.13.1 milestone May 22, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves user-visible error handling when updating an existing connection in the local database by ensuring ConnectionService.update() returns undefined on TypeORM failures (instead of rejecting), allowing existing UI “edit failed” handling to run.

Changes:

  • Wrap ConnectionService.update() in a try/catch, log the underlying error, and return undefined on failure.
  • Update the bulk-import path (importOneConnection) to explicitly fail fast when update() returns undefined, preserving prior import failure semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Member

@ysfscream ysfscream left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. The approach looks good, but one caller still needs the same failure handling: ConnectionInfo.vue currently only emits when connectionService.update() returns a result, and still fails silently when it returns undefined. Could you add the same editfailed feedback there as in ConnectionForm?

@ysfscream ysfscream dismissed their stale review May 22, 2026 02:50

Switching this to a non-blocking comment.

@ysfscream
Copy link
Copy Markdown
Member

Thanks for the fix. The approach looks good. One non-blocking follow-up: ConnectionInfo.vue still does nothing when connectionService.update() returns undefined; it would be nice to show the same editfailed feedback there as in ConnectionForm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

desktop MQTTX Desktop enhancement New feature or request fix Fix bug or issues

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants