Active label API Proposals

[jorroll]

After working on the neo4j-core .union patch, I wanted to keep up the momentum. This PR contains proposed ActiveLabel API updates. The main change from what you had (I think) is that I've separated ActiveLabel modules, which simply respond to labels, from how labels are added to nodes.

I've also made a few other changes, which were the result of thinking through the implementation process, and realizing that I don't think some of my previous ideas were readily implementable. (for example, if Actor is an ActiveLabel module, in order to allow Actor.acted_in queries, I don't think the has_many :out, :acted_in association can be called inside a included do block, but rather should be called directly on the Actor module. Ideally, I'd have liked it to be called inside an included do block to mimic ActiveSupport::Concern)

Note:
It will probably be easiest to read over if you first read the "files changed" as rendered markdown, without looking at the comments, and then afterwards go back and view the comments.

[codecov-io]

Codecov Report

Merging #1458 into active_label will increase coverage by 0.02%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           active_label    #1458      +/-   ##
================================================
+ Coverage         96.84%   96.87%   +0.02%     
================================================
  Files               205      205              
  Lines             12603    12600       -3     
================================================
  Hits              12206    12206              
+ Misses              397      394       -3

Impacted Files	Coverage Δ
lib/neo4j/tasks/migration.rake	32.14% <0%> (+0.83%)	⬆️

---

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 e0582e4...8f5b1ec. Read the comment docs.

[jorroll]

Where you had "model_class" acts as a filter... I changed it to "label_module" acts as a filter because the class is what is instantiated and so seems like the more "primary" descriptor (in my mind). Put another way, ActiveLabel is marking up ActiveNode. Also I changed from "label_class" to "label_module", because the labels are modules rather than classes.

[jorroll]

Including one ActiveLabel module in other ActiveLabel modules would be the primary way of sharing code between ActiveLabel modules rather than using a concern. To make this work, a dev would specify that the ActiveLabel module containing the shared code had multiple associated labels and would trigger if any of those labels were present. I realized that sharing code using a concern wouldn't work, because there isn't a good (any?) way of specifying that the concern's code should only apply to certain instances of a class (you can't do obj.include(TheConcern)).

Also, if you had an ActiveLabel module ("SharedModule") that was sharing code between multiple other ActiveLabel modules, and that shared module had an association ("has_many :out, :cars"), doing something like SharedModule.all.cars will work properly with the above code sharing strategy.

[jorroll]

I'm thinking that maybe this should be refactored as just included_if() do. If an argument is an array, then each label in the array must be present. So the above examples could be refactored as:

included_if :Actor, :Director do
  property :medium_ego
end

included_if [:Actor, :Director] do
  property :large_ego
end

This format would also be more powerful and allow

included_if [:Actor, Director], :Animal do
  property :name
end

[jorroll]

For that matter, the self.associated_labels = option could similarly be changed to accept a 2 dimensional array and eliminate self.associated_labels_matcher =

[jorroll]

Another issue with any potential included_if() do block: I'm not sure how the following should be handled

class Person
  include Neo4j::ActiveNode
  scope :actors, ->{ where("#{identity}:Actor") }

  included_if :Actor do
    has_many :out, :acts_in, type: :ACTS_IN, model_class: :Movie
  end
end

In the above, devs could

movie = Movie.create
actor = Person.create
actor.add_label :Actor

actor.acts_in << movie

But I could see a dev also wanting to do something like Person.actors.acts_in, which they couldn't do because the has_many :out, :actors block would never be evaluated on Person (only on instances of person).

Maybe this is fine, and would simply be a known limitation of included_if() blocks. Devs could still use the blocks to add properties, define methods, or call custom methods. Or maybe there's another option.

I definitely view anything like included_if() as a "nice to have" feature. Maybe it would be more trouble than its worth.

Another note: ActiveLabel will, I imagine, make devs want the ability to filter an association proxy by label. Something like Person.all.has_label(:Actor).acts_in.

If someone was using an ActiveLabel module they should be able to Person.actor.acts_in.

[jorroll]

This may just come down to personal preference. I originally wrote

in the example above, the Actor ActiveLabel module follows the :Actor label. You can customize the label(s) which an ActiveLabel module follows using self.associated_labels =.

But I found the follows metaphor to be less intuitive for me than the associated metaphor, which resulted in the change to self.associated_labels =.

I don't feel strongly about this though, so if others find follows more intuitive I'm all for changing it back.

[jorroll]

Note, if the Person class has label :User (or label :User, optional: true), then there would be no difference between Person.create and Person.user.create. This should probably be explicitly mentioned.

However there WOULD be a difference between label :User and label :User, optional: true when calling Person.all vs Person.user.all. If label :User, optional: true, then Person.all would match on :Person while Person.user.all would match on :Person:User. If label :User then both Person.all and Person.user.all would match on :Person:User.

This also makes me see the implementation difficulties of a Hollywood ActiveLabel module with self.associated_labels = []. If self.associated_labels = [:Actor, :Director] and self.associated_labels_matcher = :all, then Person.hollywood.all would match on :Person:Actor:Director. But if self.associated_labels_matcher = :any then Person.hollywood.all would match on ~

match("#{identity}:Person").where("#{identity}:Actor OR #{identity}:Director")

[jorroll]

I find this api to be more expressive (and concise) than

person = Person.create
person.add_label(:Actor)

The above is also two DB queries, where (Person.actor.create) could be one DB query.

[jorroll]

This being said, I'm wondering what happens given the following

module Hollywood
  include Neo4j::ActiveLabel

  self.associated_labels = [:Actor, :Director]
  self.associated_labels_matcher = :any
end

class Person
  include Neo4j::ActiveNode
  include Hollywood
end

person = Person.hollywood.create

What labels does person have? It could be that we, optionally, let the dev define their own label helper methods. Something like

module Hollywood
  include Neo4j::ActiveLabel

  self.associated_labels = [:Actor, :Director]
  self.associated_labels_matcher = :any

  class_label_maker :actor, :Actor
  class_label_maker :director, :Director
end

actor = Person.actor.create
director = Person.director.create

Below, I added a comment suggesting that self.associated_labels accept a two dimensional array instead of self.associated_labels_matcher. What would happen if
self.associated_labels = [[:Actor, :Director], :Animal]?

I'm thinking that, if self.associated_labels = [], then devs must define their own class label makers. Or maybe we could simply always make devs define their own class label makers, and make it an ActiveNode method.

module InShowbusiness
  include Neo4j::ActiveLabel

  self.associated_labels = [[:Actor, :Director], :Animal]

  class_label_maker :hollywood, [:Actor, :Director]
  class_label_maker :actor, :Actor
  class_label_maker :director, :Director
  class_label_maker :animal, :Animal
end

[klobuczek]

@thefliik again you are bringing up something I had on my mind for a very long time. My idea was to do all this natively with ruby without any changes to the DSL or the api methods.
If each Label maps to an ActiveLabel module to create a node with a set of labels you would construct an object of a class which includes all the ActiveLabels you are interested in. When you read a node from the database and the node's combination of Labels does not correspond to any class you would return an object of an anonymous class which has all the corresponding ActiveLabels included. This feels very natural and intuitive and completely in harmony with the ruby language and how neo4j labels work.

[jorroll]

@klobuczek Interesting. I think it's important for me to say that this proposal only adds to the existing API, and, even if this were implemented, anyone could continue to use the neo4j gem exactly as they have been without changes.

That being said, your suggestion sounds pretty similar to mine, except that you are getting rid of individual ActiveNode classes and simply using one, anonymous class. Are you suggesting this because you think it would be more flexible?

One downside of your approach, is that (and correct me if I'm wrong), but in your approach a specific label (e.g. :Person) could only be association with one ActiveLabel module, as oppose to my API in which the :Person label could have totally different meanings depending on the ActiveNode class of the object. It also seems like my approach would be easier to integrate into existing applications (simply mixin an ActiveLabel module to your existing ActiveNode class). I could also envision many devs, coming from an ActiveRecord background, ignoring the ActiveLabel functionality in the beginning and building out their app just using ActiveNode (ActiveNode is much more similar, conceptually, to ActiveRecord, after all). At some point in the future when they got more comfortable with Neo4j's polymorphism, they could start mixing ActiveLabel modules into their classes.

I guess what I'm trying to say, is that both strategies are basically the same: neo4jrb gets a node from the database and then wraps that node in a class (either an ActiveNode class or anonymous ActiveNode class) based on the labels the node has. Then, based on which of those labels are determined to be ActiveLabels, the object gets extended with one or more modules. Why remove standard ActiveNode classes from this process?

[klobuczek]

@thefliik Please note that ActiveNode is not a class but a module, so this is already different from active record, but does offer a huge advantage. What I am suggesting is to not be required to define classes including ActiveNode with properties and associations (which would be standard way like today), but allow defining just modules (including ActiveNode or ActiveLabel or similar,) which you can combine to classes, but which also could live alone if an unknown combination of labels is retrieved from the database. This is the only case where I would return anonymous class. This would work nicely for creation and retrieval. I have not thought through the promotion (adding a label to existing node) and demotion (removing a label), which does not seem to have a native counterpart in ruby.