Fix: Make team_foreign_key nullable in pivot tables (#2888)
#2894
+237
−10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s_permissions and model_has_roles tables and relevent tests to proof
runofthemill
left a comment
runofthemill
left a comment
There was a problem hiding this comment.
thanks @imhayatunnabi for this fix! I ran into an issue, noted a possible fix below
| $table->primary([$pivotPermission, $columnNames['model_morph_key'], 'model_type'], | ||
| 'model_has_permissions_permission_model_type_primary'); | ||
| $table->unique([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], | ||
| 'model_has_permissions_team_foreign_key_permission_model_type_unique'); |
There was a problem hiding this comment.
noting this identifier is too long for MySQL; char limit is 64, and this is 67; possible fix:
Suggested change
| 'model_has_permissions_team_foreign_key_permission_model_type_unique'); | |
| 'model_has_permissions_team_fk_permission_model_type_unique'); |
| $table->primary([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], | ||
| 'model_has_permissions_permission_model_type_primary'); | ||
| $table->unique([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], | ||
| 'model_has_permissions_team_foreign_key_permission_model_type_unique'); |
There was a problem hiding this comment.
same as above;
Suggested change
| 'model_has_permissions_team_foreign_key_permission_model_type_unique'); | |
| 'model_has_permissions_team_fk_permission_model_type_unique'); |
Contributor
Author
|
@runofthemill the suggestion and reviews has been updated. can you please check again ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Github Issue: #2888
The
team_foreign_keycolumn was nullable in therolestable but not inmodel_has_permissions&model_has_rolespivot tables. This caused errors when assigning global roles (withteam_foreign_key = null) to users, as the pivot tables rejected NULL values.Added test coverage to verify the fix and prevent regressions:
it_can_assign_global_role_with_null_team_foreign_key_to_user()- Core scenario from issueit_can_assign_global_role_to_multiple_users_across_different_teams()- Unique constraint with NULLit_can_query_global_roles_correctly_across_different_teams()- Query logic with NULLit_allows_direct_database_insertion_with_null_team_foreign_key()- Direct schema testit_handles_mixed_team_and_global_roles_correctly()- Edge casesit_allows_direct_database_insertion_with_null_team_foreign_key_for_permissions()- Permissions table