From 433b7f87074130e842efef071aac13f42fb1d437 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ricardo=20Arturo=20Cabral=20Mej=C3=ADa?=
 <me@ricardocabral.io>
Date: Mon, 19 Dec 2022 10:01:04 -0500
Subject: [PATCH] feat: ignore dupe subscriptions

---
 src/handlers/subscribe-message-handler.ts      | 18 ++++++++++++------
 .../handlers/subscribe-message-handler.spec.ts | 14 ++++++++------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/src/handlers/subscribe-message-handler.ts b/src/handlers/subscribe-message-handler.ts
index 3262906..b8dc500 100644
--- a/src/handlers/subscribe-message-handler.ts
+++ b/src/handlers/subscribe-message-handler.ts
@@ -39,7 +39,7 @@ export class SubscribeMessageHandler implements IMessageHandler, IAbortable {
     const reason = this.canSubscribe(subscriptionId, filters)
     if (reason) {
       debug('subscription %s with %o rejected: %s', subscriptionId, filters, reason)
-      this.webSocket.emit(WebSocketAdapterEvent.Message, createNoticeMessage(`Subscription request rejected: ${reason}`))
+      this.webSocket.emit(WebSocketAdapterEvent.Message, createNoticeMessage(`Subscription rejected: ${reason}`))
       return
     }
 
@@ -85,12 +85,18 @@ export class SubscribeMessageHandler implements IMessageHandler, IAbortable {
   }
 
   private canSubscribe(subscriptionId: SubscriptionId, filters: SubscriptionFilter[]): string | undefined {
+    const subscriptions = this.webSocket.getSubscriptions()
+    const existingSubscription = subscriptions.get(subscriptionId)
+
+    if (existingSubscription?.length && equals(filters, existingSubscription)) {
+        return `Duplicate subscription ${subscriptionId}: Ignorning`
+    }
+
     const maxSubscriptions = this.settings().limits.client.subscription.maxSubscriptions
-    if (maxSubscriptions > 0) {
-      const subscriptions = this.webSocket.getSubscriptions()
-      if (!subscriptions.has(subscriptionId) && subscriptions.size + 1 > maxSubscriptions) {
-        return `Too many subscriptions: Number of subscriptions must be less than or equal to ${maxSubscriptions}`
-      }
+    if (maxSubscriptions > 0
+      && !existingSubscription?.length && subscriptions.size + 1 > maxSubscriptions
+    ) {
+      return `Too many subscriptions: Number of subscriptions must be less than or equal to ${maxSubscriptions}`
     }
 
     const maxFilters = this.settings().limits.client.subscription.maxFilters
diff --git a/test/unit/handlers/subscribe-message-handler.spec.ts b/test/unit/handlers/subscribe-message-handler.spec.ts
index e1c6622..05ab195 100644
--- a/test/unit/handlers/subscribe-message-handler.spec.ts
+++ b/test/unit/handlers/subscribe-message-handler.spec.ts
@@ -30,7 +30,7 @@ const toDbEvent = (event: Event) => ({
 describe('SubscribeMessageHandler', () => {
   const subscriptionId: SubscriptionId = 'subscriptionId'
   let filters: SubscriptionFilter[]
-  let subscriptions: Map<SubscriptionId, Set<SubscriptionFilter>>
+  let subscriptions: Map<SubscriptionId, SubscriptionFilter[]>
   let handler: IMessageHandler & IAbortable
   let webSocket: IWebSocketAdapter
   let eventRepository: IEventRepository
@@ -94,7 +94,7 @@ describe('SubscribeMessageHandler', () => {
       await handler.handleMessage(message)
 
       expect(webSocketOnMessageStub).to.have.been.calledOnceWithExactly(
-        ['NOTICE', 'Subscription request rejected: reason']
+        ['NOTICE', 'Subscription rejected: reason']
       )
     })
 
@@ -273,7 +273,7 @@ describe('SubscribeMessageHandler', () => {
       expect((handler as any).canSubscribe(subscriptionId, filters)).to.be.undefined
     })
 
-    it('returns undefined if client is resubscribing', () => {
+    it('returns reason if client is sending a duplicate subscription', () => {
       settingsFactory.returns({
         limits: {
           client: {
@@ -283,9 +283,11 @@ describe('SubscribeMessageHandler', () => {
           },
         },
       })
-      subscriptions.set(subscriptionId, new Set([{}]))
+      filters = [{ authors: ['aa'] }]
+      subscriptions.set(subscriptionId, filters)
 
-      expect((handler as any).canSubscribe(subscriptionId, filters)).to.be.undefined
+      expect((handler as any).canSubscribe(subscriptionId, filters))
+        .to.equal('Duplicate subscription subscriptionId: Ignorning')
     })
 
     it('returns reason if client subscriptions exceed limits', () => {
@@ -298,7 +300,7 @@ describe('SubscribeMessageHandler', () => {
           },
         },
       })
-      subscriptions.set('other-sub', new Set())
+      subscriptions.set('other-sub', [])
 
       expect((handler as any).canSubscribe(subscriptionId, filters)).to.equal('Too many subscriptions: Number of subscriptions must be less than or equal to 1')
     })