ActiveRel: a relationship wrapper #393
Comments
|
Great. Will have a look at this later this week. I would like to explore alternatives as well, for example: maybe without using classname property and instead use labels of incoming and outgoing nodes for a relationship to decide which relationship ruby class it should be. |
|
What do you think about renaming |
|
That sounds good! I was just looking for something generic to get the ball On Wednesday, July 30, 2014, Andreas Ronge notifications@github.com wrote:
|
|
Also, I'm not really married to much of what was done in there , I just If we can find a good way to use the related nodes to determine class, that On Wednesday, July 30, 2014, Chris Grigg chris@subvertallmedia.com wrote:
|
|
I think that this should wait until the query_experiment branch is merged in and we can tackle it as a team, since there's so much to cover between specs, moving code around, and figuring out exactly how we want it to behave and interact with what's already there. What do you think? @cheerfulstoic, what about you? |
|
I agree, we should wait until query_experiment is in master. |
|
I just pushed a hideous 24bc3a2#diff-149d2ed2cc215f0946334b0d9ea975c5R8 I don't think it will be necessary to worry about finders on ActiveRel too much but having some methods to generate cypher matches will be nice. The preferred method of finding a relationship is to query for the node, then work with its rels. Otherwise, the big benefit is being able to create from ActiveRel classes and take advantage of validations and callbacks. |
|
@subvertallchris To answer your question in #401, I sure understand re: specs and work 😈 and I have, indeed, played with the existing branch. I am trying it out in my current project where relationships are used to record financial transactions... |
|
I just pushed a ton of changes to this branch. They include a ton of specs, support for all the basic persistence features you'd expect, fixed the broken before_validation callback. There are also updates on neo4j-core so Gemfile is now pointed at my dev branch. Still a few specs that should be added, specifically testing for failures, but I'm way too tired to keep going tonight. Also haven't checked this at all in JRuby, so expect nothing to work correctly in Embedded mode yet. Failing specs are result of missing changes from query_experiment, which I'm not going to bother merging until it's all done. Haven't changed Library to ActiveEntity or whatever it should be, will worry about that later. |
|
Among the nicer changes, it lazily loads related nodes if you pull up a rel using something like class MyRel
include Neo4j::ActiveRel
outbound_class Class2
inbound_class Class1
property :this_thing
end
MyRel.create(outbound: node1, inbound: node2, this_thing: 'words here')
# or
r = MyRel.new(outbound: node1, inbound: node2, this_thing: 'words_here')
r.saveClass method All callbacks and validations are working on these classes. |
|
This is really nice. Just one minor thing: I would prefer using |
|
Just got a chance to read through the thread. Seems really good overall. Haven't looked at the code yet. I was also going to mention the start / end thing. Maybe just Also, maybe use |
|
Would you guys be ok with just class MyRel
include Neo4j::ActiveNode
from_class Show
to_class Band
end
r = MyRel.new
r.from = show
r.to = band
r.saveGood idea on |
|
Changed @cheerfulstoic, when things settle down a little for you, I'd love your help making query a bit more usable. I threw something together that works unless the outbound class in the model is set to |
|
Nice that you changed to Btw the |
|
Didn't change How about this: class methods are class MyRel
include Neo4j::ActiveRel
from_class MyStartClass
to_class MyEndClass
#or!
start_class MyStartClass
end_class MyEndClass
end
MyRel.new(from_node: node1, to_node: node2)
#or
MyRel.new(start_node: node1, end_node: node2)This will let everyone use the methods that fit their workflow. |
|
You can see it at 855a2db |
|
@subvertallchris We've just gotten to the cottage where we'll be spending the next ~4 weeks (aside from the few days I'll be in Sweden, of course ;) The wi-fi's not great, so lay down your questions/ideas and I'll see what I can do to help (either here or in personal E-Mail) |
|
The big thing would really be to just take a quick look at https://github.com/andreasronge/neo4j/blob/2e04822f277e48974e9deaa48b9c924778dafcdd/lib/neo4j/active_rel/query.rb. I'm looking at the outbound class and using query_as relative to that, or I'm doing Neo4j::Session.query if outbound class is :all. It all works, I just don't love it. |
|
Beyond that, do you think we're ready to merge query_experiment in now that callbacks are implemented? |
|
I'm looking at the code now (might have to go if Theo wakes), but I'm def down with merging query_experiment |
|
Added lots of words to https://github.com/andreasronge/neo4j/wiki/Neo4j%3A%3AActiveRel. |
|
It's very much appreciated. Will have a look at it tonight. |
|
I run codeclimate on the branch (https://codeclimate.com/github/andreasronge/neo4j/compare/activerel) |
|
I pride myself on having minimal smell. On Friday, August 15, 2014, Andreas Ronge notifications@github.com wrote:
|
|
That's good since we're going to be working together in person... Brian On Friday, August 15, 2014, Chris Grigg notifications@github.com wrote:
|
|
3.8 from Codeclimate!? Unacceptable! I'll fix that. ;-) This does remind me of something, though: we might want to use this as more motivation to remove some e2e tests and create unit tests for Neo4j::Shared's methods. |
|
+1 for merging earlier to master as long it works: specs and a simple real rails app ( exempels/blog for example) |
|
I'll look at your example app, should be able to modify that. There are already specs for all the new code, 100% coverage (EDIT: except active_rel.rb itself) cause I have no life. |
|
Great work ! |
|
Yeah, it'll come out. It's an empty module, just held over from when I had something or other in there. |
|
Some comments/questions from the wiki page
Is this really true ? I would think it gets whatever you assign the property to, but maybe active_attr converts every property value to string by default.
Why does it add a _classname property if there a declared association with a What happens if two ActiveRel models have the same I still don't like What happens if the rel_type does not match. Example: class Student
include Neo4j::ActiveNode
has_many :out, :lessons, type: 'something_else_not_match_EnrolledIn_rel_type', rel_class: EnrolledIn
end |
As for the last question, until a moment ago, it would create the relationship and use whatever type was set in the model, so you were expected to list it twice, once in at least one ActiveNode model and again in ActiveRel, or you'd get mismatched types. Student.lessons << node1 would have type 'something_else_not_match_EnrolledIn_rel_type', EnrolledIn.create would have type 'enrolled_in'. This is dumb. To fix it, I'm just finishing up some tests for a change that is going to make the model definitions even easier. If you specify :rel_class, you do not need to include :origin or :type. It will use the type provided by the class. This means that you can have shorter associations when using an ActiveRel class: class EnrolledIn
include Neo4j::ActiveRel
from_class Student
to_class Lesson
type 'enrolled_in'
end
class Student
include Neo4j::ActiveNode
# has_many :out, :lessons, type: 'enrolled_in', rel_class: EnrolledIn
has_many :out, :lessons, rel_class: EnrolledIn
end
class Lesson
include Neo4j::ActiveNode
# has_many :in, :students, origin: :lessons, rel_class: EnrolledIn
has_many :in, :students, rel_class: EnrolledIn
endAnd you only have to define the type once in the ActiveRel model! Callbacks and Validations still won't run if you do student.lessons << node, you'll need to do EnrolledIn.create(student, lesson) for that to happen. I'll have this change pushed in a few minutes. |
|
I actually don't think I can do (node, node, props) without making significant changes to other code. What about a constant of illegal property names to prevent conflicts? There should probably be one of those anyway. |
|
Ok, keep it as it is now, but add some validation, e.g. declaring a property with name I really like your example where you don't have to specify origin,type since that is already given in the ActiveRel class. Nice ! I guess you need to raise an exception if the relationship type does not match, the declared type on ActiveNode class and the declared type on ActiveRel class. Btw, what happens if I add a node of wrong class to an association ? An exception is raised ? |
|
Cool, it'll be done in a few minutes. If anyone else has a property they want to prevent, they can just add it into the ILLEGAL_PROPS array in Shared::Property. Super easy. Yeah, I like how much cleaner things look when you move the type to the ActiveRel class! It raises an exception if you even pass the type or origin option along with rel_class. If you add a node of the wrong class, an exception is raised, but this is still controlled by the ActiveNode association if you're creating from an ActiveNode model. This might seem strange but bear with me here. (This is really long, sorry.) # here's where it causes a problem
class Student
include Neo4j::ActiveNode
has_many :out, :lessons, rel_class: EnrolledIn
end
class Lesson
include Neo4j::ActiveNode
has_many :in, :students, rel_class: EnrolledIn
end
class EnrolledIn
include Neo4j::ActiveRel
from_class Student
to_class Band #obviously not what we want
end
student = Student.first
lesson = Lesson.first
band = Band.first
student.lessons << band
# raises exception because Student is excepting object of class Lesson, it does not look at EnrolledIn for this
EnrolledIn.create(from_node: student, to_node: band)
# does NOT raise an exception, this matches the model's definitionThis is an example of when it goes bad but the benefits really outweigh the negatives. You can reuse an ActiveRel model for multiple ActiveNode models by using :any. class LikesRel
include Neo4j::ActiveRel
from_class User
to_class :any
type 'likes_thing'
property :my_prop
property :another_prop
validate :likeable_node
def likeable_node
errors.add(:to_node, 'destination object cannot be liked') unless to_node.respond_to?(:liked_by)
end
end
class User
include Neo4j::ActiveNode
has_many :out, :favorite_bands, model_class: Band, rel_class: LikesRel
has_many :out, :favorite_foods, model_class: Food, rel_class: LikesRel
has_many :out, :liked_things, model_class: :any, rel_class: LikesRel
end
LikesRel.new(from_node: user, to_node: Nickelback.create).valid?
# => false
# cause nobody likes Nickelback... get it? ;-)Associations build cypher queries with labels, so you can be sure that |
|
I think we're good to merge this into master if you're into it. |
|
Awesome! Want to do a new release? |
|
@subvertallchris Yes, why not. Do you have an account on rubygems.org ? |
|
Sounds good! I just registered on rubygems as subvertallchris. |
|
Great, I will give you access. Btw I've already released v3.0.0.alpha.10 |
|
Thank you! Could you give me access to neo4j-core on github? |
|
Sorry, I thought you already had access to neo4j-core. |
|
Perfect, thanks! |
Since the great debate over the has_many API has winded down, I think the time is right for another divisive discussion: relationship wrapping.
Check out the 'activerel' branch. 53688e8 It is built upon the most recent commits to the
query_experiment(prior to tonight's updates) anduuidbranches and its gemfile is pointed to my fork of neo4j-core to get some changes that make it possible. It's less finished than I'd like but since I'm leaving in the morning to go away until Friday, I wanted to share what I have to get discussion started.ActiveRel turns relationships into first-class citizens. This is important because Neo4j makes it clear that relationships are not just the by-products of an association, they are objects and they deserve the same love and respect as nodes. Being able to create/destroy/validate from a dedicated relationship classes instead of objects on either end of the relationship makes sense.
It makes sense from an OOP perspective, too. It answers the question, "If you have a relationship between Show and Band, which model should be responsible for the properties and validations of the relationship?" Rails would say either Show or Band, but that doesn't seem true to me with Neo4j, so ActiveRel answers with a third class that can be responsible for the relationship itself. (Obviously, simple associations can and should be handled by the node models, but this adds an option for heavy lifting with highly relational data.)
So that's what I've attempted to do. With ActiveRel, you can do something like this:
Because relationships can't use labels, it sticks a
_classnameproperty on relationships when defined and the wrapping process looks for this.You can use the class method
find, access it from a node usingrels, useNeo4j::Relationship.loadwith an ID if you have one, or anything else that callsNeo4j::Relationship.loadat some point.The most divisive part of what I've done might be the reorganization of ActiveNode files. When I started this, I found that ActiveNode was really built with only nodes in mind. The first draft of ActiveRel just included ActiveNode and defined its own methods when those included contained node-specific behavior, but this led to three problems:
The result of the reorganization is Neo4j::Library and neo4j/library/, which contains modules of methods extracted from Neo4j::ActiveNode modules that apply to both relationships and nodes. This is a work in progress, just like the rest of this.
Right now, what I've done is both very unfinished but very usable, depending on what you want from it. It has no specs at the moment, I ran out of time. The only failing specs other than those related to the need for an updated
has_onemethod isbefore_validation, which just does not want to work and I am way too tired to figure out what the story is with that now. (I did fix a bunch of failing specs caused by the newhas_manymethod along the way!)There's a lot to do other than specs, so think of this as a working model. In my opinion, the highest priority issues after specs:
has_many/has_oneshould delegate to the ActiveRel class when arel_classin specified, but I haven't thought through exactly how this would work. (Would you still need to declare direction, type, classes, etc,... in the has_many call? Things like that.) For the moment, all including therel_typeoption inhas_manydoes is tag it during creation so it will be wrapped if you return it in the future.inboundoroutboundnodes once a relationship is created.Things that do work:
_classnameproperty matching existing class constantcreateandfindid,neo_id,inbound,outbound,destroy,savebefore_validationAgain, sorry to be sharing something with no specs, but I wanted to show where I am and get the ball rolling since I expect to be iPhone-only until Friday and the more time that passes, the harder it will be to merge the changes into master.
The text was updated successfully, but these errors were encountered: