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

Server to client error messages #1406

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

robacourt
Copy link
Contributor

@robacourt robacourt commented Jun 27, 2024

https://linear.app/electric-sql/issue/VAX-1983/improve-error-reporting-from-server-to-client

This PR add the ability of the client to show friendly error messages when the error has originated on the server.

For example:

an error occurred in satellite: INVALID_REQUEST Unexpected value for int2 column: 32769

At this stage I've not tried to hide any noise such as the stack trace in these scenarios. So the user in fact sees all of this:

an error occurred in satellite: INVALID_REQUEST Unexpected value for int2 column: 32769
Client can't connect with the server after a fatal error. This can happen due to divergence between local client and server. Use developer tools to clear the local database, or delete the database file. We're working on tools to allow recovering the state of the local database.
/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/error.js:35
  return new SatelliteError(
         ^


SatelliteError: Fatal error: Unexpected value for int2 column: 32769. Check log for more information
    at wrapFatalError (/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/error.js:35:10)
    at SatelliteProcess._handleOrThrowClientError (/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/process.js:542:13)
    at SatelliteProcess._handleClientError (/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/process.js:525:10)
    at <anonymous> (/Users/rob/src/electric-sql/electric/clients/typescript/dist/util/asyncEventEmitter.js:103:17)
    at Array.map (<anonymous>)
    at AsyncEventEmitter.processQueue (/Users/rob/src/electric-sql/electric/clients/typescript/dist/util/asyncEventEmitter.js:101:32)
    at AsyncEventEmitter.enqueueEmit (/Users/rob/src/electric-sql/electric/clients/typescript/dist/util/asyncEventEmitter.js:129:12)
    at SatelliteClient.handleError (/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/client.js:725:18)
    at Object.SatErrorResp (/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/client.js:98:37)
    at SatelliteClient.handleIncoming (/Users/rob/src/electric-sql/electric/clients/typescript/dist/satellite/client.js:801:47) {
  code: 12
}

I've also added friendly error message generation on the server for data validation errors as an example of how you would implement the friendly error messages. Once the permissions system has been added, rejected writes could also generate a friendly error message in a similar way.

The client will now also see error messages that were already being set in the SatErrorResp such as:

  • Postgres is unavailable
  • Acknowledged unknown txn
  • Connection to central database failed, cannot continue the replication because of possible consistency issues

@robacourt robacourt marked this pull request as ready for review June 27, 2024 13:28
@@ -582,6 +583,8 @@ defmodule Electric.Satellite.Serialization do
_ = Date.from_iso8601!(val)

val
rescue
_ -> raise DataValidationError, message: "Unexpected value for :date column: #{inspect(val)}"
Copy link
Member

@alco alco Jun 28, 2024

Choose a reason for hiding this comment

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

This ignores the specific error message from any of the function calls above and substitutes that with this generic message. That's not a problem, IMO, but then we don't need those extra catch-all clauses you added below, such as Month our of range and others.

Also, this needs to be a reraise to preserve the original stacktrace:

Suggested change
_ -> raise DataValidationError, message: "Unexpected value for :date column: #{inspect(val)}"
_ -> reraise DataValidationError, [message: "Unexpected value for :date column: #{inspect(val)}"], __STACKTRACE__


val
_ ->
raise DataValidationError, message: "Non integer value given for #{type} column: #{val}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise DataValidationError, message: "Non integer value given for #{type} column: #{val}"
raise DataValidationError, message: "Non-integer value given for #{type} column: #{val}"

@@ -621,36 +633,68 @@ defmodule Electric.Satellite.Serialization do
_ = Time.from_iso8601!(val)

val
rescue
_ -> raise DataValidationError, message: "Unexpected value for :time column: #{inspect(val)}"
Copy link
Member

Choose a reason for hiding this comment

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

The same considerations as for the other use of rescue above apply here

Suggested change
_ -> raise DataValidationError, message: "Unexpected value for :time column: #{inspect(val)}"
_ -> reraise DataValidationError, [message: "Unexpected value for :time column: #{inspect(val)}"], __STACKTRACE_

@kevin-dp
Copy link
Contributor

Minor comment but i think the error that is displayed on the client should make very clear that this is a server-side error, otherwise it will confuse people and they will think there is a problem with the client.

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.

None yet

3 participants