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

Handle RecordNotUnique error in find_or_create_all_with_like_by_name correctly #1081

Conversation

kuahyeow
Copy link
Contributor

Related to #1068, #948

In the find_or_create_all_with_like_by_name method,
instead of issuing a ROLLBACK statement, we wrap the tag creation in a
transaction instead.

Issuing a ROLLBACK will break nested transactions, and also does not
respect multiple databases (available since Rails 6).

Adds a spec which attempts to simulate concurrent tag creation

@kuahyeow
Copy link
Contributor Author

kuahyeow commented Mar 30, 2022

Note this includes a commit from #1080 That PR has since been merged, so rebased away

@kuahyeow kuahyeow force-pushed the fix_rollback_find_or_create_all_with_like_by_name branch 2 times, most recently from 3bffcf8 to 643e676 Compare April 21, 2022 10:48
@kuahyeow
Copy link
Contributor Author

This is now ready for a look !

@kuahyeow kuahyeow force-pushed the fix_rollback_find_or_create_all_with_like_by_name branch from 643e676 to e1b18f4 Compare April 22, 2022 02:38
@seuros seuros self-assigned this Apr 26, 2022
In the find_or_create_all_with_like_by_name method,
instead of issuing a ROLLBACK statement, we wrap the tag creation in a
transaction instead.

Issuing a ROLLBACK will break nested transactions, and also does not
respect multiple databases (available since Rails 6).

Adds a spec which attempts to simulate concurrent tag creation. Adds
spec option to use truncation database strategy so the spec can see
committed records from another thread
@kuahyeow kuahyeow force-pushed the fix_rollback_find_or_create_all_with_like_by_name branch from e1b18f4 to 3748545 Compare May 1, 2022 01:21
@kuahyeow
Copy link
Contributor Author

Hi @seuros - Any feedback on this PR ?

@2called-chaos
Copy link

Could we get the ball rolling on this please @mbleigh @seuros ? When dealing with concurrency or bulk editing this is highly annoying and frankly dangerous as the models are seemingly saved (update/update!/save/save!) when in actuality the data has been rolled back. The taggings however get created. To add to the misery the paper trail audit record gets rolled back too. Thanks

@jakswa
Copy link

jakswa commented Jul 12, 2023

This is what would fix our data loss I believe as well. Thank you for trying to address it. 👍

@seuros seuros merged commit 12f08be into mbleigh:master Jul 12, 2023
@seuros
Copy link
Collaborator

seuros commented Jul 12, 2023

@jakswa can you try master directly ? if you confirm that it indeed resolve your issue, i will release a a new version.

I'm going to clean up the code base.

@jakswa
Copy link

jakswa commented Jul 13, 2023

@seuros we have been testing it today and so far so good 🤞 usually we'd have a few cases per day crop up

@jakswa
Copy link

jakswa commented Jul 14, 2023

Still no cases cropping back up 🎉. I'll remember to post back in here if we find any problems with this changeset, but going into the weekend feeling good! Thank you again!

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.

4 participants