Undeclared properties #1294

Merged
merged 6 commits into from Sep 29, 2016

Conversation

Projects
None yet
5 participants
@klobuczek
Contributor

klobuczek commented Sep 22, 2016

Fixes #
Introduces handling of undeclared properties via optional Neo4j::UndeclaredProperties module.
This pull introduces/changes:

  • I have tried to keep existing code without any functional change with one exception. I fixed a problem where _persisted_obj was not refreshed after model update
  • All new functionality is in a separate optional module which if included changes the behavior of the basic model manipulation method.

Pings:
@cheerfulstoic
@subvertallchris

@klobuczek

This comment has been minimized.

Show comment
Hide comment
@klobuczek

klobuczek Sep 23, 2016

Contributor

Looking for suggestion how to fix the 1 rubocop offense, which is a too long module. I just introduced the absolutely necessary hooks. I could split the persistence module into create and update persistence.

Contributor

klobuczek commented Sep 23, 2016

Looking for suggestion how to fix the 1 rubocop offense, which is a too long module. I just introduced the absolutely necessary hooks. I could split the persistence module into create and update persistence.

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Sep 23, 2016

Member

I really like the way you implemented this!

Out of curiosity, would it be preferable to require that undeclared properties be set and accessed through dedicated methods instead of mixed with existing mass-assignment? My concern is that this approach could lead to accidental, invisible setting of incorrect keys. If the user knows that they are going to be adding keys dynamically, it seems like they should be able to programmatically assign them separately. What do you think?

Full disclosure, I am not exactly the target audience for this, as I think that undeclared properties are pure evil. I won't stand in the way of a merge if others give their +1. ;-)

Member

subvertallchris commented Sep 23, 2016

I really like the way you implemented this!

Out of curiosity, would it be preferable to require that undeclared properties be set and accessed through dedicated methods instead of mixed with existing mass-assignment? My concern is that this approach could lead to accidental, invisible setting of incorrect keys. If the user knows that they are going to be adding keys dynamically, it seems like they should be able to programmatically assign them separately. What do you think?

Full disclosure, I am not exactly the target audience for this, as I think that undeclared properties are pure evil. I won't stand in the way of a merge if others give their +1. ;-)

@klobuczek

This comment has been minimized.

Show comment
Hide comment
@klobuczek

klobuczek Sep 23, 2016

Contributor

@subvertallchris I couldn't think of more intuitive way than using the standard update_attributes method. This is the way active_node gem was doing it which was inspired by mongo_mapper. Both being obsolete meanwhile I just looked at mongoid and they are even going a step further. They are even generating the accessor methods for dynamic attributes if the Dynamic module is included. https://docs.mongodb.com/ruby-driver/master/tutorials/6.0.0/mongoid-documents/#dynamic-fields . So multiple places did it intuitively in a similar way.

Furthermore, there is an additional safety net in form of permitted attributes in rails controllers. So in our case, those fields are not totally wild. The superset of allowed undeclared properties is restricted on user/client basis.

Contributor

klobuczek commented Sep 23, 2016

@subvertallchris I couldn't think of more intuitive way than using the standard update_attributes method. This is the way active_node gem was doing it which was inspired by mongo_mapper. Both being obsolete meanwhile I just looked at mongoid and they are even going a step further. They are even generating the accessor methods for dynamic attributes if the Dynamic module is included. https://docs.mongodb.com/ruby-driver/master/tutorials/6.0.0/mongoid-documents/#dynamic-fields . So multiple places did it intuitively in a similar way.

Furthermore, there is an additional safety net in form of permitted attributes in rails controllers. So in our case, those fields are not totally wild. The superset of allowed undeclared properties is restricted on user/client basis.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 23, 2016

Current coverage is 97.01% (diff: 100%)

Merging #1294 into master will increase coverage by 0.02%

@@             master      #1294   diff @@
==========================================
  Files           199        201     +2   
  Lines         12051      12148    +97   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          11688      11785    +97   
  Misses          363        363          
  Partials          0          0          

Powered by Codecov. Last update 2deda24...f3c9b63

codecov-io commented Sep 23, 2016

Current coverage is 97.01% (diff: 100%)

Merging #1294 into master will increase coverage by 0.02%

@@             master      #1294   diff @@
==========================================
  Files           199        201     +2   
  Lines         12051      12148    +97   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          11688      11785    +97   
  Misses          363        363          
  Partials          0          0          

Powered by Codecov. Last update 2deda24...f3c9b63

@subvertallchris

This comment has been minimized.

Show comment
Hide comment
@subvertallchris

subvertallchris Sep 26, 2016

Member

Got it, thanks for the background info. 👍 as far as I'm concerned, but @cheerfulstoic and/or @ProGM should probably chime in, too.

Member

subvertallchris commented Sep 26, 2016

Got it, thanks for the background info. 👍 as far as I'm concerned, but @cheerfulstoic and/or @ProGM should probably chime in, too.

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Sep 28, 2016

Member

I also really like how this is an optional module. I looked through the code and couldn't find anything bad, so 👍 from me. I'd like a final opinion from @ProGM though

Member

cheerfulstoic commented Sep 28, 2016

I also really like how this is an optional module. I looked through the code and couldn't find anything bad, so 👍 from me. I'd like a final opinion from @ProGM though

@ProGM

This comment has been minimized.

Show comment
Hide comment
@ProGM

ProGM Sep 29, 2016

Member

I like this!
The fact of being a separate model solves 90% of my concerns about accidental attributes assignment, so it looks definitely good to me! 👍

Member

ProGM commented Sep 29, 2016

I like this!
The fact of being a separate model solves 90% of my concerns about accidental attributes assignment, so it looks definitely good to me! 👍

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Sep 29, 2016

Member

Great, merging! Thanks again!

Member

cheerfulstoic commented Sep 29, 2016

Great, merging! Thanks again!

@cheerfulstoic cheerfulstoic merged commit 9b94d1f into neo4jrb:master Sep 29, 2016

3 checks passed

codecov/patch 100% of diff hit (target 96.98%)
Details
codecov/project 97.01% (+0.02%) compared to 2deda24
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

cheerfulstoic added a commit that referenced this pull request Sep 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment