From 70f5c71864c124717e675771965062d95183511e Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Thu, 28 Nov 2019 16:40:14 +0000 Subject: [PATCH] Handle duplicate-notification race condition when using forceCreate --- .../app/coffee/Notifications.coffee | 55 ++++++-------- .../unit/coffee/NotificationsTests.coffee | 73 ++++++++++--------- 2 files changed, 61 insertions(+), 67 deletions(-) diff --git a/services/notifications/app/coffee/Notifications.coffee b/services/notifications/app/coffee/Notifications.coffee index 87bac8505d..5fc4084025 100644 --- a/services/notifications/app/coffee/Notifications.coffee +++ b/services/notifications/app/coffee/Notifications.coffee @@ -15,42 +15,35 @@ module.exports = Notifications = callback err, notifications - _checkExistingNotifcationAndOverride : (user_id, notification, callback)-> - self = @ + _countExistingNotifications : (user_id, notification, callback = (err, count)->)-> query = user_id: ObjectId(user_id) key: notification.key - db.notifications.count query, (err, number)-> - if number > 0 and !notification.forceCreate - logger.log number:number, user_id:user_id, key:notification.key, "alredy has notification key for user" - return callback(number) - else if number > 0 and notification.forceCreate - self.deleteNotificationByKeyOnly notification.key, callback - else - callback() + db.notifications.count query, (err, count)-> + return callback(err) if err? + callback(null, count) addNotification: (user_id, notification, callback)-> - @_checkExistingNotifcationAndOverride user_id, notification, (err)-> - if err? - callback err - else - doc = - user_id: ObjectId(user_id) - key: notification.key - messageOpts: notification.messageOpts - templateKey: notification.templateKey - # TTL index on the optional `expires` field, which should arrive as an iso date-string, corresponding to - # a datetime in the future when the document should be automatically removed. - # in Mongo, TTL indexes only work on date fields, and ignore the document when that field is missing - # see `README.md` for instruction on creating TTL index - if notification.expires? - try - doc.expires = new Date(notification.expires) - _testValue = doc.expires.toISOString() - catch err - logger.error {user_id, expires: notification.expires}, "error converting `expires` field to Date" - return callback(err) - db.notifications.insert(doc, callback) + @_countExistingNotifications user_id, notification, (err, count)-> + return callback(err) if err? + return callback() unless count == 0 || notification.forceCreate + doc = + user_id: ObjectId(user_id) + key: notification.key + messageOpts: notification.messageOpts + templateKey: notification.templateKey + # TTL index on the optional `expires` field, which should arrive as an iso date-string, corresponding to + # a datetime in the future when the document should be automatically removed. + # in Mongo, TTL indexes only work on date fields, and ignore the document when that field is missing + # see `README.md` for instruction on creating TTL index + if notification.expires? + try + doc.expires = new Date(notification.expires) + _testValue = doc.expires.toISOString() + catch err + logger.error {user_id, expires: notification.expires}, "error converting `expires` field to Date" + return callback(err) + db.notifications.update({user_id: doc.user_id, key: notification.key}, doc, {upsert: true}, callback) removeNotificationId: (user_id, notification_id, callback)-> searchOps = diff --git a/services/notifications/test/unit/coffee/NotificationsTests.coffee b/services/notifications/test/unit/coffee/NotificationsTests.coffee index c238bda119..47319fc912 100644 --- a/services/notifications/test/unit/coffee/NotificationsTests.coffee +++ b/services/notifications/test/unit/coffee/NotificationsTests.coffee @@ -29,14 +29,17 @@ describe 'Notifications Tests', -> remove: @removeStub @mongojs.ObjectId = ObjectId - @notifications = SandboxedModule.require modulePath, requires: - 'logger-sharelatex': { - log:()-> - error:()-> - } - 'settings-sharelatex': {} - 'mongojs':@mongojs - 'metrics-sharelatex': {timeAsyncMethod: sinon.stub()} + @notifications = SandboxedModule.require modulePath, + requires: + 'logger-sharelatex': { + log:()-> + error:()-> + } + 'settings-sharelatex': {} + 'mongojs':@mongojs + 'metrics-sharelatex': {timeAsyncMethod: sinon.stub()} + globals: + console: console @stubbedNotification = {user_id: ObjectId(user_id), key:"notification-key", messageOpts:"some info", templateKey:"template-key"} @stubbedNotificationArray = [@stubbedNotification] @@ -63,33 +66,34 @@ describe 'Notifications Tests', -> messageOpts:"some info", templateKey:"template-key" } - @notifications.deleteNotificationByKeyOnly = sinon.stub() - @notifications.deleteNotificationByKeyOnly.callsArgWith(1, null) + @expectedQuery = { + user_id: @stubbedNotification.user_id, + key:"notification-key", + } + @updateStub.yields() + @countStub.yields(null, 0) it 'should insert the notification into the collection', (done)-> - @insertStub.callsArgWith(1, null) - @countStub.callsArgWith(1, null, 0) - @notifications.addNotification user_id, @stubbedNotification, (err)=> - assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument) + expect(err).not.exists + sinon.assert.calledWith(@updateStub, @expectedQuery, @expectedDocument, { upsert: true }) done() - it 'should fail insert of existing notification key', (done)-> - @insertStub.callsArgWith(1, null) - @countStub.callsArgWith(1, null, 1) - @notifications.addNotification user_id, @stubbedNotification, (err)=> - @insertStub.calledWith(@expectedDocument).should.equal false - done() + describe 'when there is an existing notification', (done) -> + beforeEach -> + @countStub.yields(null, 1) - describe "when key already exists but forceCreate is passed", (done)-> + it 'should fail to insert', (done)-> + @notifications.addNotification user_id, @stubbedNotification, (err)=> + expect(err).not.exists + sinon.assert.notCalled(@updateStub) + done() - it "should delete the old key and insert the new one", (done) -> - @insertStub.callsArgWith(1, null) - @countStub.callsArgWith(1, null, 1) + it "should update the key if forceCreate is true", (done) -> @stubbedNotification.forceCreate = true @notifications.addNotification user_id, @stubbedNotification, (err)=> - assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument) - @notifications.deleteNotificationByKeyOnly.calledWith(@stubbedNotification.key).should.equal true + expect(err).not.exists + sinon.assert.calledWith(@updateStub, @expectedQuery, @expectedDocument, { upsert: true }) done() describe 'when the notification is set to expire', () -> @@ -108,14 +112,15 @@ describe 'Notifications Tests', -> templateKey:"template-key", expires: new Date(@stubbedNotification.expires), } + @expectedQuery = { + user_id: @stubbedNotification.user_id, + key:"notification-key", + } it 'should add an `expires` Date field to the document', (done)-> - @insertStub.callsArgWith(1, null) - @countStub.callsArgWith(1, null, 0) - @notifications.addNotification user_id, @stubbedNotification, (err)=> - @insertStub.callCount.should.equal 1 - assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument) + expect(err).not.exists + sinon.assert.calledWith(@updateStub, @expectedQuery, @expectedDocument, { upsert: true }) done() describe 'when the notification has a nonsensical expires field', () -> @@ -136,13 +141,9 @@ describe 'Notifications Tests', -> } it 'should produce an error', (done)-> - @insertStub.callsArgWith(1, null) - @countStub.callsArgWith(1, null, 0) - @notifications.addNotification user_id, @stubbedNotification, (err)=> (err instanceof Error).should.equal true - @insertStub.callCount.should.equal 0 - @insertStub.calledWith(@expectedDocument).should.equal false + sinon.assert.notCalled(@updateStub) done() describe 'removeNotificationId', ->