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 ourterms of serviceand privacy statement.We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "ActiveModel::Attribute: elide dup for immutable types" #53347

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Revert "ActiveModel::Attribute: elide dup for immutable types" #53347

merged 3 commits into from
Oct 17, 2024
+31 −22

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Oct 16, 2024 •
edited
Loading

Reverts#53337
Fixes#53343

cc@casperisfine@tenderlove

I think the PR is a good idea but it introduces some dangerous behaviour. With not respecting AR config changes. By not deep-duping the Time attribute we end up stuck on however it is cast originally.

>> Default._default_attributes[ "fixed_time" ]
=>
#<ActiveModel::Attribute::FromDatabase:0x000000012dc92d58
@name= "fixed_time",
@original_attribute=nil,
@type=
#<ActiveRecord::Type::DateTime:0x000000012ad7ca50 @limit=nil, @precision=6, @scale=nil, @timezone=nil>,
@value_before_type_cast= "2004-01-01 00:00:00" >
>> Default.new.fixed_time
=> 2004-01-01 00:00:00 UTC
>> Default._default_attributes[ "fixed_time" ]
=>
#<ActiveModel::Attribute::FromDatabase:0x000000012dc92d58
@name= "fixed_time",
@original_attribute=nil,
@type=
#<ActiveRecord::Type::DateTime:0x000000012ad7ca50 @limit=nil, @precision=6, @scale=nil, @timezone=nil>,
@value=2004-01-01 00:00:00 UTC, <--- @value is now set because the attribute was shared
@value_before_type_cast= "2004-01-01 00:00:00" >

This ends up reusing Time objects which isn't safe because they're unfortunately mutable (though the attributes API doesn't tag them as such, though maybe that's what we want as theirvalueisn't changed just their zone).

>> Default.new.fixed_time.localtime
=> 2003-12-31 16:00:00 -0800
>> Default.new.fixed_time
=> 2003-12-31 16:00:00 -0800
>> Default.new.fixed_time
=> 2003-12-31 16:00:00 -0800
>> Default.new.fixed_time.utc
=> 2004-01-01 00:00:00 UTC
>> Default.new.fixed_time
=> 2004-01-01 00:00:00 UTC
>> Default.new.fixed_time
=> 2004-01-01 00:00:00 UTC

Maybe it would be safer/more efficient to replacedeep_duponly on the attributes we know it to be safe for as this feels like a separate (but very similar) concern tomutable?.

@jhawthorn jhawthorn added this to the8.0.0milestone Oct 16, 2024
@casperisfine
Copy link
Contributor

Ah good catch, I always forget Time's#zoneis mutable 😢

@byroot byroot merged commitee66185 into main Oct 17, 2024
5 checks passed
@byroot byroot deleted the revert-53337-opt-attr-dup branch October 17, 2024 06:32
@casperisfine
Copy link
Contributor

Maybe it would be safer/more efficient to replacedeep_duponly on the attributes we know it to be safe for as this feels like a separate (but very similar) concern tomutable?.

I don't know. It feels likeTimeshould be marked asmutable?,no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
activemodel activerecord
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jhawthorn @casperisfine @byroot