``ActiveLabel`` documentation proposal

[cheerfulstoic]

Just docs so far as a way to think through the notion of ActiveLabel. See also #1453

[leehericks]

I sat down this evening to contemplate this documentation based proposal about labels and what it might allow to be accomplished, because I think good support for labels overall is important.

Overall, there are many ways one can mark and match nodes. Labels provide an automatic index on a set of nodes. Properties can be set like flags and indexes can be created for a set of a label and property together. But matching multiple labels together has known performance issues. Also the more labels and indexes declared, the more the database size grows. And the biggest concern I realized was actually that the more nodes you put the same label on, the more performance problems I think will occur. Imagine if all of your models can have the :Destroyed label through the Destroyable example. Just searching for :Person:Destroyed will actually invoke an index that includes ALL nodes in the graph which were destroyed, and then have some mathematical comparison agains all nodes in the :Person label index. I don't think this is going to encourage good modeling, and if the goal of this is shared, reusable coding features, perhaps something should be implemented that doesn't rely on labels in the graph.

Focusing specifically on the proposed documentation:

I think this was a miss? (mark_destroyed and label_as_destroyed)

# `Destroyed' label is added.  `mark_destroyed` method is automatically defined via `follows_label` definition
person.label_as_destroyed

Expanding on that, the following method doesn't actually apply a label :NotDestroyed, but the action wording implies that:

person.label_as_not_destroyed

which is why I proposed the boolean format which I think provides the correct context:

label :Destroyed

# Check existence
person.labelled_destroyed?

# Add or remove named label
person.labelled_destroyed=(Boolean)

# Querying
Person.labelled_destroyed

This has the added bonus of being one naming style for each named label which has the same grammatical structure, so there is very little to learn or remember. It's consistent and it can be used on any ActiveNode with or without additional modules.

Regarding the ActiveLabel naming, if you don't by default infer the label name from the module, then I think it leads to confusion, because you made a module called Destroyable following the label :Destroyed and called Destroyable.all <- What does this return? All possibly destroyable nodes? Or all nodes with the :Destroy label?

I also feel confused how it can be written label :Destroyable on the ActiveNode, and what if the Destroyable module doesn't exist or has some namespace clash? In fact it is written label :Destroyable but a :Destroyed label will be applied to the node. This doesn't end up being very self-documenting code. Which is why if an association uses the term label_class, then the ActiveNode declaration should be label :Destroyed, label_class: :Destroyable, which would also need a mapped_label_name.

[leehericks]

I just realized, if you implement the core label support in ActiveNode as I outlined in the github issue, then anyone could just create a concern and add the label. That then makes less reason to have ActiveLabel separately because it’s just relying on concerns.

Module Destroyable
  include ActiveSupport::Concern

  included do
    label :Destroyed
  end
end

[cheerfulstoic]

Regarding querying for multiple labels, you are correct in some cases. It could certainly be the case that when doing a Person.all we would only need to use the Person label still because adding the labels to the query wouldn't really filter it, unless you were doing a scope like Person.all.labeled_as_destroyed, in which case you would need to query on both. Since that's a case where the user is specifically asking for it I think it's OK. Similarly if you did HasAddress.all it would only be searching on one label. So I think that this is the ideal way of using labels because is the general cases you're using specific labels until you have to use multiple labels.

Of course when you create a new object with a "default" ActiveLabel you'd be adding multiple labels, but that's so that you could search for either the model's label or the module's label.

I'll add my other comments inline on the code

[cheerfulstoic]

Hey @leehericks

I just saw your latest comment here. I think it's similar to what I was talking about here:

https://github.com/neo4jrb/neo4j/pull/1457/files#r157831679

[leehericks]

@cheerfulstoic What I'm saying is that I've come to realize/assume that developers already have the tools to do the job without this added API.

ActiveNode should give the appropriate means to add and remove labels and query for them. No module should be necessary to add labels to nodes and have the synthesized abilities to query based on them.

What you wanted modules for was shared code, and this example shows that developers can already use ActiveSupport::Concern for that.

module Destroyable
  include ActiveSupport::Concern

  included do
    label :Destroyed
  end

  def destroy!
    self.labelled_destroyed = true
    self.save
  end
end

class Case
  include Neo4j::ActiveNode
  include Destroyable

  property :name, type: String
end

case = Case.create(name: "New Case")
case.labelled_destroyed = true

if case.save
  puts "Case closed!"
end
# OR
case.destroy!

# Query
Case.labelled_destroyed.all
Case.where(has_labels: [:Destroyed]) # Because this literally translates to WHERE n:Destroyed and it's readable

Please go back to #1453 and consider my documentation-style example more. That's the core label functionality that ActiveNode needs.

[jorroll]

Oh geez, there's so much I want to say all at the same time.

1. As I said in #1453, I think an ActiveLabel module should be an all or nothing affair: everything in the module is added if the label is present, or nothing in the module is added. If you want something to always be included in a class (regardless of the labels present), make a separate concern for that (if you want to make sure a specific, additional, label is always present, that API is part of a different conversation, such as #1414). Yes, this potentially makes things a little more verbose, but it will really simplify the developer's understanding of what an ActiveLabel module is, and it will simplify the DSL as well. In my scenario, the ActiveLabel DSL can be the same as that of ActiveSupport::Concern.

• This simplifies the Destroyable module to

module Destroyable
  include Neo4j::ActiveLabel

  follows_label :Destroyed

  def destroy
    destroyed_at = Time.now
    super
  end

  included do
    property :destroyed_at, type: DateTime
  end

  module ClassMethods
    def destroyed_recently
      all.where("#{identity}.destroyed_at > ?", 1.week.ago)
    end
  end
end

UPDATE I've just realised the, perhaps obvious, fact that class methods are not present on an instance of a class. LOL! So perhaps it makes sense to mix in some ActiveSupport::Concern functionality. I kinda wanna say "only the ClassMethods part!" But, conceptually, I'm not sure how much sense that makes...

2. You're going to have to explain your attraction to the label :Destroyed DSL, as I'm just not getting it. As a developer, what I'm trying to do is conditionally add a module to a class. The condition is "if this label is present, add what's in the module to the object instance". I am NOT adding a label to the class. Neo4j-core already provides an api for adding labels to objects, and I'd argue any discussion about Neo4jrb's class label API should be separate from this (for example, #1414).

• Since what I'm trying to do is add a module to a class, not a label, why not simply use include TheModule rather than this custom label :TheModule abstraction? Alternatively, and perhaps better, use something like if_label_present_include ::TheModule, though it's a bit wordy. (and again, including an ActiveLabel module should always add some singleton methods to the class, such as Person.hasAddress?, so I think simply include ::TheModule might be most appropriate)

3. If you agree that ActiveLabel is just a way of conditionally specifying functionality to add to an instance of a class from a module, then ActiveLabel shouldn't know, or care, what labels are "optional" to instances of a class vs "always" added to instances of a class. I suppose an argument could be made that label :HasAddress could be used as the DSL for adding labels to a class, but that's a separate conversation (in my mind) from ActiveLabel (I'd say put that idea over in #1414).

There's more that I could say, such as a discussion of what singleton methods including an ActiveLabel module might add to a class, but I'm going to wait to see what you think of the above points, as they really define the conversation (in my mind).

[jorroll]

So I made a PR against this PR in #1458

[klobuczek]

I think this is related. I would like to be able to define: has_one :out, :part_of, type: :part_of, model_class: :Specializable where Specializable is a module. Then object.part_of ideally returns an object of an anonymous class including Specializable or with more hints object of an existing class. The currently required definition: has_one :out, :part_of, type: :part_of, model_class: [:Class1, Class2, ...Classn] breaks couple of OO paradigms where a class needs to know all classes which include a module Specializable.