fix: return 404 when deleting non-existent schedule#952
Open
Conversation
Validate schedule existence by calling get_schedules before delete_schedule. Return a 404 error with a descriptive message if the requested schedule ID is not found on the device. Closes #723 Signed-off-by: Tomer Figenblat <tomer@figenblat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer's GuideEnsure delete_schedule validates schedule existence via get_schedules before deletion, returning a 404 error with a descriptive JSON body when the schedule is not found, and add/update tests to cover both successful and failure paths with proper mocking of get_schedules. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe delete_schedule endpoint now validates that a schedule exists before attempting deletion. When a non-existent schedule ID is provided, the endpoint returns a 404 error instead of a 200 response. Tests are updated to verify this pre-check behavior and cover the 404 case. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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
Test Results69 tests 69 ✅ 1s ⏱️ Results for commit 7c0dfc6. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #952 +/- ##
==========================================
+ Coverage 96.02% 96.07% +0.04%
==========================================
Files 1 1
Lines 327 331 +4
==========================================
+ Hits 314 318 +4
Misses 13 13 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
delete_schedule, consider defensive handling for cases whereschedules_response.schedulesisNoneor missing to avoid potentialTypeErrorwhen iterating. - The test setup for stubbing
get_scheduleswith a matching schedule ID is repeated in multiple tests; extracting a small helper or fixture could reduce duplication and make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `delete_schedule`, consider defensive handling for cases where `schedules_response.schedules` is `None` or missing to avoid potential `TypeError` when iterating.
- The test setup for stubbing `get_schedules` with a matching schedule ID is repeated in multiple tests; extracting a small helper or fixture could reduce duplication and make the intent clearer.
## Individual Comments
### Comment 1
<location> `app/webapp.py:263-264` </location>
<code_context>
async with SwitcherApi(
device_type, request.query[KEY_IP], request.query[KEY_ID], login_key
) as swapi:
+ schedules_response = await swapi.get_schedules()
+ existing_ids = [s.schedule_id for s in schedules_response.schedules]
+ if schedule_id not in existing_ids:
+ return web.json_response(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid extra round-trip by letting `delete_schedule` surface non-existence errors directly.
This adds a network call on every delete and still has a race (the schedule might be removed between `get_schedules` and `delete_schedule`). If the API can distinguish not-found, it’s better to call `delete_schedule` directly, catch that specific error, and map it to a 404 instead of pre-validating with `get_schedules`.
Suggested implementation:
```python
async with SwitcherApi(
device_type, request.query[KEY_IP], request.query[KEY_ID], login_key
) as swapi:
try:
delete_result = await swapi.delete_schedule(schedule_id)
except SwitcherScheduleNotFoundError:
return web.json_response(
{"error": f"schedule {schedule_id} does not exist"},
status=404,
)
return web.json_response(_serialize_object(delete_result))
```
1. Add an import for the not-found exception type at the top of `app/webapp.py`, matching the actual API surface, for example:
`from switcher_api import SwitcherScheduleNotFoundError`
or whatever specific exception the `SwitcherApi.delete_schedule` coroutine raises when a schedule does not exist.
2. If the API uses a more generic error type (e.g. `SwitcherApiError`) with an error code or message indicating not-found, replace `SwitcherScheduleNotFoundError` in the `except` block with the correct exception and add the appropriate conditional logic inside the `except` to distinguish not-found from other errors. Map only the not-found case to a 404, and re-raise or handle other errors consistently with the rest of the file.
</issue_to_address>
### Comment 2
<location> `app/tests/test_web_app.py:514-523` </location>
<code_context>
api_client,
api_uri,
):
+ # stub get_schedules to return a schedule with matching id
+ schedule_mock = Mock()
+ schedule_mock.schedule_id = "5"
+ schedules_response = Mock()
+ schedules_response.schedules = [schedule_mock]
+ api_get_schedules.return_value = schedules_response
# stub api_delete_schedule to return mock response
api_delete_schedule.return_value = response_mock
# send delete request for delete_schedule endpoint
response = await api_client.delete(api_uri, json={webapp.KEY_SCHEDULE: "5"})
# verify mocks calling
api_connect.assert_called_once()
+ api_get_schedules.assert_called_once()
api_delete_schedule.assert_called_once_with("5")
response_serializer.assert_called_once_with(response_mock)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for when `get_schedules` itself fails (e.g. raises) to document and protect the new pre-check behavior.
Since `delete_schedule` now always calls `get_schedules` first, a failure in `get_schedules` will change the endpoint’s behavior (likely returning a 500). Please add a test that patches `SwitcherApi.get_schedules` with `side_effect=Exception("…")`, and asserts the resulting status code/body and that `delete_schedule` is not called. You can mirror `test_errorneous_delete_schedule_delete_request`, but raise from `get_schedules` instead of `delete_schedule`.
Suggested implementation:
```python
response_serializer.assert_called_once_with(response_mock)
api_disconnect.assert_called_once()
assert_that(await response.json()).contains_entry(fake_serialized_data)
@patch("aioswitcher.api.SwitcherApi.delete_schedule")
@patch("aioswitcher.api.SwitcherApi.get_schedules")
async def test_errorneous_delete_schedule_get_schedules_failure(
api_get_schedules,
api_delete_schedule,
response_serializer,
api_client,
api_uri,
):
# stub get_schedules to raise an exception
api_get_schedules.side_effect = Exception("get_schedules failure")
# send delete request for delete_schedule endpoint
response = await api_client.delete(api_uri, json={webapp.KEY_SCHEDULE: "5"})
# verify mocks calling
api_connect.assert_called_once()
api_get_schedules.assert_called_once()
api_delete_schedule.assert_not_called()
api_disconnect.assert_called_once()
# assert that the failure in get_schedules results in an error response
assert response.status == 500
@mark.parametrize(
"api_uri",
[
(delete_schedule_uri),
(delete_schedule_uri2),
],
```
1. If your test suite uses a specific helper or constant for asserting error responses (for example, checking a JSON body like `fake_error_data` or using a custom assertion helper), mirror whatever is done in `test_errorneous_delete_schedule_delete_request` inside `test_errorneous_delete_schedule_get_schedules_failure` instead of the simple `assert response.status == 500`.
2. Ensure that `api_connect` and `api_disconnect` are available in this test scope as in the surrounding tests (likely via fixtures or additional `@patch` decorators); if they are provided differently, adjust the assertions to match the existing pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
auto-me-bot
bot
added
status: review started
Cover the case where get_schedules raises an exception during the pre-deletion validation, ensuring a 500 is returned and delete_schedule is never called. Signed-off-by: Tomer Figenblat <tomer@figenblat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
Hey @dmatik , would you mind taking a look? |
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
1 participant
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
get_schedulesfirst.404with{"error": "schedule <id> does not exist"}when the requested schedule ID is not found on the device.get_schedules.Closes #723
Validation
Summary by Sourcery
Return a 404 error when attempting to delete a schedule that does not exist on the device.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests