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]

@leehericks Responding to your comment:

I can see what you're saying about the magic prefixes of label_as_ and label_as_not_, though I feel like this is a pretty common Ruby practice. To me, the act of changing the state of the node to add / remove a label is semantically an action and not setting a property. Setting a boolean value seems confusing to me because that boolean value isn't actually going anywhere

I definitely see the appeal of the consistent naming (though again, the Ruby convention that I've seen is to generally go with different phrasings of English even if they aren't exactly the same). Are you saying that we might have method_missing magic like labelled_foo_bar = true / label_as_foo_bar which would add a FooBar label even if there wasn't an ActiveLabel module?

[leehericks]

@cheerfulstoic The boolean value needs to go somewhere. Adding and removing labels should fit within the persistence workflow just like properties and be able to check them or enforce some logic in callbacks. If something fails, rolling back the save or update should also make sure the labels are not set. If labels are applied as individual actions, how do you rollback or save errors to the model? This is the same problem I have with associations autosaving right now.

[cheerfulstoic]

@leehericks Responding to your comment about following the module name

I'm definitely a bit on the fence about having a Destroyable module which defines a Destroyed label, but it was a choice that I made on purpose because I'm trying to keep a Ruby style. In Ruby you often have module names which are adjectives like Destroyable /Touchable / Whateverable, but that doesn't make sense as a label name.

The question of what Destroyable.all returns definitely makes me take a step back. Maybe it's the case that all of these nodes get a Destroyable label but then you can define something like boolean_label :Destroyed instead of follows_label :Destroyed would let you optionally add the Destroyed label to nodes and would also allow you to query for Destroyed nodes.

As I'm thinking about this more and more, this might actually be best implemented as two features. ActiveLabel might just be something which always adds the label to the module that it's defined on and we have a method like boolean_label which you can apply to ActiveNode modules (perhaps even via the included block of a module) which allows you to define labels which can be added, removed, and queried/filtered upon.

That could actually simplify defining ActiveLabels as well because rather than doing label :Destroyable in the model we could simply do include Destroyable and use Ruby's normal module logic to bring in the behavior. I was using the label method so that methods could be defined optionally.

So maybe then it does make sense to think in terms similar to what @thefliik proposed to have something like:

optional_label :Destroyed do
  property :destroyed_at, type: DateTime

  def destroy
    destroyed_at = Time.now

    super
  end
end

That could be defined on the model or inside of a included module

[jorroll]

Oops, I should have read this first.

That could actually simplify defining ActiveLabels as well because rather than doing label :Destroyable in the model we could simply do include Destroyable and use Ruby's normal module logic to bring in the behavior.

Looks like we've arrived at similar conclusions 👍

[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 feature: an ActiveLabel module for sharing ActiveNode logic #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 add: additional_mapped_label_names option to ActiveNode #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, add: additional_mapped_label_names option to ActiveNode #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 add: additional_mapped_label_names option to ActiveNode #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]

I'd replace with:
ActiveNode supports class inheritance, creating "submodels" which inherit the methods and labels of their ActiveNode parents while adding their own label & any methods / properties you define. However, inheritance is not always appropriate. Sometimes what you want to do is conditionally add a module of functionality to an ActiveNode model, but only if a specific label is present on the node. For an example of when this is needed, look at the Neo4j's example movie database (https://neo4j.com/developer/movie-database/). In this example, a Person node is sometimes an Actor, sometimes a Director, and sometimes and Actor and Director. ActiveLabel allows you to accomplish this.

[leehericks]

I still don't understand the conditional part.

Suppose you have a person model who may be an Actor or a Director or both.

1. You still have to be careful not to have namespace clash.
   Regardless of having the label or not, you still need associations like directed_movies, acted_movies for each relationship type. If each module called the association movies...

2. Is there any benefit to hiding these chunks of code when the label is not set??
   Class methods for associations are called when loading the models, right? and then instances are made. Why bother conditionally adding in the the methods? Won't you have to pepper your code with checks if the methods exist and if the label is set anyway?

[jorroll]

Regarding your first point, you'd only need a directed_movies association if someone was a director (i.e. has the :Director label). Put another way, if person.director? == false then person.directed_movies would return an unknown method error.

Regarding your second point, if you don't see a benefit to conditionally adding code based on the presence of a label, what have you been advocatingarguing for this whole time (and by arguing, I don't mean in a negative sense)? It seems clear that we've been operating with different goals.

[jorroll]

Perhaps I need to go back and re-read some of your comments, given your second point above...I've missed something.

[leehericks]

You're absolutely correct. I never really caught the conditional argument. My bad.

But my concern is, as a developer one has to check person.director? before called these methods or one has to handle the unknown method error. What I'm trying to understand then is why conditionally include the code if making the checks anyway?

[jorroll]

Oh ya, you absolutely need to check for person.director? Or in my case, I'd need to check for person.current? or person.destroyed? and handle these two cases differently. I always need to do this though. A current person node is conceptually different from a destroyed person node and needs to be handled different. Its appropriate that different methods exist on each.

It's conceptually similar to checking what type of object a variable contains.

As far as why I'd want to conditionally include the code, well mainly because a current node might have a method of the same name as a destroyed node, but the two methods do completely different things. I'd want to check to see what type of node I was dealing with, so I knew what the method did. Obviously you could achieve similar results with some type of method overloading, but there are always multiple ways to accomplish something in rubyprogramming.

[jorroll]

Also worth noting: if your query only returns :Person:Actor nodes, then you know that the resulting objects have the :Actor methods.

[leehericks]

Ok, but in the case of :Actor and :Director, a :Person could be both, and now if each one has a method with the same name, you have a clash.

[jorroll]

True, but I think that's a limitation of Ruby modules, rather than a flaw with this implementation strategy. If two modules define a method with the same name, and you include both of them in a class, the last one will overwrite the others.

[jorroll]

Put another way, if a method might apply to either an :Actor or :Director, you need to be more thoughtful about the naming than if it just applies to one or the other.

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