Generated `*OnCreateInput` types do not include fields to connect/create types with `@relationship` directives about graphql HOT 10 OPEN

Many thanks for raising this bug report @andreloeffelmann. 🐛 We will now attempt to reproduce the bug based on the steps you have provided.

Please ensure that you've provided the necessary information for a minimal reproduction, including but not limited to:

• Type definitions
• Resolvers
• Query and/or Mutation (or multiple) needed to reproduce

If you have a support agreement with Neo4j, please link this GitHub issue to a new or existing Zendesk ticket.

Thanks again! 🙏

from graphql.

Corresponding Zendesk ticket: here

from graphql.

We've been able to confirm this bug using the steps to reproduce that you provided - many thanks @andreloeffelmann! 🙏 We will now prioritise the bug and address it appropriately.

from graphql.

@a-alle, why has this been re-labelled as feature request and removed from bug triage?
It is clearly a bug. The API produces inconsistent data. @neo4j-team-graphql confirmed that.

from graphql.

@a-alle, why has this been re-labelled as feature request and removed from bug triage? It is clearly a bug. The API produces inconsistent data. @neo4j-team-graphql confirmed that.

Hi @andreloeffelmann, I appreciate the confusion here. This is more of a case of "there's nothing for us to do" - this is a shortcoming of Cypher MERGE operations in that you cannot create subsequent relationships within its ON CREATE clause, which means we can't implement this in the library at this current time.

In version 6.0.0 of the Neo4j GraphQL Library we're going to be making some drastic changes, including removing the ability to model 1:1 relationships as in this bug report - all relationships will have to be modelled as lists. Fundamentally, we've been battling the database for the past 3 years, and it's time to have the library only support features which the database itself natively supports. Until the database supports full schema including relationship cardinality, this will not be supported in the library, to avoid these kinds of issues.

from graphql.

Hi @darrellwarde, thanks for your quick reply!
I understand that a solution is not possible at the moment. If so, don't you think it would be best to remove the OnCreateInput-types completely for the cases where the corresponding type has a :1 relationship? If a feature is buggy / not working correctly it's better to not enable it at all. But that's my opinion. However, regarding this case, the current API state is bad.

Are you saying that :1 relationships or 1:1 relationships won't be supported with v6.?

Example for :1 relation:

type Product {
    id: Int
    price: Price! @relationship(type: "PRODUCT_HAS_PRICE", direction: OUT, aggregate:false)
}
type Price {
    currency: String!
    value: Int!
}

Example for 1:1 relation:

type Product {
    id: Int
    price: Price! @relationship(type: "PRODUCT_HAS_PRICE", direction: OUT, aggregate:false)
}
type Price {
    currency: String!
    value: Int!
    product: Product! @relationship(type: "PRODUCT_HAS_PRICE", direction: IN, aggregate:false)
}

Which kind of type definitions will NOT be supported with v6?

This is a huge thing. Our database and graphql API model is partially based on these kinds of type definitions.
Since cypher does not support maps (key-value pairs) as node properties - how should we re-model that stuff?
Technically and semantically it makes no sense to let the Product have a list of Price types. It's just that one price.

from graphql.

Another thought on this:
In theory it would be possible to re-model the Product-Price thing to:

type Product {
    id: Int
    price: Price! @cypher(statement: """
MATCH (this)-[:PRODUCT_HAS_PRICE]->(p:Price) RETURN {"currency":p.currency, "value":p.value} AS result
""" columnName: "result")
}
type Price {
    currency: String!
    value: Int!
}

But in that case it won't be possible to filter on the Price of a Product, like "give me all Products having a Price greater than X" since we have used a @cypher directive.
Or will this feature request finally be resolved with v6? To be able to filter on fields having a @cypher directive?

from graphql.

Hi @darrellwarde, thanks for your quick reply! I understand that a solution is not possible at the moment. If so, don't you think it would be best to remove the OnCreateInput-types completely for the cases where the corresponding type has a :1 relationship? If a feature is buggy / not working correctly it's better to not enable it at all. But that's my opinion. However, regarding this case, the current API state is bad.

Are you saying that :1 relationships or 1:1 relationships won't be supported with v6.?

Example for :1 relation:

type Product {
    id: Int
    price: Price! @relationship(type: "PRODUCT_HAS_PRICE", direction: OUT, aggregate:false)
}
type Price {
    currency: String!
    value: Int!
}

Example for 1:1 relation:

type Product {
    id: Int
    price: Price! @relationship(type: "PRODUCT_HAS_PRICE", direction: OUT, aggregate:false)
}
type Price {
    currency: String!
    value: Int!
    product: Product! @relationship(type: "PRODUCT_HAS_PRICE", direction: IN, aggregate:false)
}

Which kind of type definitions will NOT be supported with v6?

This is a huge thing. Our database and graphql API model is partially based on these kinds of type definitions. Since cypher does not support maps (key-value pairs) as node properties - how should we re-model that stuff? Technically and semantically it makes no sense to let the Product have a list of Price types. It's just that one price.

You are quite possibly right that we should remove it - it would actually be a removal of the entire connectOrCreate operation for any type which has one or more non-nullable and non-list relationship fields.

As to what won't be possible in version 6.0.0 (or whatever major version it ends up being when we release these broad API changes) - both of the examples you have given will not be supported, essentially any non-list relationship.

I know it might seem somewhat nonsensical to remove such "basic" functionality, but due to the lack of any enforcement of relationship cardinality in the database, we have been forced down the route of manual cardinality checks in our generated Cypher which are not comprehensive and poorly performing.

Another thought on this: In theory it would be possible to re-model the Product-Price thing to:

type Product {
    id: Int
    price: Price! @cypher(statement: """
MATCH (this)-[:PRODUCT_HAS_PRICE]->(p:Price) RETURN {"currency":p.currency, "value":p.value} AS result
""" columnName: "result")
}
type Price {
    currency: String!
    value: Int!
}

But in that case it won't be possible to filter on the Price of a Product, like "give me all Products having a Price greater than X" since we have used a @cypher directive. Or will this feature request finally be resolved with v6? To be able to filter on fields having a @cypher directive?

Yes! We are scheduled to work on this feature in the coming months to be released in the current major version of the library, so this could be the workaround (for read scenarios, anyway).

from graphql.

You are quite possibly right that we should remove it - it would actually be a removal of the entire connectOrCreate operation for any type which has one or more non-nullable and non-list relationship fields.

As to what won't be possible in version 6.0.0 (or whatever major version it ends up being when we release these broad API changes) - both of the examples you have given will not be supported, essentially any non-list relationship.

I think that would be a good solution! Don't get me wrong here - you guys really are doing a great job. The API is a great tool to work with and works like a charm (in most cases ;-) )

Yes! We are scheduled to work on this feature in the coming months to be released in the current major version of the library, so this could be the workaround (for read scenarios, anyway).

Glad to here that :-) That's really good news!

However, what about write scenarios? How should one model such an object-subobject thing? I think this is a quite common requirement for data models. Not in every case has an object a list of sub-objects. Sometimes it is good to structure data in a simple sub-object, just to group things. And what should be considered always: people do not only need to read from the API - they need to write too! So if you are saying this will be implemented for read-scenarios - how on earth should we write the Product-Price thing to the database?
The only way I can think of at the moment is something like this:

type Product {
    id: Int
    price_value: Int!
    price_currency: String!
}

But this is totally ridiculous and would blow-up the Product object with a lot of properties. Feels bad :( The semantical structure of the price-attributes is gone also.

I hope you consider these problems for your next major release. Right now I don't see any possibility for us to go with the update and migrate our code. A such fundamental breaking change at data-model level without any possible workaround or alternative... delicate! Or am I missing something? Do you see a solution?

from graphql.

Thanks for the kind words about the API!

I completely agree about write scenarios - we had a conversation this morning but we have a lot to think about. There are options such as exposing these as types in GraphQL, but storing them as JSON serialized strings (or similar) in node properties, but then querying in any way aside from the GraphQL API becomes clunky.

We very much appreciate that this could be quite the jarring migration, which is why we will maintain version 5.x as an LTS release. Hopefully we can influence some changes in the database which will make these changes easier!

from graphql.