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

Infer primary key from first id() column #160

Conversation

matthewjumpsoffbuildings
Copy link
Contributor

@matthewjumpsoffbuildings matthewjumpsoffbuildings commented Dec 12, 2023

Given that Spanner always requires a primary key, and other dialects of SQL often do not, as well as the fact that several official Laravel packages like Nova and Sanctum use migrations that have an id() column but do not set primary(), it makes sense to infer a primary key if none is explicitly set.

Otherwise you just cannot use migrations at all if you use Nova or Sanctum, or likely many other packages that behave in this way. Almost universally, if there is a table that has no primary key explicitly set, and an auto-generating UUID column exists, there would be very little harm in using that as the primary key to make Spanner happy.

This PR includes an update to the Schema Grammar's addPrimaryKeys function that checks for the first column that is UUID, and has a default value of Expression('GENERATE_UUID')

It works in tandem with this PR #157 as it requires the addition of automatic UUID generation for increments style columns

It also includes the fixes to add/alter column behaviour from this PR #159 - sorry its a bit cludged together. If you feel the other PRs can be merged perhaps address this one last?

Also added an additional test for 'implicit' primary key, keeping the explicit primary key test as well as the no key test

Co-authored-by: Takayasu Oyama <taka.oyama@gmail.com>
Co-authored-by: Takayasu Oyama <taka.oyama@gmail.com>
Co-authored-by: Takayasu Oyama <taka.oyama@gmail.com>
Co-authored-by: Takayasu Oyama <taka.oyama@gmail.com>
@taka-oyama
Copy link
Collaborator

it makes sense to infer a primary key if none is explicitly set

I don't agree here. Just customize the migration.

@taka-oyama taka-oyama closed this Dec 14, 2023
@matthewjumpsoffbuildings
Copy link
Contributor Author

matthewjumpsoffbuildings commented Dec 18, 2023

@taka-oyama I understand, but I would like to add some additional context.

First, having to insert migrations from 3rd party packages like Nova and Sanctum into your projects own migrations folder is fragile, since if you update the package and their migrations change, you will need to be sure to handle that

But more importantly, the core DatabaseMigrationReposity, here: https://github.com/laravel/framework/blob/56cf2018490ea0193f50bf2bff59654a46cf52d8/src/Illuminate/Database/Migrations/DatabaseMigrationRepository.php#L165 creates a table too in its internal method, the migrations table. Again this table defines no primary key, the id column is just an increments, as most other DBs dont require a primary key like Spanner.

If PR #157 is implemented the increments column will work as expected, using a uuid and GENERATE_UUID. However Spanner will still fail since no primary key is defined.

I really think its worth considering inferring a primary key from the first available auto generating uuid column available, when no primary key is explicitly set

@taka-oyama
Copy link
Collaborator

taka-oyama commented Jan 26, 2024

increments should be adding the primary key by default, so this is the wrong approach either way.

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.

2 participants