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

query grammer compileLock - ignore #156

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

matthewjumpsoffbuildings
Copy link
Contributor

While its true spanner doesnt support explicit locking, I think the compileLock method should not throw an exception. It may be better to log a warning perhaps, but since spanner has complex lock handling and consistency, just gracefully allowing the lock to generate no additional code seems acceptable.

When using spanner with other frameworks (Laravel Nova in this case), it is necessary to ignore this.

Also if you look at:
https://github.com/laravel/framework/blob/7907bfb50165b3951403e2d9882b4f8e0ee26b9e/src/Illuminate/Database/Query/Grammars/SQLiteGrammar.php#L29

and:
https://github.com/laravel/framework/blob/7907bfb50165b3951403e2d9882b4f8e0ee26b9e/src/Illuminate/Database/Query/Grammars/SqlServerGrammar.php#L340

You will see both of those return empty strings - I think graceful failure here is the much better approach

@taka-oyama
Copy link
Collaborator

Please add tests for this as well.

@taka-oyama
Copy link
Collaborator

I think fixing the ones that are failing should be enough for this case.

@matthewjumpsoffbuildings
Copy link
Contributor Author

I see the 3 tests in BuilderTest.php, test_lock, test_lockForUpdate, test_sharedLock. Currently they depend on an exception being thrown, given the compileLock is now just returning an empty string what is the expected result?

@taka-oyama
Copy link
Collaborator

Please change it so that toSql() produces the expected query.

@matthewjumpsoffbuildings
Copy link
Contributor Author

updated the 3 lock unit test methods

@taka-oyama taka-oyama merged commit 7dcb335 into colopl:master Dec 12, 2023
1 check passed
@taka-oyama
Copy link
Collaborator

Thanks.

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