Skip to content

Commit 016ae0a

Browse files
committed
fix: correct pubSubApp property name in del() and add cleanup tests
The del() method checked `appData.pubsubApp` (lowercase 's') instead of `appData.pubSubApp` (camelCase), so deleting a Pub/Sub app never cleaned up the `oapp:pub:{app}` Redis set, causing orphaned entries to accumulate. Adds del() subscriber cleanup tests and makes the mock Redis multi chain faithful to real Redis result-array behavior.
1 parent 63cd78f commit 016ae0a

File tree

2 files changed

+68
-3
lines changed

2 files changed

+68
-3
lines changed

lib/oauth2-apps.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,7 @@ class OAuth2AppsHandler {
831831
.del(`${REDIS_PREFIX}oapp:h:${id}`)
832832
.srem(this.getSubscribersKey(), id);
833833

834-
if (appData.pubsubApp) {
834+
if (appData.pubSubApp) {
835835
pipeline = pipeline.srem(this.getPubsubAppKey(appData.pubSubApp), id);
836836
}
837837

test/pubsub-recovery-test.js

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,24 +81,35 @@ function createMockRedis() {
8181
return this;
8282
},
8383
hset: function () {
84+
ops.push(() => 0);
8485
return this;
8586
},
8687
hdel: function () {
88+
ops.push(() => 0);
8789
return this;
8890
},
8991
del: function () {
92+
ops.push(() => 0);
9093
return this;
9194
},
9295
expire: function () {
96+
ops.push(() => 0);
9397
return this;
9498
},
95-
srem: function () {
99+
srem: function (key, ...members) {
100+
ops.push(() => redis.srem(key, ...members));
101+
return this;
102+
},
103+
scard: function (key) {
104+
ops.push(() => redis.scard(key));
96105
return this;
97106
},
98107
zadd: function () {
108+
ops.push(() => 0);
99109
return this;
100110
},
101111
hincrby: function () {
112+
ops.push(() => 0);
102113
return this;
103114
}
104115
};
@@ -126,7 +137,14 @@ function createMockRedis() {
126137
if (!mockSets[key]) return 0;
127138
return mockSets[key].size;
128139
},
129-
srem: async () => {},
140+
srem: async (key, ...members) => {
141+
if (!mockSets[key]) return 0;
142+
let removed = 0;
143+
for (let m of members) {
144+
if (mockSets[key].delete(m)) removed++;
145+
}
146+
return removed;
147+
},
130148
exists: async () => 0,
131149
get: async () => null,
132150
set: async () => 'OK',
@@ -708,4 +726,51 @@ test('Pub/Sub subscription recovery tests', async t => {
708726
)();
709727
});
710728
});
729+
730+
// --- del() subscriber cleanup tests ---
731+
732+
await t.test('del() subscriber cleanup tests', async t2 => {
733+
let subscribersKey = `${REDIS_PREFIX}oapp:sub`;
734+
let indexKey = `${REDIS_PREFIX}oapp:i`;
735+
let dataKey = `${REDIS_PREFIX}oapp:c`;
736+
737+
t2.beforeEach(() => {
738+
mockRedisData = {};
739+
mockSets = {};
740+
});
741+
742+
await t2.test('del() removes app from subscriber set and pubsub app set', async () => {
743+
let appId = 'del-test-app';
744+
let pubSubAppId = 'parent-pubsub-app';
745+
let pubsubAppKey = `${REDIS_PREFIX}oapp:pub:${pubSubAppId}`;
746+
let appFields = { id: appId, baseScopes: 'pubsub', provider: 'gmailService', pubSubApp: pubSubAppId };
747+
748+
mockSets[indexKey] = new Set([appId]);
749+
mockSets[subscribersKey] = new Set([appId]);
750+
mockSets[pubsubAppKey] = new Set([appId]);
751+
mockRedisData[dataKey] = { [`${appId}:data`]: msgpack.encode(appFields) };
752+
753+
await withMockedOauth2Apps({ get: async () => ({ ...appFields }) }, async () => {
754+
let result = await oauth2Apps.del(appId);
755+
assert.strictEqual(result.id, appId, 'should return correct id');
756+
assert.ok(!mockSets[subscribersKey] || !mockSets[subscribersKey].has(appId), 'app should be removed from subscriber set');
757+
assert.ok(!mockSets[pubsubAppKey] || !mockSets[pubsubAppKey].has(appId), 'app should be removed from pubsub app set');
758+
})();
759+
});
760+
761+
await t2.test('del() without pubSubApp skips pubsub app set removal', async () => {
762+
let appId = 'del-no-pubsub-app';
763+
let appFields = { id: appId, baseScopes: 'api', provider: 'gmail' };
764+
765+
mockSets[indexKey] = new Set([appId]);
766+
mockSets[subscribersKey] = new Set([appId]);
767+
mockRedisData[dataKey] = { [`${appId}:data`]: msgpack.encode(appFields) };
768+
769+
await withMockedOauth2Apps({ get: async () => ({ ...appFields }) }, async () => {
770+
let result = await oauth2Apps.del(appId);
771+
assert.strictEqual(result.id, appId, 'should return correct id');
772+
assert.ok(!mockSets[subscribersKey] || !mockSets[subscribersKey].has(appId), 'app should be removed from subscriber set');
773+
})();
774+
});
775+
});
711776
});

0 commit comments

Comments
 (0)