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

Adding GQL compliant errors and notifications to Browser #1989

Merged

Conversation

daveajrussell
Copy link
Contributor

@daveajrussell daveajrussell commented Dec 4, 2024

This PR introduces GQL errors and notifications into Browser.

For query results, GQL status objects are returned as a new array, separate from the existing notifications. We have taken the decision to only show GQL statuses for query results if the server is running a version >= v5.23.0).

If the server is older than 5.23.0, we will continue to show the existing notifications. This is because, whilst Browser uses a version of the JS driver that includes GQL statuses for query results, if the server is < v5.23.0, the statuses will be polyfilled from the notifications, and will lack accurate GQL status codes, defaulting to a generic code.

For errors, there is a new type GQLError which is an extension of the existing error object. There is no new object to consume for errors. We have taken the decision to only show GQL statuses for errors if the server is running a version >= 5.26.0)

If the server is older that 5.26.0, we will continue to show the existing error fields. For the same reason above, servers running < 5.26.0 will polyfill GQL error information and, again, will lack accurate GQL status codes, defaulting to a generic code.

We have noticed that there are some errors that do not have a full GQL code implemented, so will return the generic error code. For example, syntax errors will result in a generic error code.

For this reason, Errors are hidden behind a config flag. Since we don't want to release the 2 separately, notifications will also be placed behind this flag.

To see GQL errors and notifications, ensure you're running at least 5.23.0/5.26.0 and enable the flag by running :config enableGqlErrorsAndNotifications:true

@daveajrussell daveajrussell force-pushed the gql-errors-and-notifications branch from e464f72 to be28b5d Compare December 11, 2024 15:12
@daveajrussell daveajrussell marked this pull request as ready for review December 11, 2024 15:51
Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps there's a way to do this without adding this recursive type, but I wanted to mark all type properties of a type as partial so I didn't have to provide a ton of stubbed properties in mocked objects https://dev.to/perennialautodidact/adventures-in-typescript-deeppartial-2f2a

@@ -136,7 +136,13 @@ export const handleBoltWorkerMessage =
runningCypherQuery = false
execCloseConnectionQueue()
postMessage(
maybeCypherErrorMessage({ code: err.code, message: err.message })
maybeCypherErrorMessage({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a little different to the approach in UPX

there we are capturing the bolt protocol version inside our driver adapter, which we can then refer to when we catch an exception

here, I've found that this handler seems to a few layers of abstraction away from the driver, so I was unsuccessful in getting a reference to the protocol, much less the driver, when an exception occurs

so, I'm just sending back the GQL fields with the usual fields and doing the protocol check in the app layer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some small styling changes for nested errors

params: getParams(state),
neo4jVersion: getSemanticVersion(state)
}
const gqlErrorsEnabled = (state: GlobalState): boolean => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

despite my other comment about not being able to capture the protocol version at exception time, I think this is quite a neat way of showing either the existing error fields, or the GQL error fields

@OskarDamkjaer OskarDamkjaer merged commit 26f4817 into neo4j:master Dec 16, 2024
16 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