neo4j-contrib / neomodel

An Object Graph Mapper (OGM) for the Neo4j graph database.
https://neomodel.readthedocs.io
MIT License
955 stars 232 forks source link

specifying multiple node types in relation is broken in the feature_neo4j_2_0 branch #126

Closed lebedov closed 5 years ago

lebedov commented 10 years ago

The following code throws a TypeError exception when run with the latest commit from the feature_neo4j_2_0 branch (4ab834a) and neo4j 2.1.3 with python 2.7.6 on Linux:

import os
os.environ['NEO4J_REST_URL'] = 'http://localhost:7474/db/data/'

from neomodel import core, FloatProperty, StructuredNode, StringProperty, \
    IntegerProperty, Relationship, RelationshipTo, RelationshipFrom

class Humanbeing(StructuredNode):
    name = StringProperty()
    has_a = RelationshipTo(['Location', 'Nationality'], 'HAS_A')

class Location(StructuredNode):
    name = StringProperty()

class Nationality(StructuredNode):
    name = StringProperty()

h = Humanbeing(name='bob').save()
l = Location(name='london').save()
n = Nationality(name='british').save()

h.has_a.connect(l)

The exception occurs at line 62 in relationship_manager.py.

lebedov commented 10 years ago

Pull request fixing the problem (#127) submitted.

robinedwards commented 10 years ago

Hey @lebedov I removed support for relationships of multiple types in the neo4j 2 branch.

However I intend to add support for this in a different form so I am marking this ticket as an enhancement

lebedov commented 10 years ago

Thanks for the update.

peterclemenko commented 7 years ago

Any progress on this? This is breaking my project and I was wondering what the status was.

robinedwards commented 7 years ago

Hi sorry for the delay on this front.

It would be quite messy to try and support it through the existing match framework and i think could cause quite a bit of confusion.

I think the best way forward is for me to add a special relationship manager class to contrib for those that want multiple type relationships and know what they are doing.

How does that sound?

On 29 Jan 2017 14:30, "aoighost" notifications@github.com wrote:

Any progress on this? This is breaking my project and I was wondering what the status was.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robinedwards/neomodel/issues/126#issuecomment-275913814, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ0YTZur8fl9dIrkKIbbL0SKC2h9SL1ks5rXJRfgaJpZM4CXbk6 .

aaronst commented 7 years ago

Just want to add my vote for this feature, I need a way to specify multiple (or preferably not specify any) StructuredNode types. A "generic" relationship so-to-speak.

EDIT: I Also think it would be really cool if relationships understood inheritance. For example...

class Vehicle(StructuredNode):
    """"Base class for all vehicles."""

    __abstract_node__ = True

    # Properties

    make = StringProperty(required=True)

    model = StringProperty(required=True)

    year = StringProperty(required=True)

class Car(Vehicle):

    # Properties

    doors = IntegerProperty(default=4)

    sun_roof = BooleanProperty()

class Motorcycle(Vehicle):

    # Properties

    TYPES = ('sport', 'cruiser', 'touring')

    m_type = StringProperty(required=True, choices=TYPES)

    top_speed = FloatProperty()

class Person(StructuredNode):

    # Properties

    name = StringProperty(required=True)

    # Relationships

    vehicle = RelationshipTo('Vehicle', 'VEHICLE') # relationship valid for all sub-classes of Vehicle
rhartley commented 7 years ago

I am trying to find a solution to something in this area. @aaronst - I think the example below is similar to yours. In this example, an __abstract_node__ allows a relationship to be created between multiple Node types. The example works as desired but I am hoping I am missing some simpler syntax to avoid the need to go to Cypher query.

I am experimenting with a model motivated by trying to solve this py2neo problem posed on StackOverflow (not me) with neomodel.

pete.owns.all() returns results of type Asset but none of the properties are available. I am assuming this is a feature based on the abstract nature of the object but if someone who understands it could clarify, I would greatly appreciate it. Is there a better way to return nodes and "inflate" them to the proper type?

I setup the models as follows:

class Person(StructuredNode):
    name = StringProperty(unique_index=True)
    owns = RelationshipTo('Asset', 'OWNS')
    likes = RelationshipTo('Car', "LIKES")

class Asset(StructuredNode):
    __abstract_node__ = True
    __label__ = "Asset"
    name = StringProperty(unique_index=True)

class Car(Asset):
    pass

class House(Asset):
    city = StringProperty()

# Create Person, Car & House
pete = Person(name='Pete').save()
car = Car(name="Ferrari").save()
house = House(name="White House", city="Washington DC").save()

#Pete Likes Car
pete.likes.connect(car)

# Pete owns a House and Car
pete.owns.connect(house)
pete.owns.connect(car)

#This returns a Car Node as expected
for l in pete.likes.all():
    print(l)
# >>> {'name': 'Ferrari', 'id': 208}

# This is not as friendly as hoped
for n in pete.owns.all():
    print(n)
# returns {'id': 211}, {'id': 212} 
# How can I "inflate" these to the proper node types? 

# Cypher gets us the Assets we want
query = "MATCH (a:Person {name:'Pete'})-[:OWNS]->(n) RETURN n"
results, meta = db.cypher_query(query)
for n in results:
    print(n)
#returns exactly what we want, the nodes that Pete "OWNS" 
#[<Node id=211 labels={'Asset', 'Car'} properties={'name': 'Ferrari'}>]
#  [<Node id=212 labels={'House', 'Asset'} properties={'city': 'Washington DC', 'name': 'White House'}>]
badrihippo commented 7 years ago

I wrote some sort of a workaround here: http://stackoverflow.com/a/43391143/1196444. Perhaps we could build that out into a working implementation?

One hard part will be deciding which model to inflate to, when there are multiple options. (My solution just picks a random one, with a .pop() from the set). Maybe we could possible organise by priority, or pick the one lowest down the inheritance tree? Another option inspect the properties and see which ones match best...but no, that's heading too close to Artifical Intelligence :stuck_out_tongue:

robinedwards commented 7 years ago

Nice work. Thats one issue i had in the previous version that supported multi type rels. I went for the lowest class in the inheritance chain. But ideally i guess if you wanted a specific type you would define it as a separate rel.

I am thinking of adding some kind of label registry to neomodel to avoid conflicts.

I am wanting to add this feature back in there is just a lot of complexity and edge cases to think about. The match API for example when you are chaining method calls, traversing and filtering, you get to a multi type rel how do you handle it.

On 13 Apr 2017 13:42, "Hippo" notifications@github.com wrote:

I wrote some sort of a workaround here: http://stackoverflow.com/a/ 43391143/1196444. Perhaps we could build that out into a working implementation?

One hard part will be deciding which model to inflate to, when there are multiple options. (My solution just picks a random one, with a .pop() from the set). Maybe we could possible organise by priority, or pick the one lowest down the inheritance tree? Another option inspect the properties and see which ones match best...but no, that's heading too close to Artifical Intelligence 😛

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robinedwards/neomodel/issues/126#issuecomment-293866112, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ0YVN7HJqbZssQda0ym33FHv0PYa8Oks5rvgoLgaJpZM4CXbk6 .

mdantonio commented 7 years ago

@robinedwards do you have any update here? Can we help you somehow to solve this issue?

Polymorphic relationships would be very useful to simplify my graph model but prior to integrate into my code some of the proposed workarounds (like those proposed by @badrihippo) I would like to know if an official solution could be near to be released. Let me know if I can contribute somehow (by implementing something or just testing some preliminary fix)

aanastasiou commented 5 years ago

@mdantonio @badrihippo @rhartley @aaronst @aoighost @lebedov

Going through this issue seems to have been addressed in the last two releases of neomodel.

Relationships now support generalisation (as described in aaronst's post and with this it will be possible to also extend StructuredRel classes provided that they are only characterised by a single label.

Two questions:

  1. Would these be considered enough to close this issue?
  2. If not, can I please ask one of you guys to create a new issue with what would be remaining to be done from this request? My perception is that what would be remaining would be the "suppport for multiple type relationships" as described by @robinedwards here but it would be good to have your thoughts too (@lebedov as the "owner" of this issue, what do you think?)

All the best

lebedov commented 5 years ago

@aanastasiou From my perspective, yes (although admittedly the need that motivated my reporting this issue has long since passed).

aanastasiou commented 5 years ago

@lebedov Thank you very much for your response. You might want to re-consider neomodel in the future :)

enjoysmath commented 2 months ago

I just found out that polymorphisms doesn't work with Neomodel. So I have to re-design my models to be somewhat more stupid. For example derive two classes from a base class. Now you want to search for a connection to either type of object so under polymorphism you'd use the base class. Inflating using Base.inflate() static method will return None. Useless!

Just a warning to anyone else who thought they could have this elegant design using Neomodel and polymorphism. Simplify, simplify is the only way I see through it. I have now a boolean property in a "Node" class called is_my_other_type. And I have nullable relationships to either TypeA or TypeB's data classes.

BTW, this is just for StructuredNode subclasses. I didn't even try to use StructuredRels with inheritance because I was already aware they wouldn't work.