Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correcting ConstraintError in create_or_update when modifying __required_properties__ #575

Open
turukawa opened this issue Sep 10, 2021 · 2 comments

Comments

@turukawa
Copy link

I have data where there are unique reference fields, and required title fields. Titles are not intended to be immutable, and can be modified.

When using bulk create_or_update, updates fail with:

ConstraintError: {code: Neo.ClientError.Schema.ConstraintValidationFailed} {message: Node(6) already exists with 
label `Project` and property `identifier` = 'b812a28ffd844c7a954e03588315a002'}`

I found a number of references to similar experiences (cf #438 (comment)), but no solutions. This is a minimally-invasive change I'd like to suggest, unless there is an already-existing solution I should be using instead:

In _build_merge_query in core:

", ".join("{0}: params.create.{0}".format(getattr(cls, p).db_property or p) for p in cls.__required_properties__))

There is a specific check for both required and unique_index properties of a node using __required_properties__. My understanding is that required fields are mutable, but unique_index fields are assigned on creation as immutable references. This is something that probably doesn't catch everyone (depending on how you decide to use required), but it catches me.

My suggestion is as follows:

  1. Create a new __unique_indexed_properties__ class property:
    @classproperty
    def __unique_indexed_properties__(cls):
        return tuple(
            name
            for name, property in cls.defined_properties(aliases=False, rels=False).items()
            if property.unique_index
        )

This could be added to NodeMeta.__new__ instead along with the other property tests:

class NodeMeta(type):

  1. Update _build_merge_query as follows (replace L289-L291 with these rows):
        required_properties = cls.__required_properties__
        if update_existing:
            required_properties = cls.__unique_indexed_properties__
        n_merge = "n:{0} {{{1}}}".format(
            ":".join(cls.inherited_labels()),
            ", ".join("{0}: params.create.{0}".format(getattr(cls, p).db_property or p) for p in required_properties),
        )

That then shouldn't interfere with get_or_create. If this is of interest, I can submit an update, otherwise simply inherit from StructuredNode and update accordingly.

@whatSocks
Copy link
Collaborator

@turukawa yes please, this seems like a good suggestion.

@turukawa
Copy link
Author

@whatSocks one thing to check, though, is this where you check for required fields as well? Technically, required vs update should be different checks - an update should already have passed a required field check - but combining them (as with this) may leave a gap.

Ultimately, I'd say that's the responsibility of the developer - and there are great tools like Pydantic that can do safe sanity checks before hitting the database - but it would be good to know what is lost with this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants