Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveLabel documentation proposal #1457

Open
wants to merge 1 commit into
base: 9.1.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 135 additions & 0 deletions docs/ActiveLabel.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
ActiveLabel
===========

``ActiveNode`` provides an ability to define inheritance of models which also gives subclasess the labels of their parent models. In Ruby, however, inheritence of classes is not sufficient. Sometimes is makes more sense to be able to build a module which defines behavior (or "concerns") which could be applied to any model. This is what ``ActiveLabel`` provides.
Copy link
Contributor

@jorroll jorroll Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jorroll jorroll Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jorroll jorroll Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


``ActiveLabel`` modules can be defined in two ways:

* Default: Where the module's behavior is always defined on the ``ActiveNode`` model and the model's nodes always have a corresponding label in Neo4j
* Optional: Where the module's behavior is defined on the class only when the model's nodes have a corresponding label in Neo4j

.. code-block:: ruby

class Person
include Neo4j::ActiveNode

property :name, type: String

label :HasAddress
label :Destroyable
end

class Organization
include Neo4j::ActiveNode

property :title, type: String

label :HasAddress
label :Destroyable
end


.. code-block:: ruby

class Address
property :line1, type: String
property :line2, type: String
property :country, type: String
property :postal_code, type: String
end

module HasAddress
include Neo4j::ActiveLabel

included do
has_one :out, :address, type: :HAS_ADDRESS
end

module InstanceMethods
def distance_from(has_address_object)
address.distance_from(has_address_object.address)
end
end
end


.. code-block:: ruby

module Destroyable
include Neo4j::ActiveLabel

follows_label :Destroyed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@jorroll jorroll Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍


included do
property :destroyed_at, type: DateTime
end

module InstanceMethods
def destroy
destroyed_at = Time.now

super
end
end

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


Creating
--------

If an ``ActiveLabel`` does not declare ``follows_label``, creating a node will attach the corresponding label. Otherwise you must trigger the attachment of the label:

.. code-block:: ruby

# Node gets both `Person` and `HasAddress` labels
person = Person.create

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

# `Destroyed' label is removed
person.label_as_not_destroyed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Querying
--------

``ActiveLabel`` allows your Ruby module to act like a model class. However, since you can add a label to any module, you can query for nodes across modules:

.. code-block:: ruby

Destroyable.all

HasAddress.as(:obj).address.where(postal_code: '12345').pluck('DISTINCT obj')

By default this returns all nodes for all models where the ``ActiveLabel`` module is defined. If ``follows_label`` is declared, this returns just those nodes which have the label.

By defining the ``follows_label``, some methods are automatically provided to allow you to filter and interrogate:

.. code-block:: ruby

Person.labeled_as_destroyed

Person.first.labeled_as_destroyed?

Associations
~~~~~~~~~~~~

You can even create associations to traverse to labels:

.. code-block:: ruby

class Organization
include Neo4j::ActiveNode

has_many :out, :addressables, type: :HAS_ADDRESSABLE_OBJECT, label_class: :HasAddress

# `model_class` acts as a filter to the `label_class` argument. Both `model_class` and `label_class` can be arrays
has_many :out, :addressable_people, type: :HAS_ADDRESSABLE_OBJECT, label_class: :HasAddress, model_class: :Person
end

1 change: 1 addition & 0 deletions docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Contents:
RakeTasks

ActiveNode
ActiveLabel
ActiveRel

Properties
Expand Down