fix: close active connection on API key deletion #75
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughОбзорИзменения реализуют управление Socket.IO комнатами для операций с API ключами: сокеты теперь присоединяются к каналам, привязанным к идентификаторам ключей, а при удалении ключа все сокеты в этом канале отключаются перед удалением. Изменения
Оценка трудозатрат на код-ревью🎯 3 (Умеренная) | ⏱️ ~20 минут Стихотворение
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Руководство для ревьюера (свернуто для небольших PR)Руководство для ревьюераРеализует отзыв активного доступа по WebSocket при удалении API-ключа за счёт привязки соединений к комнате, основанной на ключе, и принудительного отключения всех сокетов в этой комнате при удалении ключа. Диаграмма последовательности для отзыва WebSocket при удалении API-ключаsequenceDiagram
actor Admin
participant ApiKeysRoute
participant Prisma
participant SocketServer
participant BotApiClient
Admin->>ApiKeysRoute: DELETE /bots/:botId/api-keys/:keyId
ApiKeysRoute->>ApiKeysRoute: parse botId and keyId
ApiKeysRoute->>SocketServer: getIO()
ApiKeysRoute->>SocketServer: namespace /bot-api
ApiKeysRoute->>SocketServer: room key_keyId disconnectSockets(force=true)
SocketServer-->>BotApiClient: disconnect WebSocket
ApiKeysRoute->>Prisma: botApiKey.deleteMany(botId, keyId)
Prisma-->>ApiKeysRoute: deletion result
ApiKeysRoute-->>Admin: 200 OK (deletion result)
Изменения на уровне файлов
Подсказки и командыВзаимодействие с Sourcery
Настройка работыПерейдите в свою панель управления, чтобы:
Получение помощи
Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideImplements revocation of active WebSocket access when an API key is deleted by tagging connections with a key-based room and force-disconnecting all sockets in that room on key deletion. Sequence diagram for WebSocket revocation on API key deletionsequenceDiagram
actor Admin
participant ApiKeysRoute
participant Prisma
participant SocketServer
participant BotApiClient
Admin->>ApiKeysRoute: DELETE /bots/:botId/api-keys/:keyId
ApiKeysRoute->>ApiKeysRoute: parse botId and keyId
ApiKeysRoute->>SocketServer: getIO()
ApiKeysRoute->>SocketServer: namespace /bot-api
ApiKeysRoute->>SocketServer: room key_keyId disconnectSockets(force=true)
SocketServer-->>BotApiClient: disconnect WebSocket
ApiKeysRoute->>Prisma: botApiKey.deleteMany(botId, keyId)
Prisma-->>ApiKeysRoute: deletion result
ApiKeysRoute-->>Admin: 200 OK (deletion result)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Привет — я оставил несколько комментариев на более высоком уровне:
- Подумайте о том, чтобы перенести вызов
require('../../real-time/socketHandler')на верхний уровень модуля вместо размещения внутри обработчика маршрута, чтобы избежать повторного разрешения модуля при каждом запросе и сохранить единообразный стиль файла. - Возможно, стоит обработать случай, когда
getIO()ещё не инициализирован (или возвращает ложное значение), перед вызовом.of('/bot-api')и.disconnectSockets(true), чтобы избежать возможных ошибок времени выполнения при запуске или в тестовой среде.
Подсказка для AI-агентов
Пожалуйста, исправьте код с учётом комментариев из этого code review:
## Общие комментарии
- Подумайте о том, чтобы перенести вызов `require('../../real-time/socketHandler')` на верхний уровень модуля вместо размещения внутри обработчика маршрута, чтобы избежать повторного разрешения модуля при каждом запросе и сохранить единообразный стиль файла.
- Возможно, стоит обработать случай, когда `getIO()` ещё не инициализирован (или возвращает ложное значение), перед вызовом `.of('/bot-api')` и `.disconnectSockets(true)`, чтобы избежать возможных ошибок времени выполнения при запуске или в тестовой среде.Sourcery бесплатен для open source — если вам нравятся наши обзоры, пожалуйста, расскажите о нас ✨
Original comment in English
Hey - I've left some high level feedback:
- Consider moving the
require('../../real-time/socketHandler')call to the module top-level instead of inside the route handler to avoid re-resolving the module on every request and to keep the file style consistent. - It may be worth handling the case where
getIO()is not yet initialized (or returns a falsy value) before calling.of('/bot-api')and.disconnectSockets(true)to avoid potential runtime errors during startup or in test environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the `require('../../real-time/socketHandler')` call to the module top-level instead of inside the route handler to avoid re-resolving the module on every request and to keep the file style consistent.
- It may be worth handling the case where `getIO()` is not yet initialized (or returns a falsy value) before calling `.of('/bot-api')` and `.disconnectSockets(true)` to avoid potential runtime errors during startup or in test environments.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/src/api/routes/apiKeys.js:
- Around line 170-172: В блоке с вызовом
io.of("/bot-api").in(`key_${keyId}`).disconnectSockets(true) отсутствует
завершающая точка с запятой; добавьте точку с запятой после вызова
disconnectSockets(true) чтобы привести стиль к остальной части файла и избежать
несоответствий с конвенцией кода.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
backend/src/api/routes/apiKeys.jsbackend/src/real-time/botApi/index.js
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/api/routes/apiKeys.js (1)
backend/src/real-time/socketHandler.js (1)
io(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
backend/src/real-time/botApi/index.js (2)
15-31: Логика исправления выглядит корректно.Добавление сокета в комнату
key_${socket.keyId}— правильный подход для группировки соединений по API-ключу. Это позволяет эффективно отключать все соединения при удалении ключа черезio.of("/bot-api").in(key_${keyId}).disconnectSockets(true).
18-19: Замечание отклонено:socket.keyIdкорректно устанавливается middleware.Middleware
authenticateApiClientпроверяет подлинность ключа (линии 35-38) и только после успешной валидации устанавливаетsocket.keyId = matchedKey.id(линия 41). Вызовnext()происходит только после установки всех свойств сокета, поэтомуsocket.keyIdгарантированно содержит числовой идентификатор из базы данных и никогда не будетundefined. Код работает корректно.backend/src/api/routes/apiKeys.js (1)
166-172: Логика отключения сокетов правильно использует комнатуkey_<keyId>.В middleware
authenticateApiClientна строке 41 файлаbackend/src/real-time/botApi/middleware.jsустановленоsocket.keyId = matchedKey.id(идентификатор ключа из базы данных). В файлеbackend/src/real-time/botApi/index.jsна строке 19 сокет присоединяется к комнатеkey_${socket.keyId}сразу после успешной аутентификации. Это обеспечивает корректное отключение всех сокетов, связанных с конкретным API ключом, при выполненииio.of("/bot-api").in(``key_${keyId}``).disconnectSockets(true).
Comment on lines
+170
to
+172
| io.of("/bot-api") | ||
| .in(`key_${keyId}`) | ||
| .disconnectSockets(true) |
There was a problem hiding this comment.
Отсутствует точка с запятой.
В строке 172 отсутствует точка с запятой после .disconnectSockets(true). Хотя ASI (Automatic Semicolon Insertion) справится с этим, это несовместимо со стилем остального файла.
🔧 Предлагаемое исправление
io.of("/bot-api")
.in(`key_${keyId}`)
- .disconnectSockets(true)
+ .disconnectSockets(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| io.of("/bot-api") | |
| .in(`key_${keyId}`) | |
| .disconnectSockets(true) | |
| io.of("/bot-api") | |
| .in(`key_${keyId}`) | |
| .disconnectSockets(true); |
🤖 Prompt for AI Agents
In @backend/src/api/routes/apiKeys.js around lines 170 - 172, В блоке с вызовом
io.of("/bot-api").in(`key_${keyId}`).disconnectSockets(true) отсутствует
завершающая точка с запятой; добавьте точку с запятой после вызова
disconnectSockets(true) чтобы привести стиль к остальной части файла и избежать
несоответствий с конвенцией кода.
mmeerrkkaa
added
area: backend
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Member
|
thx |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
On API key deletion, established websocket connections continue to have access
Steps to reproduce
websockettabChanges
Summary by Sourcery
Исправления ошибок:
Original summary in English
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
Примечания к выпуску
✏️ Tip: You can customize this high-level summary in your review settings.