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

Relationship properties #192

Closed
danstarns opened this issue May 6, 2021 · 26 comments · Fixed by #193 or #406
Closed

Relationship properties #192

danstarns opened this issue May 6, 2021 · 26 comments · Fixed by #193 or #406
Labels
enhancement New feature or request

Comments

@danstarns
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Ability to add properties on relationships

Additional context
Came from here #179

@danstarns danstarns added inbox enhancement New feature or request labels May 6, 2021
@danstarns danstarns changed the title Relationship property Relationship properties May 6, 2021
@darrellwarde darrellwarde linked a pull request May 6, 2021 that will close this issue
2 tasks
@colinskow
Copy link

Great feature! I believe this is the biggest block to being able to migrate from neo4j-graphql-js.

For reference here is the previous documentation on relationship properties.

The RFC is more complex than the previous implementation, but the extra power seems worth it!

@litewarp
Copy link
Contributor

litewarp commented May 6, 2021

Great proposal. Any chance you could (a) add a total count field to the automatically generated connection properties, or (b) allow users to augment the connection properties?

@darrellwarde
Copy link
Contributor

Great feature! I believe this is the biggest block to being able to migrate from neo4j-graphql-js.

For reference here is the previous documentation on relationship properties.

The RFC is more complex than the previous implementation, but the extra power seems worth it!

We really do recognise it is likely the biggest blocker to migration, hence why we're jumping onto this so quickly. 🙂

More complex, and sadly a couple of unavoidable breaking changes, but this will also form the foundation for the much asked for cursor-based pagination in future, which we're very excited for!

@darrellwarde
Copy link
Contributor

Great proposal. Any chance you could (a) add a total count field to the automatically generated connection properties, or (b) allow users to augment the connection properties?

Thanks for the kind words and suggestions! 🙂

The returned connection will be based on the filters provided, so it might be a confusing behaviour to add a total count field - is that the total count of all edges, or just the edges in this particular connection? If the latter, you can just get the length of the edges array.

If the former, we have another piece of work on our roadmap which is to introduce aggregation queries which will allow you to query aggregations such as count. We want to get relationship properties in first, but that is hopefully going to follow not too far behind.

Regarding customising the connection types... Maaaaaaybe, but that would be well down the line, as we want to build a stable platform for relationship properties first.

@danstarns
Copy link
Contributor Author

danstarns commented May 7, 2021

Overall this is great! Introducing relationship properties will help unblock a lot of users and better position us for things such as Relay. I do have a few concerns that I will share below.

Breaking changes that will bump the library up to V 2.0. Depending on when we implement this, that seems imminent, we could aggravate a lot of existing users.

Using interfaces to define the relationship properties. This will be subjective and it's a view on semantics. I find it hard to enjoy using the interface ActedIn here:

type Movie {
    title: String!
    actors: [Actor!]!
        @relationship(type: "ACTED_IN", properties: "ActedIn", direction: IN)
}

type Actor {
    name: String!
    movies: [Movie!]!
        @relationship(type: "ACTED_IN", properties: "ActedIn", direction: OUT)
}

interface ActedIn {
    screenTime: Int!
}

Though I realise that this would be another breaking change... I would be keen to hear thoughts on using a type plus introducing @node and @relationship directives:

type Movie @node {
    title: String!
    actors: [Actor!]!
        @relationship(type: "ACTED_IN", properties: "ActedIn", direction: IN)
}

type Actor @node {
    name: String!
    movies: [Movie!]!
        @relationship(type: "ACTED_IN", properties: "ActedIn", direction: OUT)
}

type ActedIn @relationship {
    screenTime: Int!
}

Maintaining two projection paths. Given the movies schema mentioned twice in my post, from a Movie, you will be able to query:

For just the actors:

query {
    movies(where: { title: "Forrest Gump" }) {
        title
        actors(where: { name_STARTS_WITH: "Tom" }) { # just the actors
            node
        }
    }
}

For the actor's connection:

query {
    movies(where: { title: "Forrest Gump" }) {
        title
        actorsConnection( # actor's connection
            where: { node: { name_STARTS_WITH: "Tom" } }
            sort: { node: { name: ASC } }
        ) {
            edges {
                screenTime
                node {
                    name
                }
            }
        }
    }
}

We would have to maintain both projections thus could lead to technical debt later down the line. We are breaking all the mutations but not queries.

@darrellwarde
Copy link
Contributor

Though I realise that this would be another breaking change... I would be keen to hear thoughts on using a type plus introducing @node and @relationship directives:

type Movie @node {
    title: String!
    actors: [Actor!]!
        @relationship(type: "ACTED_IN", properties: "ActedIn", direction: IN)
}

type Actor @node {
    name: String!
    movies: [Movie!]!
        @relationship(type: "ACTED_IN", properties: "ActedIn", direction: OUT)
}

type ActedIn @relationship {
    screenTime: Int!
}

As you know, my main gripe with this is that the user defined ActedIn type would magically "disappear" on schema generation in this case, and I'm a strong believer that all user inputted type definitions should be present in the output schema.

From the GraphQL documentation, "An Interface is an abstract type that includes a certain set of fields that a type must include to implement the interface". In my opinion, this is a perfect fit - users are instructing our library, "the generated relationship types must include the fields I have defined here", and we additionally insert the node field which has a different type depending on the direction, and any other fields we need to add in future.

Additionally, the @relationship directive wouldn't work because the type and direction arguments are mandatory on it as per the directive definition. So we would either need to remove these constraints which makes it less robust when used on a field, or create another directive such as @relationshipProperties.

@danstarns
Copy link
Contributor Author

@darrellwarde Thanks for this response it all seems reasonable, my comments are just thoughts out loud and I have no doubt in the reasoning behind particular decisions, although, In general, I would be intrigued to know if anyone shares similar views around the points I outlined.

@darrellwarde
Copy link
Contributor

Also, regarding something like a @node and @relationshipProperties directive - I believe users will have more types/interfaces that they want included in the database than they will want excluded from the database.

With that in mind, I think a less cumbersome approach would be to take the opposite line and decorate types that you don't want in the database with a directive, likely the existing @ignore. This should mean less boilerplate, lower barrier to entry, and faster schema-to-server development time.

This goes slightly off the topic of relationship properties, but as we grow the capabilities of the library it becomes increasingly likely that there will be "collisions", so it would be good to gather thoughts.

@colinskow
Copy link

@darrellwarde breaking changes are inevitable. Just plan for all the breaking changes you need to implement the features you want and rip off the band-aid as quickly as possible. What is most important to me is consistency throughout the library in the final API. (For example filtering and pagination work the same everywhere.)

I'm glad that Neo4j is finally taking GraphQL seriously. A few weeks ago I was afraid that I was going to have to re-write neo4j-graphql-js myself! Now about 90% of everything I need is already in place.

@MarianAlexandruAlecu
Copy link

MarianAlexandruAlecu commented May 25, 2021

When will #222 be available on NPM?

@darrellwarde
Copy link
Contributor

When will #222 be available on NPM?

As you can see, that was merged into branch 2.0.0, which is an active development branch for our 2.0.0 release containing relationship progress.

We're still working on relationship properties, merging into branch 2.0.0 as we go, and when we finally merge that into master, that is when we will release.

@MarianAlexandruAlecu
Copy link

@darrellwarde do you have a rough estimation for when you will merge 2.0.0 to master?

@darrellwarde
Copy link
Contributor

@darrellwarde do you have a rough estimation for when you will merge 2.0.0 to master?

Not right now, please see my previous message for a brief status update on this work. Thanks for the interest!

@darrellwarde
Copy link
Contributor

Hey everyone! You may or may not have watched my talk at NODES 2021, "Neo4j and GraphQL: The Past, The Present and The Future", but I made an exciting announcement!

Version 2.0.0-alpha.1 of The Neo4j GraphQL Library is now available for you to try out, with support for relationship properties!

You can install it using:

npm install @neo4j/graphql@next

# OGM
npm install @neo4j/graphql-ogm@next

You can find a migration guide to upgrade to the new version at https://neo4j.com/docs/graphql-manual/2.0/guides/rel-migration/, with most of the changes needed being on the client-side.

Please, try it out, let us know how it's working for you by raising any issues for bugs or feature requests! Please note that this is an alpha release, so some things will be broken and it may not be feature complete quite yet.

@jbhurruth
Copy link

The 2.0.0 pre-release is looking great but I'm having an awkward time with certain aspects of relationship properties.

Firstly I've noticed that the resultant relationshipNameWhere types fail to include Boolean type relationship properties. I assume this is a bug since Float properties are included. For example:

interface synonymOf {
  relevance: Float!
  verified: Boolean
}

Generates:

input synonymOfWhere {
  OR: [synonymOfWhere!]
  AND: [synonymOfWhere!]
  relevance: Float
  relevance_NOT: Float
  relevance_IN: [Float]
  relevance_NOT_IN: [Float]
  relevance_LT: Float
  relevance_LTE: Float
  relevance_GT: Float
  relevance_GTE: Float
}

Notice that verified is missing.

Also, I'm interested in being able to sort/filter on a relationship property without having to get data back according to the propertyConnection structure. For example, currently I would have to do something like this:

query {
  words {
    synonymsConnection(options: { sort: { relationship: { relevance: DESC } } }) {
      edges {
        node {
          word
        }
      }
    }
  }
}

Ideally we'd be able to do this for a simpler result structure:

query {
  words {
    synonyms(options: { sort: { relationship: { relevance: DESC } } }) {
      word
    }
  }
}

Is there any chance that this could happen? We'd like to do a lot of filtering and sorting on both relationship and node properties simultaneously but right now it means defaulting to the unwieldy Connection:edges:node form and dealing with two extra layers of JSON nesting despite not actually wanting to return the relationship properties.

@darrellwarde
Copy link
Contributor

Hey @jbhurruth, thanks for the feedback!

Firstly I've noticed that the resultant relationshipNameWhere types fail to include Boolean type relationship properties. I assume this is a bug since Float properties are included. For example:

interface synonymOf {
  relevance: Float!
  verified: Boolean
}

Generates:

input synonymOfWhere {
  OR: [synonymOfWhere!]
  AND: [synonymOfWhere!]
  relevance: Float
  relevance_NOT: Float
  relevance_IN: [Float]
  relevance_NOT_IN: [Float]
  relevance_LT: Float
  relevance_LTE: Float
  relevance_GT: Float
  relevance_GTE: Float
}

Notice that verified is missing.

Looks like a bug that's slipped through there, will take a look at that today!

Also, I'm interested in being able to sort/filter on a relationship property without having to get data back according to the propertyConnection structure. For example, currently I would have to do something like this:

query {
  words {
    synonymsConnection(options: { sort: { relationship: { relevance: DESC } } }) {
      edges {
        node {
          word
        }
      }
    }
  }
}

Ideally we'd be able to do this for a simpler result structure:

query {
  words {
    synonyms(options: { sort: { relationship: { relevance: DESC } } }) {
      word
    }
  }
}

Is there any chance that this could happen? We'd like to do a lot of filtering and sorting on both relationship and node properties simultaneously but right now it means defaulting to the unwieldy Connection:edges:node form and dealing with two extra layers of JSON nesting despite not actually wanting to return the relationship properties.

This, however, was a bit more of a conscious decision to try and break the API as little as possible for 1.x users migrating to 2.0.0. We'll give this another think though as I can absolutely feel your frustration!

@darrellwarde
Copy link
Contributor

Just tested for Boolean filters on the current 2.0.0 branch and they are present - was obviously just a missing component in the last release!

@jbhurruth
Copy link

@darrellwarde Did you by any chance confirm that it wasn't working in the current alpha.2 release? Or could the issue be on my end?

@darrellwarde
Copy link
Contributor

@darrellwarde Did you by any chance confirm that it wasn't working in the current alpha.2 release? Or could the issue be on my end?

I didn't no, just in my local 2.0.0 branch. However, we just released alpha.3 from that branch just a few hours ago, so give it an upgrade and see if it's fixed for you? Might be a few breaking changes mind - but we're pretty happy with the API at this stage and don't envision it changing much more in the lead up to a stable 2.0.0 release.

@Mohammer5
Copy link

@darrellwarde thank you very much for implementing this! I've been waiting for this 🎉
Currently there's no way to add a reference to a different node like this:

type Item {
  follows: [Item!] @relationship(type: "FOLLOWS", direction: OUT, properties: "FollowsRelation")
  followedBy: [Item!] @relationship(type: "FOLLOWS", direction: IN, properties: "FollowsRelation")

  # can be placed in multiple rows with different user-defined ordering
  rows: [Row!] @relationship(type: "INSIDE", direction: OUT)
}

type Row {
  items: [Item!]! @relationship(type: "INSIDE", direction: IN)
}

interface FollowsRelation {
  row: Row!
}

I have to use ID instead of Row.

@darrellwarde
Copy link
Contributor

@darrellwarde thank you very much for implementing this! I've been waiting for this 🎉
Currently there's no way to add a reference to a different node like this:

type Item {
  follows: [Item!] @relationship(type: "FOLLOWS", direction: OUT, properties: "FollowsRelation")
  followedBy: [Item!] @relationship(type: "FOLLOWS", direction: IN, properties: "FollowsRelation")

  # can be placed in multiple rows with different user-defined ordering
  rows: [Row!] @relationship(type: "INSIDE", direction: OUT)
}

type Row {
  items: [Item!]! @relationship(type: "INSIDE", direction: IN)
}

interface FollowsRelation {
  row: Row!
}

I have to use ID instead of Row.

Hey @Mohammer5 - that's very much by design, because relationships in Neo4j only support primitive properties. To be able to reference other object types from a relationship would be far too much opinionation at the GraphQL level.

I'm trying to understand your use case - the way you're trying to model it suggests that it would be more sensible to have Item->Row->Item as opposed to Item->Item as you have now.

@Mohammer5
Copy link

@darrellwarde thanks for replying so quickly! This is the first project I'm working on with this kind of behavior, so I'm not sure what the correct implementation would be:

The project, in terms of display, is quite similar to a kanban board where a card can be put into multiple columns. The user can re-order the cards per column. That means I have to store the order for each column somehow, so cards can't just have a "previous card" property as it's an many-to-many relationship and prevents me from storing the ordering on edges without also storing a reference to the column.

Do you have some resources, search tips or could you explain how Item->Row->Item would work in this case?

@darrellwarde
Copy link
Contributor

darrellwarde commented Aug 9, 2021

@darrellwarde thanks for replying so quickly! This is the first project I'm working on with this kind of behavior, so I'm not sure what the correct implementation would be:

The project, in terms of display, is quite similar to a kanban board where a card can be put into multiple columns. The user can re-order the cards per column. That means I have to store the order for each column somehow, so cards can't just have a "previous card" property as it's an many-to-many relationship and prevents me from storing the ordering on edges without also storing a reference to the column.

Do you have some resources, search tips or could you explain how Item->Row->Item would work in this case?

No worries @Mohammer5!

Now I've heard this use case, I don't think I would do what I suggested above. If I was to take your example of cards and columns very literally, personally I would do something like below, where I'm storing the position of the card as a property within the relationship. This obviously has some difficulties, for example you would have to aggregate all of the position properties from each relationship to get the full running order - but you need to get the cards themselves anyway so not really a huge hassle. :)

Example

There are many ways to skin a cat with graph modelling. There are guides spattered around, such as https://neo4j.com/developer/data-modeling/. :)

@Mohammer5
Copy link

@darrellwarde I'll give it a try, much appreciated! I'll try to familiarize myself a bit more with the neo4j dev guides, thanks for that!

@Mohammer5
Copy link

Mohammer5 commented Aug 9, 2021

@darrellwarde hey, I've implemented this slighty different as I've noticed one thing:

When storing information on relationships, I can't sort by position. I don't know if that's a technical limitation or event an anti-pattern? I assume I could implement that in some way through the @cypher directive, but I prefer to rely on the library's functionality as much as possible in order to benefit from all the auto-generated magic it provides. (I instead added a ColumnPosition node ((Card)<-[:HOLDS_CARD]-(ColumnPosition { index: $input.position })-[:IN_COLUMN]->(Column))

It'd be really nice to be able to sort a list of nodes by the property of a relationship, but, as I said, I don't know if that's possible, wanted or maybe even an anti-pattern.


OT: Sorry for bugging you with this.. Hope you don't mind. What software are you using to create the node/relationship illustration?

@darrellwarde
Copy link
Contributor

darrellwarde commented Aug 9, 2021

You should be able to @Mohammer5! Continuing with the example above, the GraphQL would look like:

query {
  columns {
    cardsConnection(sort: {edge: { position: ASC }}) {
      ...cardFields
    }
  }
}

With your ColumnPosition node, I imagine you could use something like:

query {
  columns {
    columnPositions(options: { sort: { position: ASC }}) {
      cards {
        ...cardFields
      }
    }
  }
}

Personally, I think card position as a property on the relationship is a lot cleaner.

The software for the diagram is https://arrows.app/, it's awesome! 🙂

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

Successfully merging a pull request may close this issue.

7 participants