Use status 404 in more places when appropriate #5480
Conversation
|
Yea the breaking change label is required unfortunately. I think for most clients the impact will be zero since its just one 4xx error to another. However the CLI is going to break with this until it is updated as it does currently assume Supervisor only returns a very small set of status codes and 404 is not one of them. |
📝 WalkthroughWalkthroughThe changes in this pull request introduce a new exception class, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Service
Client->>API: Request entity (e.g., add-on)
API->>Service: Check existence
alt Entity not found
Service-->>API: Return not found
API-->>Client: Raise APINotFound
else Entity found
Service-->>API: Return entity
API-->>Client: Return entity data
end
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tests/api/test_services.py (1)
7-16: LGTM! Consider enhancing the test description.The test is well structured using parametrization to cover multiple HTTP methods. The assertions are comprehensive, checking both the status code and error message.
Consider updating the test description to be more specific:
- """Test service not found error.""" + """Test that requests to non-existent services return 404 status."""tests/api/test_docker.py (1)
18-23: Consider parametrizing the registry not found test.For consistency with the services test and better coverage, consider testing other HTTP methods that could access a non-existent registry.
Here's a suggested implementation:
+@pytest.mark.parametrize( + ("method", "url"), + [ + ("get", "/docker/registries/bad"), + ("delete", "/docker/registries/bad") + ], +) -async def test_registry_not_found(api_client: TestClient): +async def test_registry_not_found(api_client: TestClient, method: str, url: str): """Test registry not found error.""" - resp = await api_client.delete("/docker/registries/bad") + resp = await api_client.request(method, url) assert resp.status == 404 body = await resp.json() assert body["message"] == "Hostname bad does not exist in registries"supervisor/api/docker.py (1)
Line range hint
33-47: Consider adding similar 404 handling to GET endpoint.The registries method returns registry data but doesn't handle the case where no registries exist. Consider adding similar error handling for consistency.
Here's a suggested implementation:
@api_process async def registries(self, request) -> dict[str, Any]: """Return the list of registries.""" + if not self.sys_docker.config.registries: + raise APINotFound("No registries configured") data_registries = {} for hostname, registry in self.sys_docker.config.registries.items(): data_registries[hostname] = { ATTR_USERNAME: registry[ATTR_USERNAME], } return {ATTR_REGISTRIES: data_registries}supervisor/api/discovery.py (1)
19-19: Consider enhancing the error message with the requested UUIDWhile the change to APINotFound is appropriate, the error message could be more helpful for debugging by including the requested UUID.
- raise APINotFound("Discovery message not found") + raise APINotFound(f"Discovery message with UUID {request.match_info.get('uuid')} not found")Also applies to: 39-39
supervisor/api/jobs.py (1)
Line range hint
102-107: Consider combining job existence and completion checksThe current implementation checks job existence and completion status separately. Consider combining these checks in the _extract_job method for better encapsulation.
def _extract_job(self, request: web.Request) -> SupervisorJob: """Extract job from request or raise.""" try: - return self.sys_jobs.get_job(request.match_info.get("uuid")) + job = self.sys_jobs.get_job(request.match_info.get("uuid")) + if not job.done and request.match_info.get("method") == "remove_job": + raise APIError(f"Job {job.uuid} is not done!") + return job except JobNotFound: raise APINotFound("Job does not exist") from None async def remove_job(self, request: web.Request) -> None: """Remove a completed job.""" job = self._extract_job(request) - if not job.done: - raise APIError(f"Job {job.uuid} is not done!") self.sys_jobs.remove_job(job)tests/api/test_discovery.py (1)
143-152: Good test coverage for 404 responsesThe parameterized test effectively validates the 404 handling for both GET and DELETE endpoints. Consider adding more edge cases:
- Invalid UUID formats
- Recently deleted discoveries
- Discoveries from uninstalled add-ons
@pytest.mark.parametrize( ("method", "url", "scenario"), [ ("get", "/discovery/bad", "invalid-uuid"), ("get", "/discovery/123e4567-e89b-12d3-a456-426614174000", "valid-uuid-not-found"), ("get", "/discovery/{uuid}", "recently-deleted"), ("delete", "/discovery/bad", "invalid-uuid"), ], ) async def test_discovery_not_found(api_client: TestClient, method: str, url: str, scenario: str): """Test discovery not found error with various scenarios.""" if scenario == "recently-deleted": # Setup: Create and delete a discovery resp = await api_client.post("/discovery", json={"service": "test", "config": {}}) uuid = (await resp.json())["data"]["uuid"] await api_client.delete(f"/discovery/{uuid}") url = url.format(uuid=uuid) resp = await api_client.request(method, url) assert resp.status == 404 resp = await resp.json() assert resp["message"] == "Discovery message not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
supervisor/api/addons.py(2 hunks)supervisor/api/discovery.py(2 hunks)supervisor/api/docker.py(2 hunks)supervisor/api/jobs.py(3 hunks)supervisor/api/mounts.py(5 hunks)supervisor/api/network.py(2 hunks)supervisor/api/resolution.py(5 hunks)supervisor/api/services.py(2 hunks)supervisor/api/store.py(2 hunks)tests/api/test_addons.py(3 hunks)tests/api/test_discovery.py(1 hunks)tests/api/test_docker.py(1 hunks)tests/api/test_jobs.py(2 hunks)tests/api/test_network.py(1 hunks)tests/api/test_resolution.py(10 hunks)tests/api/test_services.py(1 hunks)tests/api/test_store.py(2 hunks)
🔇 Additional comments (26)
supervisor/api/docker.py (2)
62-64: LGTM! Error handling implementation looks good.
The check for non-existent registry is correctly placed before the deletion attempt, and the error message is clear and descriptive.
19-19: Verify the impact of APINotFound usage.
Let's check if APINotFound is consistently used across the codebase for similar scenarios.
✅ Verification successful
APINotFound usage is consistent across the codebase
The verification shows that APINotFound is consistently used across various API modules to handle "not found" scenarios, including:
- Missing addons, repositories, and store items
- Non-existent services and mounts
- Invalid UUIDs and checks
- Missing backups and jobs
- Network interface issues
- Non-existent registry hostnames (your case)
The usage pattern is uniform: raising APINotFound with a descriptive message when a requested resource doesn't exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar registry-related error handling patterns
rg -A 2 "APINotFound.*registry" || echo "No other registry-related APINotFound usage found"
# Search for general APINotFound usage patterns
rg -A 2 "raise APINotFound" || echo "No other APINotFound usage found"
Length of output: 2875
supervisor/api/services.py (1)
12-12: LGTM! Appropriate use of APINotFound for non-existent services
The change from APIError to APINotFound better reflects the semantic meaning of the error condition and aligns with HTTP standards.
Also applies to: 23-23
supervisor/api/jobs.py (1)
10-10: LGTM! Well-structured job extraction helper method
Good application of DRY principle by centralizing job lookup logic with proper exception handling.
Also applies to: 26-31
supervisor/api/resolution.py (2)
33-54: Well-structured extraction methods with clear error handling!
The new extraction methods follow a consistent pattern and provide clear, specific error messages. This improves the API's error handling by properly returning 404 status codes when resources aren't found.
88-89: Good implementation of extraction methods across endpoints
The consistent use of extraction methods across all endpoints improves code maintainability and ensures uniform error handling. The changes align well with the PR's objective of enhancing 404 status code usage.
Also applies to: 94-95, 100-106, 111-112, 123-123, 134-134
supervisor/api/mounts.py (2)
27-32: Well-implemented mount extraction method!
The extraction method follows the established pattern and provides clear error messages for 404 responses.
95-98: Verify name validation in update_mount
While the extraction method is well used, there's a potential edge case in the name validation schema. The current implementation forces the name to match the current mount's name, which might prevent legitimate mount renames.
✅ Verification successful
Let me gather more context about the mount functionality to make a more informed decision.
Let me gather more specific information about the mount update functionality.
Let me try a different approach to understand the mount update functionality.
Based on the code analysis, I can now provide a definitive response about the mount name validation concern.
Mount name validation is correctly implemented
The current implementation in update_mount is intentional and correct. Here's why:
- The name schema validation
{vol.Optional(ATTR_NAME, default=current.name): current.name}ensures backward compatibility and prevents name conflicts - Mount renaming is handled through the mount manager's
create_mountmethod which properly handles the removal of old mount and creation of new one - The implementation includes proper cleanup of old mounts when updating, as seen in
create_mount:await self.remove_mount(mount.name, retain_entry=True)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if mount renaming is a supported operation
rg -A 5 "rename.*mount" || echo "No explicit mount rename functionality found"
# Check for any mount name update patterns
ast-grep --pattern 'mount.name = $_'
Length of output: 163
Script:
#!/bin/bash
# Check the full implementation of update_mount and related methods
ast-grep --pattern 'def update_mount'
# Check for mount name usage in validation or configuration
rg -A 5 'ATTR_NAME.*mount'
# Look for mount configuration patterns
rg -A 5 'Mount\('
Length of output: 96424
Script:
#!/bin/bash
# Check the update_mount implementation
ast-grep --pattern 'def update_mount'
# Check mount name validation and usage
rg -A 5 'update_mount.*name'
# Look for mount name modification patterns
rg -A 5 'mount.name = '
Length of output: 96
Script:
#!/bin/bash
# Check the mount validation schema implementation
rg -A 10 'ATTR_NAME.*mount'
# Look for mount class definition and properties
ast-grep --pattern 'class Mount'
# Check for mount creation and update API routes
rg -A 10 'async def.*mount'
Length of output: 55653
tests/api/test_jobs.py (2)
217-219: LGTM! Improved HTTP status code usage.
The change from 400 to 404 for non-existent jobs aligns better with HTTP standards, where 404 is the appropriate status code for "not found" scenarios.
222-231: LGTM! Well-structured test for job not found scenarios.
Good use of pytest's parameterization to test both GET and DELETE endpoints with invalid job IDs. The test ensures consistent 404 responses with appropriate error messages.
tests/api/test_resolution.py (3)
177-186: LGTM! Comprehensive test for issue not found scenarios.
Well-structured test that validates 404 responses for both DELETE and GET operations on non-existent issues.
189-198: LGTM! Thorough test for suggestion not found scenarios.
Good coverage of both DELETE and POST operations for non-existent suggestions.
201-210: LGTM! Complete test for check not found scenarios.
Effective validation of 404 responses for both options and run operations on non-existent checks.
supervisor/api/network.py (1)
170-170: LGTM! Improved error specificity.
Good change to use APINotFound instead of APIError. This provides more specific error handling and aligns with HTTP standards for resource not found scenarios.
supervisor/api/store.py (2)
78-82: LGTM! Improved error handling with APINotFound
The refactoring enhances error handling by using a more specific exception and providing a clearer error message.
92-95: LGTM! Consistent error handling implementation
The repository extraction follows the same improved pattern with APINotFound and provides a clear error message.
tests/api/test_store.py (4)
286-291: LGTM! Well-designed utility function
The get_message helper function effectively handles both JSON and text responses, promoting code reuse across test cases.
294-321: LGTM! Comprehensive test coverage for addon not found scenarios
The test thoroughly covers both new and legacy paths, with good parameterization and proper verification of status codes and error messages.
324-339: LGTM! Good coverage of not installed error case
The test properly verifies the 400 status code and error message for non-installed addons.
342-354: LGTM! Good test coverage for repository not found cases
The test effectively verifies both GET and DELETE operations with proper status code and error message checks.
tests/api/test_network.py (1)
405-421: LGTM! Comprehensive test coverage for network interface not found scenarios
The test effectively covers all relevant network interface operations with proper status code and error message verification.
tests/api/test_addons.py (4)
86-86: LGTM! Status code change aligns with HTTP standards.
The change from 400 to 404 for non-existing add-ons is correct as per HTTP standards, where 404 indicates a resource was not found.
372-377: LGTM! Well-structured helper function.
The get_message helper function effectively handles both JSON and plain text responses, improving code reusability and maintainability.
380-408: LGTM! Comprehensive test coverage.
The parameterized test test_addon_not_found provides thorough coverage of various API endpoints, ensuring consistent 404 responses for non-existent add-ons.
410-437: LGTM! Clear error distinction.
The parameterized test test_addon_not_installed properly differentiates between non-existent (404) and not installed (400) states across multiple endpoints.
supervisor/api/addons.py (1)
109-109: LGTM! Improved error specificity.
The change from APIError to APINotFound provides better error specificity and aligns with the HTTP status codes used in the responses.
Also applies to: 165-165
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
supervisor/api/store.py (1)
78-85: LGTM with a minor suggestion for message consistencyThe refactored addon extraction logic is more concise and uses the appropriate exception type. However, consider standardizing the error message casing.
- raise APINotFound(f"Addon {addon_slug} does not exist in the store") + raise APINotFound(f"Add-on {addon_slug} does not exist in the store")Note: The term "Add-on" is hyphenated elsewhere in the codebase (e.g., line 82).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
supervisor/api/store.py(2 hunks)
🔇 Additional comments (2)
supervisor/api/store.py (2)
54-54: LGTM: Import change aligns with PR objective
The addition of APINotFound to the imports is appropriate for implementing more specific HTTP 404 status handling.
92-95: Verify performance of repository existence check
The simplified repository existence check is cleaner and uses the appropriate exception type. However, let's verify the performance characteristics of sys_store.repositories.
✅ Verification successful
Let me analyze the repository existence check implementation based on the results.
Based on the results, I can now provide a final response about the repository existence check's performance:
Repository existence check is efficient and well-implemented
The implementation uses a dictionary-based lookup (self.repositories is a dict[str, Repository]), which provides O(1) constant time access. The check is:
- Memory efficient: Only stores repository references
- Time efficient: Uses native Python dictionary membership testing
- Thread-safe: Operates on an instance-level dictionary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of sys_store.repositories to ensure efficient lookup
# Search for the implementation of repositories property
ast-grep --pattern 'class $_ {
$$$
@property
def repositories($_):
$$$
}'
# Search for any usage patterns that might indicate performance considerations
rg -A 5 "repositories.*="
Length of output: 65650
Script:
#!/bin/bash
# Search for sys_store implementation to understand repositories access
ast-grep --pattern 'class $_ {
$$$
@property
def repositories(self):
$$$
}'
# Search for any repository access patterns
rg -A 3 "self.repositories"
Length of output: 2643
mdegat01
force-pushed
the
404-for-not-found
branch
from
1930fc3 to
eff3bc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/api/test_jobs.py (1)
222-231: LGTM! Well-structured parameterized testThe test effectively covers both GET and DELETE scenarios for non-existent jobs. The parameterization keeps the code DRY.
Consider enhancing the test description to be more specific:
- """Test job not found error.""" + """Test API returns 404 when job is not found for both GET and DELETE methods."""tests/api/test_mounts.py (1)
861-874: LGTM! Comprehensive parameterized test for mount endpointsThe test effectively covers PUT, DELETE, and POST/reload scenarios for non-existent mounts. The parameterization keeps the code DRY and maintains consistency with the job tests.
Consider enhancing the test description to be more specific:
- """Test mount not found error.""" + """Test API returns 404 when mount is not found for PUT, DELETE, and reload operations."""tests/api/test_store.py (1)
286-291: LGTM! Consider adding return type hintThe helper function effectively handles both JSON and plain text responses, reducing code duplication across tests.
Consider adding a return type hint to the function signature for better code clarity:
-async def get_message(resp: ClientResponse, json_expected: bool) -> str: +async def get_message(resp: ClientResponse, json_expected: bool) -> str: """Get message from response based on response type."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/api/store.py(2 hunks)tests/api/test_jobs.py(2 hunks)tests/api/test_mounts.py(1 hunks)tests/api/test_store.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supervisor/api/store.py
🔇 Additional comments (5)
tests/api/test_jobs.py (1)
217-219: LGTM! Improved HTTP status code usage
The change from 400 to 404 status code is more appropriate for a non-existent resource scenario, following REST API best practices.
tests/api/test_store.py (4)
239-239: LGTM! Error message improvements
The error messages have been updated to be more specific about store existence, which aligns well with the PR's objective of improving HTTP status code usage and error handling.
Also applies to: 283-283
294-319: LGTM! Comprehensive 404 test coverage
Excellent test coverage for addon not found scenarios:
- Covers both new and legacy API paths
- Tests various HTTP methods
- Validates both status codes and error messages
- Handles both JSON and plain text responses
321-337: LGTM! Clear distinction between 400 and 404 cases
The test properly validates that attempting to update an uninstalled addon returns a 400 status code, maintaining a clear distinction from the 404 "not found" scenarios.
339-351: LGTM! Repository not found handling
The test properly validates 404 responses for non-existent repositories, maintaining consistency with the overall error handling approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/api/test_store.py (1)
294-319: LGTM! Comprehensive test coverage for 404 responsesExcellent test coverage that:
- Tests all addon-related endpoints
- Covers both new and legacy paths
- Verifies both status codes and error messages
- Handles different response types
Consider adding a test case for
/store/addons/bad/changelogand/store/addons/bad/documentationto maintain complete coverage of all endpoints.tests/api/test_addons.py (1)
372-378: Consider adding error handling for malformed JSON responsesWhile the helper function is well-structured, it could benefit from handling potential JSON parsing errors.
Consider updating the implementation to handle JSON parsing errors:
async def get_message(resp: ClientResponse, json_expected: bool) -> str: """Get message from response based on response type.""" if json_expected: - body = await resp.json() - return body["message"] + try: + body = await resp.json() + return body["message"] + except ValueError: + return f"Failed to parse JSON response: {await resp.text()}" return await resp.text()tests/api/test_discovery.py (1)
143-152: LGTM! Consider enhancing test coverage.The parameterized test effectively verifies 404 responses for non-existent discovery messages.
Consider enhancing the test coverage by:
- Testing with different invalid UUID formats
- Verifying the response content-type header
- Adding assertions for the response structure (e.g., checking for 'result' field)
Example enhancement:
@pytest.mark.parametrize( ("method", "url"), - [("get", "/discovery/bad"), ("delete", "/discovery/bad")], + [ + ("get", "/discovery/bad"), + ("delete", "/discovery/bad"), + ("get", "/discovery/00000000-0000-0000-0000-000000000000"), + ("delete", "/discovery/invalid-uuid"), + ], ) async def test_discovery_not_found(api_client: TestClient, method: str, url: str): """Test discovery not found error.""" resp = await api_client.request(method, url) assert resp.status == 404 + assert resp.content_type == "application/json" resp = await resp.json() assert resp["message"] == "Discovery message not found" + assert resp["result"] == "error"tests/api/test_jobs.py (1)
222-231: Consider centralizing error messages and enhancing test coverage.The parameterized test effectively verifies 404 responses for non-existent jobs.
Consider these improvements:
- Centralize the error message string to avoid duplication:
# In supervisor/const.py or similar JOB_NOT_FOUND_MESSAGE = "Job does not exist" # In test file from supervisor.const import JOB_NOT_FOUND_MESSAGE ... assert body["message"] == JOB_NOT_FOUND_MESSAGE
- Enhance test coverage similar to the discovery tests:
@pytest.mark.parametrize( ("method", "url"), - [("get", "/jobs/bad"), ("delete", "/jobs/bad")], + [ + ("get", "/jobs/bad"), + ("delete", "/jobs/bad"), + ("get", "/jobs/00000000-0000-0000-0000-000000000000"), + ("delete", "/jobs/invalid-uuid"), + ], ) async def test_job_not_found(api_client: TestClient, method: str, url: str): """Test job not found error.""" resp = await api_client.request(method, url) assert resp.status == 404 + assert resp.content_type == "application/json" body = await resp.json() assert body["message"] == "Job does not exist" + assert body["result"] == "error"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
supervisor/api/addons.py(2 hunks)supervisor/api/discovery.py(2 hunks)supervisor/api/docker.py(2 hunks)supervisor/api/jobs.py(3 hunks)supervisor/api/mounts.py(5 hunks)supervisor/api/network.py(2 hunks)supervisor/api/resolution.py(5 hunks)supervisor/api/services.py(2 hunks)supervisor/api/store.py(2 hunks)tests/api/test_addons.py(3 hunks)tests/api/test_discovery.py(1 hunks)tests/api/test_docker.py(1 hunks)tests/api/test_jobs.py(2 hunks)tests/api/test_mounts.py(1 hunks)tests/api/test_network.py(1 hunks)tests/api/test_resolution.py(10 hunks)tests/api/test_services.py(1 hunks)tests/api/test_store.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/api/test_services.py
- supervisor/api/docker.py
- tests/api/test_network.py
- supervisor/api/discovery.py
- tests/api/test_docker.py
- supervisor/api/services.py
- supervisor/api/addons.py
- supervisor/api/network.py
- supervisor/api/store.py
- supervisor/api/jobs.py
- tests/api/test_resolution.py
🔇 Additional comments (11)
tests/api/test_store.py (3)
239-239: LGTM! Error messages are now more consistent
The updated error messages in the changelog and documentation tests now consistently indicate that the addon doesn't exist in the store, which aligns well with the PR's objective of improving HTTP status code usage.
Also applies to: 283-283
286-291: LGTM! Well-structured utility function
The get_message utility function is a good addition that:
- Reduces code duplication across tests
- Properly handles both JSON and plain text responses
- Has clear and descriptive naming
321-351: LGTM! Well-structured error case tests
The additional test cases effectively verify:
- 400 status for update attempts on non-installed addons
- 404 status for operations on non-existent repositories
- Appropriate error messages in both scenarios
supervisor/api/resolution.py (2)
33-54: LGTM! Well-structured entity extraction methods.
The new private methods follow a consistent pattern and provide clear error messages. They properly centralize the entity extraction logic and use appropriate status codes.
88-89: LGTM! Consistent use of extraction methods.
The refactored methods now use the centralized extraction logic, improving code maintainability and error handling consistency.
Also applies to: 94-95, 100-106, 111-112, 134-135
supervisor/api/mounts.py (2)
27-32: LGTM! Well-implemented mount extraction method.
The method follows the established pattern and provides clear error messages when a mount is not found.
95-98: LGTM! Consistent usage of mount extraction.
The refactored methods properly utilize the new extraction method, maintaining consistency with the codebase's error handling approach.
Also applies to: 118-119, 130-131, 134-134
tests/api/test_mounts.py (1)
861-874: LGTM! Comprehensive test coverage for not found scenarios.
The parameterized test effectively covers all relevant HTTP methods and verifies both the status code and error message. This ensures the new error handling behavior is properly tested.
tests/api/test_addons.py (2)
86-86: LGTM: Status code change aligns with PR objective
The change from 400 to 404 status code is appropriate for the case when an addon doesn't exist, following HTTP standards where 404 indicates a resource not found.
380-437: LGTM: Comprehensive test coverage for addon not found scenarios
The new parameterized tests provide excellent coverage:
- Tests all relevant addon endpoints
- Properly differentiates between non-existent (404) and not installed (400) cases
- Handles both JSON and plain text responses
- Well-structured using pytest parameterization
This ensures consistent error handling across all addon-related endpoints.
tests/api/test_jobs.py (1)
217-219: LGTM! Status code change aligns with REST API best practices.
The change from 400 to 404 for non-existent jobs is appropriate and consistent with REST API conventions.
Proposed change
Continuing #5479 , we should be using proper status codes. This makes many more places return 404 when the resource isn't found.
Marking draft as there's still some more to do.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Release Notes
New Features
APINotFoundexception for more specific error handling across various API endpoints.Bug Fixes
Tests