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

Add error codes from MySQL 8 #2386

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ARitz-Cracker
Copy link

@ARitz-Cracker ARitz-Cracker commented Aug 10, 2020

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:

  1. Manually edit errors.js to include the non-obsolete definitions for the MySQL 5 error codes.
  2. Allow for 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 with OBSOLETE_, it'll fall back to the MySQL 5 definition.

Please let me know what's preferred so I can make the adjustments.

@dougwilson
Copy link
Member

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 OBSOLETE_ prefix be sufficient?

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.

@ARitz-Cracker
Copy link
Author

Hello, @dougwilson!
This PR does include ER_WRONG_SRID_FOR_COLUMN which #2325 refers to. I'll remove the reference to #2350 as I can't find the string "Connection was killed" on its own anywhere in the MySQL 8 sources.

As for making the tool simply removing the OBSOLETE_ prefix, I don't think that's a good idea. The most obvious reason being that some 5.x codes are defined as "OBSOLETE_ER_UNUSED" in 8.x. like here

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. 😆

@ARitz-Cracker
Copy link
Author

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?

@ARitz-Cracker
Copy link
Author

@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.

@dougwilson
Copy link
Member

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.

@ARitz-Cracker
Copy link
Author

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.

@ARitz-Cracker
Copy link
Author

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.

@ARitz-Cracker
Copy link
Author

I've just re-written history. I removed the changes to errors.js during my 3 wip commits, and added a final commit where I ran the tool. Things look like they work as expected now!

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;
Copy link
Member

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?

Copy link
Author

@ARitz-Cracker ARitz-Cracker Aug 15, 2020

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!

@ARitz-Cracker
Copy link
Author

Hello! I don't know why the AppVeyor build failed. Should I rebase this PR?

vlasky added a commit to vlasky/mysql that referenced this pull request Mar 24, 2021
Incorporated ARitz-Cracker's PR mysqljs#2386 "Add error codes from MySQL 8"

mysqljs#2386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants