Skip to content

Handle db validation errors gracefully#1895

Merged
charisk merged 2 commits intomainfrom
charisk/handle-validation-errors
Dec 21, 2022
Merged

Handle db validation errors gracefully#1895
charisk merged 2 commits intomainfrom
charisk/handle-validation-errors

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Dec 21, 2022

We have a mixture of validation checks and unhandled exceptions in various places. This PR aims to tidy that up so that:

  • Validation errors are handled gracefully in the UI (DbPanel), using error notifications rather than triggering the global error handler.
  • The DbConfigStore throws exceptions in the unlikely event that a caller has asked it to do something that would get the config in an invalid state (e.g. to add a list with an empt name).
  • The DbManager should just pass through without doing the same checks.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk added the secexp label Dec 21, 2022
@charisk charisk requested a review from a team as a code owner December 21, 2022 10:59
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks 🧹 A couple of initial comments, just from poking around locally!

Comment thread extensions/ql-vscode/src/databases/ui/db-panel.ts
Comment thread extensions/ql-vscode/src/databases/ui/db-panel.ts
Copy link
Copy Markdown
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for cleaning this up!

Copy link
Copy Markdown
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for cleaning this up!

Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks for the fixes 😸

@charisk charisk enabled auto-merge (squash) December 21, 2022 12:45
@charisk charisk merged commit 2493b0f into main Dec 21, 2022
@charisk charisk deleted the charisk/handle-validation-errors branch December 21, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants