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

fix: Define index prefix length for MySQL #431

Merged
merged 3 commits into from
Sep 3, 2023
Merged

Conversation

iv-craig
Copy link
Contributor

@iv-craig iv-craig commented Apr 6, 2023

I ran into the issue described in #426 while setting up this package on a new project, MySQL requires a key length for the index prefix when creating an index on a text column. This issue would have been introduced in #396 when switching the credentialId column from a varchar to mediumtext.

I'm not sure if additional work would be needed for other database drivers, it seems like defining index prefix lengths in ways compatible with the different grammars has come up for discussion before but was dropped: laravel/framework#9293 (comment).

Please let me know if there's anything else that should be added to this PR.

@grafst
Copy link

grafst commented Apr 20, 2023

for me $table->index([DB::raw('credentialId(255)')], "credential_index"); seems also to work and is simpler IMHO. But if it works it works... 👍

@iv-craig
Copy link
Contributor Author

iv-craig commented May 4, 2023

@grafst That definitely looks better, thanks! That's added to the PR.

@iv-craig
Copy link
Contributor Author

iv-craig commented Aug 9, 2023

@asbiin Please let me know if there's anything else that should be added to this PR.

@asbiin
Copy link
Owner

asbiin commented Aug 16, 2023

@iv-craig @gawsoftpl Thank you for your solution! I though that defining index length globally as explained here https://laravel.com/docs/10.x/migrations#index-lengths-mysql-mariadb was an easy solution. It has to be implemented in the application, but instead I'm mistaken, it should solve it, no?

@iv-craig
Copy link
Contributor Author

iv-craig commented Sep 1, 2023

@asbiin Unfortunately, that doesn't seem to do the trick. The Laravel documentation specifies MySQL versions older than 5.7.7, but I'm experiencing this issue on MySQL 8.0.31. The defaultStringLength also seems to only be used in the char() and string() methods for migrations, not when adding an index: https://github.com/laravel/framework/blob/1cdace4e54d517c2bee16669ded9c9abca7e1f44/src/Illuminate/Database/Schema/Blueprint.php#L723-L742

Here are some more details from the discussion on the core framework issue: laravel/framework#9293 (comment)

Setting the MySQL index prefix length at 255 would make it so only the first 255 characters of the credentialId would be indexed. I understand that the column was changed from a string for some longer values. Using a Yubikey, I'm not seeing any values hitting 90 characters, but I'm guessing there are other methods that would produce longer values. If you'd like, I could update the PR to increase the prefix length to 750, as mentioned in the issue above.

@asbiin
Copy link
Owner

asbiin commented Sep 3, 2023

@iv-craig Thank you with all those research. I don't see any reason not to merge this after all! Thank you
Also I agree that 255 should be enough, so let's go with it.

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.

3 participants