-
Notifications
You must be signed in to change notification settings - Fork 12
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
Do something meaningful with SQL exceptions #496
Comments
We don't have a good way in the business logic layer (facades & actions) to pass detailed information to the upper layer in case of error. I would create an OperationResult class that would be returned by CRUD operations in the business logic layer. This class would have a boolean success field, and fields for an error number and message. It could even contain a list of tasks, the ones that we failed to create/update/delete. Then, web services would check the OperationResult and return the information in it in case of error. |
Danielle started working on this problem and I've been following up. Some fixes have been merged lately. From PR #608:
From PR #614:
|
We had a specific operation in the backend to update only certain fields of a project, but the UI was always sending the full project data. We think the ability of updating only some fields of the project is not important, and we prefer to minimize the amount of code to maintain. So we have: * Switched the web service to the previously unused UpdateProjectAction and ProjectDAO::update. * Modify ProjectDAO::update to detect specific errors and return an OperationResult, just like the partialUpdate operation did. * Modify ProjectDAO::update to use PDO. * Remove all code related to PartialUpdateProject.
We had a specific operation in the backend to update only certain fields of a project, but the UI was always sending the full project data. We think the ability of updating only some fields of the project is not important, and we prefer to minimize the amount of code to maintain. So we have: * Switched the web service to the previously unused UpdateProjectAction and ProjectDAO::update. * Modify ProjectDAO::update to detect specific errors and return an OperationResult, just like the partialUpdate operation did. * Modify ProjectDAO::update to use PDO. * Remove all code related to PartialUpdateProject.
Merged in #616:
|
Merged in #615:
|
We build an OperationResult for every situation where a task update is denied, and we also capture SQL exceptions and wrap them in OperationResult objects.
We build an OperationResult for every situation where a task update is denied, and we also capture SQL exceptions and wrap them in OperationResult objects.
We build an OperationResult for every situation where a task update is denied, and we also capture SQL exceptions and wrap them in OperationResult objects.
We build an OperationResult for every situation where a task update is denied, and we also capture SQL exceptions and wrap them in OperationResult objects.
Web services return the code contained in the first OperationResult. Codes have been reviewed to use 400 (bad request) and 403 (forbidden) instead of 500 when they make more sense, which is most known error cases.
The HybridUserDAO matches user data with the LDAP and keeps them in sync with the local database, to maintain the relations with other data tables. We modify the create operation in this DAO to return OperationResult, matching the new expectations of the upper layers. We change the Hybrid DAO to extend the Postgres DAO, so we can reuse the SQL operations. In this case, we call the create operation from the Postgres DAO after we do the corresponding LDAP checks.
This operation did not do anything related with LDAP, so we simply run the corresponding SQL operation. We don't modify the behavior of the operation, so we add a comment about a detected problem. In the web service layer, we add try/catch blocks for the LDAPInvalidOperationException like the ones in the create and delete user services, to prevent crashing on operations that aren't defined in the LDAP backend (namely, assigning users to groups).
We build an OperationResult for every situation where a task update is denied, and we also capture SQL exceptions and wrap them in OperationResult objects.
Web services return the code contained in the first OperationResult. Codes have been reviewed to use 400 (bad request) and 403 (forbidden) instead of 500 when they make more sense, which is most known error cases.
Merged in #618:
|
This operation did not do anything related with LDAP, so we simply run the corresponding SQL operation. We don't modify the behavior of the operation, so we add a comment about a detected problem. In the web service layer, we add try/catch blocks for the LDAPInvalidOperationException like the ones in the create and delete user services, to prevent crashing on operations that aren't defined in the LDAP backend (namely, assigning users to groups).
Merged in PR #621:
|
Certain DAO operations may throw SQL exceptions, one example is
PostgreSQLTaskDAO::create()
, which may throwSQLUniqueViolationException
orSQLQueryErrorException
. We currently let them flow up in the code, eventually crashing and resulting in an internal server error and a HTTP status 500 in the response.We should do something with these exceptions and make the backend fail gracefully, returning a meaningful error message. I think the DAO layer can still throw these exceptions, but the business logic layer (Facade/Action classes) should catch them and act in consequence.
This is the cause of some reported errors, like #492 and #495. It's very related with #173, too.
The text was updated successfully, but these errors were encountered: