Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions docs/ActiveNode.rst
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ Or by using ``where``:
Media.as(:m).where('m.type <> ?', Media.types[:image])
# => CYPHER: "MATCH (result_media:`StoredFile`) WHERE (result_media.type <> 0)"

By default, every ``enum`` property will be defined as ``unique``, to improve query performances. If you want to disable this, simply pass ``_index: false`` to ``enum``:
By default, every ``enum`` property will require you to add an associated index to improve query performance. If you want to disable this, simply pass ``_index: false`` to ``enum``:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also updated the enum _index option docs


.. code-block:: ruby

Expand All @@ -200,16 +200,24 @@ By default, every ``enum`` property will be defined as ``unique``, to improve qu
enum type: [:image, :video, :unknown], _index: false
end

Sometimes it is desirable to have a default value for an ``enum`` property. To acheive this, you can simply define a property with the same name which defines a default value:
Sometimes it is desirable to have a default value for an ``enum`` property. To acheive this, you can simply pass the ``_default`` option when defining the enum:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was outdated

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm, interesting. I actually changed this recently because default wasn't working, but it might just be that I didn't know about the _default option. I think we tried default, so at some point in the future it might be good to have the underscored and non-underscored version of all of these options work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right. A _default option is whitelisted in the source and I assumed it worked. Just testing though, I see it does not work. I'll change it back to the old documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait no, the _default option does work. There's just a bug that if someone submits an enum param that isn't among the list of options, then the record is persisted with the first option. It should either be persisted with the _default option or it should raise an exception. Do you have an opinion either way?

Copy link
Contributor Author

@jorroll jorroll Sep 22, 2017

Choose a reason for hiding this comment

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

Also, regarding _default vs default, personally, I don't really like the idea of accepting both. More code would need to be added to make sure only one option was given and it feels like clutter. Given that this is already a breaking change, now might be the time to remove _ from all these options.

This being said, I'm happy to do it your way if that's what you decide. It would probably involve adding a normalize_options_listfunction to both normalize the new options and spit out depreciation warnings.

Copy link
Contributor Author

@jorroll jorroll Sep 22, 2017

Choose a reason for hiding this comment

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

So looking into persistence a bit more. Currently, if a _default option isn't set, and a submitted value doesn't match a permitted value, then the node will be persisted with a value of 0.

This is a bug because if someone has defined their enums using a hash like enum property: {one: 'ONE', two: 'TWO'} and execute Model.create(property: 'TEST'), then the object will be persisted with property = 0 when the only allowed values are 'ONE' or 'TWO'. It's a little hard to diagnose because the Neo4jrb object shows property = nil (but the db has property = 0).

This is what I think the fix should be:

  1. If _default isn't specified, and a given param doesn't match one of the values in the enum set, an exception should be raised. It could be an ArgumentError, but since these happen for a variety of reasons, I'm thinking a special InvalidEnumValueError (which could inherit ArgumentError) would be better (allowing someone to rescue that, specifically).
  2. If _default is specified, and a given param doesn't match one of the keys in the enum set (including if param = nil), then the _default value is used as if Model.create(property: default_value) had been called (i.e. default_value will be matched against the enum keys to determine what value should be persisted to the db).

The problem with the above, is if someone wants property to have a default value, but only if Model.create is called with a property key (i.e. they don't want every Model object to be persisted with a property value, but, if it is persisted with a property value, the default value should be x). This scenario could be solved manually by their rescuing InvalidEnumValueError, or we could provide a new option for it (default_if_present). I'm kinda thinking that we should just let people deal with that scenario themselves, as this is really a params issue and folks should validate their params so Model.create is never be called with an invalid option (and if it was, then that indicates something has gone wrong and its appropriate for an error to be raised).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, fair enough about supporting both. Let's stick with the underscore for now because it's what ActiveRecord does (even though it's really strange...)

Really good catch about it defaulting to 0 in all cases. That's pretty ugly...

One thing that I thought of: if we have a _default we should raise an exception if that isn't in the list of enum options.

I like 1), though regarding 2) I think that if you specify a _default and you give a value that isn't in the list of enums it should raise an exception.

In the end, I think you're absolutely right. If they give something that's not in the params list we should raise an exception. I definitely like the idea of having an InvalidEnumValueError which inherits from ArgumentError so that users can account for this.

Just checking in really quickly (still working through the backlog of issues and PRs, don't worry ;) ). Let me know if I missed anything here

Copy link
Contributor Author

@jorroll jorroll Sep 23, 2017

Choose a reason for hiding this comment

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

Will do. And no rush! I've thrown a lot at you this week 👍 .


.. code-block:: ruby

class Media
include Neo4j::ActiveNode

enum type: [:image, :video, :unknown], _default: :video
end

By default, enum setters are `case insensitive` (in the example below, ``Media.create(type: 'VIDEO').type == :video``). If you wish to disable this for a specific enum, pass the ``_case_sensitive: true`` option. if you wish to change the global default for ``_case_sensitive`` to ``true``, use Neo4jrb's ``enums_case_sensitive`` config option (detailed in the :ref:`configuration-variables` section).
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the global option, thanks!


.. code-block:: ruby

class Media
include Neo4j::ActiveNode

enum type: [:image, :video, :unknown]
property :type, default: :video
enum type: [:image, :video, :unknown], _case_sensitive: false
end

.. _activenode-scopes:
Expand Down
63 changes: 35 additions & 28 deletions docs/Configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,40 +18,40 @@ Variables

.. glossary::

**skip_migration_check**
**Default:** ``false``
**association_model_namespace**
Copy link
Contributor Author

@jorroll jorroll Sep 20, 2017

Choose a reason for hiding this comment

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

I took a moment to rearrange the Configuration::Variables section of the docs to be in alphabetical order (it seemed to be randomly ordered before)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

**Default:** ``nil``

Prevents the ``neo4j`` gem from raising ``Neo4j::PendingMigrationError`` in web requests when migrations haven't been run. For environments (like testing) where you need to use the ``neo4j:schema:load`` rake task to build the database instead of migrations. Automatically set to ``true`` in Rails test environments by default
Associations defined in node models will try to match association names to classes. For example, ``has_many :out, :student`` will look for a ``Student`` class. To avoid having to use ``model_class: 'MyModule::Student'``, this config option lets you specify the module that should be used globally for class name discovery.

.. _configuration-class_name_property:
Of course, even with this option set, you can always override it by calling ``model_class: 'ClassName'``.

**class_name_property**
**Default:** ``:_classname``

Which property should be used to determine the `ActiveNode` class to wrap the node in
Which property should be used to determine the ``ActiveNode`` class to wrap the node in

If there is no value for this property on a node the node`s labels will be used to determine the `ActiveNode` class
If there is no value for this property on a node the node`s labels will be used to determine the ``ActiveNode`` class

.. seealso:: :ref:`activenode-wrapping`

**enums_case_sensitive**
**Default:** ``false``

Determins whether enums property setters should be case sensitive or not.

.. seealso:: :ref:`activenode-enums`

**include_root_in_json**
**Default:** ``true``

When serializing ``ActiveNode`` and ``ActiveRel`` objects, should there be a root in the JSON of the model name.

.. seealso:: http://api.rubyonrails.org/classes/ActiveModel/Serializers/JSON.html

**transform_rel_type**
**Default:** ``:upcase``

**Available values:** ``:upcase``, ``:downcase``, ``:legacy``, ``:none``

Determines how relationship types as specified in associations are transformed when stored in the database. By default this is upper-case to match with Neo4j convention so if you specify an association of ``has_many :in, :posts, type: :has_post`` then the relationship type in the database will be ``HAS_POST``
**logger**
**Default:** ``nil`` (or ``Rails.logger`` in Rails)

``:legacy``
Causes the type to be downcased and preceded by a `#`
``:none``
Uses the type as specified
A Ruby ``Logger`` object which is used to log Cypher queries (`info` level is used). This is only for the ``neo4j`` gem (that is, for models created with the ``ActiveNode`` and ``ActiveRel`` modules).

**module_handling**
**Default:** ``:none``
Expand All @@ -62,18 +62,6 @@ Variables

The `:demodulize` option uses ActiveSupport's method of the same name to strip off modules. If you use a `proc`, it will the class name as an argument and you should return a string that modifies it as you see fit.

**association_model_namespace**
**Default:** ``nil``

Associations defined in node models will try to match association names to classes. For example, `has_many :out, :student` will look for a `Student` class. To avoid having to use `model_class: 'MyModule::Student'`, this config option lets you specify the module that should be used globally for class name discovery.

Of course, even with this option set, you can always override it by calling `model_class: 'ClassName'`.

**logger**
**Default:** ``nil`` (or ``Rails.logger`` in Rails)

A Ruby ``Logger`` object which is used to log Cypher queries (`info` level is used). This is only for the ``neo4j`` gem (that is, for models created with the ``ActiveNode`` and ``ActiveRel`` modules).

**pretty_logged_cypher_queries**
**Default:** ``nil``

Expand All @@ -84,11 +72,30 @@ Variables

A Rails-inspired configuration to manage inclusion of the Timestamps module. If set to true, all ActiveNode and ActiveRel models will include the Timestamps module and have ``:created_at`` and ``:updated_at`` properties.

**skip_migration_check**
**Default:** ``false``

Prevents the ``neo4j`` gem from raising ``Neo4j::PendingMigrationError`` in web requests when migrations haven't been run. For environments (like testing) where you need to use the ``neo4j:schema:load`` rake task to build the database instead of migrations. Automatically set to ``true`` in Rails test environments by default

.. _configuration-class_name_property:

**timestamp_type**
**Default:** ``DateTime``

This method returns the specified default type for the ``:created_at`` and ``:updated_at`` timestamps. You can also specify another type (e.g. ``Integer``).

**transform_rel_type**
**Default:** ``:upcase``

**Available values:** ``:upcase``, ``:downcase``, ``:legacy``, ``:none``

Determines how relationship types as specified in associations are transformed when stored in the database. By default this is upper-case to match with Neo4j convention so if you specify an association of ``has_many :in, :posts, type: :has_post`` then the relationship type in the database will be ``HAS_POST``

``:legacy``
Causes the type to be downcased and preceded by a `#`
``:none``
Uses the type as specified

**wait_for_connection**
**Default:** ``false``

Expand Down
4 changes: 4 additions & 0 deletions lib/neo4j/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ def association_model_namespace_string
return nil if namespace.nil?
"::#{namespace}"
end

def enums_case_sensitive
Neo4j::Config[:enums_case_sensitive] || false
end
end
end
end
34 changes: 27 additions & 7 deletions lib/neo4j/shared/enum.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module Enum
extend ActiveSupport::Concern

class ConflictingEnumMethodError < Neo4j::Error; end
class InvalidEnumValueError < Neo4j::InvalidParameterError; end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm inheriting from InvalidParameterError rather than ArgumentError. When I suggested ArgumentError, I didn't know about InvalidParameterError. Seeing as we're talking about passing an invalid enum value through a param, this seemed more appropriate. Let me know if you prefer ArgumentError though.


module ClassMethods
attr_reader :neo4j_enum_data
Expand Down Expand Up @@ -43,7 +44,7 @@ module ClassMethods
def enum(parameters = {})
options, parameters = *split_options_and_parameters(parameters)
parameters.each do |property_name, enum_keys|
enum_keys = normalize_key_list enum_keys
enum_keys = normalize_key_list enum_keys, options
@neo4j_enum_data ||= {}
@neo4j_enum_data[property_name] = enum_keys
define_property(property_name, enum_keys, options)
Expand All @@ -53,18 +54,28 @@ def enum(parameters = {})

protected

def normalize_key_list(enum_keys)
def normalize_key_list(enum_keys, options)
case_sensitive = options.fetch(:_case_sensitive, Neo4j::Config.enums_case_sensitive)

case enum_keys
when Hash
enum_keys
when Array
Hash[enum_keys.each_with_index.to_a]
enum_keys = Hash[enum_keys.each_with_index.to_a]
else
fail ArgumentError, 'Invalid parameter for enum. Please provide an Array or an Hash.'
end

unless case_sensitive
enum_keys.keys.each do |key|
fail ArgumentError, 'Enum keys must be lowercase unless _case_sensitive = true' unless key.downcase == key
end
end

enum_keys
end

VALID_OPTIONS_FOR_ENUMS = [:_index, :_prefix, :_suffix, :_default]
VALID_OPTIONS_FOR_ENUMS = [:_index, :_prefix, :_suffix, :_default, :_case_sensitive]

def split_options_and_parameters(parameters)
options = {}
Expand All @@ -80,9 +91,8 @@ def split_options_and_parameters(parameters)
end

def define_property(property_name, enum_keys, options)
property_options = build_property_options(enum_keys, options)
property property_name, property_options
serialize property_name, Neo4j::Shared::TypeConverters::EnumConverter.new(enum_keys, property_options)
property property_name, build_property_options(enum_keys, options)
serialize property_name, Neo4j::Shared::TypeConverters::EnumConverter.new(enum_keys, build_enum_options(enum_keys, options))
end

def build_property_options(_enum_keys, options = {})
Expand All @@ -91,6 +101,16 @@ def build_property_options(_enum_keys, options = {})
}
end

def build_enum_options(enum_keys, options = {})
if options[:_default] && not(enum_keys.include?(options[:_default]))
fail ArgumentError, 'Enum default value must match an enum key'
end

build_property_options(enum_keys, options).tap do |enum_options|
enum_options[:case_sensitive] = options[:_case_sensitive]
end
end

def define_enum_methods(property_name, enum_keys, options)
define_enum_methods_?(property_name, enum_keys, options)
define_enum_methods_!(property_name, enum_keys, options)
Expand Down
10 changes: 9 additions & 1 deletion lib/neo4j/shared/type_converters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ class EnumConverter
def initialize(enum_keys, options)
@enum_keys = enum_keys
@options = options

return unless @options[:case_sensitive].nil?

@options[:case_sensitive] = Neo4j::Config.enums_case_sensitive
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of these two lines, what about just:

@options[:case_sensitive] ||= Neo4j::Config.enums_case_sensitive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work because if @options[:case_sensitive] == false and Neo4j::Config.enums_case_sensitive == true, then @options[:case_sensitive] will be set to true when it should remain false (i.e. local settings should always take precedence over global settings). As it stands, global settings only kick in if @options[:case_sensitive].nil?.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, good point ;)

end

def converted?(value)
Expand Down Expand Up @@ -298,8 +302,12 @@ def to_ruby(value)
def to_db(value)
if value.is_a?(Array)
value.map(&method(:to_db))
elsif @options[:case_sensitive]
@enum_keys[value.to_s.to_sym] ||
fail(Neo4j::Shared::Enum::InvalidEnumValueError, 'Value passed to an enum property must match one of the enum keys')
else
@enum_keys[value.to_s.to_sym] || 0
@enum_keys[value.to_s.downcase.to_sym] ||
fail(Neo4j::Shared::Enum::InvalidEnumValueError, 'Case-insensitive (downcased) value passed to an enum property must match one of the enum keys')
end
end
end
Expand Down
60 changes: 60 additions & 0 deletions spec/e2e/enum_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
enum type: [:unknown, :image, :video], _default: :unknown
enum size: {big: 100, medium: 7, small: 2}, _prefix: :dimension
enum flag: [:clean, :dangerous], _suffix: true
enum type_format: [:Mpeg, :Png], _case_sensitive: true, _index: false

has_one :in, :uploader, rel_class: :UploaderRel
end
Expand All @@ -32,8 +33,21 @@
expect(StoredFile.types).to eq(unknown: 0, image: 1, video: 2)
expect(StoredFile.sizes).to eq(big: 100, medium: 7, small: 2)
expect(StoredFile.flags).to eq(clean: 0, dangerous: 1)
expect(StoredFile.type_formats).to eq(Mpeg: 0, Png: 1)
expect(UploaderRel.origins).to eq(disk: 0, web: 1)
end

it 'respects _index = false option' do
expect { StoredFile.as(:f).pluck('f.type_format') }.to_not raise_error
end

it 'raises error if keys are invalid' do
expect { StoredFile.enum something: [:value1, :Value1] }.to raise_error(ArgumentError)
end

it "raises error if _default option doesn't match key" do
expect { StoredFile.enum something: [:value1, :value2], _default: :value3 }.to raise_error(ArgumentError)
end
end

describe 'getters and setters' do
Expand Down Expand Up @@ -68,6 +82,52 @@
expect(StoredFile.as(:f).where(id: file.id).pluck('f.flag')).to eq([nil])
expect(file.reload.flag).to eq(nil)
end

it "raises error if value doesn't match an enum key" do
file = StoredFile.new
file.type = :audio
expect { file.save! }.to raise_error(
Neo4j::Shared::Enum::InvalidEnumValueError,
'Case-insensitive (downcased) value passed to an enum property must match one of the enum keys'
)
end

it 'respects local _case_sensitive option' do
file = StoredFile.new
file.type_format = :png
expect { file.save! }.to raise_error(Neo4j::Shared::Enum::InvalidEnumValueError, 'Value passed to an enum property must match one of the enum keys')

file.type_format = :Png
file.save!
expect(StoredFile.as(:f).pluck('f.type_format')).to eq([1])
expect(file.reload.type_format).to eq(:Png)
end

it 'respects global _case_sensitive = false default' do
file = StoredFile.new
file.type = :VIdeO
file.save!
expect(StoredFile.as(:f).pluck('f.type')).to eq([2])
expect(file.reload.type).to eq(:video)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

You could test the configuration by doing:

context 'global enums_case_sensitive config is set to true' do
  let_config(:enums_case_sensitive) { true }
  # spec here testing, for example, that you can't set `type` to `VIdeO`
end

We define the let_config helper here:

https://github.com/neo4jrb/neo4j/blob/master/spec/spec_helper.rb#L135

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thats cool


context 'global enums_case_sensitive config is set to true' do
let_config(:enums_case_sensitive, true) do
it 'respects global _case_sensitive = true default' do
file = StoredFile.new
file.type = :VIdeO
expect { file.save! }.to raise_error(Neo4j::Shared::Enum::InvalidEnumValueError, 'Value passed to an enum property must match one of the enum keys')
end

it 'still accepts valid params' do
file = StoredFile.new
file.type = :video
file.save!
expect(StoredFile.as(:f).pluck('f.type')).to eq([2])
expect(file.reload.type).to eq(:video)
end
end
end
end

describe 'scopes' do
Expand Down