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

Manual error status for EdgeDB server #4625

Closed
tailhook opened this issue Nov 3, 2022 · 2 comments · Fixed by #4974
Closed

Manual error status for EdgeDB server #4625

tailhook opened this issue Nov 3, 2022 · 2 comments · Fixed by #4974
Assignees

Comments

@tailhook
Copy link
Contributor

tailhook commented Nov 3, 2022

Motivation

edgedb watch command is expected to be running in the background and update schema in the database. Sometimes schema updates cannot be applied because of syntax of schema files is wrong or because database changes are conflicting. In those cases we have to notify users somehow that schema changes could not be applied.

It's hard to make notifications from the background process, so the simplest solution is to error out all clients. So all subsequent testing that user does fails with the same error until schema files are fixed.

Overview

We use configuration mechanism to propagate error to all the clients that use the same database.

Example on conflicting type:

CONFIGURE DATABASE SET database_failure := "edgedb watch: SchemaError: property 'value' of object type 'default::Bar' cannot be cast automatically from scalar type 'std::int32' to scalar type 'std::str. Please fix syntax error before proceeding."

Example syntax error:

CONFIGURE DATABASE SET database_error := "edgedb watch: Error reading schema files. Please fix syntax error before proceeding."
CONFIGURE DATABASE SET database_error_info := '{"file": "dbschema/default.esdl", "line": 10, "column": 16, "message": "Syntax error: unexpected `)`"}'

Example when database upgrade needed:

CONFIGURE DATABASE SET database_warning := "edgedb watch: current EdgeDB version is 2.3+deadbee while minimum of 2.7 required by edgedb.toml"

Specification

New error type must be introduced. DevModeError (better name?), probably a subclass of BackendError (or ConfigurationError?)

Config parameters:

  • database_error -- this error is thrown in every client when any non-configuration query is performed (i.e. any CONFIGURE is allowed as well as selecting from cfg:: and sys::get_version*, maybe all stdlib is allowed?)
  • database_error_info -- JSON with extra information (format to be determined). This can be used in UI-based tools to provide nicer error. Has to be fetched manually via SELECT cfg::Config.
  • database_warning -- sent as a warning to all the clients (when we start supporting warning mechanism)

This mechanism is error by default, but generally advisory. To opt-out of error you can do:

CONFIGURE SESSION SET database_error := {};

This mechanism will be used by CLI for two things:

  1. For edgedb watch / edgedb migrate to fix the schema after resetting changes
  2. edgedb (REPL) will print the failure and clean database_error for the session, so you can inspect things while error is there.
@elprans
Copy link
Member

elprans commented Nov 4, 2022

The mechanism looks reasonable to me, however I wonder if we can (or should) generalize this to allow specifying the error class and whether or not the state is advisory. One other potential use case I'm contemplating is Cloud upgrades, where the source instance will be placed into read-only mode.

@tailhook
Copy link
Contributor Author

tailhook commented Nov 7, 2022

The mechanism looks reasonable to me, however I wonder if we can (or should) generalize this to allow specifying the error class and whether or not the state is advisory. One other potential use case I'm contemplating is Cloud upgrades, where the source instance will be placed into read-only mode.

Yeah. This sounds like a good idea. But I'm not sure about specifics.

Also I'm not sure how to make "non-advisory" state, until we have rule-based access controls (RBAC). Someone should have a permission to reset that state, and since all users are superusers every user can do that.

@msullivan msullivan self-assigned this Feb 1, 2023
msullivan added a commit that referenced this issue Feb 2, 2023
Implement a configuration field that causes the server to return an
error for all non-configuration commands.
See #4625 for details and motivation. Closes #4625.

The details here are a little different than proposed there.
There is one config property, `force_database_error`, that
accepts a json encoded string with the following format:
`false` means no error, and an object means to raise an error:
```
{
  "type": <name of exception class>,
  "message": <exception message>,
  "hint": <exception hint>,
  "details": <exception details>,
  "context": {
    "line": <line number>,
    "col": <column number>,
    "start": <start position>,
    "end": <end position>,
    "filename": <file name for error>
  }
}
```
All fields except for `type` are optional.
msullivan added a commit that referenced this issue Feb 3, 2023
Implement a configuration field that causes the server to return an
error for all non-configuration commands.
See #4625 for details and motivation. Closes #4625.

The details here are a little different than proposed there.
There is one config property, `force_database_error`, that
accepts a json encoded string with the following format:
`false` means no error, and an object means to raise an error:
```
{
  "type": <name of exception class>,
  "message": <exception message>,
  "hint": <exception hint>,
  "details": <exception details>,
  "context": {
    "line": <line number>,
    "col": <column number>,
    "start": <start position>,
    "end": <end position>,
    "filename": <file name for error>
  }
}
```
All fields except for `type` are optional.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants