Audit Appwrite's generic error messages #5738

gewenyu99 · 2023-06-22T21:35:43Z

What does this PR do?

(Provide a description of what this PR does and why it's needed.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)

Related PRs and Issues

🐛 Bug Report: Email invite error, doesn't show properly formated html. console#443

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

gewenyu99 · 2023-07-10T21:08:54Z

app/config/errors.php

@@ -10,61 +10,64 @@
 /** General Errors */
 Exception::GENERAL_UNKNOWN => [
 'name' => Exception::GENERAL_UNKNOWN,
- 'description' => 'An unknown error has occured. Please check the logs for more information.',
+ 'description' => 'An unknown error has occured. Please submit a support ticket at [TODO: Where do people get help?]() with reproduction steps for further assistance.',


Is there a places we want to direct users who need support? Maybe an email address?

This is kind of a catch-all exception. I'd like to see a GitHub issue for this with:

reproduction steps

if self-hosting, logs from the appwrite container

if on cloud, project ID so we can try to lookup the logs

Then, we can look into how to catch the generic exception and return an better exception.

Sounds good,

gewenyu99 · 2023-07-10T21:09:01Z

app/config/errors.php

 'code' => 500,
 ],
 Exception::GENERAL_MOCK => [
 'name' => Exception::GENERAL_MOCK,
- 'description' => 'General errors thrown by the mock controller used for testing.',
+ 'description' => 'General errors thrown by the mock controller used for testing. If you\'re not building Appwrite from source, please raise an issue at [TODO: Where do people get help?]()',


Is there a places we want to direct users who need support? Maybe an email address?

I've never seen anyone ask about this exception. Maybe they should reach out on discord so we can have a discussion about what they're doing and why they're getting this.

Okay, this should never occur on Cloud so I'm gonna say "raise an issue on GitHub or reach out to us on Discord."

@gewenyu99 this is only used as for in our test suite ( for the /v1/mock API ) So I wont be too worried. This is not a dev facing endpoint / error. A well framed exception and message should be okay 👍

gewenyu99 · 2023-07-10T21:09:45Z

app/config/errors.php

 'code' => 400,
 ],
 Exception::GENERAL_ACCESS_FORBIDDEN => [
 'name' => Exception::GENERAL_ACCESS_FORBIDDEN,
- 'description' => 'Access to this API is forbidden.',
+ 'description' => 'Access to this API is forbidden. Please [action to get access]',


When is this error API ever used? Why would access be forbidden? What are practical next steps here?

Theoretically, this happens when someone tries to access the console or project APIs, but specifies a different project:

appwrite/app/controllers/api/console.php

Line 13 in 27adf07

throw new Exception(Exception::GENERAL_ACCESS_FORBIDDEN);

appwrite/app/controllers/api/projects.php

Line 45 in 27adf07

throw new Exception(Exception::GENERAL_ACCESS_FORBIDDEN);

We can say to please use console as the project for this request.

That is better, I think for "contributor devex" this is important still. Thanks

gewenyu99 · 2023-07-10T21:13:08Z

app/config/errors.php

 'code' => 503,
 ],
 Exception::GENERAL_ARGUMENT_INVALID => [
 'name' => Exception::GENERAL_ARGUMENT_INVALID,
- 'description' => 'The request contains one or more invalid arguments. Please refer to the endpoint documentation.',
+ // TODO: This is used by the DB, can we make this more specific instead of a generic error or add some kind of templating into the message?


Argument invalid is very generic and difficult to debug, what would be a better way to handle these errors? I'm unfamiliar with the exact logic behind the many places that use this error.

This description is not used/shown to the user when the API throws this error. The message is determined by what the validator found to be incorrect:

appwrite/app/controllers/api/databases.php

Line 2907 in 27adf07

throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, $queriesValidator->getDescription());

Ahhh, okay, let me investigate if the description generated from the query validator is good or not :D

gewenyu99 · 2023-07-10T21:14:05Z

app/config/errors.php

 'code' => 400,
 ],
 Exception::GENERAL_QUERY_LIMIT_EXCEEDED => [
 'name' => Exception::GENERAL_QUERY_LIMIT_EXCEEDED,
+ // TODO: Would it be possible to indicate which attribute? It is really hard to debug when it just says "current attribute"


Is it possible to also say exactly which attribute exceeded the limit? It is much easier to debug when we can be more specific.

I don't think this exception is even used:

Search: https://github.com/search?q=repo%3Aappwrite%2Fappwrite%20GENERAL_QUERY_LIMIT_EXCEEDED&type=code

Should we keep it around?

@gewenyu99 yes. I remember that it was used when we first implemented it.
https://github.com/appwrite/appwrite/blob/0.13.x/app/controllers/api/database.php#L1728-L1736

I think it was then moved to the queries validator. Please check with @abnegate that we throw this error somewhere else. If we do, we can remove it from the error codes.

gewenyu99 · 2023-07-10T21:16:46Z

app/config/errors.php

 'description' => 'Query limit exceeded for the current attribute. Usage of more than 100 query values on a single attribute is prohibited.',
 'code' => 400,
 ],
 Exception::GENERAL_QUERY_INVALID => [
 'name' => Exception::GENERAL_QUERY_INVALID,
+ // TODO: Can we provide more detailed feedback about which part of the query is invalid? 


How do we parse queries? Is it possible to indicate exactly which part of a query is invalid? This message is vague.

This is also not currently used.

Search: https://github.com/search?q=repo%3Aappwrite%2Fappwrite+GENERAL_QUERY_INVALID&type=code

Okay cooolllll. Should this exist then?

@gewenyu99 similar to my comment above. It was used when it was initially implemented. But it has then been moved to the queries validator. Lets check with @abnegate

gewenyu99 · 2023-07-10T21:17:43Z

app/config/errors.php

 'code' => 400,
 ],
 Exception::GENERAL_SERVER_ERROR => [
+ // TODO: This is used often. Can we take a look at the usage here and make sure it's not abused?


I see this error used a lot when I search it. Can we make sure all the usage is appropriate (I'm guessing it's not, or it really shouldn't be.

This is a generic 500. We could try to create a specific exception for each of these...or it might be better to just start with some of the most common ones.

This error code is used for 500 errors. When a 500 error occurs, its beyond the control of the developer and they need to reach out to us via a support ticket to get it investigated @gewenyu99

We don't need to change the error type of the error code. Instead we can customise the messages we use at each occurrence

christyjacob4 · 2023-08-01T13:04:10Z

app/config/errors.php

@@ -75,17 +78,19 @@
 ],
 Exception::GENERAL_CURSOR_NOT_FOUND => [
 'name' => Exception::GENERAL_CURSOR_NOT_FOUND,
- 'description' => 'The cursor is invalid. This can happen if the item represented by the cursor has been deleted.',
+ 'description' => 'The cursor used in pagination is invalid. This can happen if the item represented by the cursor has been deleted. Retry this request using another item from the previous as the cursor.',


Suggested change

'description' => 'The cursor used in pagination is invalid. This can happen if the item represented by the cursor has been deleted. Retry this request using another item from the previous as the cursor.',

'description' => 'The cursor used in pagination is invalid. This can happen if the item represented by the cursor has been deleted or doesn't exist. Retry this request using another item from the previous as the cursor.',

Also the last part Retry this request using another item from the previous as the cursor. is not very clear and looks like a mouthful to read & parse 👀

Test batch of error message changes

3732cb1

gewenyu99 marked this pull request as draft June 22, 2023 21:35

Add some comments about suspect usage of generic errors

90a211f

gewenyu99 changed the title ~~Improve Appwrite's error messages.~~ Audit Appwrite's generic error messages Jul 10, 2023

gewenyu99 commented Jul 10, 2023

View reviewed changes

stnguyen90 mentioned this pull request Jul 22, 2023

Create GH workflow to generate github metrics appwrite/.github#23

Merged

This was referenced Jul 25, 2023

PRs Opened in June 2023 appwrite/.github#37

Closed

PRs Opened in June 2023 appwrite/.github#43

Closed

PRs Opened in June 2023 appwrite/.github#49

Closed

PRs Opened in June 2023 appwrite/.github#54

Closed

christyjacob4 reviewed Aug 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit Appwrite's generic error messages #5738

Audit Appwrite's generic error messages #5738

gewenyu99 commented Jun 22, 2023 •

edited by stnguyen90

Loading

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

gewenyu99 Jul 11, 2023

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

gewenyu99 Jul 11, 2023

christyjacob4 Aug 1, 2023

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

gewenyu99 Jul 11, 2023

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

gewenyu99 Jul 11, 2023

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

gewenyu99 Jul 11, 2023

christyjacob4 Aug 1, 2023 •

edited

Loading

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

gewenyu99 Jul 11, 2023

christyjacob4 Aug 1, 2023

gewenyu99 Jul 10, 2023

stnguyen90 Jul 10, 2023

christyjacob4 Aug 1, 2023

christyjacob4 Aug 1, 2023

	'description' => 'The cursor used in pagination is invalid. This can happen if the item represented by the cursor has been deleted. Retry this request using another item from the previous as the cursor.',
	'description' => 'The cursor used in pagination is invalid. This can happen if the item represented by the cursor has been deleted or doesn't exist. Retry this request using another item from the previous as the cursor.',

Audit Appwrite's generic error messages #5738

Are you sure you want to change the base?

Audit Appwrite's generic error messages #5738

Conversation

gewenyu99 commented Jun 22, 2023 • edited by stnguyen90 Loading

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christyjacob4 Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gewenyu99 commented Jun 22, 2023 •

edited by stnguyen90

Loading

christyjacob4 Aug 1, 2023 •

edited

Loading