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

Drop subclasses of SequelizeConnectionError #17240

Open
4 of 8 tasks
ephys opened this issue Apr 2, 2024 · 0 comments
Open
4 of 8 tasks

Drop subclasses of SequelizeConnectionError #17240

ephys opened this issue Apr 2, 2024 · 0 comments
Labels
type: feature For issues and PRs. For new features. Never breaking changes.

Comments

@ephys
Copy link
Member

ephys commented Apr 2, 2024

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Feature Description

Describe the feature you'd like to see implemented

  • Drop all subclasses of ConnectionError
  • Make ConnectionAcquireTimeoutError a subclass of BaseError instead of ConnectionError

Describe why you would like this feature to be added to Sequelize

Basically, these subclasses don't make much sense when supporting so many dialects that each have their own behavior.

While working on #17222, I also noticed that many of the tests that make sure dialect-specific errors are wrapped in one of those subclasses were broken, and that most implementations just threw the generic ConnectionError.

I believe the classes AccessDeniedError, HostNotReachableError, InvalidConnectionError, ConnectionRefusedError, HostNotFoundError, and ConnectionTimedOutError should be removed in favor of throwing a generic ConnectionError. Error details can be accessed on error.cause, which exposes all necessary information

The only exception to this is the ConnectionAcquireTimeoutError class. This error is completely different from the others because it is thrown by the pool, and is not an error that occurs when connecting to the database, unlike the others

Because it is not a database connection error, it should not extend ConnectionError. In fact, it should probably be renamed to be more explicit that this is a pool error.

Another exception could be made for ConnectionTimedOutError, as it is the only connection error that makes sense to retry automatically.

I therefore propose to remove AccessDeniedError, HostNotReachableError, InvalidConnectionError, ConnectionRefusedError, HostNotFoundError; and to make ConnectionAcquireTimeoutError extend BaseError

Is this feature dialect-specific?

  • No. This feature is relevant to Sequelize as a whole.
  • Yes. This feature only applies to the following dialect(s):

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in implementing my feature.

Indicate your interest in the addition of this feature by adding the 👍 reaction. Comments such as "+1" will be removed.

@ephys ephys added type: feature For issues and PRs. For new features. Never breaking changes. pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet labels Apr 2, 2024
@WikiRik WikiRik removed the pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

No branches or pull requests

2 participants