-
Notifications
You must be signed in to change notification settings - Fork 276
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
base: 9.1.x
Are you sure you want to change the base?
Conversation
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 I also feel confused how it can be written |
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 |
Regarding querying for multiple labels, you are correct in some cases. It could certainly be the case that when doing a Of course when you create a new object with a "default" I'll add my other comments inline on the code |
person.label_as_destroyed | ||
|
||
# `Destroyed' label is removed | ||
person.label_as_not_destroyed |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
module Destroyable | ||
include Neo4j::ActiveLabel | ||
|
||
follows_label :Destroyed |
There was a problem hiding this comment.
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 ActiveLabel
s 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 include
d module
There was a problem hiding this comment.
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 👍
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 |
@cheerfulstoic 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. |
Oh geez, there's so much I want to say all at the same time.
UPDATE
There's more that I could say, such as a discussion of what singleton methods including an |
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
-
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 associationmovies
... -
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
So I made a PR against this PR in #1458 |
I think this is related. I would like to be able to define: |
Just docs so far as a way to think through the notion of
ActiveLabel
. See also #1453