feat: Add a new beforeLiveQueryEvent trigger
#9445
Conversation
Thanks for opening this pull request! |
|
@codeKonami please let us know when this is ready for review and change the PR state. |
The code is ready, I'd be interested to have your opinion on the approach before going further on documentation and probably more unit tests. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce a new Parse Server trigger called Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ParseServer
participant CloudCode
participant LiveQueryServer
Client->>ParseServer: Save or update object
ParseServer->>CloudCode: Run beforeLiveQueryEvent trigger (if defined)
alt beforeLiveQueryEvent sets preventLiveQuery
CloudCode-->>ParseServer: preventLiveQuery = true
ParseServer-->>LiveQueryServer: (No notification sent)
else beforeLiveQueryEvent allows event
CloudCode-->>ParseServer: allow event
ParseServer->>LiveQueryServer: Notify LiveQuery event
end
ParseServer->>CloudCode: Run afterSave trigger (if defined)
ParseServer-->>Client: Respond to save/update
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/cloud-code/Parse.Cloud.js (1)
631-631: Fix indentation issue.There's an extra space at the beginning of this line causing an ESLint error.
- ParseCloud.beforeLiveQueryEvent = function (parseClass, handler, validationHandler) { + ParseCloud.beforeLiveQueryEvent = function (parseClass, handler, validationHandler) {🧰 Tools
🪛 ESLint
[error] 631-631: Expected indentation of 0 spaces but found 1.
(indent)
src/RestWrite.js (1)
1667-1675: Trigger type name may be inconsistent with public APIThe new hook is exposed to users as
beforeLiveQueryEvent, yet the internal lookup usestriggers.Types.beforeEvent.
IfbeforeEventis not aliased tobeforeLiveQueryEventinsidetriggers.js, user-registered callbacks will never be found and the feature will appear broken.Please verify that:
triggers.Types.beforeEventis exported under the documented name, or- This lookup is switched to
beforeLiveQueryEvent.Otherwise the hook will never fire.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
spec/ParseLiveQuery.spec.js(1 hunks)src/RestWrite.js(2 hunks)src/cloud-code/Parse.Cloud.js(1 hunks)src/triggers.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/ParseLiveQuery.spec.js (2)
spec/helper.js (3)
reconfigureServer(171-205)Parse(4-4)TestObject(257-259)spec/ParseLiveQueryServer.spec.js (6)
Parse(1-1)query(293-299)query(330-336)query(377-383)query(1102-1108)query(1152-1158)
🪛 ESLint
src/cloud-code/Parse.Cloud.js
[error] 631-631: Expected indentation of 0 spaces but found 1.
(indent)
spec/ParseLiveQuery.spec.js
[error] 1312-1312: 'it' is not defined.
(no-undef)
[error] 1313-1313: 'reconfigureServer' is not defined.
(no-undef)
[error] 1324-1324: 'expect' is not defined.
(no-undef)
[error] 1325-1325: 'expect' is not defined.
(no-undef)
[error] 1328-1328: 'TestObject' is not defined.
(no-undef)
[error] 1331-1331: 'expect' is not defined.
(no-undef)
[error] 1332-1332: 'done' is not defined.
(no-undef)
[error] 1335-1335: 'TestObject' is not defined.
(no-undef)
[error] 1340-1340: 'it' is not defined.
(no-undef)
[error] 1341-1341: 'reconfigureServer' is not defined.
(no-undef)
[error] 1349-1349: 'TestObject' is not defined.
(no-undef)
[error] 1354-1354: 'expect' is not defined.
(no-undef)
[error] 1357-1357: 'expect' is not defined.
(no-undef)
[error] 1361-1361: 'expect' is not defined.
(no-undef)
[error] 1362-1362: 'expect' is not defined.
(no-undef)
[error] 1363-1363: 'expect' is not defined.
(no-undef)
[error] 1366-1366: 'TestObject' is not defined.
(no-undef)
[error] 1369-1369: 'expect' is not defined.
(no-undef)
[error] 1377-1377: 'it' is not defined.
(no-undef)
[error] 1378-1378: 'reconfigureServer' is not defined.
(no-undef)
[error] 1389-1389: 'expect' is not defined.
(no-undef)
[error] 1395-1395: 'TestObject' is not defined.
(no-undef)
[error] 1398-1398: 'fail' is not defined.
(no-undef)
[error] 1401-1401: 'TestObject' is not defined.
(no-undef)
[error] 1406-1406: 'it' is not defined.
(no-undef)
[error] 1407-1407: 'reconfigureServer' is not defined.
(no-undef)
[error] 1415-1415: 'TestObject' is not defined.
(no-undef)
[error] 1420-1420: 'expect' is not defined.
(no-undef)
[error] 1426-1426: 'TestObject' is not defined.
(no-undef)
[error] 1429-1429: 'fail' is not defined.
(no-undef)
🔇 Additional comments (4)
src/cloud-code/Parse.Cloud.js (1)
607-641: Well-implemented new trigger function!The new
beforeLiveQueryEventhook follows the established pattern of other trigger functions in the codebase. It's properly integrated with the trigger system and includes comprehensive documentation.🧰 Tools
🪛 ESLint
[error] 631-631: Expected indentation of 0 spaces but found 1.
(indent)
src/triggers.js (3)
17-17: Appropriately added new trigger type.The
beforeEventtrigger type has been correctly added to theTypesobject.
282-284: Correct integration with context handling.The
beforeEventtrigger type has been properly integrated with the context copying mechanism in thegetRequestObjectfunction.
890-892: Properly integrated with context merging.The
beforeEventtrigger type has been correctly added to the condition that merges the request context back into the external context, which is essential for thepreventLiveQueryfunctionality.
| Parse.Cloud.beforeLiveQueryEvent('TestObject', async req => { | ||
| expect(req.object.get('foo')).toBe('baz'); | ||
| if (req.object.get('foo') === 'baz') { | ||
| req.context.preventLiveQuery === true; | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix critical bug in preventLiveQuery logic.
Same issue as in the previous test - you're using a comparison operator (===) instead of an assignment operator (=).
- if (req.object.get('foo') === 'baz') {
- req.context.preventLiveQuery === true;
+ if (req.object.get('foo') === 'baz') {
+ req.context.preventLiveQuery = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Parse.Cloud.beforeLiveQueryEvent('TestObject', async req => { | |
| expect(req.object.get('foo')).toBe('baz'); | |
| if (req.object.get('foo') === 'baz') { | |
| req.context.preventLiveQuery === true; | |
| } | |
| }) | |
| Parse.Cloud.beforeLiveQueryEvent('TestObject', async req => { | |
| expect(req.object.get('foo')).toBe('baz'); | |
| if (req.object.get('foo') === 'baz') { | |
| req.context.preventLiveQuery = true; | |
| } | |
| }) |
🧰 Tools
🪛 ESLint
[error] 1420-1420: 'expect' is not defined.
(no-undef)
🤖 Prompt for AI Agents (early access)
In spec/ParseLiveQuery.spec.js around lines 1419 to 1424, the code mistakenly uses the comparison operator (===) instead of the assignment operator (=) when setting req.context.preventLiveQuery. Change req.context.preventLiveQuery === true to req.context.preventLiveQuery = true to correctly assign the value and fix the preventLiveQuery logic.
| Parse.Cloud.beforeLiveQueryEvent('TestObject', req => { | ||
| expect(req.object.get('foo')).toBe('bar'); | ||
| if (req.object.get('foo') === 'bar') { | ||
| req.context.preventLiveQuery === true; | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix critical bug in preventLiveQuery logic.
You're using a comparison operator (===) instead of an assignment operator (=), which means you're comparing the value rather than setting it. This would cause the filtering functionality to fail.
- if (req.object.get('foo') === 'bar') {
- req.context.preventLiveQuery === true;
+ if (req.object.get('foo') === 'bar') {
+ req.context.preventLiveQuery = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Parse.Cloud.beforeLiveQueryEvent('TestObject', req => { | |
| expect(req.object.get('foo')).toBe('bar'); | |
| if (req.object.get('foo') === 'bar') { | |
| req.context.preventLiveQuery === true; | |
| } | |
| }) | |
| Parse.Cloud.beforeLiveQueryEvent('TestObject', req => { | |
| expect(req.object.get('foo')).toBe('bar'); | |
| if (req.object.get('foo') === 'bar') { | |
| req.context.preventLiveQuery = true; | |
| } | |
| }) |
🧰 Tools
🪛 ESLint
[error] 1389-1389: 'expect' is not defined.
(no-undef)
🤖 Prompt for AI Agents (early access)
In spec/ParseLiveQuery.spec.js around lines 1388 to 1393, replace the comparison operator (===) with the assignment operator (=) when setting req.context.preventLiveQuery to true. This change will correctly assign the value and enable the intended filtering functionality.
| it('test beforeLiveQueryEvent ran while creating an object', async function () { | ||
| await reconfigureServer({ | ||
| liveQuery: { | ||
| classNames: ['TestObject'], | ||
| }, | ||
| startLiveQueryServer: true, | ||
| verbose: false, | ||
| silent: true, | ||
| }); | ||
|
|
||
|
|
||
| Parse.Cloud.beforeLiveQueryEvent('TestObject', req => { | ||
| expect(req.user).toBeUndefined(); | ||
| expect(req.object.get('foo')).toBe('bar'); | ||
| }) | ||
|
|
||
| const query = new Parse.Query(TestObject); | ||
| const subscription = await query.subscribe(); | ||
| subscription.on('create', object => { | ||
| expect(object.get('foo')).toBe('bar'); | ||
| done(); | ||
| }); | ||
|
|
||
| const object = new TestObject(); | ||
| object.set('foo', 'bar'); | ||
| await object.save(); | ||
| }); |
There was a problem hiding this comment.
Fix test callback handling.
This test is using done() on line 1332, but the function doesn't receive the done parameter. The test should either:
- Receive
doneas a parameter and call it at the end, or - Return a promise or use
async/awaitproperly
- it('test beforeLiveQueryEvent ran while creating an object', async function () {
+ it('test beforeLiveQueryEvent ran while creating an object', async function (done) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('test beforeLiveQueryEvent ran while creating an object', async function () { | |
| await reconfigureServer({ | |
| liveQuery: { | |
| classNames: ['TestObject'], | |
| }, | |
| startLiveQueryServer: true, | |
| verbose: false, | |
| silent: true, | |
| }); | |
| Parse.Cloud.beforeLiveQueryEvent('TestObject', req => { | |
| expect(req.user).toBeUndefined(); | |
| expect(req.object.get('foo')).toBe('bar'); | |
| }) | |
| const query = new Parse.Query(TestObject); | |
| const subscription = await query.subscribe(); | |
| subscription.on('create', object => { | |
| expect(object.get('foo')).toBe('bar'); | |
| done(); | |
| }); | |
| const object = new TestObject(); | |
| object.set('foo', 'bar'); | |
| await object.save(); | |
| }); | |
| it('test beforeLiveQueryEvent ran while creating an object', async function (done) { | |
| await reconfigureServer({ | |
| liveQuery: { | |
| classNames: ['TestObject'], | |
| }, | |
| startLiveQueryServer: true, | |
| verbose: false, | |
| silent: true, | |
| }); | |
| Parse.Cloud.beforeLiveQueryEvent('TestObject', req => { | |
| expect(req.user).toBeUndefined(); | |
| expect(req.object.get('foo')).toBe('bar'); | |
| }); | |
| const query = new Parse.Query(TestObject); | |
| const subscription = await query.subscribe(); | |
| subscription.on('create', object => { | |
| expect(object.get('foo')).toBe('bar'); | |
| done(); | |
| }); | |
| const object = new TestObject(); | |
| object.set('foo', 'bar'); | |
| await object.save(); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 1312-1312: 'it' is not defined.
(no-undef)
[error] 1313-1313: 'reconfigureServer' is not defined.
(no-undef)
[error] 1324-1324: 'expect' is not defined.
(no-undef)
[error] 1325-1325: 'expect' is not defined.
(no-undef)
[error] 1328-1328: 'TestObject' is not defined.
(no-undef)
[error] 1331-1331: 'expect' is not defined.
(no-undef)
[error] 1332-1332: 'done' is not defined.
(no-undef)
[error] 1335-1335: 'TestObject' is not defined.
(no-undef)
🤖 Prompt for AI Agents (early access)
In spec/ParseLiveQuery.spec.js around lines 1312 to 1338, the test uses done() without receiving done as a parameter, causing callback handling issues. Fix this by adding done as a parameter to the test function and calling done() inside the subscription event handler after assertions, or refactor the test to properly use async/await and return a promise without using done().
| this.config.database.loadSchema().then(schemaController => { | ||
| // Notify LiveQueryServer if possible | ||
| const perms = schemaController.getClassLevelPermissions(publishedObject.className); | ||
| this.config.liveQueryController.onAfterSave( | ||
| publishedObject.className, | ||
| publishedObject, | ||
| originalObject, | ||
| perms | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Await the schema call to avoid silent LiveQuery failures
this.config.database.loadSchema() returns a promise, but the surrounding async function does not await or return it.
If the DB lookup rejects (e.g. transient connectivity, schema inconsistency) the rejection becomes an unhandled promise, the catch in the caller never fires and LiveQuery delivery silently fails.
- this.config.database.loadSchema().then(schemaController => {
- // Notify LiveQueryServer if possible
- const perms = schemaController.getClassLevelPermissions(publishedObject.className);
- this.config.liveQueryController.onAfterSave(
- publishedObject.className,
- publishedObject,
- originalObject,
- perms
- );
- });
+ const schemaController = await this.config.database.loadSchema();
+ // Notify LiveQueryServer if possible
+ const perms = schemaController.getClassLevelPermissions(publishedObject.className);
+ this.config.liveQueryController.onAfterSave(
+ publishedObject.className,
+ publishedObject,
+ originalObject,
+ perms
+ );This keeps the method “fire-and-forget” semantics for LiveQuery success but still propagates unexpected errors up the promise chain for proper logging / retries.
Please consider the change or add an explicit .catch() to prevent unhandled rejections.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.config.database.loadSchema().then(schemaController => { | |
| // Notify LiveQueryServer if possible | |
| const perms = schemaController.getClassLevelPermissions(publishedObject.className); | |
| this.config.liveQueryController.onAfterSave( | |
| publishedObject.className, | |
| publishedObject, | |
| originalObject, | |
| perms | |
| ); | |
| }); | |
| const schemaController = await this.config.database.loadSchema(); | |
| // Notify LiveQueryServer if possible | |
| const perms = schemaController.getClassLevelPermissions(publishedObject.className); | |
| this.config.liveQueryController.onAfterSave( | |
| publishedObject.className, | |
| publishedObject, | |
| originalObject, | |
| perms | |
| ); |
🤖 Prompt for AI Agents (early access)
In src/RestWrite.js around lines 1677 to 1686, the call to this.config.database.loadSchema() returns a promise that is neither awaited nor returned, causing unhandled promise rejections if it fails. To fix this, modify the code to either await the loadSchema() call or return the promise chain, and add a .catch() handler to properly handle and propagate errors, ensuring that any failures in loading the schema do not silently fail and are logged or retried as needed.
Pull Request
Issue
Closes: #9431
Approach
I added a beforeLiveQueryEvent trigger which allows the user to modify the updated object before it is passed to the LiveQuery server (via PubSub). It is a cloned version of the object so it won't modify the one passed to the afterSave trigger.
I've added an option which I'm not really convinced. You can set the context of the request to
req.context.preventLiveQuery === trueto prevent the publish to be made by the Parse Server and so limiting some network and CPU consumption depending on some business logic.Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests