Skip to content

Commit 95e5e45

Browse files
danny-aviladanny-avila
committed
fix: address code review findings from PR #12451
- Restore typed `AnyBulkWriteOperation<AclEntry>[]` on bulkWriteAclEntries, cast to untyped only at the tenantSafeBulkWrite call site (Finding 1) - Type `findUser` model accessor consistently with `findUsers` (Finding 2) - Replace numbered Record casts with `.not.toHaveProperty()` in role.methods.spec for SHARED_GLOBAL assertions (Finding 6) - Use per-test ObjectIds instead of shared testUserId in openid.spec (Finding 4) - Replace inline `import()` type annotations with top-level SessionData import in sessionCache spec (Finding 5) - Add `packages/client/node_modules` to typecheck CI cache paths (Finding 7) - Remove extraneous blank line in user.ts searchUsers (Finding 8)
1 parent d84582f commit 95e5e45

File tree

7 files changed

+30
-61
lines changed

7 files changed

+30
-61
lines changed

‎.github/workflows/backend-review.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ jobs:
118118
node_modules
119119
api/node_modules
120120
packages/api/node_modules
121+
packages/client/node_modules
121122
packages/data-provider/node_modules
122123
packages/data-schemas/node_modules
123124
key: node-modules-backend-${{ runner.os }}-20.19-${{ hashFiles('package-lock.json') }}

‎packages/api/src/auth/openid.spec.ts‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import { logger } from '@librechat/data-schemas';
44
import type { IUser, UserMethods } from '@librechat/data-schemas';
55
import { findOpenIDUser } from './openid';
66

7-
const testUserId = new Types.ObjectId();
7+
function newId() {
8+
return new Types.ObjectId();
9+
}
810

911
jest.mock('@librechat/data-schemas', () => ({
1012
...jest.requireActual('@librechat/data-schemas'),
@@ -27,7 +29,7 @@ describe('findOpenIDUser', () => {
2729
describe('Primary condition searches', () => {
2830
it('should find user by openidId', async () => {
2931
const mockUser: IUser = {
30-
_id: testUserId,
32+
_id: newId(),
3133
provider: 'openid',
3234
openidId: 'openid_123',
3335
email: 'user@example.com',
@@ -54,7 +56,7 @@ describe('findOpenIDUser', () => {
5456

5557
it('should find user by idOnTheSource', async () => {
5658
const mockUser: IUser = {
57-
_id: testUserId,
59+
_id: newId(),
5860
provider: 'openid',
5961
idOnTheSource: 'source_123',
6062
email: 'user@example.com',
@@ -81,7 +83,7 @@ describe('findOpenIDUser', () => {
8183

8284
it('should find user by both openidId and idOnTheSource', async () => {
8385
const mockUser: IUser = {
84-
_id: testUserId,
86+
_id: newId(),
8587
provider: 'openid',
8688
openidId: 'openid_123',
8789
idOnTheSource: 'source_123',
@@ -112,7 +114,7 @@ describe('findOpenIDUser', () => {
112114
describe('Email-based searches', () => {
113115
it('should find user by email when primary conditions fail and openidId matches', async () => {
114116
const mockUser: IUser = {
115-
_id: testUserId,
117+
_id: newId(),
116118
provider: 'openid',
117119
openidId: 'openid_123',
118120
email: 'user@example.com',
@@ -180,7 +182,7 @@ describe('findOpenIDUser', () => {
180182
describe('Provider conflict handling', () => {
181183
it('should return error when user has different provider', async () => {
182184
const mockUser: IUser = {
183-
_id: testUserId,
185+
_id: newId(),
184186
provider: 'google',
185187
email: 'user@example.com',
186188
username: 'testuser',
@@ -205,7 +207,7 @@ describe('findOpenIDUser', () => {
205207

206208
it('should reject email fallback when existing openidId does not match token sub', async () => {
207209
const mockUser: IUser = {
208-
_id: testUserId,
210+
_id: newId(),
209211
provider: 'openid',
210212
openidId: 'openid_456',
211213
email: 'user@example.com',
@@ -229,7 +231,7 @@ describe('findOpenIDUser', () => {
229231

230232
it('should allow email fallback when existing openidId matches token sub', async () => {
231233
const mockUser: IUser = {
232-
_id: testUserId,
234+
_id: newId(),
233235
provider: 'openid',
234236
openidId: 'openid_123',
235237
email: 'user@example.com',
@@ -255,7 +257,7 @@ describe('findOpenIDUser', () => {
255257
describe('User migration scenarios', () => {
256258
it('should prepare user for migration when email exists without openidId', async () => {
257259
const mockUser: IUser = {
258-
_id: testUserId,
260+
_id: newId(),
259261
email: 'user@example.com',
260262
username: 'testuser',
261263
// No provider and no openidId - needs migration
@@ -284,7 +286,7 @@ describe('findOpenIDUser', () => {
284286

285287
it('should reject when user already has a different openidId', async () => {
286288
const mockUser: IUser = {
287-
_id: testUserId,
289+
_id: newId(),
288290
provider: 'openid',
289291
openidId: 'existing_openid',
290292
email: 'user@example.com',
@@ -308,7 +310,7 @@ describe('findOpenIDUser', () => {
308310

309311
it('should reject when user has no provider but a different openidId', async () => {
310312
const mockUser: IUser = {
311-
_id: testUserId,
313+
_id: newId(),
312314
openidId: 'existing_openid',
313315
email: 'user@example.com',
314316
username: 'testuser',
@@ -415,7 +417,7 @@ describe('findOpenIDUser', () => {
415417

416418
it('should pass email to findUser for case-insensitive lookup (findUser handles normalization)', async () => {
417419
const mockUser: IUser = {
418-
_id: testUserId,
420+
_id: newId(),
419421
provider: 'openid',
420422
openidId: 'openid_123',
421423
email: 'user@example.com',
@@ -451,7 +453,7 @@ describe('findOpenIDUser', () => {
451453

452454
it('should reject email fallback when openidId is empty and user has a stored openidId', async () => {
453455
const mockUser: IUser = {
454-
_id: testUserId,
456+
_id: newId(),
455457
provider: 'openid',
456458
openidId: 'existing-real-id',
457459
email: 'user@example.com',

‎packages/api/src/cache/__tests__/cacheFactory/sessionCache.cache_integration.spec.ts‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { MemoryStore } from 'express-session';
1+
import type { MemoryStore, SessionData } from 'express-session';
22
import type { RedisStore as ConnectRedis } from 'connect-redis';
33

44
interface TestSessionData {
@@ -20,13 +20,7 @@ describe('sessionCache', () => {
2020
const asyncStore = (store: CacheSessionStore) => ({
2121
set: (id: string, data: TestSessionData) =>
2222
new Promise<void>((resolve) =>
23-
store.set(
24-
id,
25-
data as Partial<
26-
import('express-session').SessionData
27-
> as import('express-session').SessionData,
28-
() => resolve(),
29-
),
23+
store.set(id, data as Partial<SessionData> as SessionData, () => resolve()),
3024
),
3125
get: (id: string) =>
3226
new Promise<TestSessionData | null | undefined>((resolve) =>
@@ -35,13 +29,7 @@ describe('sessionCache', () => {
3529
destroy: (id: string) => new Promise<void>((resolve) => store.destroy(id, () => resolve())),
3630
touch: (id: string, data: TestSessionData) =>
3731
new Promise<void>((resolve) =>
38-
store.touch(
39-
id,
40-
data as Partial<
41-
import('express-session').SessionData
42-
> as import('express-session').SessionData,
43-
() => resolve(),
44-
),
32+
store.touch(id, data as Partial<SessionData> as SessionData, () => resolve()),
4533
),
4634
});
4735

‎packages/api/src/utils/sanitizeTitle.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* ES2024 String.prototype.isWellFormed — not in this TS lib version */
1+
/* String.prototype.isWellFormed — ES2024 API, available in Node 20+ but not in TS 5.3 lib */
22
declare global {
33
interface String {
44
isWellFormed(): boolean;

‎packages/data-schemas/src/methods/aclEntry.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type {
77
DeleteResult,
88
Model,
99
} from 'mongoose';
10-
import type { IAclEntry } from '~/types';
10+
import type { AclEntry, IAclEntry } from '~/types';
1111
import { tenantSafeBulkWrite } from '~/utils/tenantBulkWrite';
1212

1313
export function createAclEntryMethods(mongoose: typeof import('mongoose')) {
@@ -375,11 +375,11 @@ export function createAclEntryMethods(mongoose: typeof import('mongoose')) {
375375
* @param options - Optional query options (e.g., { session })
376376
*/
377377
async function bulkWriteAclEntries(
378-
ops: AnyBulkWriteOperation[],
378+
ops: AnyBulkWriteOperation<AclEntry>[],
379379
options?: { session?: ClientSession },
380380
) {
381381
const AclEntry = mongoose.models.AclEntry as Model<IAclEntry>;
382-
return tenantSafeBulkWrite(AclEntry, ops, options || {});
382+
return tenantSafeBulkWrite(AclEntry, ops as AnyBulkWriteOperation[], options || {});
383383
}
384384

385385
/**

‎packages/data-schemas/src/methods/role.methods.spec.ts‎

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -289,16 +289,8 @@ describe('updateAccessPermissions', () => {
289289
// SHARED_GLOBAL=false → SHARE=false (inherited)
290290
expect(updatedRole.permissions[PermissionTypes.AGENTS]!.SHARE).toBe(false);
291291
// SHARED_GLOBAL cleaned up
292-
const promptsPerms = updatedRole.permissions[PermissionTypes.PROMPTS] as Record<
293-
string,
294-
boolean | undefined
295-
>;
296-
const agentsPerms = updatedRole.permissions[PermissionTypes.AGENTS] as Record<
297-
string,
298-
boolean | undefined
299-
>;
300-
expect(promptsPerms.SHARED_GLOBAL).toBeUndefined();
301-
expect(agentsPerms.SHARED_GLOBAL).toBeUndefined();
292+
expect(updatedRole.permissions[PermissionTypes.PROMPTS]).not.toHaveProperty('SHARED_GLOBAL');
293+
expect(updatedRole.permissions[PermissionTypes.AGENTS]).not.toHaveProperty('SHARED_GLOBAL');
302294
});
303295

304296
it('should respect explicit SHARE in update payload and not override it with SHARED_GLOBAL', async () => {
@@ -318,11 +310,7 @@ describe('updateAccessPermissions', () => {
318310
const updatedRole = await getRoleByName(SystemRoles.USER);
319311

320312
expect(updatedRole.permissions[PermissionTypes.PROMPTS]!.SHARE).toBe(false);
321-
const promptsPerms2 = updatedRole.permissions[PermissionTypes.PROMPTS] as Record<
322-
string,
323-
boolean | undefined
324-
>;
325-
expect(promptsPerms2.SHARED_GLOBAL).toBeUndefined();
313+
expect(updatedRole.permissions[PermissionTypes.PROMPTS]).not.toHaveProperty('SHARED_GLOBAL');
326314
});
327315

328316
it('should migrate SHARED_GLOBAL to SHARE even when the permType is not in the update payload', async () => {
@@ -350,11 +338,7 @@ describe('updateAccessPermissions', () => {
350338
// SHARE should have been inherited from SHARED_GLOBAL, not silently dropped
351339
expect(updatedRole.permissions[PermissionTypes.PROMPTS]!.SHARE).toBe(true);
352340
// SHARED_GLOBAL should be removed
353-
const promptsPerms3 = updatedRole.permissions[PermissionTypes.PROMPTS] as Record<
354-
string,
355-
boolean | undefined
356-
>;
357-
expect(promptsPerms3.SHARED_GLOBAL).toBeUndefined();
341+
expect(updatedRole.permissions[PermissionTypes.PROMPTS]).not.toHaveProperty('SHARED_GLOBAL');
358342
// Original USE should be untouched
359343
expect(updatedRole.permissions[PermissionTypes.PROMPTS]!.USE).toBe(true);
360344
// The actual update should have applied
@@ -382,11 +366,7 @@ describe('updateAccessPermissions', () => {
382366

383367
const updatedRole = await getRoleByName(SystemRoles.USER);
384368

385-
const promptsPerms4 = updatedRole.permissions[PermissionTypes.PROMPTS] as Record<
386-
string,
387-
boolean | undefined
388-
>;
389-
expect(promptsPerms4.SHARED_GLOBAL).toBeUndefined();
369+
expect(updatedRole.permissions[PermissionTypes.PROMPTS]).not.toHaveProperty('SHARED_GLOBAL');
390370
expect(updatedRole.permissions[PermissionTypes.PROMPTS]!.SHARE).toBe(true);
391371
expect(updatedRole.permissions[PermissionTypes.MULTI_CONVO]!.USE).toBe(true);
392372
});

‎packages/data-schemas/src/methods/user.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,13 @@ export function createUserMethods(mongoose: typeof import('mongoose')) {
3535
searchCriteria: FilterQuery<IUser>,
3636
fieldsToSelect?: string | string[] | null,
3737
): Promise<IUser | null> {
38-
const User = mongoose.models.User;
38+
const User = mongoose.models.User as mongoose.Model<IUser>;
3939
const normalizedCriteria = normalizeEmailInCriteria(searchCriteria);
4040
const query = User.findOne(normalizedCriteria);
4141
if (fieldsToSelect) {
4242
query.select(fieldsToSelect);
4343
}
44-
return (await query.lean()) as IUser | null;
44+
return await query.lean();
4545
}
4646

4747
async function findUsers(
@@ -301,8 +301,6 @@ export function createUserMethods(mongoose: typeof import('mongoose')) {
301301
.sort((a, b) => b._searchScore - a._searchScore)
302302
.slice(0, limit)
303303
.map((user) => {
304-
// Remove the search score from final results
305-
306304
const { _searchScore, ...userWithoutScore } = user;
307305
return userWithoutScore;
308306
});

0 commit comments

Comments
 (0)