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

Improved error catching and logging #343

Merged
merged 10 commits into from
Jul 8, 2024
Merged

Improved error catching and logging #343

merged 10 commits into from
Jul 8, 2024

Conversation

mwilde345
Copy link
Member

@mwilde345 mwilde345 commented Jul 3, 2024

Ticket(s): FE-5448

Problem

  • once core enforces v4 access (https://github.com/fauna/core/pull/11546), shell attempts to query v4 will result in 410's directly from fauna api
  • Executing a v10 query within v4 client results in an unhandled error, a symptom of bad error catching

Solution

  • check for and display err.message and err.description from fauna. This will display the nice message from core regarding v4 access
  • eval will properly catch errors from the async wrapQueries block

Result

  • Users see helpful error messages

Testing

  • added a test that mocks 410 response when setting up v4 client

@echo-bravo-yahoo
Copy link
Collaborator

FYI, we'll probably respond with a 410 (that's what we return right now on account disablement).

@mwilde345 mwilde345 marked this pull request as ready for review July 5, 2024 15:53
Comment on lines 95 to 104
} else if (
err.description === "UnknownError" &&
err?.requestResult?.statusCode === 410 &&
version === "4"
) {
this.error(
`This account is not allowed to query Fauna v4. Please use the v10 endpoint.`
);
} else {
this.error(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems error prone, in that it assumes all unknown 410 errors are due to v4 being disabled. I think you should be able to get access to more error details via requestResult rather than relying on heuristics

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ashton and I tested locally and found no reason not to just forward the nice message from core: https://github.com/fauna/core/blob/33ae43bb9aeabdea0f596020b0514975020dcf9b/ext/api/src/main/scala/api/ExceptionResponseHelpers.scala#L68

Updated this block to just properly handle description and message

@mwilde345 mwilde345 requested a review from freels July 8, 2024 13:37
@mwilde345 mwilde345 changed the title nicer message when unable to call v4 Improved error catching and logging Jul 8, 2024
Copy link
Member

@freels freels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this looks like a much better approach!

@mwilde345 mwilde345 merged commit c8ade45 into main Jul 8, 2024
1 check passed
@mwilde345 mwilde345 deleted the v4-access branch July 8, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants