Open
Conversation
…o feature/capr-47-implement-db
…o feature/capr-47-implement-db
…ped endpoints, robust JSON/error handling, and updated database tests/lint fixes
…o feature/capr-47-implement-db
…tions, users, redirects
simtiaz5
requested review from
Copilot and
shamikkarkhanis
and removed request for
Copilot
Contributor
Reviewer's GuideIntroduces a new asynchronous HTTP-based backend client abstraction (misnamed as a database pool) for the Discord bot, wires it into bot startup/shutdown via configuration-driven environment selection, and adds comprehensive tests for the client and configuration behavior. Sequence diagram for bot startup and backend client lifecyclesequenceDiagram
actor DiscordBot
participant Bot as Bot.setup_hook
participant Pool as init_database_pool
participant Client as BackendAPIClient
participant HTTPX as httpx.AsyncClient
DiscordBot->>Bot: setup_hook()
activate Bot
Bot->>Pool: init_database_pool(database_url, config)
activate Pool
alt client not yet initialized
Pool->>Client: BackendAPIClient(database_url, config)
activate Client
Client->>HTTPX: create AsyncClient(base_url=/v1,...)
HTTPX-->>Client: AsyncClient instance
Client-->>Pool: BackendAPIClient instance
deactivate Client
Pool->>Client: start()
activate Client
Client-->>Pool: mark _started = True
deactivate Client
Pool-->>Bot: global BackendAPIClient
else client already initialized
Pool-->>Bot: existing BackendAPIClient
end
deactivate Pool
Bot-->>DiscordBot: bot ready with backend client
deactivate Bot
Note over DiscordBot,Bot: On shutdown
DiscordBot->>Bot: close()
activate Bot
Bot->>Pool: close_database_pool()
activate Pool
alt client exists
Pool->>Client: close()
activate Client
Client->>HTTPX: aclose()
HTTPX-->>Client: closed
Client-->>Pool: mark _started = False
deactivate Client
Pool-->>Bot: client cleared
else no client
Pool-->>Bot: no-op
end
deactivate Pool
Bot-->>DiscordBot: super().close()
deactivate Bot
Class diagram for new backend API client and global poolclassDiagram
class BackendConfigurationError {
}
class BackendClientNotInitializedError {
}
class BackendAPIError {
+int status_code
+dict~str, Any~ payload
+__init__(message: str, status_code: int, payload: dict~str, Any~)
}
BackendConfigurationError --|> RuntimeError
BackendClientNotInitializedError --|> RuntimeError
BackendAPIError --|> RuntimeError
class BackendClientConfig {
<<dataclass>>
+str bot_token
+str auth_cookie
+float timeout_seconds
+int max_connections
+int max_keepalive_connections
}
class BackendAPIClient {
-AsyncClient _client
-bool _started
+BackendAPIClient(base_url: str, config: BackendClientConfig)
+start() async void
+close() async void
+__aenter__() async BackendAPIClient
+__aexit__() async void
+is_started bool
+auth_me() async UserAuthResponse
+auth_refresh() async AuthResponse
+auth_logout() async void
+auth_google_redirect() async void
+auth_google_callback(code: str, state: str) async void
+auth_microsoft_redirect() async void
+auth_microsoft_callback(code: str, state: str) async void
+bot_me() async BotTokenResponse
+list_bot_tokens() async list~BotTokenResponse~
+create_bot_token(data: CreateBotTokenRequest) async BotTokenResponse
+revoke_bot_token(token_id: str) async void
+list_events(limit: int, offset: int) async list~EventResponse~
+list_events_by_organization(organization_id: str, limit: int, offset: int) async list~EventResponse~
+get_event(event_id: str) async EventResponse
+create_event(data: CreateEventRequest) async EventResponse
+update_event(event_id: str, data: UpdateEventRequest) async EventResponse
+delete_event(event_id: str) async void
+register_event(event_id: str, data: RegisterEventRequest) async void
+unregister_event(event_id: str, uid: str) async void
+list_event_registrations(event_id: str) async list~EventRegistrationResponse~
+list_organizations(limit: int, offset: int) async list~OrganizationResponse~
+get_organization(organization_id: str) async OrganizationResponse
+create_organization(data: CreateOrganizationRequest) async OrganizationResponse
+update_organization(organization_id: str, data: UpdateOrganizationRequest) async OrganizationResponse
+delete_organization(organization_id: str) async void
+list_organization_events(organization_id: str, limit: int, offset: int) async list~EventResponse~
+list_organization_members(organization_id: str) async list~OrganizationMemberResponse~
+add_organization_member(organization_id: str, data: AddMemberRequest) async void
+remove_organization_member(organization_id: str, user_id: str) async void
+get_user(user_id: str) async UserResponse
+update_user(user_id: str, data: UpdateUserRequest) async UserResponse
+delete_user(user_id: str) async void
+list_user_events(user_id: str) async list~EventResponse~
+list_user_organizations(user_id: str) async list~OrganizationResponse~
-_request(method: str, path: str, params: dict~str, Any~, json_body: object, expected_statuses: set~int~) async object
-_request_without_response_body(method: str, path: str, params: dict~str, Any~, expected_statuses: set~int~) async void
-_ensure_started() void
}
BackendAPIClient --> BackendClientConfig : uses
BackendAPIClient --> BackendAPIError : throws
BackendAPIClient --> BackendClientNotInitializedError : throws
BackendAPIClient --> BackendConfigurationError : throws
class _ClientState {
+BackendAPIClient client
}
_ClientState --> BackendAPIClient
class GlobalPoolFunctions {
<<module>>
+init_database_pool(database_url: str, config: BackendClientConfig) async BackendAPIClient
+get_database_pool() BackendAPIClient
+close_database_pool() async void
}
GlobalPoolFunctions ..> _ClientState : manages
GlobalPoolFunctions ..> BackendAPIClient : creates
class Settings {
+str backend_environment
+str backend_api_dev_base_url
+str backend_api_prod_base_url
+str backend_api_bot_token
+str backend_api_auth_cookie
+float backend_api_timeout_seconds
+int backend_api_max_connections
+int backend_api_max_keepalive_connections
+backend_api_base_url str
}
Settings ..> BackendAPIClient : configures via init_database_pool
class DiscordBot {
+setup_hook() async void
+close() async void
}
DiscordBot ..> GlobalPoolFunctions : calls init_database_pool
DiscordBot ..> GlobalPoolFunctions : calls close_database_pool
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.
Hey - I've found 3 issues, and left some high level feedback:
- The naming of the global helpers and module (
database.py,init_database_pool,get_database_pool,close_database_pool) is misleading now that this is an HTTP backend API client rather than a real DB pool; consider renaming these to reflect the backend API client to avoid confusion for future contributors. - Within
BackendAPIClient, thestart()method only flips an internal flag without performing any I/O, yet it is required before any request; you might simplify usage by removing the started-state check entirely or lazily initializing on first request so callers don’t need to remember to callstart().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The naming of the global helpers and module (`database.py`, `init_database_pool`, `get_database_pool`, `close_database_pool`) is misleading now that this is an HTTP backend API client rather than a real DB pool; consider renaming these to reflect the backend API client to avoid confusion for future contributors.
- Within `BackendAPIClient`, the `start()` method only flips an internal flag without performing any I/O, yet it is required before any request; you might simplify usage by removing the started-state check entirely or lazily initializing on first request so callers don’t need to remember to call `start()`.
## Individual Comments
### Comment 1
<location path="capy_discord/database.py" line_range="253-247" />
<code_context>
self.tree.on_error = self.on_tree_error # type: ignore
await self.load_extensions()
+ async def close(self) -> None:
+ """Close bot resources before shutting down."""
+ await close_database_pool()
</code_context>
<issue_to_address>
**suggestion:** Consider always closing the underlying HTTP client, even if `start()` was never called.
Right now `close()` is a no-op if `_started` is `False`, so constructing `BackendAPIClient` and calling `close()` without `start()` leaves the underlying `httpx.AsyncClient` open. This is surprising for callers and risks connection leaks in future usages. You could instead always call `self._client.aclose()` in `close()` and then set `_started = False`, or introduce a separate flag to track whether the HTTP client has already been closed.
</issue_to_address>
### Comment 2
<location path="tests/capy_discord/test_config.py" line_range="27" />
<code_context>
+ assert settings.backend_api_base_url == "https://api.example.com"
+
+
+def test_backend_api_base_url_requires_prod_url_when_prod_environment():
+ settings = Settings(
+ backend_environment="prod",
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test for missing dev base URL when environment is `dev`.
The `prod` path is covered when `backend_api_prod_base_url` is missing, but the analogous `dev` path is not. Please add a test with `backend_environment="dev"` and an empty `backend_api_dev_base_url` that asserts `backend_api_base_url` raises `ValueError` with the expected message, so validation is exercised for both environments.
Suggested implementation:
```python
assert settings.backend_api_base_url == "https://api.example.com"
def test_backend_api_base_url_requires_dev_url_when_dev_environment():
with pytest.raises(ValueError, match="backend_api_dev_base_url"):
Settings(
backend_environment="dev",
backend_api_dev_base_url="",
backend_api_prod_base_url="https://api.example.com",
)
def test_backend_api_base_url_requires_prod_url_when_prod_environment():
```
1. Ensure `pytest` is imported at the top of `tests/capy_discord/test_config.py` (e.g., `import pytest`) if it is not already.
2. If the actual validation error message for the dev URL is more specific, update the `match="backend_api_dev_base_url"` argument to match the exact expected message format used elsewhere (e.g., you may mirror the pattern used in `test_backend_api_base_url_requires_prod_url_when_prod_environment`).
</issue_to_address>
### Comment 3
<location path="tests/capy_discord/test_database.py" line_range="7" />
<code_context>
+import httpx
+import pytest
+
+from capy_discord.database import (
+ BackendAPIClient,
+ BackendAPIError,
</code_context>
<issue_to_address>
**suggestion (testing):** Factor out repeated pool setup/teardown into a fixture to improve readability and isolation.
Multiple tests manually call `await close_database_pool()` at the start and end. To avoid duplication and missed cleanup, introduce an async pytest fixture that closes the pool before and after each test (either `autouse` or explicitly used), so tests can focus solely on their behavior while the client lifecycle is handled centrally.
Suggested implementation:
```python
from capy_discord.database import (
BackendAPIClient,
BackendAPIError,
BackendClientConfig,
BackendClientNotInitializedError,
BackendConfigurationError,
HTTP_STATUS_BAD_REQUEST,
HTTP_STATUS_CREATED,
HTTP_STATUS_NOT_FOUND,
_normalize_api_base_url,
close_database_pool,
get_database_pool,
init_database_pool,
)
@pytest.fixture(autouse=True)
async def database_pool_cleanup():
"""
Ensure the database pool is closed before and after each test.
Some tests explicitly initialize the pool and expect to start from a clean
state. This fixture guarantees that any existing pool is torn down before
the test runs and again after it completes.
"""
await close_database_pool()
try:
yield
finally:
await close_database_pool()
```
1. Remove any manual `await close_database_pool()` calls at the beginning and end of tests in this file; the `database_pool_cleanup` fixture now guarantees cleanup for every test.
2. If any test relies on the pool *remaining* open across tests, adjust that test to manage its own lifecycle explicitly (or disable `autouse` for this fixture and opt in only where needed).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| max_keepalive_connections=client_config.max_keepalive_connections, | ||
| ), | ||
| ) | ||
| self._started = False |
Contributor
There was a problem hiding this comment.
suggestion: Consider always closing the underlying HTTP client, even if start() was never called.
Right now close() is a no-op if _started is False, so constructing BackendAPIClient and calling close() without start() leaves the underlying httpx.AsyncClient open. This is surprising for callers and risks connection leaks in future usages. You could instead always call self._client.aclose() in close() and then set _started = False, or introduce a separate flag to track whether the HTTP client has already been closed.
| assert settings.backend_api_base_url == "https://api.example.com" | ||
|
|
||
|
|
||
| def test_backend_api_base_url_requires_prod_url_when_prod_environment(): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a complementary test for missing dev base URL when environment is dev.
The prod path is covered when backend_api_prod_base_url is missing, but the analogous dev path is not. Please add a test with backend_environment="dev" and an empty backend_api_dev_base_url that asserts backend_api_base_url raises ValueError with the expected message, so validation is exercised for both environments.
Suggested implementation:
assert settings.backend_api_base_url == "https://api.example.com"
def test_backend_api_base_url_requires_dev_url_when_dev_environment():
with pytest.raises(ValueError, match="backend_api_dev_base_url"):
Settings(
backend_environment="dev",
backend_api_dev_base_url="",
backend_api_prod_base_url="https://api.example.com",
)
def test_backend_api_base_url_requires_prod_url_when_prod_environment():- Ensure
pytestis imported at the top oftests/capy_discord/test_config.py(e.g.,import pytest) if it is not already. - If the actual validation error message for the dev URL is more specific, update the
match="backend_api_dev_base_url"argument to match the exact expected message format used elsewhere (e.g., you may mirror the pattern used intest_backend_api_base_url_requires_prod_url_when_prod_environment).
| import httpx | ||
| import pytest | ||
|
|
||
| from capy_discord.database import ( |
Contributor
There was a problem hiding this comment.
suggestion (testing): Factor out repeated pool setup/teardown into a fixture to improve readability and isolation.
Multiple tests manually call await close_database_pool() at the start and end. To avoid duplication and missed cleanup, introduce an async pytest fixture that closes the pool before and after each test (either autouse or explicitly used), so tests can focus solely on their behavior while the client lifecycle is handled centrally.
Suggested implementation:
from capy_discord.database import (
BackendAPIClient,
BackendAPIError,
BackendClientConfig,
BackendClientNotInitializedError,
BackendConfigurationError,
HTTP_STATUS_BAD_REQUEST,
HTTP_STATUS_CREATED,
HTTP_STATUS_NOT_FOUND,
_normalize_api_base_url,
close_database_pool,
get_database_pool,
init_database_pool,
)
@pytest.fixture(autouse=True)
async def database_pool_cleanup():
"""
Ensure the database pool is closed before and after each test.
Some tests explicitly initialize the pool and expect to start from a clean
state. This fixture guarantees that any existing pool is torn down before
the test runs and again after it completes.
"""
await close_database_pool()
try:
yield
finally:
await close_database_pool()- Remove any manual
await close_database_pool()calls at the beginning and end of tests in this file; thedatabase_pool_cleanupfixture now guarantees cleanup for every test. - If any test relies on the pool remaining open across tests, adjust that test to manage its own lifecycle explicitly (or disable
autousefor this fixture and opt in only where needed).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary by Sourcery
Integrate a configurable backend HTTP API client into the Discord bot and wire it into the bot lifecycle.
New Features:
Build:
Tests: