[Regression on closed #541] TeamsService.create and updateSplits are back to one SELECT per member to check existence

Đang mở
  • Có gì không ổn?
#730 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức phù hợp với người mới
75/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
postgresql, typescript
Lĩnh vực
backend, databases, performance, testing

Hướng nghiên cứu

Start in src/teams/teams.service.ts at create and updateSplits, where each member is checked with a separate findOne. Read the related team service spec and inspect how repository queries and transactions are used in both methods. Replace the repeated checks with a shared batched existence check, then add a spec confirming one user query for a multi-member payload and run the team service tests.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

bug performance

Location — src/teams/teams.service.ts:28-35 (create) and src/teams/teams.service.ts:80-87 (updateSplits).

Problem — The per-member existence loop closed as #541 is present again, duplicated verbatim in both methods:

// Verify all member userIds exist
for (const member of dto.members) {
  const userExists = await this.userRepo.findOne({
    where: { id: member.userId },
  });
  if (!userExists) {
    throw new NotFoundException(`User ${member.userId} not found`);
  }
}

Each iteration is an awaited round trip, so the queries are strictly sequential: a 20-member team issues 20 serial SELECT * FROM users WHERE id = $1 LIMIT 1 statements before any write happens. findOne with no select also hydrates the full User row (including payout address and other PII columns) when only the row's existence is needed.

Why it matters — Request latency scales linearly with member count at full network round-trip cost, on a write path that then holds a transaction open. It is also a needless source of partial-failure windows: by the time member 20 is checked, member 1 may already have been deleted, so the FK can still reject the insert (see the non-transactional delete/insert problem in updateSplits). And the same five lines now exist in two places, so the next fix has to be applied twice — exactly how this regressed.

Reproduction / failure path

  1. Enable DATABASE_LOGGING=true.
  2. POST /teams with 20 distinct valid member userIds.
  3. The log shows 20 separate users SELECTs before the teams INSERT, instead of one.
  4. Same for PATCH /teams/:id/splits with 20 members.

Suggested fix

  • Replace both loops with one batched query and a set difference: 
    const ids = members.map((m) => m.userId);
const found = await this.userRepo.find({ where: { id: In(ids) }, select: { id: true } });
const missing = ids.filter((id) => !found.some((u) => u.id === id));
if (missing.length) throw new NotFoundException(`User(s) not found: ${missing.join(', ')}`);
    Note this also reports every missing id at once instead of only the first.
  • Extract it into one private helper (e.g. assertMembersExist(members, manager?)) used by both create and updateSplits, taking an optional EntityManager so it can run inside the same transaction as the write.
  • Add a spec asserting userRepo.find is called exactly once for a multi-member payload, so a future merge cannot quietly reintroduce the loop.
Ngôn ngữ chính
TypeScript
Star
2
Fork
90
Merge trung bình
31 phút
Pull request đã merge (30 ngày)
20

Chuẩn bị môi trường

  • Có Dockerfile hoặc tệp Docker Compose
  • Không có mẫu pull request
  • Không có hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của MergeFi/backend

Tất cả issue của MergeFi/backend

Issue tương tự

Thêm issue về TypeScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.