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

use set_server_option if possible #33363

Merged
merged 1 commit into from
Jul 18, 2018
Merged

Conversation

ahorek
Copy link

@ahorek ahorek commented Jul 15, 2018

Summary

original issue #31422 (since rails 5.2.0)

We loose all open transactions; Inside the original code, when inserting fixtures, a transaction is open. Multple delete statements are executed and finally the fixtures are inserted. The problem with this patch is that we need to open the transaction only after we reconnect to the DB otherwise reconnecting drops the open transaction which doesn't commit all delete statements and inserting fixtures doesn't work since we duplicated them (Primary key duplicate exception)...

it breaks https://github.com/DatabaseCleaner/database_cleaner transaction strategy

open a transaction
insert fixtures
run a test
rollback

the problem is that inserting fixtures reconnects to the db that drops all active transactions, so the next rollback won't work.

Other Information

I used brianmario/mysql2#943 for this purpose, but it's available only in mysql2 0.5.0+. There's a fallback to the previous (current) behaviour, but it won't fix the bug.
I added an additional workaround, if you set up Mysql2::Client::MULTI_STATEMENTS in a database config manually, it won't require reconnect. This way it will work even with previous versions of mysql2 gem.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@ahorek ahorek changed the title WIP: use set_server_option if possible use set_server_option if possible Jul 15, 2018
@ahorek
Copy link
Author

ahorek commented Jul 18, 2018

I think that this should be backported to 5.2.x because it's a regression between 5.1.6 -> 5.2.
Transaction strategy is the fastest option for testing and now it fails without any error (mysql only). The transaction is just dropped silently.
Unfortunatelly this can't be fixed if mysql2 gem 4.x is used. Maybe this limitation should be documented somewhere?

@rafaelfranca rafaelfranca merged commit 371e375 into rails:master Jul 18, 2018
rafaelfranca added a commit that referenced this pull request Jul 18, 2018
use set_server_option if possible
kamipo added a commit that referenced this pull request Jul 19, 2018
#33363 has two regressions. First one is that `insert_fixtures_set` is
failed if flags is an array. Second one is that connection flags are not
restored if `set_server_option` is not supported.
kamipo added a commit that referenced this pull request Jul 19, 2018
#33363 has two regressions. First one is that `insert_fixtures_set` is
failed if flags is an array. Second one is that connection flags are not
restored if `set_server_option` is not supported.
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

3 participants