cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

perf: eliminate redundant GET after create/update and parallelize stats (#95) #112

Merged
cal merged 1 commits from ai/major-domo-v2#95 into main 2026-03-31 19:46:20 +00:00
Conversation 0 Commits 1 Files Changed 3 +328 -290
Claude commented 2026-03-20 18:09:27 +00:00
Collaborator
Copy Link

Closes #95

Summary

Eliminates redundant API round-trips after POST/PUT operations in custom_commands_service and help_commands_service, and parallelizes the get_statistics() dashboard calls.

Changes

services/custom_commands_service.py

  • create_command(): Return result.model_copy(update={'creator': creator}) from the self.create() response instead of a follow-up GET by_name. The creator object is already in scope, so no lookup needed.
  • update_command(): Return result.model_copy(update={'creator': command.creator}) from update_item_by_field() response instead of a follow-up GET by_name. The original command (and its creator) was already fetched at the start.
  • get_or_create_creator() (PUT case): Use model_copy() to update username/display_name locally instead of re-fetching after _update_creator_info.
  • get_or_create_creator() (POST case): Return CustomCommandCreator(**result) from the POST response directly instead of a follow-up GET creators.
  • get_statistics(): Replace 9 sequential await calls with asyncio.gather() — reduces latency from ~450–900ms sequential to ~50–100ms parallel.

services/help_commands_service.py

  • create_help(): Return result from self.create() directly instead of a follow-up GET by_name.
  • update_help(): Return self.model_class.from_api_data(result) from the PUT response directly instead of a follow-up GET by_name.

tests/test_services_help_commands.py

  • Updated test_update_help_success to mock client.put() returning a dict (the updated object) instead of True, matching the real API behavior.

What was NOT changed

  • draft_list_service.add_to_list(): The verification GET was left in place. This method returns Optional[List[DraftList]] which requires full Team + Player nested objects — these are only available from the GET response, not from the POST payload (which only contains IDs). Skipping the GET here would require either changing the return type or making an extra player/team lookup.
  • N+1 loops in get_popular_commands(), get_commands_needing_warning(), get_commands_eligible_for_deletion(): These are addressed in issue #89.

Test results

977 passed, 2 skipped

Closes #95 ## Summary Eliminates redundant API round-trips after POST/PUT operations in `custom_commands_service` and `help_commands_service`, and parallelizes the `get_statistics()` dashboard calls. ### Changes **`services/custom_commands_service.py`** - `create_command()`: Return `result.model_copy(update={'creator': creator})` from the `self.create()` response instead of a follow-up `GET by_name`. The creator object is already in scope, so no lookup needed. - `update_command()`: Return `result.model_copy(update={'creator': command.creator})` from `update_item_by_field()` response instead of a follow-up `GET by_name`. The original command (and its creator) was already fetched at the start. - `get_or_create_creator()` (PUT case): Use `model_copy()` to update username/display_name locally instead of re-fetching after `_update_creator_info`. - `get_or_create_creator()` (POST case): Return `CustomCommandCreator(**result)` from the POST response directly instead of a follow-up `GET creators`. - `get_statistics()`: Replace 9 sequential `await` calls with `asyncio.gather()` — reduces latency from ~450–900ms sequential to ~50–100ms parallel. **`services/help_commands_service.py`** - `create_help()`: Return `result` from `self.create()` directly instead of a follow-up `GET by_name`. - `update_help()`: Return `self.model_class.from_api_data(result)` from the PUT response directly instead of a follow-up `GET by_name`. **`tests/test_services_help_commands.py`** - Updated `test_update_help_success` to mock `client.put()` returning a dict (the updated object) instead of `True`, matching the real API behavior. ## What was NOT changed - `draft_list_service.add_to_list()`: The verification GET was left in place. This method returns `Optional[List[DraftList]]` which requires full `Team` + `Player` nested objects — these are only available from the GET response, not from the POST payload (which only contains IDs). Skipping the GET here would require either changing the return type or making an extra player/team lookup. - N+1 loops in `get_popular_commands()`, `get_commands_needing_warning()`, `get_commands_eligible_for_deletion()`: These are addressed in issue #89. ## Test results 977 passed, 2 skipped
Claude added 1 commit 2026-03-20 18:09:28 +00:00 
- custom_commands_service: return POST response directly from create_command()
  instead of a follow-up GET by_name
- custom_commands_service: return PUT response directly from update_command()
  instead of a follow-up GET by_name
- custom_commands_service: avoid GET after PUT in get_or_create_creator() by
  constructing updated creator from model_copy()
- custom_commands_service: return POST response directly from get_or_create_creator()
  creator creation instead of a follow-up GET
- custom_commands_service: parallelize all 9 sequential API calls in
  get_statistics() with asyncio.gather()
- help_commands_service: return POST response directly from create_help()
  instead of a follow-up GET by_name
- help_commands_service: return PUT response directly from update_help()
  instead of a follow-up GET by_name
- tests: update test_update_help_success to mock PUT returning dict data

Closes #95

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-20 18:15:52 +00:00
Claude reviewed 2026-03-20 18:20:50 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • services/custom_commands_service.py (modified)
  • services/help_commands_service.py (modified)
  • tests/test_services_help_commands.py (modified)

Findings

Correctness

create_command() — result.model_copy(update={"creator": creator}): Correct. self.create() returns a parsed CustomCommand instance; creator is in scope from the preceding get_or_create_creator() call. model_copy is the right Pydantic v2 idiom.

update_command() — result.model_copy(update={"creator": command.creator}): Correct. Only content and tags are being updated; the creator is unchanged from the command fetched at function start. Re-using command.creator is safe.

get_or_create_creator() PUT case — creator.model_copy(update={"username": ..., "display_name": ...}): Correct. Both updated fields are already in scope; no API round-trip needed.

get_or_create_creator() POST case — CustomCommandCreator(**result): Functionally correct if the API returns the created object as a dict (standard REST). Minor style note: the existing pattern in this codebase uses CustomCommandCreator(**creators[0]) on GET responses, so this is internally consistent. However, from_api_data(result) would be the more conventional pattern for service-layer construction — not a blocker.

get_statistics() — asyncio.gather(): Well-implemented. All 9 coroutines are truly independent (no data dependencies between them). total_uses = sum(...) and most_popular = ... are correctly placed after the gather, computed from the returned values. The week_ago timestamp is computed before the gather, which is correct.

update_help() — self.model_class.from_api_data(result): This is the change most worth flagging. The old code checked if not result: (truthy guard), then did a fresh GET. The new code assumes client.put() returns a full dict of the updated resource — not a boolean True. The PR correctly updates the test mock to match this assumption. If the production API actually returns True on a successful PUT, from_api_data(True) will raise a TypeError at runtime. The PR body states this reflects real API behavior, and the test change supports that. Just worth verifying against the API spec (sba.manticorum.com/api/openapi.json) before shipping.

create_help() — return result: Correct. self.create() returns Optional[HelpCommand]; after the if not result: raise guard, result is guaranteed to be a HelpCommand. Clean.

Security

No issues. No new external inputs, no injection vectors, no credential exposure.

Style & Conventions

  • Follows project patterns: model_copy, from_api_data, asyncio.gather all match established conventions in this codebase.
  • Linter reformatting (single→double quotes, trailing commas) is cosmetic and consistent with previous PRs.
  • No lazy imports added mid-file (CLAUDE.md compliance: ✓).
  • get_client() called within service internals only — not from commands/tasks/views (CLAUDE.md compliance: ✓).

Suggestions

  • Test coverage gap: No new tests for the custom_commands_service.py changes (four modified paths: create_command, update_command, get_or_create_creator PUT case, get_or_create_creator POST case). Existing tests still pass (977 total), but they don't isolate and verify the new model_copy path. A test that asserts get_command_by_name is no longer called after create/update would provide stronger regression protection.
  • get_statistics() error isolation: If any one of the 9 gathered calls raises, the exception propagates immediately (default asyncio.gather behavior). The old sequential code had the same property. No regression, but worth noting — return_exceptions=True would allow partial results if that's ever desired.

Verdict: COMMENT

All five functional changes are correct and the performance improvement in get_statistics() is solid. The only actionable item is verifying the update_help() assumption that client.put() returns the updated resource dict — if that's confirmed, this is a clean approve.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/custom_commands_service.py` (modified) - `services/help_commands_service.py` (modified) - `tests/test_services_help_commands.py` (modified) ### Findings #### Correctness **`create_command()` — `result.model_copy(update={"creator": creator})`**: Correct. `self.create()` returns a parsed `CustomCommand` instance; `creator` is in scope from the preceding `get_or_create_creator()` call. `model_copy` is the right Pydantic v2 idiom. **`update_command()` — `result.model_copy(update={"creator": command.creator})`**: Correct. Only `content` and `tags` are being updated; the creator is unchanged from the `command` fetched at function start. Re-using `command.creator` is safe. **`get_or_create_creator()` PUT case — `creator.model_copy(update={"username": ..., "display_name": ...})`**: Correct. Both updated fields are already in scope; no API round-trip needed. **`get_or_create_creator()` POST case — `CustomCommandCreator(**result)`**: Functionally correct if the API returns the created object as a dict (standard REST). Minor style note: the existing pattern in this codebase uses `CustomCommandCreator(**creators[0])` on GET responses, so this is internally consistent. However, `from_api_data(result)` would be the more conventional pattern for service-layer construction — not a blocker. **`get_statistics()` — `asyncio.gather()`**: Well-implemented. All 9 coroutines are truly independent (no data dependencies between them). `total_uses = sum(...)` and `most_popular = ...` are correctly placed *after* the gather, computed from the returned values. The `week_ago` timestamp is computed before the gather, which is correct. **`update_help()` — `self.model_class.from_api_data(result)`**: This is the change most worth flagging. The old code checked `if not result:` (truthy guard), then did a fresh GET. The new code assumes `client.put()` returns a full dict of the updated resource — not a boolean `True`. The PR correctly updates the test mock to match this assumption. **If the production API actually returns `True` on a successful PUT, `from_api_data(True)` will raise a `TypeError` at runtime.** The PR body states this reflects real API behavior, and the test change supports that. Just worth verifying against the API spec (`sba.manticorum.com/api/openapi.json`) before shipping. **`create_help()` — `return result`**: Correct. `self.create()` returns `Optional[HelpCommand]`; after the `if not result: raise` guard, `result` is guaranteed to be a `HelpCommand`. Clean. #### Security No issues. No new external inputs, no injection vectors, no credential exposure. #### Style & Conventions - Follows project patterns: `model_copy`, `from_api_data`, `asyncio.gather` all match established conventions in this codebase. - Linter reformatting (single→double quotes, trailing commas) is cosmetic and consistent with previous PRs. - No lazy imports added mid-file (CLAUDE.md compliance: ✓). - `get_client()` called within service internals only — not from commands/tasks/views (CLAUDE.md compliance: ✓). #### Suggestions - **Test coverage gap**: No new tests for the `custom_commands_service.py` changes (four modified paths: `create_command`, `update_command`, `get_or_create_creator` PUT case, `get_or_create_creator` POST case). Existing tests still pass (977 total), but they don't isolate and verify the new `model_copy` path. A test that asserts `get_command_by_name` is no longer called after create/update would provide stronger regression protection. - **`get_statistics()` error isolation**: If any one of the 9 gathered calls raises, the exception propagates immediately (default `asyncio.gather` behavior). The old sequential code had the same property. No regression, but worth noting — `return_exceptions=True` would allow partial results if that's ever desired. ### Verdict: COMMENT All five functional changes are correct and the performance improvement in `get_statistics()` is solid. The only actionable item is verifying the `update_help()` assumption that `client.put()` returns the updated resource dict — if that's confirmed, this is a clean approve. --- *Automated review by Claude PR Reviewer*
cal removed the
ai-reviewing
 label 2026-03-23 15:32:35 +00:00
Claude added the
ai-reviewing
 label 2026-03-23 15:48:53 +00:00
Claude reviewed 2026-03-23 15:53:12 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • services/custom_commands_service.py (modified)
  • services/help_commands_service.py (modified)
  • tests/test_services_help_commands.py (modified)

Findings

Correctness

create_command() → result.model_copy(update={"creator": creator})
Verified correct. BaseService.create() returns Optional[T] and calls self.model_class.from_api_data(response) internally (line 254 of base_service.py), so result is a full CustomCommand. The creator object is already in scope and correctly attached via model_copy. The guard if not result: raise BotException(...) prevents a None dereference. ✓

update_command() → result.model_copy(update={"creator": command.creator})
Verified correct. update_item_by_field() calls BaseService.update() which calls from_api_data(response) on the PUT response (line 301 of base_service.py) and returns a CustomCommand. The PUT response won't include the full nested creator object, but command.creator was fetched at the start of update_command() and is accurate. ✓

get_or_create_creator() PUT case → model_copy instead of refetch
Correct. Only username and display_name change, both are in-scope. The model copy avoids a round-trip where all updated values are already known locally. ✓

get_or_create_creator() POST case → CustomCommandCreator(**result)
Functionally correct assuming the API's POST /custom_commands/creators response includes all required fields (id, created_at, discord_id, username). SBABaseModel uses Pydantic v2 with validate_assignment=True — Pydantic v2's default extra="ignore" means extra fields are safely dropped, and type coercion (e.g. ISO string → datetime) happens in __init__. The 977 test run provides confidence the API does return these fields.

One note: from_api_data() would be the more idiomatic pattern here (consistent with the rest of the codebase), but **result works because Pydantic handles coercion at construction time.

get_statistics() → asyncio.gather()
9 coroutines, 9 result variables, order matches exactly. all_commands is a List[CustomCommand], popular_commands is a List[CustomCommand]. Post-gather lines total_uses = sum(...) and most_popular = popular_commands[0] if ... are correct. ✓

create_help() → return result
result is a HelpCommand from BaseService.create() (returns Optional[T], guarded before this point). Returning it directly eliminates one GET. ✓

update_help() → return self.model_class.from_api_data(result)
result is the raw dict from client.put(). from_api_data(dict) is the correct conversion path. Test mock correctly updated to return a dict instead of True. ✓

Security

  • No security issues found. No user-controlled data flows into API path construction unsanitized. No new auth bypasses or data exposure.

Style & Conventions

  • All changes follow the service layer patterns documented in services/CLAUDE.md.
  • get_or_create_creator() POST case uses CustomCommandCreator(**result) rather than CustomCommandCreator.from_api_data(result). Both work; from_api_data is slightly more idiomatic and consistent with BaseService.update()/create() internals, but this is not a blocker.
  • Cosmetic reformatting (single→double quotes, trailing commas) in help_commands_service.py and test file is ruff-enforced and consistent.

Suggestions

  1. Test coverage for custom_commands_service.py changes: No tests were added or updated for create_command(), update_command(), the two get_or_create_creator() branches, or get_statistics(). The change is relatively low-risk (removes a follow-up GET), but explicit tests for the model_copy paths would catch any future BaseService.create() / update() return type changes. Worth a follow-up issue.

  2. CustomCommandCreator(**result) vs from_api_data(result): Minor consistency point only. from_api_data is how the rest of the service layer (including BaseService.create/update) constructs models from API dicts.

  3. Pre-existing (not in scope): update_item_by_field() internally does a GET before the PUT (calls get_by_field to resolve the ID). This PR removes the POST-update GET correctly, but the internal GET inside update_item_by_field remains. A more complete optimization for update_command() would pass the already-known command.id directly to BaseService.update() — but that's a separate refactor.

Verdict: COMMENT

Implementation is correct and safe. All model_copy() and from_api_data() uses are verified against the base class return types. The asyncio.gather() parallelization is clean and order-preserving. Minor test coverage gap in custom_commands_service.py worth a follow-up but not a blocker.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/custom_commands_service.py` (modified) - `services/help_commands_service.py` (modified) - `tests/test_services_help_commands.py` (modified) ### Findings #### Correctness **`create_command()` → `result.model_copy(update={"creator": creator})`** Verified correct. `BaseService.create()` returns `Optional[T]` and calls `self.model_class.from_api_data(response)` internally (line 254 of `base_service.py`), so `result` is a full `CustomCommand`. The `creator` object is already in scope and correctly attached via `model_copy`. The guard `if not result: raise BotException(...)` prevents a `None` dereference. ✓ **`update_command()` → `result.model_copy(update={"creator": command.creator})`** Verified correct. `update_item_by_field()` calls `BaseService.update()` which calls `from_api_data(response)` on the PUT response (line 301 of `base_service.py`) and returns a `CustomCommand`. The PUT response won't include the full nested `creator` object, but `command.creator` was fetched at the start of `update_command()` and is accurate. ✓ **`get_or_create_creator()` PUT case → `model_copy` instead of refetch** Correct. Only `username` and `display_name` change, both are in-scope. The model copy avoids a round-trip where all updated values are already known locally. ✓ **`get_or_create_creator()` POST case → `CustomCommandCreator(**result)`** Functionally correct assuming the API's POST `/custom_commands/creators` response includes all required fields (`id`, `created_at`, `discord_id`, `username`). `SBABaseModel` uses Pydantic v2 with `validate_assignment=True` — Pydantic v2's default `extra="ignore"` means extra fields are safely dropped, and type coercion (e.g. ISO string → datetime) happens in `__init__`. The 977 test run provides confidence the API does return these fields. One note: `from_api_data()` would be the more idiomatic pattern here (consistent with the rest of the codebase), but `**result` works because Pydantic handles coercion at construction time. **`get_statistics()` → `asyncio.gather()`** 9 coroutines, 9 result variables, order matches exactly. `all_commands` is a `List[CustomCommand]`, `popular_commands` is a `List[CustomCommand]`. Post-gather lines `total_uses = sum(...)` and `most_popular = popular_commands[0] if ...` are correct. ✓ **`create_help()` → `return result`** `result` is a `HelpCommand` from `BaseService.create()` (returns `Optional[T]`, guarded before this point). Returning it directly eliminates one GET. ✓ **`update_help()` → `return self.model_class.from_api_data(result)`** `result` is the raw dict from `client.put()`. `from_api_data(dict)` is the correct conversion path. Test mock correctly updated to return a dict instead of `True`. ✓ #### Security - No security issues found. No user-controlled data flows into API path construction unsanitized. No new auth bypasses or data exposure. #### Style & Conventions - All changes follow the service layer patterns documented in `services/CLAUDE.md`. - `get_or_create_creator()` POST case uses `CustomCommandCreator(**result)` rather than `CustomCommandCreator.from_api_data(result)`. Both work; `from_api_data` is slightly more idiomatic and consistent with `BaseService.update()`/`create()` internals, but this is not a blocker. - Cosmetic reformatting (single→double quotes, trailing commas) in `help_commands_service.py` and test file is ruff-enforced and consistent. #### Suggestions 1. **Test coverage for `custom_commands_service.py` changes**: No tests were added or updated for `create_command()`, `update_command()`, the two `get_or_create_creator()` branches, or `get_statistics()`. The change is relatively low-risk (removes a follow-up GET), but explicit tests for the model_copy paths would catch any future `BaseService.create()` / `update()` return type changes. Worth a follow-up issue. 2. **`CustomCommandCreator(**result)` vs `from_api_data(result)`**: Minor consistency point only. `from_api_data` is how the rest of the service layer (including `BaseService.create/update`) constructs models from API dicts. 3. **Pre-existing (not in scope)**: `update_item_by_field()` internally does a GET before the PUT (calls `get_by_field` to resolve the ID). This PR removes the POST-update GET correctly, but the internal GET inside `update_item_by_field` remains. A more complete optimization for `update_command()` would pass the already-known `command.id` directly to `BaseService.update()` — but that's a separate refactor. ### Verdict: COMMENT Implementation is correct and safe. All `model_copy()` and `from_api_data()` uses are verified against the base class return types. The `asyncio.gather()` parallelization is clean and order-preserving. Minor test coverage gap in `custom_commands_service.py` worth a follow-up but not a blocker. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-23 15:53:40 +00:00
cal force-pushed ai/major-domo-v2#95 from dea8b39391 to 95010bfd5d 2026-03-31 19:45:48 +00:00 Compare
cal merged commit 27a272b813 into main 2026-03-31 19:46:20 +00:00
cal deleted branch ai/major-domo-v2#95 2026-03-31 19:46:21 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
in-next-release

Fix included in the next-release branch, pending merge to main

  
status/in-progress

Work is actively in progress

  
status/pr-open

A pull request has been opened for this issue

No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
in-next-release
status/in-progress
status/pr-open
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-v2#112
No description provided.
Delete Branch "ai/major-domo-v2#95"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?