neo4j-contrib / neomodel

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

`UniqueIdProperty` makes `get_or_create()` function does not work as expected #788

Open agnoam opened 5 months ago

agnoam commented 5 months ago

Expected Behavior (Mandatory)

This function is expected to find matching nodes by a filter object. In case no nodes were found, The function should create a node.

Actual Behavior (Mandatory)

The function creates a duplicated node, even if the matching object returns a node (by manual check)

Simple Example

Declarations

class BaseNodeModel(StructuredNode):
    uid = UniqueIdProperty()
    created_at = DateTimeProperty(True)
    updated_at = DateTimeProperty(True)

class Person(BaseNodeModel):
    name = StringProperty(required=True)

Example

first_tim = Person.get_or_create({'name': 'Tim'})[0]
should_be_same_tim = Person.get_or_create({'name': 'Tim'})[0]

assert first_tim.uid == should_be_same_tim.uid

Specifications (Mandatory)

Versions

mariusconjeaud commented 4 months ago

To extend on this, I did some testing like below ; I thought the inheritance might be the problem, but no, it is indeed the presence of a UniqueIdProperty() that you don't pass at creation time.

async def test_issue_788():
    class BaseNodeModel(AsyncStructuredNode):
        uid = UniqueIdProperty()
        created_at = DateTimeProperty(True)
        updated_at = DateTimeProperty(True)
        base_prop = StringProperty()

    class InheritedPerson(BaseNodeModel):
        name = StringProperty(required=True)

    base_tim = (await BaseNodeModel.get_or_create({"base_prop": "Tim"}))[0]
    should_be_same_base_tim = (await BaseNodeModel.get_or_create({"base_prop": "Tim"}))[
        0
    ]
    # This will pass if uid is commented out, but fail otherwise
    assert base_tim.element_id == should_be_same_base_tim.element_id

    first_tim = (await InheritedPerson.get_or_create({"name": "Tim"}))[0]
    should_be_same_tim = (await InheritedPerson.get_or_create({"name": "Tim"}))[0]

    # This will pass if uid is commented out, but fail otherwise
    assert first_tim.element_id == should_be_same_tim.element_id

Meanwhile, this test works fine ; the difference being that on get_or_create, we do pass the uid explicitly, so it looks like that is the key.

async def test_unique_id_property_batch():
    users = await UniqueUser.create(
        {"name": "bob", "age": 2}, {"name": "ben", "age": 3}
    )

    assert users[0].uid != users[1].uid

    users = await UniqueUser.get_or_create(
        {"uid": users[0].uid}, {"name": "bill", "age": 4}
    )

    assert users[0].name == "bob"
    assert users[1].uid
mariusconjeaud commented 4 months ago

Source of the issue is that the generate MERGE query looks like this : 'UNWIND $merge_params as params\n MERGE (n:BaseNodeModel {uid: params.create.uid})\n ON CREATE SET n = params.create\n RETURN n'

The key for the MERGE is the list of required properties ; in my above test, that is the uid property, which is auto-generated by the neomodel => does not match any existing node in the database => create instead of get