-
Notifications
You must be signed in to change notification settings - Fork 165
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
make sure getters always return Symbols #30
base: master
Are you sure you want to change the base?
Conversation
@@ -71,12 +71,12 @@ def define_field_accessor(name, field_name, options) | |||
|
|||
def define_array_field_accessor(name, field_name) | |||
class_eval "def #{name}=(vals) self.write_attribute(:#{field_name}, Array(vals).compact.map(&:to_sym)) end" | |||
class_eval "def #{name}() self.read_attribute(:#{field_name}) end" | |||
class_eval "def #{name}() self.send(:#{field_name}).map{ |i| i.try(:to_sym) } end" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [90/80]
+1. Monkey-patched in my project with this module. The solution in this PR resulted in stack overflow: module CoreExtensions
module Mongoid
module Enum
module EnsureSymbol
extend ActiveSupport::Concern
included do |base|
base.instance_eval do
def define_array_field_accessor(name, field_name)
class_eval <<-EORUBY, __FILE__, __LINE__ + 1
def #{name}=(vals)
self.write_attribute(:#{field_name}, Array(vals).compact.map(&:to_sym))
end
def #{name}()
self.read_attribute(:#{field_name}).map{ |i| i.try(:to_sym) }
end
EORUBY
end
def define_string_field_accessor(name, field_name)
class_eval <<-EORUBY, __FILE__, __LINE__ + 1
def #{name}=(val)
self.write_attribute(:#{field_name}, val.try(:to_sym))
end
def #{name}()
self.read_attribute(:#{field_name}).try(:to_sym)
end
EORUBY
end
end
end
end
end
end
end Applied by: Mongoid::Enum.include CoreExtensions::Mongoid::Enum::EnsureSymbol |
any chance to have this merged? |
* Refer to constant instead of passing around the values
validates field_name, :inclusion => {:in => values.map(&:to_sym)}, :allow_nil => !options[:required] | ||
validates field_name, :'mongoid/enum/validators/multiple' => { :in => self.const_get(name.to_s.upcase), :allow_nil => !options[:required] } | ||
elsif options[:validate] | ||
validates field_name, :inclusion => {:in => self.const_get(name.to_s.upcase)}, :allow_nil => !options[:required] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [122/80]
Use the new Ruby 1.9 hash syntax.
Redundant self
detected.
Space inside { missing.
Space inside } missing.
can we have this merged please? |
I'd question this change vs a change that converts everything to strings. The reason is that the "symbol" type is deprecated by BSON and is deprecated in Mongo 3.2. I opened an issue specifically for this to discuss: #41 |
@jeremyhaile I am not quite sure whether changes on the db level should have impact on how the enum abstraction works in Ruby (unless of course there is deprecation of Symbols in Ruby itself). Perhaps a way forward might be to add config option (Symbol/String)? |
We have ran into several issues when relying on
enum
to returnSymbol
type.This PR replaces
read_attribute
bysend
, since theread_attribute
returns unprocessed (raw) value before typecast (demongoize
) and therefore can potentially return something else thanSymbol
.Also, this PR makes sure all Array values (in multiple option) are
Symbols
as well.For your reference, here you can see how Mongoid uses
read_attribute
to get raw value which is then processed to a proper type usingdemongoize
: https://github.com/mongodb/mongoid/blob/master/lib/mongoid/fields.rb#L411-L424This is extermely important especially now, when
Symbols
are being deprecated and an app using Mongoid can potentially start returningStrings
instead ofSymbols
. See here: https://jira.mongodb.org/browse/RUBY-1075?jql=project%20%3D%20RUBY%20AND%20resolution%20%3D%20Unresolved%20ORDER%20BY%20priority%20DESC%2C%20key%20DESC