-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add error codes from MySQL 8 #2386
base: master
Are you sure you want to change the base?
Conversation
Thank so much @ARitz-Cracker ! Yea, we definately need some solution to the 5 vs 8 issue, as we cannot just drop this change in as-is with changing all those code to include the word obsolete -- as code that uses this library relies on the code names there for detection of what type of error occurred vs the number, so it would be at least a semver major change for this module (not even considering if it makes sense to lead them with obsolete). Perhaps, if all the codes are included in 5 sources still, would just having the tool simply remove the As for your linked issues, I see you say this fixes those two bug reports. Can you list here which code number fixes #2325 and which code number fixes #2350 ? We don't want to close those out without being sure this fixes those issues, and the way you have the description written GitHub will automatically close them once this is merged, so want to be sure it makes sense. |
Hello, @dougwilson! As for making the tool simply removing the Going down this path, I think making the tool read both the 5.x sources and the 8.x sources is the best solution. Also, the CI is failing due to linting errors, I'll make sure to run eslint this time. 😆 |
By the way, you may notice that this PR still replaces some of the generated output with "unused" or "obsolete", this must have been done sometime between 5.7.29 and 5.7.31. So there are still some definitions which both versions mark as obsolete. Should I make the script remove those last remaining obsolete and unused definitions from the list? |
@dougwilson one more thing! Since I'm going through the trouble of modifying the error constants generator anyway, I can further extend the tool to read the MariaDB sources as well, and thus solve #1982! the MariaDB 10 sources have them defined in a similar manner as MySQL 5 with some minor tweaks. So it definitely won't be difficult to do. |
That would be neat. I think I tried that and ran into issues. There may be a branch in this repo with that work already if you want to take a look. But regardless, please make sure to do the MariaDB changes as a separate PR from this one. I have been at work since I first commented and will take a look at the new changes of this PR after work. |
Sounds great! I'll create a new branch which extends onto this one. Also, just made one final change to make sure that this tool will always prioritize MySQL 5 definitions over MySQL 8 so that there are no breaking changes. |
I've made another branch here, which contains the MariaDB codes and the MySQL 8 codes. I'll submit a PR for that one once this one is approved. |
bdf31bc
to
390cf5c
Compare
I've just re-written history. I removed the changes to |
lib/protocol/constants/errors.js
Outdated
exports.ER_GROUPING_ON_TIMESTAMP_IN_DST = 3912; | ||
exports.ER_TABLE_NAME_CAUSES_TOO_LONG_PATH = 3913; | ||
exports.ER_AUDIT_LOG_INSUFFICIENT_PRIVILEGE = 3914; | ||
exports.OBSOLETE_ER_AUDIT_LOG_PASSWORD_HAS_BEEN_COPIED = 3915; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there not still be keys that are starting with OBSOLETE_
in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, these were marked as obsolete before 8 was released, but didn't exist in 5 so I didn't think it mattered as nobody would see them. But! I've made the tool now auto-remove the OBSOLETE_
prefix if found!
390cf5c
to
4b692d4
Compare
Hello! I don't know why the AppVeyor build failed. Should I rebase this PR? |
4b692d4
to
2ca1a2a
Compare
Incorporated ARitz-Cracker's PR mysqljs#2386 "Add error codes from MySQL 8" mysqljs#2386
Fixes #2325
Fixes #2402
Fixes #2410
This PR makes the
generate-error-constants.js
tool able to get all the error codes from the MySQL 8 sources, as the txt file which defines all of them was in a different location compared to MySQL 5.I've also included the output from the tool.
However, there appears to be one issue. The MySQL 8 sources marks a lot of the MySQL 5 error codes as "obsolete". Since MySQL 5 is going to go EOL in Oct 2023, I'm assuming that these "obsolete" codes shouldn't be marked as such, at least until then. In order to solve this, we can do one of the two things:
errors.js
to include the non-obsolete definitions for the MySQL 5 error codes.generate-error-constants.js
to be passed 2 directories, one pointing the the MySQL 5 sources, and the other to the MySQL 8 sources. If it detects a MySQL 8 definition which starts withOBSOLETE_
, it'll fall back to the MySQL 5 definition.Please let me know what's preferred so I can make the adjustments.