Merge pull request #12 from overleaf/spd-force-race

Handle duplicate-notification race condition when using forceCreate

services/notifications/app/coffee/Notifications.coffee

@@ -15,42 +15,35 @@ module.exports = Notifications =
 			callback err, notifications
 
 
-	_checkExistingNotifcationAndOverride : (user_id, notification, callback)->
+	_countExistingNotifications : (user_id, notification, callback = (err, count)->)->
-		self = @
 		query =
 			user_id: ObjectId(user_id)
 			key: notification.key
-		db.notifications.count query, (err, number)->
+		db.notifications.count query, (err, count)->
-			if number > 0 and !notification.forceCreate
+			return callback(err) if err?
-				logger.log number:number, user_id:user_id, key:notification.key, "alredy has notification key for user"
+			callback(null, count)
-				return callback(number)
-			else if number > 0 and notification.forceCreate
-				self.deleteNotificationByKeyOnly notification.key, callback
-			else
-				callback()
 
 	addNotification: (user_id, notification, callback)->
-		@_checkExistingNotifcationAndOverride user_id, notification, (err)->
+		@_countExistingNotifications user_id, notification, (err, count)->
-			if err?
+			return callback(err) if err?
-				callback err
+			return callback() unless count == 0 || notification.forceCreate
-			else
+			doc =
-				doc =
+				user_id: ObjectId(user_id)
-					user_id: ObjectId(user_id)
+				key: notification.key
-					key: notification.key
+				messageOpts: notification.messageOpts
-					messageOpts: notification.messageOpts
+				templateKey: notification.templateKey
-					templateKey: notification.templateKey
+			# TTL index on the optional `expires` field, which should arrive as an iso date-string, corresponding to
-				# 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.
-				# 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
-				# 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
-				# see `README.md` for instruction on creating TTL index
+			if notification.expires?
-				if notification.expires?
+				try
-					try
+					doc.expires =  new Date(notification.expires)
-						doc.expires =  new Date(notification.expires)
+					_testValue = doc.expires.toISOString()
-						_testValue = doc.expires.toISOString()
+				catch err
-					catch err
+					logger.error {user_id, expires: notification.expires}, "error converting `expires` field to Date"
-						logger.error {user_id, expires: notification.expires}, "error converting `expires` field to Date"
+					return callback(err)
-						return callback(err)
+			db.notifications.update({user_id: doc.user_id, key: notification.key}, doc, {upsert: true}, callback)
-				db.notifications.insert(doc, callback)
 
 	removeNotificationId: (user_id, notification_id, callback)->
 		searchOps =

services/notifications/test/unit/coffee/NotificationsTests.coffee

@@ -29,14 +29,17 @@ describe 'Notifications Tests', ->
 				remove: @removeStub
 		@mongojs.ObjectId = ObjectId
 
-		@notifications = SandboxedModule.require modulePath, requires:
+		@notifications = SandboxedModule.require modulePath,
-			'logger-sharelatex': {
+			requires:
-				log:()->
+				'logger-sharelatex': {
-				error:()->
+					log:()->
-			}
+					error:()->
-			'settings-sharelatex': {}
+				}
-			'mongojs':@mongojs
+				'settings-sharelatex': {}
-			'metrics-sharelatex': {timeAsyncMethod: sinon.stub()}
+				'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()
+			@expectedQuery = {
-			@notifications.deleteNotificationByKeyOnly.callsArgWith(1, null)
+				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)->
+		describe 'when there is an existing notification', (done) ->
-			@insertStub.callsArgWith(1, null)
+			beforeEach ->
-			@countStub.callsArgWith(1, null, 1)
+				@countStub.yields(null, 1)
-			@notifications.addNotification user_id, @stubbedNotification, (err)=>
-				@insertStub.calledWith(@expectedDocument).should.equal false
-				done()
 
-		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) ->
+			it "should update the key if forceCreate is true", (done) ->
-				@insertStub.callsArgWith(1, null)
-				@countStub.callsArgWith(1, null, 1)
 				@stubbedNotification.forceCreate = true
 				@notifications.addNotification user_id, @stubbedNotification, (err)=>
-					assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument)
+					expect(err).not.exists
-					@notifications.deleteNotificationByKeyOnly.calledWith(@stubbedNotification.key).should.equal true
+					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
+					expect(err).not.exists
-					assert.deepEqual(@insertStub.lastCall.args[0], @expectedDocument)
+					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
+					sinon.assert.notCalled(@updateStub)
-					@insertStub.calledWith(@expectedDocument).should.equal false
 					done()
 
 	describe 'removeNotificationId', ->