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

Restore frozen state on rollback, fixes #6417 #6420

Merged
merged 1 commit into from May 22, 2012

Conversation

Projects
None yet
2 participants
@chancancode
Member

chancancode commented May 21, 2012

My first patch for rails!

When saving a frozen? record, active record would raise an exception to complain about the frozen attributes, rollback the transaction. However, it also has the side effect of unfreezing the record after the rollback. This behaviour is undesirable and incorrect (saving a frozen? record twice in a roll would fail for the first time and succeed in the second time). This patch fixes the bug by explicitly restoring the frozen? state after a rollback.

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 21, 2012

Member

Can you add a few lines of explanation in commit message? Short description of behavior that this commit fixes.

Member

drogus commented May 21, 2012

Can you add a few lines of explanation in commit message? Short description of behavior that this commit fixes.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 21, 2012

Member

Rebased and fixed the commit message. Is this better?

Member

chancancode commented May 21, 2012

Rebased and fixed the commit message. Is this better?

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 21, 2012

Member

Sorry for being picky, but can you do it according to info added here: ae8b09c?

Member

drogus commented May 21, 2012

Sorry for being picky, but can you do it according to info added here: ae8b09c?

Restore the frozen state on rollback. Fixes #6417.
Currently, when saving a frozen record, an exception would be thrown
which causes a rollback. However, there is a bug in active record that
"defrost" the record as a side effect:

    >> t = Topic.new
    => #<Topic id: nil, ...>
    >> t.freeze
    => #<Topic id: nil, ...>
    >> t.save
    RuntimeError: can't modify a frozen Hash
    >> t.frozen?
    => false
    >> t.save
    => true

This patch fixes the bug by explictly restoring the frozen state on the
attributes Hash after every rollback.
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 21, 2012

Member

No worries, thanks for updating the docs. I was just looking through the pull requests for an example this morning. Can you review the message?

Member

chancancode commented May 21, 2012

No worries, thanks for updating the docs. I was just looking through the pull requests for an example this morning. Can you review the message?

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 22, 2012

Member

@chancancode looks great!

Member

drogus commented May 22, 2012

@chancancode looks great!

drogus added a commit that referenced this pull request May 22, 2012

Merge pull request #6420 from chancancode/master_restore_frozen_state…
…_on_rollback

Restore frozen state on rollback, fixes #6417

@drogus drogus merged commit 1447aca into rails:master May 22, 2012

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 22, 2012

Member

@drogus Thanks for the merge <3<3<3

Is this a valid candidate for a 3-2-stable backport? If so, should I apply the patch to 3-2-stable and open a separate pull request? I was trying to look up the rules/workflow for that but I couldn't seem to find that anywhere. If you can briefly explain it I could update the guides via docrails.

Member

chancancode commented May 22, 2012

@drogus Thanks for the merge <3<3<3

Is this a valid candidate for a 3-2-stable backport? If so, should I apply the patch to 3-2-stable and open a separate pull request? I was trying to look up the rules/workflow for that but I couldn't seem to find that anywhere. If you can briefly explain it I could update the guides via docrails.

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 22, 2012

Member

@chancancode yes, please open pull request. Basically, the general rule is that we can backport bugfixes and security fixes. Backports are sometimes skipped, especially if it's hard to backport and issue is not critical. There are no strict rules about this mostly because it depends on specific cases and on contributors - sometimes there is no interest in backporting a fix.

Member

drogus commented May 22, 2012

@chancancode yes, please open pull request. Basically, the general rule is that we can backport bugfixes and security fixes. Backports are sometimes skipped, especially if it's hard to backport and issue is not critical. There are no strict rules about this mostly because it depends on specific cases and on contributors - sometimes there is no interest in backporting a fix.

chancancode added a commit to chancancode/rails that referenced this pull request May 22, 2012

Restore the frozen state on rollback. Fixes rails#6417.
This is a 3-2-stable backport for rails#6420 which was merged into master.

Currently, when saving a frozen record, an exception would be thrown
which causes a rollback. However, there is a bug in active record that
"defrost" the record as a side effect:

    >> t = Topic.new
    => #<Topic id: nil, ...>
    >> t.freeze
    => #<Topic id: nil, ...>
    >> t.save
    RuntimeError: can't modify a frozen Hash
    >> t.frozen?
    => false
    >> t.save
    => true

This patch fixes the bug by explictly restoring the frozen state on the
attributes Hash after every rollback.

rafaelfranca added a commit that referenced this pull request May 22, 2012

Merge pull request #6445 from chancancode/3-2-stable_restore_frozen_s…
…tate_on_rollback

Restore the frozen state on rollback. (Backports #6420)

chancancode added a commit to chancancode/rails that referenced this pull request May 23, 2012

Added instructions for backporting changes to guides.
I was looking for instructions on backporting changes the other day and
wasn't able to find it anywhere. I updated the contrib guides based on
the disccusion in rails#6420, rails#6215 and rails#6447.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment