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

Increase slug size #183

Open
wants to merge 2 commits into
base: 1.0
Choose a base branch
from

Conversation

konovalov-nk
Copy link

This commit increases initial value for slug field in permissions table and adds an exception when JSON representation of slug fields exceeds 1024 characters.
The reason for that is permission rules are stored as a JSON string and when user assign too many rules to permission it would give him a DB error without any description what happened.

@mtveerman
Copy link

This seems to conflict with Laravel 5.4 using the default usage of utf8mb4. I get a SQLSTATE 42000 error when running the migrations.

I believe indexed fields can’t be longer than 191 characters.

@konovalov-nk
Copy link
Author

@mtveerman That kind of sucks. The problem with this field is that it needs to be indexed because it is used when checking the permissions in SQL queries and could be potential bottleneck if used at a scale.

I suggest two ways for solving this issue:

  1. Limiting slug field to 190 by default then.
  2. Changing the column type from string to text.

Pros/cons for 1st case:

[+] Forces user to split permission slugs into smaller groups
[+] Faster SQL queries (more efficient indexing vs text type)
[-] Could be bad for use-cases where app needs a lot of permissions grouped into one database row to be effective

Pros/cons for 2nd case:

[+] Any amount of slug rules
[-] Could be one of the bottleneck issues if used at a scale

@mtveerman
Copy link

And what if we change the column type from string to json? Seems like the slug is stored as JSON anyway.

See also https://dev.mysql.com/doc/refman/5.7/en/create-table-secondary-indexes.html#json-column-indirect-index

@konovalov-nk
Copy link
Author

konovalov-nk commented Feb 3, 2017

@mtveerman That would be almost same as text or even worse because:

As noted elsewhere, JSON columns cannot be indexed directly.

as stated from manual.

@mtveerman
Copy link

@konovalov-nk Right....

Maybe a more dramatic option would be to create a separate table for slug, related to the permission. This would allow for indexing, since there is one record per slug.

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.

None yet

2 participants