Skip to content

Conversation

@reuben453
Copy link

@reuben453 reuben453 commented Aug 2, 2021

When a new database record has a column that is set to its database default value, the "#{attribute}_changed?" method returns false, which causes the connection_default_value_defined value to be true and causes default_value_for to overwrite the database default.

This PR changes this behavior to not overwrite when the column is being set by the initialization attributes. It will continue to overwrite the database default if the attribute is not present in the initialization attributes.

Example code:

ActiveRecord::Base.connection.create_table(:books, :force => true) do |t|
  t.integer :count, :null => false, :default => 1
end

Book.default_value_for(:count, :allows_nil => false) { 1234 }

Book.new(count: 1).count # This should return `1`. Currently, this returns `1234`. This PR fixes this.
Book.new.count # This should continue to return `1234`.

def test_overwrites_string_blank_value_in_mass_assignment
Book.default_value_for(:type, :allows_nil => false) { 'string' }
assert_equal 'string', Book.new(type: '').type
end
Copy link
Author

Choose a reason for hiding this comment

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

I noticed that this case did not have a test, so added one here.

@reuben453 reuben453 marked this pull request as ready for review August 2, 2021 07:38
@reuben453 reuben453 force-pushed the master branch 2 times, most recently from 78b2524 to f77e56d Compare August 2, 2021 07:56
When a new database record has a column that is set to its database
default value, the "#{attribute}_changed?" method returns false,
which causes default_value_for to overwrite the database default.

This commit changes this behavior to not overwrite when the column
is being set by the initialization attributes.
@reuben453
Copy link
Author

@chrisarcand Would you mind having a look at these changes please? 🙏

@jrafanie
Copy link
Collaborator

@reuben453 sorry you haven't had an update on this PR. I'm now maintaining it in my free time. Is this still an issue for you? I see the test does still fail when I run master code against it.

I don't think the intention was to use column defaults with default_value_for. When I tested column defaults, it's great if it's supported in your database but it's limited in it's usage. On the other hand, it's in the database so likely more performant than doing things in ruby. I see you're intention was to use both but the default from the attributes should "win". I don't know if that's a good idea for this gem. I could be convinced though if you have use case to use both attribute defaults and default_value_for. My instincts say we should use attribute defaults where it works for us and use default_value_for to fill an use cases missing in attribute defaults.

I'll keep this open for a week but I think I'll close it unless we can better describe good use cases for using both sets of defaults. Thanks!

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.

2 participants