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

Do not overwrite the database default value if provided by attributes #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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