breaking change: enums case insensitive by default #1419

jorroll · 2017-09-20T11:49:07Z

Enums are now by default case insensitive. To make things easy for everyone, I've added a global config option enums_case_sensitive if people want to revert to the old standard. This being said, this is still a breaking change to Neo4jrb. I've also updated the docs.

Fixes #1418

This pull introduces/changes:

enums setters are now case insensitive by default
to override this locally, you can pass _case_sensitive: true when defining an enum.
to override this default globally, you can set config.neo4j.enums_case_sensitive = true
unless the enum is case sensitive, it's keys must be downcased or an exception will be raised

Pings:
@cheerfulstoic
@subvertallchris

jorroll · 2017-09-20T11:59:05Z

docs/Configuration.rst


-  **skip_migration_check**
-    **Default:** ``false``
+  **association_model_namespace**


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)

jorroll · 2017-09-20T11:59:57Z

docs/ActiveNode.rst

    # => 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``:


Also updated the enum _index option docs

jorroll · 2017-09-20T12:00:36Z

docs/ActiveNode.rst

    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:


this was outdated

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

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.

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?

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.

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:

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).

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).

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

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

codecov-io · 2017-09-20T12:10:07Z

Codecov Report

Merging #1419 into master will decrease coverage by 0.06%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #1419      +/-   ##
==========================================
- Coverage   96.87%   96.81%   -0.07%     
==========================================
  Files         205      205              
  Lines       12457    12511      +54     
==========================================
+ Hits        12068    12112      +44     
- Misses        389      399      +10

Impacted Files	Coverage Δ
lib/neo4j/shared/type_converters.rb	`95.79% <100%> (+0.08%)`	⬆️
lib/neo4j/shared/enum.rb	`98.63% <100%> (+0.21%)`	⬆️
lib/neo4j/config.rb	`93.87% <100%> (+0.26%)`	⬆️
spec/e2e/enum_spec.rb	`92.06% <73.68%> (-4.63%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e4f5af...9d56246. Read the comment docs.

ghost · 2017-09-20T17:39:41Z

Is someone able to describe the use case for this change beyond it being "nice to do"?

jorroll · 2017-09-20T18:02:08Z

@brandon-zeeb-nw, in languages with an enum type, convention is for enum variables be given in screaming case (which is case insensitive / only one case). Currently, if you want your neo4jrb enums to accept screaming case params, you need to define the enums in screaming case enum property: [:OPTION_ONE, :OPTION_TWO] which produces related methods in the form Model.OPTION_ONE or Model.prefix_OPTION_ONE which break ruby conventions.

This change allows you to define enum, property: [:option_one, :option_two], still accept Model.create(property: 'OPTION_ONE') or Model.create(property: 'option_one'), and interact with it Model.prefix_option_one. Given that the property will be persisted to the DB as an Integer, the capitalization of the name really shouldn't matter unless you have options that are only differentiated by capitalization--which I imagine is an unusual use case.

For folks who reject this change, a global config is available for them to change the default back to case sensitive.

Personally, I would describe the current behavior of allowing case sensitive enums to be a bug.

jorroll · 2017-09-20T18:04:19Z

@brandon-zeeb-nw this being said, do you have a use case for case sensitive enums?

ghost · 2017-09-20T18:56:24Z

thanks @thefliik, that helps. I fully support the idea of sticking to conventions, but I'm not sure this is the way. This feels more like a mapping problem to me, to map the ruby conventions into a normalized form and back. Case insensitivity is a way around it, Imo. That said I do fully support the spirit of the change.

jorroll · 2017-09-20T19:10:21Z

I thought about simply normalizing the method names and not the enums themselves, but enums should be case insensitive, per convention (well, all caps). This strategy encourages developers to use enums that fall in line with other languages--especially useful if 3rd party clients will be sending the params and you can't guarantee their formatting.

ghost · 2017-09-20T20:20:16Z

@thefliik I had a somewhat incomplete view of the angle on this change, I retract my objection, I think your change is pretty legit. Thanks for working with me.

jorroll · 2017-09-20T20:21:52Z

@brandon-zeeb-nw for sure!

cheerfulstoic

Just a few comments. Looks great otherwise, thanks!

cheerfulstoic · 2017-09-21T17:48:26Z

docs/ActiveNode.rst

    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:


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

cheerfulstoic · 2017-09-21T17:49:01Z

docs/ActiveNode.rst

+      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).


Love the global option, thanks!

cheerfulstoic · 2017-09-21T17:49:55Z

docs/Configuration.rst


-  **skip_migration_check**
-    **Default:** ``false``
+  **association_model_namespace**


cheerfulstoic · 2017-09-22T01:10:47Z

lib/neo4j/shared/enum.rb

-      def normalize_key_list(enum_keys)
+      def normalize_key_list(enum_keys, options)
+        case_sensitive = options[:_case_sensitive]
+        case_sensitive = Neo4j::Config.enums_case_sensitive if case_sensitive.nil?


You could do

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

or

options.fetch(:_case_sensitive, Neo4j::Config.enums_case_sensitive)

The difference between the two is that the first would fall back to Neo4j::Config.enums_case_sensitive if you get a nil value from options[:_case_sensitive] regardless of if there was no key or if there was a key but it just happened to be nil. In the second one if options[:_case_sensitive] is set to nil then the result will be nil because fetch only falls back to the default when the key isn't set in the hash.

I'll update it to use fetch, the first one doesn't quite work because if options[:_case_sensitive] = false and Neo4j::Config.enums_case_sensitive = true, then case_sensitive = true, when it should equal false. Because case_sensitive is only used in unless case_sensitive, it's ok for case_sensitive to be nil in the unlikely (incorrect) event that someone sets _case_sensitive: nil locally (though the way it is currently ensures that case_sensitive is never nil).

cheerfulstoic · 2017-09-22T01:15:46Z

lib/neo4j/shared/enum.rb

+      def build_enum_options(enum_keys, options = {})
+        enum_options = build_property_options(enum_keys, options)
+        enum_options[:case_sensitive] = options[:_case_sensitive]
+        enum_options


Maybe a bit cleaner:

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

cheerfulstoic · 2017-09-22T01:21:32Z

spec/e2e/enum_spec.rb

+      expect(StoredFile.type_formats).to eq(Mpeg: 0, Png: 1)
      expect(UploaderRel.origins).to eq(disk: 0, web: 1)
    end
  end


There should be a spec somewhere is this file which tests that the ArgumentError gets raised when a non-lowercase option is passed to enum. Probably you can dynamically, in the spec, call StoredFile.enum something: [:value1, :Value1]

cheerfulstoic · 2017-09-22T01:23:32Z

Oh, also! The build is failing because rubocop is failing:

https://travis-ci.org/neo4jrb/neo4j/builds/278210221?utm_source=github_status&utm_medium=notification

If you run rubocop -aD locally it will fix any rubocop issues that it can automagically and tell you about the ones that it couldn't fix. They have pretty good documentation on what the different rules are about, but let me know if you have any questions or need any help.

jorroll · 2017-09-22T02:22:41Z

Ya, I have overcommit installed locally so I was under the impression that rubocop was automatically run on commit? It's certainly stopped a few commits because of whitespace errors. This commit ran fine locally though, so I was a bit surprised to see the Travis failure. Or does overcommit run a stripped down version of the rubocop checks?

cheerfulstoic · 2017-09-22T21:02:41Z

That's certainly odd... overcommit should catch rubocop issues. It basically just runs on the files that you have staged when you go to commit. I think I've noticed that when I have multiple versions of rubocop on my system overcommit doesn't do a bundle exec and so it might pull in the wrong version (but that's a long shot guess)

jorroll · 2017-09-23T02:47:46Z

Well I confirmed that overcommit is letting stuff through when I committed #1422. I first ran the commit. overcommit fixed some stuff then successfully committed. I then ran rubocob and ran into errors that I needed to fix.

It's definitely not a rubocob version problem, and this is running inside a VM containing nothing but the Neo4j repo.

jorroll · 2017-09-23T05:26:13Z

lib/neo4j/shared/enum.rb

    extend ActiveSupport::Concern

    class ConflictingEnumMethodError < Neo4j::Error; end
+    class InvalidEnumValueError < Neo4j::InvalidParameterError; end


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.

cheerfulstoic · 2017-09-23T17:58:42Z

I just resolved a conflict in the Vagrantfile that looked like this:

<<<<<<< case_insensitive_enums
  config.vm.network 'forwarded_port', guest: 7474, host: 7474, host_ip: '127.0.0.1'
  config.vm.network 'forwarded_port', guest: 7473, host: 7473, host_ip: '127.0.0.1'
  config.vm.network 'forwarded_port', guest: 7472, host: 7472, host_ip: '127.0.0.1'
=======
  config.vm.network 'forwarded_port', guest: 7474, host: 9474, host_ip: '127.0.0.1'
  config.vm.network 'forwarded_port', guest: 7473, host: 9473, host_ip: '127.0.0.1'
  config.vm.network 'forwarded_port', guest: 7472, host: 9472, host_ip: '127.0.0.1'
>>>>>>> master

I went with the master block, but I wanted to let you know in case that was wrong.

cheerfulstoic · 2017-09-23T17:59:23Z

Let me know if you're done with this PR and I'll take another look!

jorroll · 2017-09-23T22:31:50Z

I just resolved a conflict in the Vagrantfile that looked like this:

Ooopss!! The change from 7474 --> 9474, etc, wasn't suppose to be committed! I just changed that on my personal computer to avoid a port conflict with multiple instances of Neo4j. That was a big mistake on my part. Very sorry. Thanks for calling that out!

I'm not sure where I made that mistake, but, excluding it, this PR should be finished. Let me know if I need to change those numbers back to 7474,7473,7472 somewhere.

cheerfulstoic

I'm really happy with this, thanks! I have a few changes which I think are pretty minor and then I'd be happy to merge

cheerfulstoic · 2017-09-24T02:38:31Z

lib/neo4j/shared/type_converters.rb

+
+        return unless @options[:case_sensitive].nil?
+
+        @options[:case_sensitive] = Neo4j::Config.enums_case_sensitive


Instead of these two lines, what about just:

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

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?.

Ah, right, good point ;)

cheerfulstoic · 2017-09-24T02:46:26Z

spec/e2e/enum_spec.rb

+      file.save!
+      expect(StoredFile.as(:f).pluck('f.type')).to eq([2])
+      expect(file.reload.type).to eq(:video)
+    end


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

Oh thats cool

cheerfulstoic · 2017-09-24T02:47:59Z

spec/e2e/enum_spec.rb

+    it 'respects local _case_sensitive option' do
+      file = StoredFile.new
+      file.type_format = :png
+      expect { file.save! }.to raise_error(Neo4j::Shared::Enum::InvalidEnumValueError)


For a stronger assertion this could be:

expect { file.save! }.to raise_error(Neo4j::Shared::Enum::InvalidEnumValueError, 'Value passed to an enum property must match one of the enum keys')

cheerfulstoic · 2017-09-24T02:48:20Z

spec/e2e/enum_spec.rb

+    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


For a stronger assertion this could be:

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')

…ive_enums

…to case_insensitive_enums

jorroll · 2017-09-24T16:31:21Z

Ok, I think its done. Let me know if there's anything else though.

cheerfulstoic · 2017-09-25T16:45:48Z

Excellent, thanks!

cheerfulstoic · 2017-09-25T17:03:58Z

I believe that #1414 and #1424 (however we go with that) would be backwards-compatible, right? If so I'll release this as 9.0.0 separately

jorroll · 2017-09-25T17:08:00Z

Yes, both of those would be backwards-compatible. I'm not sure if #1428 is backwards compatible or not. Depends on whether the current behavior is considered a bug (though, even if its a bug, its so pervasive fixing it might be considered a breaking change).

John Carroll and others added 2 commits September 20, 2017 11:35

add: case sensitive option for enums

4535277

upd: fix formatting

8caba45

jorroll commented Sep 20, 2017

View reviewed changes

jorroll changed the title ~~add: case sensitive option for enums~~ major change: enums case insensitive by default Sep 20, 2017

jorroll changed the title ~~major change: enums case insensitive by default~~ breaking change: enums case insensitive by default Sep 20, 2017

jorroll mentioned this pull request Sep 20, 2017

feature: case-insensitive enums #1418

Closed

John Carroll added 2 commits September 21, 2017 10:08

fix: potential bug if enums keys upcased but _case_sensitive = false

6c9ea56

Fix: rubocop violations

11d0e38

cheerfulstoic suggested changes Sep 22, 2017

View reviewed changes

Upd: incorporated review comments

9635450

John Carroll added 4 commits September 23, 2017 03:59

merged master updates

372f75d

Upd: validate enum params & args and throw errors as appropriate

78289cb

Fix: rubocop violations

06e1a84

Add: enum value & key validation specs

a592a91

jorroll mentioned this pull request Sep 23, 2017

Fix: Vagrantfile rubocop violations #1427

Merged

jorroll commented Sep 23, 2017

View reviewed changes

Merge branch 'master' into case_insensitive_enums

a5466bc

cheerfulstoic mentioned this pull request Sep 23, 2017

Add: has_one(chainable: true) option #1422

Merged

cheerfulstoic suggested changes Sep 24, 2017

View reviewed changes

jorroll mentioned this pull request Sep 24, 2017

Fix / upd: properties declared on parent after inheritance takes place are now passed to children #1428

Merged

John Carroll added 4 commits September 24, 2017 15:53

integrating upstream changes

2e94657

Upd: case-insensitive enum specs

24d5a14

Merge branch 'master' of github.com:thefliik/neo4j into case_insensit…

c61390d

…ive_enums

Merge branch 'case_insensitive_enums' of github.com:thefliik/neo4j in…

9d56246

…to case_insensitive_enums

cheerfulstoic approved these changes Sep 25, 2017

View reviewed changes

cheerfulstoic merged commit 55445bf into neo4jrb:master Sep 25, 2017

jorroll deleted the case_insensitive_enums branch September 25, 2017 17:11


		return unless @options[:case_sensitive].nil?

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

breaking change: enums case insensitive by default #1419

breaking change: enums case insensitive by default #1419

Uh oh!

Conversation

jorroll commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorroll Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorroll Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorroll Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorroll Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ghost commented Sep 20, 2017

Uh oh!

jorroll commented Sep 20, 2017

Uh oh!

jorroll commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 20, 2017

Uh oh!

jorroll commented Sep 20, 2017

Uh oh!

ghost commented Sep 20, 2017

Uh oh!

jorroll commented Sep 20, 2017

Uh oh!

cheerfulstoic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorroll Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheerfulstoic commented Sep 22, 2017

Uh oh!

jorroll commented Sep 22, 2017

jorroll commented Sep 20, 2017 •

edited

Loading

jorroll Sep 20, 2017 •

edited

Loading

jorroll Sep 22, 2017 •

edited

Loading

jorroll Sep 22, 2017 •

edited

Loading

jorroll Sep 23, 2017 •

edited

Loading

codecov-io commented Sep 20, 2017 •

edited

Loading

jorroll commented Sep 20, 2017 •

edited

Loading

jorroll Sep 22, 2017 •

edited

Loading

jorroll commented Sep 23, 2017 •

edited

Loading