Open jorroll opened 6 years ago
I think I'd change the ActiveModule
DSL to specifically use that of ActiveSupport::Concern
i.e.
module User
include Neo4j::ActiveModule
def stats
puts "the stats"
end
included do
property :login
property :password
property :roles
end
end
@leehericks, what do you think? ^^^
Seperately, regarding:
If you wanted a class to always have a ActiveModule's label / property, you could add something like by_default_has :User
Your ~ label :User, default: true
is probably better. It still doesn't feel 100% right to me though, as, currently in ActiveNode, similar method calls (i.e. property :name
, has_one :person
, has_many :people
) all add something to the model. In the label :User
case (at least in my DSL) it's configuring something on the model.
First, I don’t know why you opened another issue. There are enough open issues in this project and it cuts off a whole part of the discussion.
Second, if_is is waaaaaaay too much domain specific to your modeled domain example.
labels are concrete things added to the node, same as properties and associations. The functionality that can be associated with it is the optional domain-agnostic part
I want to follow up that you are full of great ideas too and I’m just saying the framework should sit under you and allow you to design an if_is for your case. We just have to be sure what gets created and supported at the core is flexible and useful for everyone. I think that is why Brian introduced his thoughts with hesitation and the note that he wants to take his time on it. If we keep having good discourse we’ll nail something awesome.
Also you raising the issue of the movie database is a great real world example! It’s definitely a situation where inheritance is not the answer. Gonna dwell on it!!!
I'm fine with another issue, the other one was feeling a bit overwhelming ;). I'm just happy if we can agree on a proposal ;). I haven't read the other thread yet and I'll need to get back to work after this comment, but I thought I would start with this issue.
What you're suggesting @thefliik is, I think, closer to what I was thinking. Some feedback:
Regarding ActiveModule
, I actually like ActiveLabel
because it works at the same level as ActiveNode
/ ActiveRel
(it describes the concept that it matches up with from the database).
Totally makes sense to have included
in the modules based on ActiveSupport::Concern
. The thing I would say about if_is()
, though, is that part of the point of modules in Ruby is to have a centralized place to define behavioral functions, so I think that that method would belong inside of the module. But, of course, you don't want to have the function there if the label isn't attached to the node, so it should probably be inside of a block so that the methods only get attached to the object if the label is there on the node in the DB. I think that might look like:
module User
include Neo4j::ActiveLabel
def stats
puts "the stats"
end
module InstanceMethods
def admin_powers(id)
Person.find(id).destroy
end
end
included do
property :login
property :password
property :roles
end
end
To me this also highlights the question of the difference between the stats
method and the admin_powers
method. Since you're using include User
inside of the Person
model, that means that the Stats
method will be defined on Person
objects regardless of if the User
label was there or not. Was that intentional? Also, if we delegated the included
block directly to ActiveSupport::Concern
I think that the same problem would occur (the block would be run at the point that the include User
line is run, thus always defining those properties). I think it would be pretty easy, though, to have ActiveLabel
's implementation of included
only conditionally run the block based on if the label was there.
All of that said, I like the idea from the other thread about calling some method and giving the module so that the entire module will only be included depending on what things are defined. Something like:
class Person
include Neo4j::ActiveNode
label :User
end
This also gives us the power to have a single method with which to implement both cases we've been talking about: One where the label is always there (the default: true
case) and the case where the label is only sometimes there (I might actually specify the option conditional: true
or optional: true
instead of the default: true
in the other case).
I think that this would also give us the ability to say director.user?
as well. If you just did include User
, then you would only be able to have that return true
in the case that it was included or it would raise a "method not found" error if it wasn't included. Probably the user?
methods would only be defined in the optional: true
case, though, because otherwise it would always return true
and there's not really a point to it.
Regarding
I like the idea from the other thread about calling some method and giving the module so that the entire module will only be included depending on what things are defined.
label :User
Ah, so I didn't quite see it before, but I approached this from a slightly different viewpoint. My thinking with the above was that you include a module in a class and with that module comes a label. Your thinking seems to be, you include a label in a class, and with that label comes functionality. The difference between the two approaches is that, in your approach (I think), every ActiveNode
in the app gains the functionality associated with a label if the label is present on the node. In my version, an ActiveNode
only gains the functionality associated with a label if the module is included in the class and the label is present on the node. Put another way, you're adding powers to labels, whereas I was adding powers to modules. I'm a little hesitant about working with globals, but I can see how your approach makes sense for Neo4j. It certainly forces developers to always be thoughtful about their use of labels. I suppose in my scenario, for a (probably legacy) app, you could create two different ActiveLabel
modules that both responded to the same label, but include them in different classes. Meaning that the different classes responded to the same label in fundamentally different ways. (which would make sense in this scenario, because devs wouldn't look to labels to see what functionality their classes had, they'd look to the included modules)
Regarding ActiveModule, I actually like ActiveLabel because it works at the same level as ActiveNode / ActiveRel (it describes the concept that it matches up with from the database).
Makes sense, especially given my new understanding of your approach 👍 .
The thing I would say about if_is(), though, is that part of the point of modules in Ruby is to have a centralized place to define behavioral functions, so I think that that method would belong inside of the module. But, of course, you don't want to have the function there if the label isn't attached to the node, so it should probably be inside of a block so that the methods only get attached to the object if the label is there on the node in the DB.
This thought makes sense given #1
. In my DSL, you would need to include the User
module in Person
to gain its properties. In this scenario, an if_is()
block inside Person
makes sense (I think) because the person class is defining everything that applies to it. i.e. the Person class gets the User
module, conditionally the Person
class gets some methods if the User
module is active. etc. In your scenario though, the User
activelabel is independent from the Person
class. I guess my last thought on this is that, conceptually, the "if this label is on a Person
do this block" doesn't belong to the User
Activelabel any more than it belongs to the Person
Activenode. I'm fine with choosing to put it inside the module. Thinking about this more, in your DSL, I'd still advocate for putting this conditional block inside the class. In your scenerio, the ActiveLabel is truly global whereas the class already has at least one conditional (i.e. it might have an additional label). I'd say, keep the conditionals in the class (i.e. it might have an additional label, and if it does do this).
Update
: actually I forgot that a label could say something like "if this is included in the Person class OR if this is included in the Car class, do this". Code like that would definitely be better placed in the module, so it makes sense to put all conditional code there.
To me this also highlights the question of the difference between the stats method and the admin_powers method. Since you're using include User inside of the Person model, that means that the Stats method will be defined on Person objects regardless of if the User label was there or not.
So in my DSL, I had an unstated, subconscious, expectation that anything inside an ActiveLabel
module only applies if it is included inside an ActiveNode AND the node has the label associated with the ActiveModule. If you wanted to always include functionality in a class, you'd need to make a separate concern. So in the above, any class that inherits the User
module gains the stats
method only if the node also has a :User
label. And only the person class gains the admin_powers
method if a person node also has a :User
label.
Fleshing my subconscious expectations out further, including a User
ActiveLabel in a class would always add some general methods, such as the ability to call .user?
on any instance of that class, or the ability to call a .user
singleton method to create a new person with a user label (i.e. Person.user.new
)
I think that this would also give us the ability to say director.user? as well. If you just did include User, then you would only be able to have that return true in the case that it was included or it would raise a "method not found" error if it wasn't included.
This was intentional on my part, I was thinking that you would only want the ability to call .user?
on classes that you explicitly included the User
module in. Are you thinking that every ActiveNode
in the app would gain the ability to call .user?
I imagine you'd only have the ability to call .user?
on classes that called label :User
.
So if my understanding of your ActiveLabel
concept is correct, and any node with the ActiveLabel
's label will gain the ActiveLabel
's properties, I've thought of an issue in my app:
In my app, I track node state through :Current
and :Destroyed
labels (you can learn more here). Supporting this are a set of custom CRUD methods. I also want to track relationship state, but Neo4j doesn't support multiple labels on a relation, so I model stateful relations using two relations with a node in the middle: (person)-[:FRIEND]->(:FriendRel:Current)-[:FRIEND]->(person)
To mark the relation as destroyed: (person)-[:FRIEND]->(:FriendRel:Destroyed)-[:FRIEND]->(person)
. Tracking relations also makes use of :Current
/ :Destroyed
labels, but the associated CRUD operations (i.e. ActiveLabel methods / properties) are completely different. The ActiveLabel
scheme I proposed supports this, I could create two different ActiveLabel modules, both of which are associated with :Current
labels, but do different things. Classes representing a trackable relation would include one version of the ActiveLabel
module, classes representing trackable nodes would include a different version of the ActiveLabel module.
Do you envision this use case being supported in your ActiveLabel scheme? How?
To pile on a bit more, I can see how my scheme could be extended to create a module that only applies if TWO labels are attached to a class. Something like this, highly contrived, example:
class Animal
include Actor
include Living
include Extraordinary
end
class Person
include Actor
include Living
end
Where the Extraordinary
module was associated with BOTH Living
and Actor
labels but would only "activate" if both labels were present on an Animal
.
Animal.actor.living.new
# same as
Animal.extraordinary.new
Now thats some polymorphism :)
@thefliik Regarding tracking relationships, I think you're missing a very simple model.
(p1:Person)-[:FRIENDSHIP]->(p2:Person) (p1:Person)-[:DESTROYED_FRIENDSHIP]->(p2:Person)
This article from Neo4j Blog: https://neo4j.com/blog/neo4j-genericvague-relationship-names/
As much as we want to live in an ideal beautiful data modeling world, Neo4j has performance quirks to force us to optimize.
class Person
include Neo4j::ActiveNode
has_many :both, :friends, type: :FRIENDSHIP, model_class: :Person
has_many :both, :destroyed_friends, type: :DESTROYED_FRIENDSHIP, model_class: :Person
# Might use ActiveRel for timestamps
# Add logic for destroying friendships
end
To pile on a bit more, I can see how my scheme could be extended to create a module that only applies if TWO labels are attached to a class.
before_save, if labels contains :A and :B then do something. or define a method that checks the existence of the two labels and returns boolean and use it around your code. Something like this that transcends the duties of one label should probably be put in a module and included, being separate from the things that one label adds.
Regarding the movie database, whether labeled an Actor, Director, or some combination thereof, first and foremost the nodes have a shared base label. They are a Person, but they might be multiple things. This is where inheritance fails.
This is also why the things granted by the existence of a label can't be conditionally granted in terms of adding removing model functionality and one has to be careful about clashing names.
A director has movies they directed An actor has movies they acted in
The associations of each label can't all be named movies.
This is why I caution that we add the ActiveLabel feature and you make a module to share which checks for the existence of multiple labels separately. That's a Ruby language feature that doesn't need any special Neo4j.rb additions.
I was also going to suggest either the FRIENDSHIP
/ DESTROYED_FRIENDSHIP
solution or having a destroyed
property on the relationships. Both have performance indications, but both would probably perform better than two relationships and a node (though on a small to mediumish scale it probably doesn't matter).
Regarding the last thing that you said @thefliik about polymorphism, it would be super cool for ActiveLabel
modules to automatically add scopes. I would have a where_
prefix for clarity, so:
Animal.where_actor.where_living.new
Ah, so I didn't quite see it before, but I approached this from a slightly different viewpoint. My thinking with the above was that you include a module in a class and with that module comes a label. Your thinking seems to be, you include a label in a class, and with that label comes functionality. The difference between the two approaches is that, in your approach (I think), every ActiveNode in the app gains the functionality associated with a label if the label is present on the node. In my version, an ActiveNode only gains the functionality associated with a label if the module is included in the class and the label is present on the node.
I think we're getting close, but not quite. I'm not suggesting that models ever get the behavior from a module if that module wasn't explicitly defined on the model. I think it definitely makes sense that the designer of the app needs to call out specifically on what models an ActiveLabel
module can affect behavior of objects of that model class. So I think we're on the same page there.
Where I'm coming from is that I think that I'm seeing two different use cases for ActiveLabel
modules:
ActiveLabel
module when the label is appliedActiveLabel
module and the label will always be applied on nodes created for that module.I think that Destroyed
is a good example of the first use case.
I see the second case as basically multiple inheritance (which is really the point of modules in Ruby anyway). It would be as if the class had more than one parent class, meaning that the objects would always have the labels for the modules and searching on the module like TheModule.all
would give you a proper superset of all the nodes for the classes that have that module (because, again, the label would always be there).
I could say more, but since we have a couple of threads going I wanted to make sure we're on the same page with this point first.
@cheerfulstoic @thefliik Let's first decide on these core API which sit under other features such as ActiveLabel. With these core API in place, anyone can easily mixing and add functionality to their models based on node labels.
ActiveNode:
# Fixed model labels
# Deprecate self.mapped_label_name, setting it calls self.model_labels = [mapped_label_name]
Model.model_labels=([:Label1, :Label2])
# Synthesizing optional labels
Model.optional_label(name, options)
# Manipulating labels
model.labels # Array of label names which includes ActiveNode model/inheritance labels and applied optional labels
# Querying
model.has_labels([:Label1, :Label2])
model.where(has_labels: [:Label1, :Label2])
optional_label
Options
Calling optional_label
synthesizes:
# Add or remove the label
model.labelled_[name]=(Boolean)
# Check Existence
model.labelled_[name]? # Returns boolean
# Querying
model.labelled_[name] # Query scope, Person.labelled_actor.labelled_director
Labels should get persisted on call to save
to allow validations and custom logic in callbacks.
Example:
class Case
include Neo4j::ActiveNode
optional_label :Current, default: true
property :title, type: String
def archive
labelled_current = false
end
end
Case.all.labelled_current
Remember that learning a framework is hard and this is on top of Rails, Ruby language, understanding Neo4j and how to data model for the graph, etc. I'm not a genius and I struggle a lot. Ambiguous naming can be really confusing. Clear context is everything.
Well, I see add_labels, remove_labels, mapped_label_names, etc. already exist. ^^;
I could respond, but I'd like to try something that I hope might be more effective. One idea that I've had for developing something in the past which I haven't really tried is "documentation driven design". That is, before writing any code, write the documentation for how it would work so that you're thinking of it from the user's perspective (kind of what @leehericks is doing). Once you're happy with that you can write the code and you should have really good documentation for it.
So what about this: We have a PR which defines documentation for non-existent code. This way we can discuss things in the comments, but we can also comment on specific lines / sections via GitHub's awesome PR UI.
Worth a shot?
Yes, I have no experience with github PRs, but can we all edit the example code or does one person initiate it and then update the documentation according to comments and consensus?
@cheerfulstoic At some point as the lead developer with a deeper understanding of the framework you do have to comment. 🤪
I assume if you see something that is impossible or misunderstood, you'll let me know. I don't want to labor too long under misunderstandings.
P.S. I am trying to dig into some of the source code/documentation to get better acquainted with the project. :)
Yeah, I definitely have comments, but I feel like we're three people pulling in three directions on a variety of issues and it's really hard to keep track of it.
With GitHub PRs, one person makes a branch/fork and submits a PR for the patch of those changes. PRs are just issues in GitHub, but you can also see the different proposed file changes and click on individual lines to start a conversation about that line (or the block of code above that line). There's more, but that's the main thing I wanted to take advantage of.
@thefliik ?
So are we all going to create our own PR then? Like fork the project and add a markdown file or a ruby file and create a pull request for that?
I was thinking of having one person create a PR and we can all comment on it (and maybe allow changing it too for efficiency since we're sort of spread out across the world). But we could also do separate PRs and then we could each comment on each others' and probably solidify on one. It would certainly be a nice way to be able to show where we are the same and different.
Regarding what we would create, this repo has a docs directory which is where all of the documentation is kept. The files are in reStructedText format. I chose that because it's the default choice of readthedocs. RTD supports markdown, but it doesn't provide many features in that context (RTD uses Sphinx, a popular tool in the python community, to generate a documentation suite). So a PR would probably just need to include a new docs/ActiveLabel.rst
file (along with the corresponding change to the table of contents at the top of docs/index.rst
).
Ok, I've put together a bit of documentation:
https://github.com/neo4jrb/neo4j/pull/1457
Definitely not complete! Definitely didn't get everything in there! Just trying to get a start on something where we can be more focused on how we have a conversation.
We can discuss just on there or we can have other proposals too. Happy to go in whatever direction brings clarity
Thank you Brian! Sorry, currently having a school IT crisis. FML lol
No problem, I'll be busy in the upcoming days, so take your time ;)
H'okey, back. Ahhh...again I find myself wishing that Github allowed nested replies.
Regarding
@thefliik Regarding tracking relationships, I think you're missing a very simple model. (p1:Person)-[:FRIENDSHIP]->(p2:Person) (p1:Person)-[:DESTROYED_FRIENDSHIP]->(p2:Person)
You're totally right. Until I read that GraphAware article you shared, @leehericks, I don't think I realized how expensive filtering on a label was. I'm definitely going to update my tracked relations with _CURRENT
or _DESTROYED
suffixes (It's important for me to call out _CURRENT
relations as well, because it distinguishes between a tracked / untracked relation). Unfortunately, this does not let me rid myself of the general (person)-[:FRIEND]->(:FriendRel:Current)-[:FRIEND]->(person)
format, as I also track which user created the relation, which necessitates a (user)-[:CREATED]->(:FriendRel)
. Still, for most queries this will allow (person)-[:FRIEND_CURRENT*2]->(person)
which looks like it will provide a nice speedup (and also simplifies the query). Separately, I'll need to think about if I need (:FriendRel:Current)
or if I can get away with just (:FriendRel)
.
Regarding
To pile on a bit more, I can see how my scheme could be extended to create a module that only applies if TWO labels are attached to a class.
before_save, if labels contains :A and :B then do something. or define a method that checks the existence of the two labels and returns boolean and use it around your code. Something like this that transcends the duties of one label should probably be put in a module and included, being separate from the things that one label adds.
Maybe you know of a Ruby secret I don't, but conditionally adding a module to an instance of a class isn't as easy as you imply. Ya, testing for the condition is easy, but how do you include the module? I can use obj.extend
to include the method definitions, I'd need to, seperately, evaluate the included
block. (And currently, Neo4jrb doesn't add a property
method to instances of a class). This is the whole point of this conversation (for me at least). And if you do know of some easy way of accomplishing this, then I might agree that the gem should stay as is and we can just update the documentation. As it stands, I think there are some steps the gem could take to make dev's lives easier.
Let's first decide on these core API which sit under other features such as ActiveLabel.
Well, I see add_labels, remove_labels, mapped_label_names, etc. already exist. ^^;
Ya
@cheerfulstoic
Yeah, I definitely have comments, but I feel like we're three people pulling in three directions on a variety of issues and it's really hard to keep track of it.
I agree that we are currently three people pulling in different directions. Jumping over to look at the documentation though, I see some assumptions there that I disagree with. Most importantly, I'm still arguing that, at the core, what an ActiveLabel
should do is add stuff to an instance of a class if a specific label is associated with the instance. How that label is added to an instance of the class, in my mind, is a separate conversation (though it would certainly be present in the documentation).
A point against the documentation approach, is that it immediately mixes in all of these different concerns together in the conversation (e.g. your documentation touches on "how do we add additional mapped label names to a class?"). I like the idea of starting with the documentation, but, personally, I'd like to understand what ActiveLabel
modules are trying to accomplish first.
My argument: ActiveLabel
modules should add all their custom functionality if a specific label is present on an instance of the class. Otherwise, they should add none of the custom functionality. ActiveLabel
modules should always add some singleton methods to the class (to be determined).
If folks agree with this, personally I can move forward, but if folks don't agree with this, I'd like to hear their thoughts on why not? Looking at the documentation, you've put together @cheerfulstoic, it appears that your ActiveLabels
are trying to combine multiple ideas (i.e. some ActiveSupport::Concern
, some additional_mapped_label_names
, some conditionally adding custom behavior to a class instance if a label is present, etc). Currently, I'm not in favor of this approach, though maybe it's simply because I don't understand your reasoning.
If your approach is the same as mine, then label :Destroyable, optional: true
doesn't seem necessary. When you include the ActiveLabel
module in the class, that already implies that the :Destroyable
label is optional. You'd only need some kind of label :Destroyable
dsl if you wanted to specify that :Destroyable
was an additional_mapped_label_name (i.e. always present).
Alternatively, if you're thinking of the label :Destroyable, optional: true
api as a way to simply include an ActiveLabel
module, that's (obviously) really confusing for me (because it looks like you're trying to describe what labels are present on the class). If you want to include a module in a class, Ruby already has the include
api for this.
class Person
include Destroyable
# Developer opens the destroyable module, sees that it's an `ActiveLabel` module,
# and understands that Person nodes may have a `:Destroyable` label
end
A developer could add an optional label to a class that isn't associated with any ActiveLabel
functionality, after all.
When I opened this thread, I made the mistake of mixing in a number of separate ideas based on unspoken assumptions I had. I should have started more focused, waited for thoughts, and moved out from there.
Looking some more, it really looks the label :SomeLabel
API is a combination of "add some label" with "add this module if this label is present." My vote is to separate those two resulting in something like
If the additional label is always present
class Person
include Neo4j::ActiveNode
include Destroyable
add_label :Destroyed
end
# OR
class Person
include Neo4j::ActiveNode
include Destroyable
add_label Destroyable.follows_label # .follows_label would return whatever label the `ActiveLabel` module is associated with
end
# OR using #1414
class Person
include Neo4j::ActiveNode
include Destroyable
self.additional_mapped_label_names = [:Destroyed]
end
If the additional label is optional
class Person
include Neo4j::ActiveNode
include Destroyable
end
I think issue #1424 has started to bloom into multiple issues. I'm opening this up to continue the discussion of creating some sort of includeable ActiveLabel module functionality.
Preface
@cheerfulstoic began this conversation in #1424 but I didn't really "get it" (see the use case) until, unrelated, I just embarked on a side quest to benchmark the effect of label ordering on query time. As part of this, I downloaded the Neo4j example movie database to test against. Lo-and-behold, as I tried to use ActiveNode to model the data, I begun to see problems....
I initially thought to model the database like so:
If you check out the example movie database, you'll notice problems with the above though. This is because a node can have
:Person:Actor:User
labels associated with it (i.e. can be both an Actor and a User at the same time). So inheritance doesn't work, because then a node would be wrapped as either an Actor or a User, but would not be both (when it should be both).Adding multiple labels to
Person
(such as using modules) doesn't work either though, as thenPerson
will always have those labels (which is inappropriate). So:User
,:Actor
, and:Director
labels are truly optional to the person node, except their presence alter's the properties of the person node. In the case of:User
, it adds properties.The exciting part...
So after reading the ideas in #1424 and trying to solve the movie database problem, here's my take which, the more I think about, the more I like. This is definitely similar to what @leehericks wrote down in #1424 -- I just didn't understand it at the time.
(it's also possible that this is exactly what @cheerfulstoic and @leehericks where getting at over in #1424)So in the above we can see a few things going on. We have a base
Person
class. This person class includesUser
,Actor
, andDirector
ActiveModules. These ActiveModules are associated with a label of the same name. If a:Person
node is retrieved from the database, and it includes a label associated with one of the ActiveModules, lets say theUser
module, then the node gets wrapped as aPerson
object but gains the properties associated with aUser
.User
properties can be defined in theUser
module. They can also be defined in a specialif_is(User) do
block inside thePerson
class. This way, if aUser
module is included in several classes, those classes all gain the properties associated with theUser
modules. Additionally, a specific class could say that it's users gain properties / methods that other classes don't get.At this point, I should also mention that I'm not committed to any of the naming / DSL, but hopefully you can see what I'm going for here.
Things like
If you wanted a class to always have a ActiveModule's label / property, you could