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

Allow AR::Enum definitions with boolean values #38086

Conversation

yhirano55
Copy link
Contributor

@yhirano55 yhirano55 commented Dec 24, 2019

Summary

If AR::Enum is used for boolean field, it would be not expected behavior for us.

fixes #38075

Problem

In case of using boolean for enum, we can set with string (hash key)
to instance, but we cannot set with actual value (hash value).

class Post < ActiveRecord::Base
  enum status: { enabled: true, disabled: false }
end

post.status = 'enabled'
post.status # 'enabled'

post.status = true
post.status # 'enabled'

post.status = 'disabled'
post.status # 'disabled'

post.status = false
post.status # nil (This is not expected behavior)

After looking into AR::Enum::EnumType#cast, I found that blank? method converts from false value to nil (it seems it may not intentional behavior).

def cast(value)
return if value.blank?
if mapping.has_key?(value)
value.to_s
elsif mapping.has_value?(value)
mapping.key(value)
else
assert_valid_value(value)
end
end

In this patch, I improved that if it defines enum with boolean, it returns reasonable behavior.

Importance (Why it would be better to merge this patch)

  • I am working on a legacy project that has enums with booleans as their values.
  • The project's team had never noticed this behavior before because they had been assigning values using the enum key strings, i.e. post.status = 'disabled'.
  • I stumbled upon this behavior when I wrote a test that called post.disabled!.
  • The assertion at assert_valid_enum_definition_values permits boolean-valued hashes, which tells me that this is a feature that we may want to support, so I made these changes to reflect that.

^ This messages are translated by my co-worker @rastamhadi

If `AR::Enum` is used for boolean field, it would be not expected
behavior for us.

fixes rails#38075

Problem:

In case of using boolean for enum, we can set with string (hash key)
to instance, but we cannot set with actual value (hash value).

```ruby
class Post < ActiveRecord::Base
  enum status: { enabled: true, disabled: false }
end

post.status = 'enabled'
post.status # 'enabled'

post.status = true
post.status # 'enabled'

post.status = 'disabled'
post.status # 'disabled'

post.status = false
post.status # nil (This is not expected behavior)
```

After looking into `AR::Enum::EnumType#cast`, I found that `blank?`
method converts from false value to nil (it seems it may not intentional behavior).

In this patch, I improved that if it defines enum with boolean,
it returns reasonable behavior.
@ugisozols
Copy link
Contributor

I understand the legacy project issue but using enum in a place where a boolean attribute should have been used in the first place feels very unnatural. It's also asking for some head-scratching down the road if someone adds more statuses like enum status: { enabled: true, disabled: false, pending: 1 }. Just my 2c ❤️ 😉

@kamipo
Copy link
Member

kamipo commented Dec 26, 2019

Can you add test mapping nil (@book.read_status = nil) too?
This implementation will also change the mapping nil behavior regardless of whether what you want.

This commit will be squished into f4fbdb1 after maintainer's review.
@yhirano55
Copy link
Contributor Author

@kamipo Thank you for the comment. I added a test case against mapping nil.

@rafaelfranca rafaelfranca merged commit fe097fa into rails:master Dec 27, 2019
kamipo added a commit to kamipo/rails that referenced this pull request Aug 7, 2020
Follow up to rails#38086.

User assigned nil is type casted by rails#38086, but loaded nil from database
does not yet, this fixes that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActiveRecord::NotNullViolation raised when update boolean field using enum
4 participants