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

Serialize symbols to strings in ImmutableString serialize method #36907

Conversation

@wjessop
Copy link
Contributor

commented Aug 10, 2019

Summary

Symbols serialize to strings when persisted:

model.some_string = :foo
model.save! # "foo" is persisted

This PR updates the immutable string class to serialize symbols to strings to mirror this behavior as ActiveModel::Attribute calls this serialize method to determine the return value for changed_in_place? Prior to this change this code would report that "something" had changed:

comment = Comment.create! # (has a string "something" field)
comment.update_column :something, :anything # persists "anything" to the "something" field of the comments table
comment.something  # or comment.attributes
comment.something_change # will be ["anything", "anything"], note these are `to` and `from` values, but are identical

After this PR the comment.something_change will return nil for this situation.

Fixes #36463

Other Information

I bisected and it looks like c342d58 introduced the change in behavior.

Symbols serialize to strings when persisted:

    model.some_string = :foo
    model.save! # "foo" is persisted

This PR updates the immutable string class to serialize symbols to strings to mirror this behavior as ActiveModel::Attribute calls this serialize method to determine the return value for changed_in_place? Prior to this change this code would report that "something" had changed:

    comment = Comment.create! # (has a string "something" field)
    comment.update_column :something, :anything # persists "anything" to the "something" field of the comments table
    comment.something  # or comment.attributes
    comment.something_change # will be ["anything", "anything"], note these are `to` and `from` values, but are identical

After this PR the comment.something_change will return nil for this situation.

Fixes #36463
@sikachu

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Given that 5.2 and 6.0 now have this current (broken?) behavior, I wonder if this behavior will now be considered as breaking change.

Maybe @rafaelfranca or @matthewd can help me decide?

@wjessop

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

The current behaviour is definitely broken (at least in my mind). I don't think I'd consider it a feature just because it's been around a while.

@matthewd

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Seems fair: we often work hard to make symbols and strings interchangeable in contexts where it makes sense, and given that, it seems pretty weird for us to claim a value has changed from & to the same value.

@matthewd matthewd merged commit ade4885 into rails:master Sep 28, 2019
2 checks passed
2 checks passed
buildkite/rails Build #62996 passed (9 minutes, 7 seconds)
Details
codeclimate All good!
Details
@wjessop wjessop deleted the wjessop:string_attribute_should_compare_with_typecast_symbol_after_update branch Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.