Open jorroll opened 6 years ago
Hey, thanks for this and #1414. I have been putting off looking at them because this is something that I want to proceed with carefully and so I've wanted to have some time to sit and think about it. I've run into such issues either on my team or via issues that people have submitted, and I think if we find the right abstraction we could have a powerful addition to the gem.
My first question is, why have Current
/ Destroyed
labels rather than current
and destroyed
boolean properties (or a destroyed
boolean, perhaps, if the two states are mutually exclusive). I think that labels are often a poor fit for describing the state of things rather than the type of things.
Secondly, why have classes called Question
and Signup
? Are those things that you plan on querying for as a group (that is, if you did Signup.all
you would get both List
and Shift
nodes). If not, I would suggest making Question
and Signup
modules and not having inheritance. For example:
module Question
module Signup
class List
self.mapped_label_name = 'Question::Signup::List' # This should actually be the default label anyway for nested `ActiveNode` models
end
end
end
I ask these questions because I want first make sure any potential design questions first before deciding on if a new feature is warranted. If you can do everything that you need with the existing gem, then maybe we don't need something new (yet). If we were to do this, it would certainly make more sense on an association rather than the model (as in #1414) because a model defines something more "permanent" and if I were to use the User
model it would feel weird if that model also had the Author
and Admin
labels. With an association you can create as many associations as you want referring to whatever combination of models / relationship / options. You may have already decided the same because you created this issue, but I wanted to put that out there ;)
All of that said, the way in which I've seen a need to be able to have some flexibiliy in the models is because I think sometimes it make sense for labels to map to "behavior" as well as "type". As an example, take these generic models:
# Assume these are all `ActiveNode` models
class Foo
end
class Bar
end
class Baz
end
You might want to be able to apply Ruby modules like Bizzable
and WillKlonk
to the classes like:
# Assume these are all `ActiveNode` models
class Foo
include Bizzable
end
class Bar
include Bizzable
include WillKlonk
end
class Baz
include WillKlonk
end
This sort of cross-cutting, duck-typing behavior is something that Neo4j's labels are really fabulous for. Unfortunately I don't think that you can completely take advantage of them in ActiveNode
. But if this could be done, you could have something like:
# Returns a set of nodes which could be `Foo` or `Bar`, but we don't care
# because all we care about is that they behave like a `Bizzable` object and
# respond to the methods defined in `Bizzable`
Bizzable.all
I realize that I may have misunderstood your use-case and you might have no need for what I'm talking about, but I wanted to put that all out there to help facilitate the discussion to something that could be useful for everybody ;)
Thanks again!
Hey @cheerfulstoic. I got pulled into another project so just now getting back to this. A few things:
self.mapped_label_name
to be explicit in my issue.Question.all
or Signup.all
.
Question
/ Signup
modules rather than use inheritance seems like a style choice. Or is there a performance reason to avoid inheritance?Secondly, I disagree with your implication that ActiveNode models shouldn't have multiple labels associated with them
if I were to use the User model it would feel weird if that model also had the Author and Admin labels.
If I constructed the following model
class User
class Admin < User; end
class Author < User; end
end
Admin.new
will have both user
and admin
labels. Which is perfectly natural in Neo4j. I.e. a model is, already, not defined by a single label, it is defined by the set of labels associated with it. Here the set was defined using inheritance, but, I would argue, it is perfectly valid to, instead, define the set using a method like self.mapped_label_names
(#1414) and/or inheritance.
Thirdly, while I would agree that a state like "on" or "off" should be kept as a property on the node, for "destroyed" vs "not destroyed," specifically, I think labels are appropriate for several reasons:
Car
model can be running: true
, so it is natural to keep this state as a property on the associated node. With :Car:Destroyed
however, I'm not describing the state of the car as being destroyed, I'm describing the state of the node as being destroyed. Put another way (and also pausing to acknowledge that this is, entirely, an argument over semantics), the :Destroyed
label is describing database state, rather than object state. It feels appropriate to use a label in this case. (A :Car:Current
node could have a destroyed: true
property on it which describes the state of the car as having been destroyed, even though the node itself isn't destroyed).:Destroyed
nodes, most commonly when running chores such as to permanently find and delete old destroyed nodes (i.e. MATCH (n:Destroyed) WHERE n.updated_at < $six_months_ago DELETE n
).Car::Current
vs Car::Destroyed
). In many situations, you would only want methods to operate on a non-destroyed node, and a clean way of handling this is to have different object types for non-destroyed vs destroyed and just construct a method to accept / expect non-destroyed type(s)..destroy
and a destroyed node will respond to .reincarnate
/ .undestroy
, but there can be others as well. It makes sense to have different classes for destroyed vs non-destroyed objects, to cleanly support these differences. On a more practical level, I originally made the decision to create different current
vs destroyed
classes because I wanted my associations to only return non-destroyed nodes and, when I was building out this functionality, bug #1413 prevented my scopes
from working property. I didn't realize that scopes
weren't working because of a bug, all I knew is that they didn't do what I wanted them to do.
The strongest argument I see in favor of describing current
vs destroyed
nodes using a property rather than labels, is that the property method makes it easier for Neo4jrb to construct optimized queries. Assuming that it's true, Neo4j query performance is affected by label ordering (I haven't had time to test this myself yet), my solution creates a problem in that the current
/ destroyed
labels will always be placed at the front of a query, while they should come last. Of course, your Bizzable
and WillKlonk
modules example can also run into this problem. E.g.
class Foo
include WillKlonk
class Bar
include Bizzable
end
end
class Baz
include Bizzable
end
In the above, I think Foo::Bar.all
will query on the labels :`Foo::Bar`:Bizzable:Foo:WillKlonk
, but the optimal label ordering is actually :`Foo::Bar`:Foo:WillKlonk:Bizzable
.
Even in the current version of Neo4jrb, label ordering could be a problem if someone's schema was like so:
class Cartoon
self.mapped_label_name = 'Cartoon'
class Animal < Cartoon
self.mapped_label_name = 'Animal'
class Dragon < Animal
self.mapped_label_name = 'Dragon'
end
class Monkey < Animal
self.mapped_label_name = 'Monkey'
end
end
end
class Animal
self.mapped_label_name = 'Animal'
class Gecko < Animal
self.mapped_label_name = 'Gecko'
end
end
In the above, the optimal query for Cartoon::Animal::Dragon.all
might (depending on the database) order the labels :Dragon:Cartoon:Animal
. Currently, Neo4j would order the query as :Dragon:Animal:Cartoon
. Admittedly, this is a contrived example because it can be prevented by not demodulizing the label names. In fact, I can't immediately think of an example where you shouldn't just modulize the label names, so this may not actually be an issue (an exception would be if you wanted Neo4jrb to play nicely with existing databases which were created outside of Ruby and/or without Neo4jrb--in this case it's likely label names would not be modulized).
Anyway, before I go on, what are your thoughts about all this? Ultimately, my particular issue (using :Current
label vs current: true
property) is arguably a style / personal preference one, as this problem could certainly be solved either way. Given that Neo4jrb's inspiration, ActiveRecord, draws heavily from convention over configuration, I think it would be perfectly understandable to decide something along the lines ~ "In this case, the convention Neo4jrb supports is using properties to solve this problem rather then labels. As such, this isn't a feature we're looking to include at this time."
I imagine you're just busy with other things (which is fine), but, in case this slipped off your radar: @cheerfulstoic...ping
Sorry, yes, busy with other things, unfortunately. It's on my todo list, but I've just been going through things one or two at a time in between other things. Will definitely respond soon
No worries! Just wanted to check in.
I started to read through, but I feel like this is going to be difficult to talk through in a GitHub issue thread. Can we have a voice and/or video chat at some point? Probably the best way to contact me is to send a message using the form on the neo4jrb website
Thanks for the chat. I've gotten a bit of time to sit down and think about this.
I'm honestly not really opposed to having a new option for has_one
/ has_many
(though I would call it labels
instead of model_labels
because it may be referring to multiple models). But I think there is the possibility for a larger feature which could give a smooth path for these use-cases which anybody could use.
This is perhaps a bit rough, but this is the general solution I'm thinking about:
class List < Signup
include Current # My use-case
# or...
conditional_label :Current # Your use-case
end
module Current
include Neo4j::Label
# `ActiveSupport` is a dependency of the `neo4j` gem, so we could use `ActiveSupport::Concern` under the covers
def some_instance_method
end
class_methods do
def some_class method
end
end
end
Having the Neo4j::Label
included into the Current
module could allow you to do things like Current.all
/ Current.first
/ Current.where
/ etc...
In my use-case, a node wouldn't be wrapped as an object of the class List
unless it had the labels List
, Signup
, and Current
(ignoring Question
for now to keep things simpler). Since it's using the built-in Ruby include
the node would have all of the methods defined in the module.
In your use case nodes would be wrapped the way they are now, but if there was also a Current
label on a node then the object would get the methods defined in the module.
It's certainly more work than an association option, though I think it could be a pretty awesome feature. What do you think?
So actually, I think your proposed module setup is already largely doable in Neo4j (though I may have misinterpreted your thoughts). If you create a module which responds to self.mapped_label_name
, and you include that module in an ActiveNode class, that class will inherit the module's label (mapped label names are found through ~~ ancestors.respond_to?(:mapped_label_name)
call, which includes modules). I make use of this in my app. While I've never done it before, you should also be able to use this strategy to get scopes included in a model, since scopes are also found through ~~ ancestors.respond_to?(:scopes)
. The only bit that isn't provided automagically by neo4jrb already is the ability to call Module.all
/ Module.first
, but that should be a pretty easy feature to add.
All this being said, I don't think you're proposal addresses my issue. Given models that look like this:
class Question
class Signup < Question
class List < Signup
class Current < List; end
class Destroyed < List; end
end
class Shift < Signup
class Current < Shift; end
class Destroyed < Shift; end
end
end
end
I want to create an association that links to all current signup questions (something akin to has_one :current_signup_question, model_class: :"Question::Signup::Current"
. In cypher, this is easy :`Question::Signup`:`Current`
. Currently, in neo4jrb though, that association cannot be made unless a Question::Signup::Current
class exists. It doesn't look like your proposal helps to solve this problem?
Maybe I should also add, that all my Current
/ Destroyed
classes are already defined using a simple module include
.
Put another way, this:
In my use-case, a node wouldn't be wrapped as an object of the class List unless it had the labels List, Signup, and Current
Is already doable / is already a feature of this gem :)
(I'm actually pretty sure I learned about this feature from advice you gave to someone else in an old issue)
Maybe you were thinking that, in your scenario, I could create an association directly to the Current
module? But that wouldn't address my problem either as I'm trying to create an association to multiple labels :`Question::Signup`:`Current`
Right, I forgot about using the self.mapped_label_name
in a module. So yeah, it could be pretty easy to wrap it all into a Labels
module which automatically does that based on the module's name as well as adding the extra methods (I think that since they are also used for ActiveNode
there would need to be some shared place that the logic lives)
And I definitely missed how my solution doesn't apply to your issue. Thinking about it on the fly I could imagine having something like this:
has_one :current_signup_question, model_class: [:'Question::Signup', :Current]
Where Current
is a module which responds to mapped_label_name
/ mapped_label_names
. That way you could change the label of the classes / modules and the associations would still work (assuming that you ran migrations to change the labels).
The big problem that I see with that, though, is that the current semantics for passing an array to model_class
is that the labels for the association are defined by just one of the models, not all. So to distinguish you might have to have something like this:
has_one :current_signup_question, model_class: [[:'Question::Signup', :Current], :OtherModel]
Which is a bit ugly... Alternatively the model_class
option could be used in conjunction with something like the labels
option to show which of the included Ruby modules with Neo4j::Label
defined in them is used:
# In my case `Current` is always associated with `Signup`, so it would filter by the `Current` label here
has_one :current_signup_question, model_class: :'Question::Signup'
# In your case, you would need to specify that you want to only look for things with a conditional label
has_one :current_signup_question, model_class: :'Question::Signup', conditional_labels: [:Current]
In my case where a module which is always included, I think that Current
isn't a good example of a module, by the way. I think it would be something like Fooable
All of this said, I think that having a labels
option as you originally proposed would be a perfectly fine thing to add to the gem which doesn't interfere with any of this stuff that I'm proposing and would give people the ability to configure things exactly how they want if the gem doesn't do quite what they want.
These are the concrete goals I identify in this proposal/discussion: (Goals, e.g. G1)
I see this discussion largely as enhancing the framework to use labels for: (Use Cases, e.g. UC1)
Possible ramifications:
Please correct me on any misunderstood parts concerning the goals and use cases. The following is my imagined API to take better advantage of labels:
# The following simple implementation meets G1 and G2, UC 3
class Case # :Case is set by default for an active node, is never removed, needs no extra declaration
include Neo4j::ActiveNode
# This declares an optional label for the model Case, which is applied by default to new instances
label :active_case, default: true
label :needs_review
property :title, type: String
has_many :out, :comments, type: :CASE_COMMENT, model_class: :Comment
end
# Create a new case
the_case = Case.new # Case.new(active_case: true)
the_case.title = "A case for more labels"
the_case.save #=> (c:Case:ActiveCase {title: "A case for more labels"})
# See all labels
the_case.labels #=> [:Case, :ActiveCase]
# Remove the active_case label (upon persisting call)
the_case.active_case = false
the_case.save #=> (c:Case {title: "A case for more labels"}) CASE CLOSED :P
# The following contemplates G3 and G4
Case.all.where(has_labels: [:ActiveCase, :NeedsReview])
class Case
# This could be synthesized automatically from the `label :active_case` declaration
# Matching only :ActiveCase (purposefully named) is MUCH more performant than :Case:ActiveCase
scope :labelled_active_case, ->{ where(has_labels: [:ActiveCase]) }
end
Case.labelled_active_case
Case.all.has_labels([:ActiveCase, :NeedsReview])
# Let's look at the possibility of ActiveLabel
class Case
label :needs_review, label_class: :NeedsReview
# or just?
label NeedsReview
end
# Something like this which allows class_eval on the models to insert crosscutting properties and methods
module Neo4j::ActiveLabel
def labels_models(models, &block)
models.each |m|
# get class from symbol
model_class.class_eval &block
end
end
end
class NeedsReview
include Neo4j::ActiveLabel
labels_models [:Case, :Report] do
property :reviewed, type: Boolean, default: false
has_one :in, :reviewer, model_class: Reviewer, origin: :review_items
def something
end
end
# Some callbacks like before_save, before_remove_label?
end
NeedsReview.labelled_models
NeedsReview.all
NeedsReview.cases
NeedsReview.reports.where(...)
# Polymorphic associations
class Reviewer
include Neo4j::ActiveNode
property :name, type: String
# Notice the consistency of has_labels across all examples.
# Should require that the to_node has the labels. Run into issue of removing label now needs
# to remove these relationships, and then you are have built a lot of complexity.
# nodes added to this relationship would get the NeedsReview label set
has_many :out, :review_items, type: :REVIEWING, model_class: nil, has_labels: [:NeedsReview]
end
Functioning example code:
module Extendable # ActiveNode
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def property(name)
puts name.to_s + " defined!"
end
# label(ActiveLabelClass)
def extend_with(extension_class)
if extension_class.extension_defined?
extension_class.apply_extension self
end
end
end
end
module Extension # ActiveLabel
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
@@extension_block = nil
def extend_instance(&block)
@@extension_block = block
end
def extension_defined?
@@extension_block != nil
end
def apply_extension(to_class)
to_class.instance_eval(&@@extension_block) unless @@extension_block == nil
end
end
end
# Make a dog extension for barking
class DogExtension # Here is our example "label" which would get applied to nodes
include Extension # ActiveLabel
extend_instance do # extend_node, because of instance_eval we can call the methods of ActiveNode
property :color
def bark
puts "Woof!"
end
end
end
# Make a barking Cat class
class Cat # Here is our example node which would call "label NeedsReview"
include Extendable # ActiveNode
extend_with DogExtension
end
# The test
puts "Ever hear a cat bark?"
Cat.bark
So there are some great ideas here! A lot of this is tangential to my original issue (concerning polymorphic associations, specifically), so let me first respond to that before responding to the other stuff:
I haven't read anything that addresses my issue better than my original proposal, so I'm going to take @cheerfulstoic's blessing and roll with it :)
I think that having a labels option as you originally proposed would be a perfectly fine thing to add to the gem which doesn't interfere with any of this stuff that I'm proposing and would give people the ability to configure things exactly how they want if the gem doesn't do quite what they want.
Regarding the DSL for this feature, I was originally thinking of passing in an array of cypher formatted label strings and then parsing them, something like:
has_one :current_signup_question, node_labels: [":`Question::Signup`:Question:Current", ":OtherModel"]
But testing this out just now in an editor, I find a matrix of label symbols to actually be more readable.
has_many :test, node_labels: [[:"Person::Current", :Person, :Current], [:"Question:Current"]]
I agree that the matrix is a bit ugly, but A) I find it very readable (the format doesn't abstract too far away from the cypher) B) this isn't a feature that people will use often, and if they want to simplify it they can pass in variables.
has_many :test, node_labels: [[:"Person::Current", :Person, :Current], Question::Current.mapped_label_names]
# or
question_labels = Question::Current.mapped_label_names
person_labels = [:"Person::Current", :Person, :Current]
has_many :test, node_labels: [person_labels, question_labels]
I don't like the idea of creating a DSL that is more ActiveNode specific (e.g. model_class: :'Question::Signup', conditional_labels: [:Current]
). I find that much harder to understand. This specific example of a different DSL is also bad because, in my case, :Current
isn't conditional it is additional. What if I wanted to match on
(model class Question::Signup
containing :Current
label) OR (Form
model)?
It's complicating things by trying to match on the wrong concept --> what I really want to do is match on labels.
I think the idea of creating an official way to extract ActiveNode functionality into composable models is a great idea. It's something I make extensive use of, unofficially, in my app already. I also don't think it should be that difficult, as I believe most of the groundwork for this has been laid. The possibility of making an association directly to one of these modules is also intriguing, as that's not something I've thought about doing before. (Though this possibility also calls into question the name of the model_class:
association option.)
@leehericks, thanks for the feedback! Regarding your goal#2, is this a new idea you're introducing? Or were you responding to your understanding of this conversation? In my mind, and in my understanding of @cheerfulstoic's module idea, an ActiveModule
(for lack of a better term at this moment) would have one label associated with it. If the ActiveModule
was included in a class, that class would always inherit the ActiveModule
's label, in the same way as the class inherits any ancestors label and this inheritance would not be optional.
Regarding your second comment with example code, that's an interesting manner of accomplishing extension. I haven't encountered a method like that before. You've defined the reusable code inside of a class that (presumably) makes use of the code, rather than inside a module. I've never seen this before, but I don't think I like it. For one, now Cat.ancestors
will return Extendable
not DogExtension
(I'd prefer it to return DogExtension
). In general, I believe the convension Neo4j has embraced is that of ActiveSupport::Concern for code reuse. (or maybe I'm missing an advantage to your method?):
module Animal
include Neo4j::ActiveModule
scope :is_scary, -> { where(scary: true) }
def bark
puts "Woof!"
end
included do
property :scary, type: Boolean
property :species
end
end
class Dog
include Neo4j::ActiveNode
include Animal
property :fluffy_tail, type: Boolean
end
Embracing polymorphism also brings up the ordering of labels. If I remember correctly, @cheerfulstoic, you mentioned that you confirmed label ordering affects count
queries? @leehericks, I found the GraphAware article you shared to be very informative. It seems to imply that adding more than one label to a query always decreases query performance. This is something I want to test right now, as I think the results will affect an ActiveModule
type feature....BRB
@thefliik
1) Goal 2: Allow optional labels, and the ability to add or remove them
I understand this conversation to be that you are adding labels to nodes, sometimes optionally. I don't understand the full domain of your graph/project, but I feel really uncomfortable about the way you are overloading labels. I would border on saying you may be abusing the construct. But then again, you seem to have more experience than I do.
The GraphAware article also supports my assertion that you should find a different, more performant way to model your data. If you consider a relational database, one of the downfalls of tables are that they all have to be joined and filtered. When you apply multiple labels, you are causing the indexes for those labels to be unioned. That's why they say make a :BlogPost:ActivePost and just match on :ActivePost.
2) Regarding the second comment with code, I just imagined and implemented this solution
It is a small, working concrete piece of the overall goal. What you missed is that Extendable
is just ActiveNode
with the label
DSL which is completely descriptive and understandable in the domain of Neo4j. I'm not sure why you want to mess with ancestors or inheritance chains.
Going back to my first comment you would of course have the associated methods synthesized. You could call labels
to get all labels and check for the existence of one on the node. You could use label=
with a boolean to add/remove the label when save
is called.
Which also leads to the polymorphic association discussion.
has_many :test, node_labels: [[:"Person::Current", :Person, :Current], [:"Question:Current"]]
I have no understanding of why you have arrays of arrays of labels, nor why you have Person, Current, and Person::Current, but it feels like you're fighting the framework.
One example of clean API, I will attempt to model more and towards your needs, in hopes of finding consistency.
class Person
include Neo4j::ActiveNode
property :name, type: String
label :current, default: true
has_many :out, :review_items, type: :REVIEWING, has_labels: [UnderReview]
end
class Question
include Neo4j::ActiveNode
property :text, type: String
label :current, default: true
label UnderReview
end
class UnderReview
include Neo4j::ActiveLabel
labelled_node do
property :reviewed, type: Bool, default: false
has_one :in, :reviewer, model_class: :Person, origin: :review_items
def set_reviewed!
reviewed = true
reviewer = nil
return save
end
end
end
p = Person.first
p.labelled_current? # true
p.labels # [:Person, :Current]
q = Question.new(text: "Have many labels could a labeller label, if a labeller could label labels?")
q.labels # [:Question, :Current]
q.reviewer = p
q.save
q.labels # [:Question, :Current, :NeedsReview]
Question.has_labels([:UnderReview]).reviewer # All Persons currently reviewing questions (which are under review)
Person.first.review_items # All models the person is currently reviewing, marked :UnderReview
q1 = Question.first.reviewed? # false
if q1.set_reviewed!
puts "One down, many more to go!"
q1.reviewed # true
q1.labels # [:Question, :Current]
q1.reviewer # Empty
end
I would also like to say that I never knew inheritance fused names together to produce one long label. I think adding labels with functionality through ActiveLabel is nice when inheritance isn't needed.
class Character
include Neo4j::ActiveNode
property :unicode, type: String
property :literal, type: String
end
class Kanji < Character
include Neo4j::ActiveNode
has_many :out, :readings, model_class: :KanjiReading
end
class Hiragana < Character
include Neo4j::ActiveNode
# Something Hiragana specific
end
But with ActiveLabel
class CharacterLabel
include Neo4j::ActiveLabel
mapped_label_name = "Character"
labelled_node do
property :unicode, type: String
property :literal, type: String
def to_s
literal
end
end
end
class Kanji
include Neo4j::ActiveNode
label CharacterLabel, removable: false
has_many :out, :readings, model_class: :KanjiReading
end
class Hiragana
include Neo4j::ActiveNode
label CharacterLabel, removable: false
end
CharacterLabel.all # All characters
Hiragana.first.literal # "あ"
Kanji.all.each {|k| puts k }
Kanji.first.labels # [:Kanji, :Character]
class CharacterList
include Neo4j::ActiveNode
has_many :out, :characters, type: :HAS_CHARACTER, has_labels: [CharacterLabel]
end
list = CharacterList.create
list.characters << Kanji.first
list.characters << Hiragana.first
list.characters.count # 2
class Destroyed
include Neo4j::ActiveLabel
labelled_node do
scope :current, -> { all.where_not(has_labels: [Destroyed])}
end
end
class Event
include Neo4j::ActiveNode
property :name, type: String
label :destroyed
end
e = Event.create(name: "Ruby Kaigi")
e.labels # [:Event]
e.labelled_destroyed? # false
e.destroyed = true
e.save
e.labels # [:Event, :Destroyed]
Event.all.where_not(has_labels: [:destroyed]) # And this is where you learn negation in Neo4j is expensive
Event.current
class Dragon
include Neo4j::ActiveNode
property :color, type: String
label :animal, removeable: false # implies default: true
label :cartoon, removeable: false
end
Dragon.first.labels # [:Animal, :Cartoon, :Dragon]
# Ok, you have the three labels you wanted, order doesn't matter, and you can query has_labels
# And for times when you don't want to create an ActiveLabel subclass:
ActiveNode.has_labels(:Cartoon, :Animal).all
class Zoo
include Neo4j::ActiveNode
has_many :out, :animals, type: :ZOO_MEMBER, has_labels: [:Animal]
def escapees
animals.has_labels([:Escaped])
end
end
z = Zoo.first
z.animals.each { |a| a.add_label! :Escaped }
z.escapees # Good god, they've all escaped!
z.animals.first.labels # [:Animal, :Cartoon, :Dragon, :Escaped] <- Oh, our dragon! Catch him!
z.animals.first.remove_label! :Escaped
It really needs to be clearly defined what an association which has labels means.
Also there should be callbacks for adding and removing the labels.
So, I really appreciate your taking the time to write all this out and there are some good ideas in here. I also think this issue thread contains two (maybe three) separate conversations.
@cheerfulstoic kinda misunderstood my issue originally, or maybe just got distracted by my unusual example, and started to go off in a tangential direction. I think I got him back on the same page as me now. I think you've read @cheerfulstoic's ideas, and (understandably) started to build off of them. I can't tell if you are independently excited about these ideas (which, if so, is awesome) or if you're trying to help me solve a problem I don't have. I don't mean to be negative, I'd just hate to have you waste your time on a misunderstanding.
So, having said this, lets discuss creating an ActiveLabel
like feature (which again, is a tangential issue-->that is, it's kinda related, but not really)
While I definitely understood what @cheerfulstoic was going for in his example, I didn't appreciate the need for the idea -- or your ActiveLabel idea, until I tried to model Neo4j's example movie database in ActiveNode. At the moment, you can't really. So then I "got it." Looking through what you and @cheerfulstoic put down, I'm not loving the DSL though. I have some ideas which build off of yours and @cheerfulstoic's, and should solve the movie database problem. For my own sanity, let's move the conversation over some sort of module feature to a new issue: #1453.
regarding
I would also like to say that I never knew inheritance fused names together to produce one long label.
Actually, inheritance does not do this. I'm guessing you're referring to Ruby's namespacing. Take this example:
class Person
class User
end
end
The User
class is namespaced as Person::User
, even though User
has not inherited Person
and shares no code with person. Similarly:
class Person
end
class User < Person
end
In this example User
has inherited Person
but is namespaced as simply User
(not Person::User
).
By default, Neo4j, uses modularized label names (so in the first example, the User
model would have labels = Person::User
. In the second example, the User
model would have labels = :Person:User
. In this example:
class Person
class User < Person
end
end
User would have the labels = :`Person::User`:Person
. The docs kinda explain this.
Regarding
When you apply multiple labels, you are causing the indexes for those labels to be unioned. That's why they say make a :BlogPost:ActivePost and just match on :ActivePost.
This makes sense, but it also goes against other performance tests I've read. If true, it also would imply that ActiveNode's habit of querying on all of a model's labels is oftentimes bad (given that, usually, the first, modularized label is all that is needed for the query). I imagine this is something @cheerfulstoic knows about. Regardless, I want to do some more research into this.
@thefliik can you give some concrete examples of what you don’t like about the DSL? It very much tries to keep in line with what ActiveNode and ActiveRel already are.
Thanks for clarifying about the module namespacing. Because I didn’t know about it, I was very confused.
But can you give me a concrete example of why they are useful?
I guess I could put a Reading inside Kanji to get Kanji::Reading label while keeping a short, reusable model name...but I don’t understand the benefit of this.
Simply KanjiReading
suffices.
@leehericks well the more I read over your code, the more I understand it. I think what I wrote down is really similar to what you're going for.
Comments on the DSL:
I'm not loving the required labelled_node do
block
class Destroyed
include Neo4j::ActiveLabel
labelled_node do
scope :current, -> { all.where_not(has_labels: [Destroyed])}
end
end
If we're going to give ActiveLabel its own DSL, I'd say we should try and eliminate the need for a block as much as possible. i.e.
class Destroyed
include Neo4j::ActiveLabel
scope :current, -> { all.where_not(has_labels: [Destroyed])}
end
This is basically what I used in #1453. The more I think about it though, I don't like the idea of creating more ActiveLabel specific DSL. I'd vote for simply using ActiveSupport::Concern
DSL, which I think most developers will be familiar with
class Destroyed
include Neo4j::ActiveLabel
included do
scope :current, -> { all.where_not(has_labels: [Destroyed])}
end
end
This format makes it easier for folks to mix in their own logic in the same manner that they do outside of Neo4jrb.
I'm also not loving the need for label
methods in the class.
class Dragon
include Neo4j::ActiveNode
property :color, type: String
label :animal, removeable: false # implies default: true
label :cartoon, removeable: false
end
I'd prefer to see someone include the module and then have the module's label automatically added as either an optional label OR automatically added as a required label. Either way, a developer would only need to add some kind of label :animal, optional: true
method if they were bucking convention.
@thefliik I think you have to think this through more deeply.
We declare properties and associations. They are concrete, readable schemas. Those calls also synthesize a lot of helper methods. Likewise, a label is a concrete thing applied to a node and making a call like label :animal
is the same as property or has_many. Declarative syntax is good and readable.
ActiveRecord::Concern is a way to mixin, yes. But this is more than mixing in a module. So if you say include do
, you haven't done anything except change labelled_node do
and the latter provides context. And that context is well defined by calling label
with options as mentioned in 1. And you may want to add logic to your label class which at the label level, not one of the things which are added to labelled nodes.
If ActiveLabel classes can respond to callbacks, such as before_save, after_label_removed, etc. then you can't define that in the ActiveLabel class and include it in the ActiveNode without introducing conflicts. ActiveRel has callbacks too and by specifying the rel_class they get hooked into the process. This is no different.
Too much magic is confusing.
Problem
Currently, when defining a
has_many
orhas_one
association, you indicate what the association's targets are usingmodel_class
. This can be limiting. For example, given the models below, I want the person'shas_many
association to target current signup questions AKA nodes with(:`Question::Signup`:`Current`)
labels. This could be accomplished by creating a class specifically for this purpose (i.e.Current = Class.new(Question::Signup){mapped_label_name = "Current"}
) but the class would only exist to work around neo4jrb's limitations.Proposal
Would you accept a pull request to add a
model_labels
option tohas_many()
andhas_one()
? This option could be used instead ofmodel_class
(anArgumentError
would be raised if they both were included) and would indicate the labels to match against. To use the option, you would include an array of strings where each string was formatted like the label part of a cypher query.