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

Do something meaningful with SQL exceptions #496

Open
jaragunde opened this issue Apr 23, 2021 · 7 comments
Open

Do something meaningful with SQL exceptions #496

jaragunde opened this issue Apr 23, 2021 · 7 comments

Comments

@jaragunde
Copy link
Member

jaragunde commented Apr 23, 2021

Certain DAO operations may throw SQL exceptions, one example is PostgreSQLTaskDAO::create(), which may throw SQLUniqueViolationException or SQLQueryErrorException. 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.

@jaragunde
Copy link
Member Author

jaragunde commented Apr 26, 2021

This is also the root cause behind #110 and #175.

@jaragunde
Copy link
Member Author

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.

jaragunde added a commit that referenced this issue Feb 17, 2023
Since we had to rewrite the delete DAO operation, we did it in PDO
terms, addressing #513 too.
jaragunde added a commit that referenced this issue Feb 23, 2023
Since we had to rewrite the delete DAO operation, we did it in PDO
terms, addressing #513 too.
jaragunde added a commit that referenced this issue Feb 23, 2023
Since we had to rewrite the delete DAO operation, we did it in PDO
terms, addressing #513 too.
jaragunde added a commit that referenced this issue Feb 23, 2023
Also rewrite the operation in PDO terms, addressing #513.
@jaragunde
Copy link
Member Author

Danielle started working on this problem and I've been following up. Some fixes have been merged lately.

From PR #608:

a968bd17 Merge pull request #608 from Igalia/bubble-up-sql-errors [Danielle M]
7ec2b2fe Fix string construction in service [Danielle Mayabb]
207ea879 (origin/bubble-up-sql-errors) Clean up formatting, use getters [Danielle Mayabb]
5bfa360e Clean up some xml so that messages are returned correctly [Danielle Mayabb]
a4175f5b Fix formatting/naming, return error code on error [Danielle Mayabb]
cfcdc447 Reload data store if creation fails [Danielle Mayabb]
a15d8cae Update save of dates, facade, and service [Danielle Mayabb]
d1940ea5 Add error handling for not null violation [Danielle Mayabb]
7762bffd Update object metadata and fix date conversion [Danielle Mayabb]
b9d4be4a Update copyright and remove trailing whitespace [Danielle Mayabb]
6c080269 Update var convention and delete whitespace [Danielle Mayabb]
90337f62 Expand exception handling in projectManagement.js [Danielle Mayabb]
6ad9871c Update createProjectsService to check new property isSuccessful [Danielle Mayabb]
074632f4 Update Project action, facade, and DAO [Danielle Mayabb]
d58e254f Update ProjectDAO create to use PDO and return OperationResult [Danielle Mayabb]
441c31a4 Add OperationResult class [Danielle Mayabb]

From PR #614:

f0ae97d8 Delete unused test code. [Jacobo Aragunde Pérez]
3145b3b3 [#172] Delete user assignment when deleting a project. [Jacobo Aragunde Pérez]
55d87e67 [#496,#513] Return more specific errors on project deletion. [Jacobo Aragunde Pérez]
1b1aa84c Remove unused constructor for ProjectDAO. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Feb 24, 2023
Also rewrite the operation in PDO terms, addressing #513.
jaragunde added a commit that referenced this issue Feb 24, 2023
We capture SQL exceptions that could happen in task creation and
wrap them into an OperationResult to be returned by the web service.

Fixes #492 because it handles the situation that caused that error.
jaragunde added a commit that referenced this issue Feb 27, 2023
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.
jaragunde added a commit that referenced this issue Feb 28, 2023
Also rewrite the operation in PDO terms, addressing #513.
jaragunde added a commit that referenced this issue Feb 28, 2023
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.
jaragunde added a commit that referenced this issue Feb 28, 2023
We capture SQL exceptions that could happen in task creation and
wrap them into an OperationResult to be returned by the web service.

Fixes #492 because it handles the situation that caused that error.
jaragunde added a commit that referenced this issue Feb 28, 2023
We capture SQL exceptions that could happen in task creation and
wrap them into an OperationResult to be returned by the web service.

Fixes #492 because it handles the situation that caused that error.
@jaragunde
Copy link
Member Author

Merged in #616:

f3f8c5f3 [#492, #496] Manage and return SQL errors on task creation. [Jacobo Aragunde Pérez]
00088a42 [#173] Allow to report multiple errors on the same creation batch. [Jacobo Aragunde Pérez]
f4700807 [#173] Report several kinds of errors on task creation. [Jacobo Aragunde Pérez]
a2a96ce8 Prevent calling task create without overlap check. [Jacobo Aragunde Pérez]

@jaragunde
Copy link
Member Author

Merged in #615:

0aeaf6e6 [#496, #513] Replace project partial update operation. [Jacobo Aragunde Pérez]
3fe93733 [#496,#513] Return more specific errors on project update. [Jacobo Aragunde Pérez]
b7b339bd Simplify project create operation. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Feb 28, 2023
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.
jaragunde added a commit that referenced this issue Feb 28, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 1, 2023
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.
jaragunde added a commit that referenced this issue Mar 1, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 6, 2023
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.
jaragunde added a commit that referenced this issue Mar 6, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 7, 2023
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.
jaragunde added a commit that referenced this issue Mar 7, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 7, 2023
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.
jaragunde added a commit that referenced this issue Mar 8, 2023
Modify the user creation pipeline (service, facade, DAO) to use
OperationResult and return better errors.

This affects only the postgres implementation but there is another
DAO for LDAP+DB that will be addressed in a subsequent patch.
jaragunde added a commit that referenced this issue Mar 8, 2023
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.
jaragunde added a commit that referenced this issue Mar 8, 2023
Modify the user update pipeline (service, facade, DAO) to use
OperationResult. HybridUserDAO will be addressed in a subsequent patch.
jaragunde added a commit that referenced this issue Mar 8, 2023
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).
jaragunde added a commit that referenced this issue Mar 8, 2023
Modify the user delete pipeline (service, facade, DAO) to use
OperationResult. HybridUserDAO is changed to reuse the same operation
from Postgres DAO, since it did not have any code related to LDAP.
jaragunde added a commit that referenced this issue Mar 8, 2023
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.
jaragunde added a commit that referenced this issue Mar 8, 2023
The DAO operation is also ported to PDO, addressing #513.
jaragunde added a commit that referenced this issue Mar 8, 2023
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.
@jaragunde
Copy link
Member Author

Merged in #618:

82d4d972 [#496] Review error response codes in task operations. [Jacobo Aragunde Pérez]
ddbeb195 [#173] Make error messages more noticeable. [Jacobo Aragunde Pérez]
13e9c2b2 [#496,#513] Return more detailed errors on task delete. [Jacobo Aragunde Pérez]
e49f20e5 [#496] Return more detailed errors on task update. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Mar 20, 2023
Modify the user update pipeline (service, facade, DAO) to use
OperationResult. HybridUserDAO will be addressed in a subsequent patch.
jaragunde added a commit that referenced this issue Mar 20, 2023
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).
jaragunde added a commit that referenced this issue Mar 20, 2023
Modify the user delete pipeline (service, facade, DAO) to use
OperationResult. HybridUserDAO is changed to reuse the same operation
from Postgres DAO, since it did not have any code related to LDAP.
@jaragunde
Copy link
Member Author

Merged in PR #621:

04cc1fdb [#173,#496] Report better errors on user CRUD operations [Jacobo Aragunde Pérez]
e0fd7893 [#173] Report warning message from LDAP backend to users. [Jacobo Aragunde Pérez]
73b21e5f [#173,#496] Report detailed errors on user delete. [Jacobo Aragunde Pérez]
d9b60724 [#173,#496] Report better errors from LDAP user update DAO. [Jacobo Aragunde Pérez]
999ec3f3 [#173,#496] Report detailed errors on user update. [Jacobo Aragunde Pérez]
0708ec42 [#173,#496] Report better errors from LDAP user create DAO. [Jacobo Aragunde Pérez]
2ea7e95f [#173,#496] Report detailed errors on user creation. [Jacobo Aragunde Pérez]
4d307f27 [#431] Fix calling sizeof on non-countable. [Jacobo Aragunde Pérez]

jaragunde added a commit that referenced this issue Mar 20, 2023
Merge pull request #621 from Igalia/user-create-report-errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant