Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling on backend and code readability #5197

Open
1 of 4 tasks
Aadesh-Baral opened this issue Jun 27, 2022 · 4 comments
Open
1 of 4 tasks

Improve error handling on backend and code readability #5197

Aadesh-Baral opened this issue Jun 27, 2022 · 4 comments
Assignees
Labels

Comments

@Aadesh-Baral
Copy link
Contributor

Aadesh-Baral commented Jun 27, 2022

The current approach to handling backend exceptions has resulted in code readability issues and challenges in maintaining the codebase.

1. Fix messed-up code readability #5911, #5909
The way exceptions are currently handled in the code has made it harder to read and comprehend. Combining SubCodes and error messages when raising exceptions can confuse new contributors trying to understand the error-handling logic.
For example, we have an endpoint that updates the project metadata. This is only permitted to users with Project Management permission. To return an error if the request is made by the user without Project Management permission we raise an exception like ValueError with the message “This action is only allowed to Project Managers”. Since we also have a SubCode parameter in the error response we are passing a subcode in the message itself like:

   raise ValueError("PM_ROLE_REQUIRD-  This action is only allowed to Project Managers")

On the view function, then we split the error string by - to get the subcode and error message like:

try:
    update_project(request.json)
except ValueError as e:
    return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403 

Proposed Solution:
Create a custom exceptions on which we can easily pass the extra information like sub-code, messages and any other args and the exception class should be able to prepare response based on the information passed.

2. Excessive try-except blocks: #5912
The code contains multiple try-except blocks to handle various types of exceptions. This can lead to code redundancy and reduced clarity, as each exception is managed separately.

For instance, if the view function expects NotFound, ValueError, and DataError exceptions, it handles them individually:

try:
    my_function()
except NotFound:
    return {"SubCode": "RESOURCE_NOT_FOUND", "Error": "The requested resource was not found in the server"}, 404
except ValueError as e:
    return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403
except DataError as e:
    return {"Error": str(e), "SubCode": "INVALID_REQUEST"}, 400
except Exception as e:
    return {"Error": str(e), "SubCode": "INTERNAL_SERVER_ERROR"}, 500

Proposed Solution:
The new custom exceptions should be handled using flask error handling mechanism instead of using try except blocks. All of the unhandled exceptions should be captured by flask generic error handler and return standard error response.

3. More detailed and standardized error response: #5910, #5909
Improving the error response to provide more information is essential. It can include detailed SubCodes, clear error messages, and additional context, making it easier for frontend developers and end-users to understand the issues.

For example, instead of a simple "RESOURCE_NOT_FOUND" SubCode, we can use "PROJECT_NOT_FOUND" and include the project ID in the response:

{
    "error": {
        "message": "The requested project was not found.",
        "sub_code": "PROJECT_NOT_FOUND",
        "code": 404,
        "details": {
            "project_id": "12345"
        }
    }
}

In case of invalid request we can provide in which field the issue occurred like in case of missing required field we can raise following error response:

{
    "error": {
        "message": "This field is required.",
        "sub_code": "FIELD_REQUIRED",
        "code": 400,
        "details": {
            "field_name": "name"
        }
    }
}

4. Separation of error messages from the code: #5909
Storing error messages in a separate JSON file provides a more organized and flexible approach. This allows easy updates and modifications of error messages without modifying the codebase.
Create an error_messages.json file to store all error messages, each with a unique sub-code, descriptive message, and additional information if required.

{
    "PM_ROLE_REQUIRED": "This action is only allowed for Project Managers.",
    "PROJECT_NOT_FOUND": "The requested project was not found.",
    "INVALID_REQUEST": "Cannot validate the request.",
    "FIELD_REQUIRED": "This field is required."
}
@joanjeremiah
Copy link

I would like to solve this issue. Kindly assign this task to me.

@Aadesh-Baral
Copy link
Contributor Author

@joanjeremiah feel free to work on this.

@Aadesh-Baral Aadesh-Baral self-assigned this Jun 5, 2023
@Aadesh-Baral Aadesh-Baral changed the title Improve error handling on backend Improve error handling on backend and code readability Jun 13, 2023
@Aadesh-Baral Aadesh-Baral moved this from Todo/Backlog to In Progress in Tasking Manager Roadmap 2022 Jun 15, 2023
@Bonkles
Copy link
Contributor

Bonkles commented Jul 31, 2023

@tsmock can you take a look here and provide the team some feedback on this error handling scheme under proposal?

@tsmock
Copy link
Contributor

tsmock commented Jul 31, 2023

My first response is "LGTM". I spent quite a bit of time tracking down errors when the stack traces weren't being printed, and it looks like #5899 will help fix that once they move over to the specific exceptions.

Beyond that:

  • I like that they are using a single base class for all TM exceptions (makes it easy to "catch" if we really need to)
  • Not really certain about loading error messages in __init__.py -- I would be surprised if it was loaded multiple times if the error messages were loaded in exceptions.py. I'd have to run it through a debugger though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: In Progress
Development

No branches or pull requests

5 participants