From bf14bc07efb6d8e6e819b0e4afaefadf7ed9eb5b Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Wed, 22 Jan 2025 08:57:49 +0000 Subject: [PATCH 1/2] feat: implement unsubscribe by id --- src/bidiMapper/BidiNoOpParser.ts | 7 +++++++ src/bidiMapper/BidiParser.ts | 1 + src/bidiMapper/CommandProcessor.ts | 2 +- .../modules/session/EventManager.ts | 5 +++++ .../modules/session/SessionProcessor.ts | 6 +++++- .../modules/session/SubscriptionManager.ts | 21 +++++++++++++++++-- src/bidiTab/BidiParser.ts | 3 +++ src/protocol-parser/protocol-parser.ts | 9 ++++++++ tests/session/test_subscription.py | 12 +++++++++++ tests/test_helpers.py | 2 +- 10 files changed, 63 insertions(+), 5 deletions(-) diff --git a/src/bidiMapper/BidiNoOpParser.ts b/src/bidiMapper/BidiNoOpParser.ts index 6f499982e7..53d193a2ca 100644 --- a/src/bidiMapper/BidiNoOpParser.ts +++ b/src/bidiMapper/BidiNoOpParser.ts @@ -218,6 +218,13 @@ export class BidiNoOpParser implements BidiCommandParameterParser { parseSubscribeParams(params: unknown): Session.SubscriptionRequest { return params as Session.SubscriptionRequest; } + parseUnsubscribeParams( + params: unknown, + ): Session.UnsubscribeByAttributesRequest | Session.UnsubscribeByIdRequest { + return params as + | Session.UnsubscribeByAttributesRequest + | Session.UnsubscribeByIdRequest; + } // keep-sorted end // Storage module diff --git a/src/bidiMapper/BidiParser.ts b/src/bidiMapper/BidiParser.ts index 29682f210b..0dc30e50c6 100644 --- a/src/bidiMapper/BidiParser.ts +++ b/src/bidiMapper/BidiParser.ts @@ -138,6 +138,7 @@ export interface BidiCommandParameterParser { // Session module // keep-sorted start block=yes parseSubscribeParams(params: unknown): Session.SubscriptionRequest; + parseUnsubscribeParams(params: unknown): Session.UnsubscribeParameters; // keep-sorted end // Storage module diff --git a/src/bidiMapper/CommandProcessor.ts b/src/bidiMapper/CommandProcessor.ts index 1268cddcbe..74b6a65e16 100644 --- a/src/bidiMapper/CommandProcessor.ts +++ b/src/bidiMapper/CommandProcessor.ts @@ -390,7 +390,7 @@ export class CommandProcessor extends EventEmitter { ); case 'session.unsubscribe': return await this.#sessionProcessor.unsubscribe( - this.#parser.parseSubscribeParams(command.params), + this.#parser.parseUnsubscribeParams(command.params), command.channel, ); // keep-sorted end diff --git a/src/bidiMapper/modules/session/EventManager.ts b/src/bidiMapper/modules/session/EventManager.ts index ac7c3c753d..e4678aebeb 100644 --- a/src/bidiMapper/modules/session/EventManager.ts +++ b/src/bidiMapper/modules/session/EventManager.ts @@ -306,6 +306,11 @@ export class EventManager extends EventEmitter { await this.toggleModulesIfNeeded(); } + async unsubscribeByIds(subscriptionIds: string[]): Promise { + this.#subscriptionManager.unsubscribeById(subscriptionIds); + await this.toggleModulesIfNeeded(); + } + async toggleModulesIfNeeded(): Promise { // TODO(1): Only update changed subscribers // TODO(2): Enable for Worker Targets diff --git a/src/bidiMapper/modules/session/SessionProcessor.ts b/src/bidiMapper/modules/session/SessionProcessor.ts index cbcd6df59e..7f80fe9007 100644 --- a/src/bidiMapper/modules/session/SessionProcessor.ts +++ b/src/bidiMapper/modules/session/SessionProcessor.ts @@ -157,9 +157,13 @@ export class SessionProcessor { } async unsubscribe( - params: Session.SubscriptionRequest, + params: Session.UnsubscribeParameters, channel: BidiPlusChannel = {}, ): Promise { + if ('subscriptions' in params) { + await this.#eventManager.unsubscribeByIds(params.subscriptions); + return {}; + } await this.#eventManager.unsubscribe( params.events as ChromiumBidi.EventNames[], params.contexts ?? [], diff --git a/src/bidiMapper/modules/session/SubscriptionManager.ts b/src/bidiMapper/modules/session/SubscriptionManager.ts index 37cc4ad436..324863057c 100644 --- a/src/bidiMapper/modules/session/SubscriptionManager.ts +++ b/src/bidiMapper/modules/session/SubscriptionManager.ts @@ -85,6 +85,7 @@ export type Subscription = { export class SubscriptionManager { #subscriptions: Subscription[] = []; + #knownSubscriptionIds = new Set(); #browsingContextStorage: BrowsingContextStorage; constructor(browsingContextStorage: BrowsingContextStorage) { @@ -222,6 +223,7 @@ export class SubscriptionManager { channel, }; this.#subscriptions.push(subscription); + this.#knownSubscriptionIds.add(subscription.id); return subscription; } @@ -349,8 +351,23 @@ export class SubscriptionManager { /** * Unsubscribes by subscriptionId. */ - unsubscribeById(_subscription: string) { - // TODO: implement. + unsubscribeById(subscriptionIds: string[]) { + const subscriptionIdsSet = new Set(subscriptionIds); + const unknownIds = difference( + subscriptionIdsSet, + this.#knownSubscriptionIds, + ); + + if (unknownIds.size !== 0) { + throw new InvalidArgumentException('No subscription found'); + } + this.#subscriptions = this.#subscriptions.filter((subscription) => { + return subscriptionIdsSet.has(subscription.id); + }); + this.#knownSubscriptionIds = difference( + this.#knownSubscriptionIds, + subscriptionIdsSet, + ); } } diff --git a/src/bidiTab/BidiParser.ts b/src/bidiTab/BidiParser.ts index da303e11de..bbcc0fcaff 100644 --- a/src/bidiTab/BidiParser.ts +++ b/src/bidiTab/BidiParser.ts @@ -217,6 +217,9 @@ export class BidiParser implements BidiCommandParameterParser { parseSubscribeParams(params: unknown): Session.SubscriptionRequest { return Parser.Session.parseSubscribeParams(params); } + parseUnsubscribeParams(params: unknown): Session.UnsubscribeParameters { + return Parser.Session.parseUnsubscribeParams(params); + } // keep-sorted end // Storage module diff --git a/src/protocol-parser/protocol-parser.ts b/src/protocol-parser/protocol-parser.ts index 90ae967900..e32036d1eb 100644 --- a/src/protocol-parser/protocol-parser.ts +++ b/src/protocol-parser/protocol-parser.ts @@ -278,6 +278,15 @@ export namespace Session { WebDriverBidi.Session.SubscriptionRequestSchema, ) as Protocol.Session.SubscriptionRequest; } + + export function parseUnsubscribeParams( + params: unknown, + ): Protocol.Session.UnsubscribeParameters { + return parseObject( + params, + WebDriverBidi.Session.UnsubscribeParametersSchema, + ) as Protocol.Session.UnsubscribeParameters; + } } export namespace Input { diff --git a/tests/session/test_subscription.py b/tests/session/test_subscription.py index a2f27f89a3..c894169bdd 100644 --- a/tests/session/test_subscription.py +++ b/tests/session/test_subscription.py @@ -525,6 +525,18 @@ async def test_subscribeWithoutContext_bufferedEventsFromNotClosedContextsAreRet assert {"type": "success", "id": command_id, 'result': ANY} == resp +@pytest.mark.asyncio +async def test_unsubscribe_by_id(websocket): + res = await subscribe(websocket, ["log.entryAdded"]) + await execute_command( + websocket, { + "method": "session.unsubscribe", + "params": { + "subscriptions": [res["subscription"]] + } + }) + + @pytest.mark.asyncio @pytest.mark.parametrize("channel_name", ["channel", "goog:channel"]) async def test_unsubscribeIsAtomic(websocket, context_id, iframe_id, diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 4d8dad1b14..b367eff535 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -66,7 +66,7 @@ async def subscribe(websocket, command[channel_name if channel_name is not None else "goog:channel"] = channel - await execute_command(websocket, command) + return await execute_command(websocket, command) async def send_JSON_command(websocket, command: dict) -> int: From 2eaebb60d926a8d6f27bea9e6b9ad5cbb2c070cf Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Wed, 22 Jan 2025 11:41:38 +0000 Subject: [PATCH 2/2] add unit tests --- .../session/SubscriptionManager.spec.ts | 64 +++++++++++++++++++ .../modules/session/SubscriptionManager.ts | 2 +- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/bidiMapper/modules/session/SubscriptionManager.spec.ts b/src/bidiMapper/modules/session/SubscriptionManager.spec.ts index a67f33825a..dc62a14960 100644 --- a/src/bidiMapper/modules/session/SubscriptionManager.spec.ts +++ b/src/bidiMapper/modules/session/SubscriptionManager.spec.ts @@ -203,6 +203,18 @@ describe('SubscriptionManager', () => { ).to.equal(false); }); + it('should unsubscribe by id', () => { + const {id} = subscriptionManager.subscribe( + [SOME_EVENT], + [], + SOME_CHANNEL, + ); + subscriptionManager.unsubscribeById([id]); + expect( + subscriptionManager.isSubscribedTo(SOME_EVENT, SOME_CONTEXT), + ).to.equal(false); + }); + it('should not unsubscribe on error', () => { subscriptionManager.subscribe([SOME_EVENT], [], SOME_CHANNEL); expect(() => @@ -299,6 +311,18 @@ describe('SubscriptionManager', () => { ).to.equal(false); }); + it('should unsubscribe by id', () => { + const {id} = subscriptionManager.subscribe( + [SOME_EVENT], + [SOME_CONTEXT], + SOME_CHANNEL, + ); + subscriptionManager.unsubscribeById([id]); + expect( + subscriptionManager.isSubscribedTo(SOME_EVENT, SOME_CONTEXT), + ).to.equal(false); + }); + it('should partially unsubscribe from a context', () => { subscriptionManager.subscribe( [SOME_EVENT], @@ -430,6 +454,46 @@ describe('SubscriptionManager', () => { }); }); + describe('unsubscribeById', () => { + it('should keep subscription if one of the IDs is not known', () => { + const {id} = subscriptionManager.subscribe( + [SOME_EVENT], + [], + SOME_CHANNEL, + ); + expect( + subscriptionManager.isSubscribedTo(SOME_EVENT, SOME_CONTEXT), + ).to.equal(true); + expect(() => { + subscriptionManager.unsubscribeById([id, 'wrong']); + }).to.throw('No subscription found'); + expect( + subscriptionManager.isSubscribedTo(SOME_EVENT, SOME_CONTEXT), + ).to.equal(true); + }); + + it('should throw an error if an ID is not know', () => { + expect(() => { + subscriptionManager.unsubscribeById(['wrong']); + }).to.throw('No subscription found'); + }); + + it('should throw an error if a subscription is used multiple times', () => { + const {id} = subscriptionManager.subscribe( + [SOME_EVENT], + [], + SOME_CHANNEL, + ); + expect( + subscriptionManager.isSubscribedTo(SOME_EVENT, SOME_CONTEXT), + ).to.equal(true); + subscriptionManager.unsubscribeById([id]); + expect(() => { + subscriptionManager.unsubscribeById([id]); + }).to.throw('No subscription found'); + }); + }); + describe('cartesian product', () => { it('should return empty array for empty array', () => { expect(cartesianProduct([], [])).to.deep.equal([]); diff --git a/src/bidiMapper/modules/session/SubscriptionManager.ts b/src/bidiMapper/modules/session/SubscriptionManager.ts index 324863057c..a3f585573a 100644 --- a/src/bidiMapper/modules/session/SubscriptionManager.ts +++ b/src/bidiMapper/modules/session/SubscriptionManager.ts @@ -362,7 +362,7 @@ export class SubscriptionManager { throw new InvalidArgumentException('No subscription found'); } this.#subscriptions = this.#subscriptions.filter((subscription) => { - return subscriptionIdsSet.has(subscription.id); + return !subscriptionIdsSet.has(subscription.id); }); this.#knownSubscriptionIds = difference( this.#knownSubscriptionIds,