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.

Sign up for GitHub

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

Jump to bottom

Fix reverse of record_state after rollback #12922

Closed
wants to merge 1 commit into from
Closed

Fix reverse of record_state after rollback #12922

wants to merge 1 commit into from

Conversation

iantropov
Copy link
Contributor

Fix reverse of record_state(new_record and id) after rollback.
Fix #7807

@iantropov
Copy link
Contributor Author

@rafaelfranca @robin850 @pftg please review this patch

melekes
melekes reviewed Nov 17, 2013
View reviewed changes
activerecord/lib/active_record/transactions.rb
protected

# Save the new record state and id of a record so it can be restored later if a transaction fails.
def remember_transaction_record_state #:nodoc:
@_start_transaction_state[:id] = id if has_attribute?(self.class.primary_key)
if has_attribute?(self.class.primary_key) and !@_start_transaction_state.include?(:id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer &&/|| over and/or

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you're right!

@iantropov
Copy link
Contributor Author

@akalyaev please, look again - I've corrected the patch

@melekes
Copy link
Contributor

melekes commented Nov 17, 2013

@iantropov looks good to me 😄

@iantropov
Copy link
Contributor Author

Thanks @akalyaev !

@vipulnsward
Copy link
Member

@iantropov, the build seems to be broken, can you rebase and push an updated PR please?

@iantropov
Copy link
Contributor Author

@vipulnsward I've rebased diff. Please review PR.

@vipulnsward
Copy link
Member

@iantropov mysql seems to break due to this, can you check the cause? https://travis-ci.org/rails/rails/jobs/14770763

@rafaelfranca
Copy link
Member

cc @arthurnn

@repinel
Copy link
Member

repinel commented Jul 21, 2015

If you follow #7807 and eval @new_record, you will get a non-accurate value. By calling new_record? instead of, you would allow the record to sync its state with the transaction and return the expected true. It works on 4-2-stable and master.

@rafaelfranca #7807 is already closed based on #8577 (comment) and maybe this one should be too.

@repinel
Copy link
Member

repinel commented Jul 21, 2015

I take it back. The state of the record should probably be fully restored. This PR needs some work since the code base has change considerably.

@iantropov Would you mind if I give it a shot?

@repinel repinel mentioned this pull request Jul 24, 2015
Closed
@arthurnn
Copy link
Member

arthurnn commented Oct 4, 2015

Is this fixing this case too #20730 ?

@arthurnn arthurnn added the stale label Jan 13, 2016
@rafaelfranca rafaelfranca added stale and removed stale labels Jul 13, 2016
@rails-bot rails-bot closed this Jul 13, 2016
@rails-bot
Copy link

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arthurnn arthurnn

Labels
activerecord attached PR stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

State be restored to when transaction rolls back is not right?
8 participants
@iantropov @melekes @vipulnsward @rafaelfranca @repinel @arthurnn @rails-bot @maclover7