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

More specific exception classes #870

Closed
wants to merge 3 commits into from
Closed

More specific exception classes #870

wants to merge 3 commits into from

Conversation

casperisfine
Copy link
Contributor

Fix: #404

I've just implemented 2 of them to showcase what it could look like. I can go over all error codes if dimmed necessary.

@brianmario @sodabrew @rafaelfranca any thoughts on this?

@rafaelfranca
Copy link

I like this direction. Rails already has to do similar thing and it would be great if exceptions raised by the adapter also were specific https://github.com/brianmario/mysql2/blob/master/ext/mysql2/client.c#L644.

@casperisfine
Copy link
Contributor Author

The issue is that there is hundred of possible error codes, maintaining that by hand migth be hard.

I'm thinking of automating it by parsing MySQL / Mariadb source files: https://raw.githubusercontent.com/MariaDB/mariadb-connector-c/master/include/mysqld_error.h

@casperisfine
Copy link
Contributor Author

So I went a bit farther with this. I think there is just way too much error codes (over a thousand) to classify them all, I think what most people need is to have an easy way to differentiate big classes of errors.

So I started with the 2 most painful one for us at Shopify:

  • ConnectionError: anything network or credential related
  • TimeoutError: be it read timeout or transaction lock timeout.

@sodabrew
Copy link
Collaborator

Thank you @casperisfine for kicking this off! I also like the general direction of this PR, and I also agree that copying a few hundred error codes is going to be problematic.

Off the cuff, I wonder how stable the codes are - or rather, how stable the constants are for a given code value - so that if the constants are then used to generate the error class, the same code number will have a stable text representation. Also there must be a code-to-language file, and that might even be internationalized. (This goes down a rabbit hole fast!)

Another concern I will suggest for guidance is to avoid mass-defining of classes for the various error codes, because that would have an impact on module load / application startup time / memory usage.

@sodabrew
Copy link
Collaborator

(I started writing my last reply before I saw your post about the two main buckets of error codes. Yes, I agree with this wholeheartedly. Error code buckets are a 90% solution for 10% of the cost!)

@casperisfine
Copy link
Contributor Author

I wonder how stable the codes are

I looked a bit around, the most important ones seem stable since a long time.

Also there must be a code-to-language file, and that might even be internationalized. (This goes down a rabbit hole fast!)

So in MySQL errors are mostly an error code. They have an associated name which is just a C lang #define, and then a text representation that is indeed internationalized.

But it's not really a problem, we just want to map the numeric code with a Ruby class, the internationalized text is just received and used as is by the client.

Another concern I will suggest for guidance is to avoid mass-defining of classes for the various error codes, because that would have an impact on module load / application startup time / memory usage.

That is definitely a concern of us. Right now I only defined ConnectionError and TimeoutError. I can think of a handful more that would be interesting to specialize, like SyntaxError, but we should definitely not define thousands of useless exception classes.

Would you be fine with merging just those 2 new classes and then continue in a followup ? What would be needed for this PR to be mergeable ?

I'm willing to put the time in this project, but I need a bit more direction from a maintainer.

@sodabrew sodabrew added this to the 0.5.0 milestone Nov 14, 2017
@sodabrew
Copy link
Collaborator

I cut the branch for 0.4.x maintenance, and opened master for development. I'd love to get this PR merged if you've got some time to continue working on it. Thank you!

@casperisfine
Copy link
Contributor Author

I'd be happy to. I'll start by rebasing it.

@casperisfine
Copy link
Contributor Author

Closing in favor of #911

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.

4 participants