Apply newly defined NotFound exceptions to handle resource not found errors.
#5927
Conversation
This file contains 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
Aadesh-Baral
force-pushed
the
enhance/5912-error-handling
branch
from
776c0e0 to
7ed6473
Compare
|
Before moving on to the next stages of this change, #5933 must be merged. |
Aadesh-Baral
force-pushed
the
enhance/5912-error-handling
branch
from
d1b0c60 to
7e78da6
Compare
Aadesh-Baral
changed the title
NotFound exceptions to handle resource not found errors.
d-rita
reviewed
Aug 3, 2023
There was a problem hiding this comment.
Tested almost all endpoints (except annotations and some…) and the Not Found exception handler works very well.
However, I noted some issues (some of which may be beyond the scope of this PR as well) but have to be noted either way:
| API Resource | Endpoint | Method | Issues | Status |
|---|---|---|---|---|
| Tasks | projects/project_id/tasks | GET | Getting particular tasks: non-existent task results in an internal server error | Fixed |
| Tasks | stop-validation | POST | Does not show 404 for non-existent mapping issue category | Out of scope |
| Tasks | tasks/xml, tasks/gpx | GET | Does not include non-existent task_id in error message | Fixed |
| Teams | project/teams | GET | Returns empty list for non-existent project | Fixed |
| Teams | project/teams/team_id | DELETE | 1. Docstrings do not include the project_id and team_id parameters. 2. user_id is not being captured hence resulting in an internal server error. | Out of scope |
| Teams | teams/team_id/actions/leave | POST | Remove existent but non-member from a team results in an internal server error | Fixed |
| Comments | projects/project_id/comments/tasks/task_id | POST, DELETE | Does not check if the project exists first. Hence error message does not show non existent project | Fixed |
| Projects | /projects | GET | Getting all organisations with varying non existent parameters results in an empty list | Needs Investigation (Out of scope) |
| Projects | /projects/project_id (get project including its area) | GET | Does not serve the 404 error. It is logged but it gives an internal server error for non existent projects | Fixed |
| Projects | projects/proj_id/actions/transfer-ownership | POST | Returns internal server error for non-existent project | Fixed |
| Projects | projects/proj_id/queries/nogeometries | Internal server error for non-existent project | Fixed | |
| Organisations | /organisations | POST | 1. Add type parameter to docstrings since it is required. 2. If other parts of the request body already exist, it gives the same error, i.e. Name already exists. | Needs Investigation (Out of scope) |
| Organisations | /organisations/org_id | GET | Non existent org _id not availed in error message (which is inconsistent with the PATCH method 404 error where it is shown) | Fixed |
| Organisations | organisations/org_slug | GET | Non existent org_slug not availed in 404 error | Fixed |
| Organisations | /organisations | GET | Returns empty list for non-existent manager parameter | Needs Investigation (Out of scope) |
| Messages | notifications/message_id | GET, DELETE | MessageService.get_message() - raise NotFound | Fixed |
| Campaigns | organisations/org_id/campaigns/ | GET | Returns empty list for non-existent organisation | Fixed |
| Campaigns | organisations/org_id/campaigns/campaign_id | POST | Internal server error for non existent campaignInternal server error for non existent organisation | Fixed |
| Campaigns | organisations/org_id/campaigns/campaign_id | DELETE | Internal server error if both org and campaign are real but unassigned. | Fixed |
| Campaigns | projects/proj_id/campaigns/campaign_id | DELETE | Generic Not found for existing campaign and project that are not attached | Fixed |
| Comments | projects/project_id/comments/tasks/task_id | GET | Remove body from dosctrings | Out of scope |
| Grid | projects/actions/intersecting-tiles | Docstrings don’t show all required fields in the example schema. | Out of scope |
d-rita
suggested changes
Aug 3, 2023
d-rita
reviewed
Aug 3, 2023
There was a problem hiding this comment.
We don't have tests for the Project Teams but the Project Teams API endpoints are failing since the user_id is not being read. That issue is solved for in this PR #5631 containing the tests and fix.
cc: @Aadesh-Baral
Aadesh-Baral
force-pushed
the
enhance/5912-error-handling
branch
from
7e78da6 to
30b146d
Compare
------------------------------------ We defined custom HttpExceptions on #5899 to better handle errors but these exceptions were not used anywhere. This commit has replaced the previous NotFound custom exception which was simply a basic exceptio class with the new implementation.
---------------------------------------- We are now using our new NotFound exception, which derives from the werkzeug HTTPException class. We no longer need to use it in the try except block to catch and return error message because Flask will handle this HTTPException automatically.
…n appropriate error messages In the prev implementation even if the project id is non existent error message was returned as project_id and task_id are primary keys for tasks. THis addition will check if the project exists before checking for task so we would already know if project exists and return error.
Aadesh-Baral
force-pushed
the
enhance/5912-error-handling
branch
from
30b146d to
b06ab6c
Compare
…ise NotFound exception
Aadesh-Baral
force-pushed
the
enhance/5912-error-handling
branch
from
b06ab6c to
4e009cd
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@d-rita Thanks for thorough testing. The problems that were stated and fall under the scope of this PR has been solved. The table has been updated with the status of each problems. Can you open a new issue for the problems that this PR doesn't cover? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR does the floowing:
backend.exceptions.NotFound(defined in Define custom exceptions for better handling backend errors #5909) replacesbackend.models.postgis.utils.Notfoundall over the code.subcodeand other provided parameters.How to review:
Going through each commit in turn is advised because this PR has numerous file modifications. The change is explained in the commit message.