Removed unnecessary async from Settings model validators #25585
Conversation
Validators were marked async but didn't use await, causing Bluebird to warn about promises created but not returned in Bookshelf's event handlers.
WalkthroughThis PR changes lifecycle control flow and return propagation across many models and fixes returned values in action helpers. Seven validator methods in ghost/core/core/server/models/settings.js were changed from async to synchronous signatures (removed async; logic unchanged). Numerous model lifecycle hooks (onCreated, onUpdated, onDestroyed) now call the base model lifecycle, capture its return value, emit the corresponding change event, and return the captured result (i.e., superclass result is propagated). Plugins in ghost/core/core/server/models/base/plugins/actions.js and .../events.js now return the results/promises from internal add/insertAction calls so callers receive those results. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Files/areas needing extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-06-18T10:56:19.906ZApplied to files:
🧬 Code graph analysis (2)ghost/core/core/server/models/user.js (4)
ghost/core/core/server/models/post.js (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (6)
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 |
The insertAction function was creating a fire-and-forget Promise when adding audit log entries. This triggered Bluebird's warning about promises created but not returned in Promise.mapSeries handlers. Fixed by returning the Promise through the chain: - insert() now returns the Promise - insertAction() returns insert() result when not in transaction - addAction() returns insertAction() result - onCreated/onUpdated/onDestroyed return addAction() result The .catch() still swallows errors so action logging failures won't fail the main operation.
The Settings model overrides onCreated, onUpdated, and onDestroyed but wasn't returning the Promise from the prototype call, causing Bluebird's warning about promises created but not returned.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/models/settings.js (1)
131-139: Optional: Consider removing unnecessary await in future PR.Lines 134 and 137 use
awaiton the now-synchronous validators. While harmless (await will convert synchronous returns to resolved promises), theawaitis technically unnecessary. However, removing it would complicate the code since validators could be either sync or async, and keeping this PR focused on fixing the Bluebird warning is appropriate.Based on learnings, keeping PRs focused on their primary objective is preferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/models/settings.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24651
File: ghost/core/test/utils/urlUtils.js:53-57
Timestamp: 2025-08-11T19:39:00.428Z
Learning: In Ghost's test utilities, when fixing specific issues like async behavior, it's preferred to maintain existing error handling patterns (even if suboptimal) to keep PRs focused on their primary objective. Error handling improvements can be addressed in separate, dedicated PRs.
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
🧬 Code graph analysis (1)
ghost/core/core/server/models/settings.js (2)
ghost/core/core/server/models/post.js (5)
options(1070-1070)options(1197-1197)options(1288-1288)options(1341-1341)options(1365-1365)ghost/core/core/server/models/user.js (8)
options(451-451)options(535-535)options(635-635)options(706-706)options(728-728)options(1051-1051)options(1083-1083)options(1152-1152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Ghost-CLI tests
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push
🔇 Additional comments (8)
ghost/core/core/server/models/settings.js (8)
317-336: LGTM! Correctly removed async from synchronous validator.The
allvalidator contains only synchronous validation logic and doesn't useawaitor return promises, so removingasyncis correct and eliminates unnecessary Promise creation.
337-347: LGTM! Correctly removed async from synchronous validator.The
labsvalidator contains only synchronous validation logic (JSON parsing and flag checking), so removingasyncis correct.
348-382: LGTM! Correctly removed async from synchronous validator.The
stripe_plansvalidator performs only synchronous operations (JSON parsing, loops, and validation checks), so removingasyncis correct.
385-398: LGTM! Correctly removed async from synchronous validator.The
stripe_secret_keyvalidator performs only synchronous regex validation, so removingasyncis correct.
399-412: LGTM! Correctly removed async from synchronous validator.The
stripe_publishable_keyvalidator performs only synchronous regex validation, so removingasyncis correct.
413-426: LGTM! Correctly removed async from synchronous validator.The
stripe_connect_secret_keyvalidator performs only synchronous regex validation, so removingasyncis correct.
427-440: LGTM! Correctly removed async from synchronous validator.The
stripe_connect_publishable_keyvalidator performs only synchronous regex validation, so removingasyncis correct.
110-129: No issues found with the lifecycle hook order change.The lifecycle hooks now emit events before returning the super call. Event listeners in CacheManager and Stripe service register once and respond to these events regardless of their timing relative to the parent class's lifecycle. The tests confirm both event types fire correctly (e.g.,
settings.addedandsettings.description.added). This order change is safe and actually improves promise handling for async code consuming these lifecycle methods.
Models that override onCreated, onUpdated, or onDestroyed were calling the prototype method but not returning the Promise, causing Bluebird warnings about promises created but not returned. Fixed models: - comment-like.js - comment-report.js - comment.js - email.js - integration.js - label.js - member.js - post.js - tag.js - user.js - webhook.js
Updated all model event handlers (onCreated, onUpdated, onDestroyed) to call the prototype method first, then emit custom events, to ensure the prototype's setup (like model._changed) happens before events fire. This preserves the original order of operations while still returning the Promise from the prototype method to fix the Bluebird warning. refs #25582
no issue
Summary
Problem
When saving settings, Bluebird logged this warning:
The validators (
all,labs,stripe_plans,stripe_secret_key,stripe_publishable_key,stripe_connect_secret_key,stripe_connect_publishable_key) were marked async but contained only synchronous code. This caused unnecessary Promise creation inside Bookshelf's event handler chain, triggering Bluebird's warning.