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

Sort service errors into the proper subclass #252

Merged
merged 8 commits into from
May 6, 2024
Merged

Conversation

ptpaterson
Copy link
Contributor

@ptpaterson ptpaterson commented Apr 2, 2024

Ticket(s): FE-5040

Problem

HTTPStreamClient implementations need to throw service errors if the initial response is a non-200. The logic to convert QueryFailure responses to the appropriate sub type is currently a private method on the Client class.

The StreamClient also needs to throw the appropriate service error class when a type=error event is received.

Solution

Move the Client.#getServiceError method to the errors module so it can be reused. Use it to throw the appropriate error if a type=error event is received or if the initial response is a non-200.

There is no http status code associated with streaming error events, so we should update error handling to rely first on the error code, then fallback to the base ServiceError if the error code is not matched.

Out of scope

N/A

Testing

Updated tests to check that the appropriate errors are thrown.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ptpaterson ptpaterson requested review from cynicaljoy, adambollen and cleve-fauna and removed request for adambollen April 2, 2024 16:09
@cleve-fauna
Copy link
Contributor

@ptpaterson you've got some tests failing.

@ptpaterson
Copy link
Contributor Author

ptpaterson commented Apr 3, 2024

It's unrelated to the streaming tests. Looks like a check for client-timeout is being flaky.

@cleve-fauna
Copy link
Contributor

It's unrelated to the streaming tests. Looks like a check for client-timeout is being flaky.

Can you try to deflake it?

@ptpaterson ptpaterson requested a review from cleve-fauna April 3, 2024 19:50
@ptpaterson ptpaterson requested a review from pnwpedro April 11, 2024 14:31
Copy link
Contributor Author

@ptpaterson ptpaterson left a comment

Choose a reason for hiding this comment

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

Error classes have been updated.

Fixes:

  • invalid_query is the only real QueryCheckError code

Streaming changes

  • FetchClient and NodeHTTP2Client return NetworkErrors when problems are encountered after the stream is connected.

New breaking changes compared to v1.3.1

  • The httpStatus field for ServiceErrors is now optional
  • Added a ConstraintFailureError class and broke that out from QueryRuntimeError
  • Merged the ServiceTimeoutError class into the QueryTimeoutError class. They share the same time_out code.

readonly httpStatus: number;
readonly httpStatus?: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose to make httpStatus optional. This seemed more consistent with encouraging folks to not rely on the HTTP status code than the sentinel -1.

However, it will break any existing Typescript code that reads error.httpStatus without guarding against null.

Comment on lines -199 to 236
* A failure due to the timeout being exceeded, but the timeout
* was set lower than the query's expected processing time.
* This response is distinguished from a ServiceTimeoutException
* in that a QueryTimeoutError shows Fauna behaving in an expected
* manner.
* A failure due to the query timeout being exceeded.
*
* This error can have one of two sources:
* 1. Fauna is behaving expectedly, but the query timeout provided was too
* aggressive and lower than the query's expected processing time.
* 2. Fauna was not available to service the request before the timeout was
* reached.
*
* In either case, consider increasing the `query_timeout_ms` configuration for
* your client.
*/
export class QueryTimeoutError extends ServiceError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged the ServiceTimeoutError class into QueryTimeoutError. We talk about "client timeouts" vs "query timeouts" so I think that's the appropriate name, and QueryTimeoutError already had optional stats field defined.

@ptpaterson ptpaterson merged commit cf6929c into beta May 6, 2024
5 checks passed
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.

2 participants