Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LiveQueryEvent Error Logging Improvements #6951

Merged
merged 9 commits into from
Oct 21, 2020
Merged

LiveQueryEvent Error Logging Improvements #6951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
18dc84b
LiveQueryEvent Improvements
dblythy Oct 20, 2020
a76b7f0
Update ParseLiveQueryServer.js
dblythy Oct 20, 2020
968de7d
Update ParseLiveQueryServer.js
dblythy Oct 20, 2020
2510b66
More Tests
dblythy Oct 20, 2020
fae8223
Update ParseLiveQueryServer.js
dblythy Oct 20, 2020
fe2812d
Pass thrown errors to subscription
dblythy Oct 21, 2020
c2368cd
Update ParseLiveQueryServer.js
dblythy Oct 21, 2020
9b0face
Update ParseLiveQueryServer.js
dblythy Oct 21, 2020
d51a3f5
Remove ACL error
dblythy Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions spec/ParseLiveQuery.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ describe('ParseLiveQuery', function () {
object.set({ foo: 'bar' });
await object.save();
});

it('can handle afterEvent throw', async done => {
await reconfigureServer({
liveQuery: {
Expand All @@ -245,6 +244,37 @@ describe('ParseLiveQuery', function () {
const object = new TestObject();
await object.save();

Parse.Cloud.afterLiveQueryEvent('TestObject', () => {
throw 'Throw error from LQ afterEvent.';
});

const query = new Parse.Query(TestObject);
query.equalTo('objectId', object.id);
const subscription = await query.subscribe();
subscription.on('update', () => {
fail('update should not have been called.');
});
subscription.on('error', e => {
expect(e).toBe('Throw error from LQ afterEvent.');
done();
});
dblythy marked this conversation as resolved.
Show resolved Hide resolved
object.set({ foo: 'bar' });
await object.save();
});

it('can handle afterEvent sendEvent to false', async done => {
await reconfigureServer({
liveQuery: {
classNames: ['TestObject'],
},
startLiveQueryServer: true,
verbose: false,
silent: true,
});

const object = new TestObject();
await object.save();

Parse.Cloud.afterLiveQueryEvent('TestObject', req => {
const current = req.object;
const original = req.original;
Expand All @@ -254,7 +284,7 @@ describe('ParseLiveQuery', function () {
}, 2000);

if (current.get('foo') != original.get('foo')) {
throw "Don't pass an update trigger, or message";
req.sendEvent = false;
}
});

Expand Down
41 changes: 34 additions & 7 deletions src/LiveQuery/ParseLiveQueryServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,19 +182,33 @@ class ParseLiveQueryServer {
clients: this.clients.size,
subscriptions: this.subscriptions.size,
useMasterKey: client.hasMasterKey,
installationId: client.installationId
installationId: client.installationId,
sendEvent: true,
};
return maybeRunAfterEventTrigger('afterEvent', className, res);
})
.then(() => {
if (!res.sendEvent) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed no?

Copy link
Member Author

@dblythy dblythy Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That is in relation to preventing events from firing, which was previously achieved by throwing in the trigger. Setting request.sendEvent = false in LQ trigger returns before the events are pushed to the LQ.

Copy link
Member

Choose a reason for hiding this comment

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

Since maybeRunAfterEventTrigger is the only code that can throw an error this block will be skipped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry @dplewis, I'm not sure what the issue is, what would you like me to change? 😊

Copy link
Member

Choose a reason for hiding this comment

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

Throwing an error handles events from firing. sendEvent can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, no worries. I added sendEvent because if you are using a highly restrictive LQ, you'll end up with afterEvent events filling up your logs, which I'm not sure is desired.

For example, only passing LQ update on "foo" change:

Parse.Cloud.afterLiveQueryEvent('TestObject', req => {
      if (req.object.get('foo') != req.original.get('foo')) {
          // if TestObject has a lot of fields, this will throw often
          throw 'Foo was not updated';
      }
});

Would log on every single LQ event for TestObject, and pass a subscription.on('error') event. The idea behind the sendEvent is a way to stop events firing, without it sending errors to the client, and without it filling up logLevel: 'error' logs. However happy to remove it if you see it unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see

return;
}
if (res.object && typeof res.object.toJSON === 'function') {
deletedParseObject = res.object.toJSON();
deletedParseObject.className = className;
}
client.pushDelete(requestId, deletedParseObject);
})
.catch(error => {
logger.error('Matching ACL error : ', error);
Client.pushError(
client.parseWebSocket,
error.code || 141,
error.message || error,
false,
requestId
);
logger.error(
`Failed running afterLiveQueryEvent on class ${className} for event ${res.event} with session ${res.sessionToken} with:\n Error: ` +
JSON.stringify(error)
);
});
}
}
Expand Down Expand Up @@ -297,7 +311,6 @@ class ParseLiveQueryServer {
isCurrentMatched,
subscription.hash
);

// Decide event type
let type;
if (isOriginalMatched && isCurrentMatched) {
Expand All @@ -322,12 +335,16 @@ class ParseLiveQueryServer {
clients: this.clients.size,
subscriptions: this.subscriptions.size,
useMasterKey: client.hasMasterKey,
installationId: client.installationId
installationId: client.installationId,
sendEvent: true,
};
return maybeRunAfterEventTrigger('afterEvent', className, res);
})
.then(
() => {
if (!res.sendEvent) {
return;
}
if (res.object && typeof res.object.toJSON === 'function') {
currentParseObject = res.object.toJSON();
currentParseObject.className =
Expand All @@ -349,7 +366,17 @@ class ParseLiveQueryServer {
}
},
error => {
logger.error('Matching ACL error : ', error);
Client.pushError(
client.parseWebSocket,
error.code || 141,
error.message || error,
false,
requestId
);
logger.error(
`Failed running afterLiveQueryEvent on class ${className} for event ${res.event} with session ${res.sessionToken} with:\n Error: ` +
JSON.stringify(error)
);
}
);
}
Expand Down Expand Up @@ -658,7 +685,7 @@ class ParseLiveQueryServer {
} catch (error) {
Client.pushError(
parseWebsocket,
error.code || 101,
error.code || 141,
error.message || error,
false
);
Expand Down Expand Up @@ -776,7 +803,7 @@ class ParseLiveQueryServer {
} catch (e) {
Client.pushError(
parseWebsocket,
e.code || 101,
e.code || 141,
e.message || e,
false,
request.requestId
Expand Down
1 change: 1 addition & 0 deletions src/cloud-code/Parse.Cloud.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,7 @@ module.exports = ParseCloud;
* @property {Parse.Object} original If set, the object, as currently stored.
* @property {Integer} clients The number of clients connected.
* @property {Integer} subscriptions The number of subscriptions connected.
* @property {Boolean} sendEvent If the LiveQuery event should be sent to the client. Set to false to prevent LiveQuery from pushing to the client.
*/

/**
Expand Down