Skip to content
This repository has been archived by the owner on Sep 3, 2021. It is now read-only.

Interfaces - Handle multiple fragments. #270

Merged
merged 9 commits into from
Aug 12, 2019

Conversation

Pruxis
Copy link
Contributor

@Pruxis Pruxis commented Jul 6, 2019

In this PR I have changed how the fragments are handled a bit.

Instead of relying on the node matching I would match all nodes that are related with the relation you are fetching. And pass the label as a FRAGMENT_TYPE so the default resolver can do the rest.

This will support default interface queries & multiple fragments and will still return the correct type.

However if your graph would contain multiple labels for a node we would have to pass the entire array of labels and check that instead in the default resolver, I didn't do that yet but if core contributors would like to see this "improvement" I can surely add this here.

I'm quite blocked by this PR so it would be great if there is some feedback here that we can solve this issue as fast as possible.

Looking forward to contribute more to this package as we started to rely heavily on neo4j & apollo.

@Pruxis
Copy link
Contributor Author

Pruxis commented Jul 8, 2019

As I was testing my current implementation I noticed that attempting to query subfields that have @relation / @cypher or just a type that isn't one of the default scalars they do not seem to get resolved.

I'll have to dig deeper to find out why..

@Pruxis
Copy link
Contributor Author

Pruxis commented Jul 9, 2019

Eureka! @johnymontana I've also solved an issue when you query fields of a type that do not exist on the interface that the type implements. I've done this by passing the schemaType of the type of the fragment instead of the interface schemaType.

Example:

interface Person {
id: ID
}

type Man {
id: ID
beard: Boolean
}

then beard was not queryable before this fix.

@Pruxis
Copy link
Contributor Author

Pruxis commented Jul 9, 2019

There are still several issues regarding to interfaces that have not been handled yet such as:

  • Fragment spreading in an interface
  • Fragment spreading in a type that implements an interface.

Currently just having working fragments is sufficient for my use case (so far) so I'll touch those if anyone requires them.

@johnymontana
Copy link
Contributor

Thanks @Pruxis! Would you mind adding a couple of tests showing some examples of what this fixes?

We were envisioning interfaces making use of multiple labels in the database. For example:

interface Person {
  name: String
}

type User implements Person {
  name: String
  userId: ID
}

type Employee implements Person {
  name: String
  employeeId: ID
}

would correspond in nodes in the database (:Person:User) and (:Person:Employee). In that case, we would need some logic to figure out which label is the interface and which is the implementing type for the FRAGMENT_TYPE value. Of course, we would need to add this logic to the generated Create mutations to add both labels, which we haven't don't yet, so I'm OK with not handling the case of multiple labels for now.

@codecov-io
Copy link

codecov-io commented Jul 23, 2019

Codecov Report

Merging #270 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #270   +/-   ##
=======================================
  Coverage   94.47%   94.47%           
=======================================
  Files           4        4           
  Lines         326      326           
=======================================
  Hits          308      308           
  Misses         18       18

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b76f84...fc9a414. Read the comment docs.

@johnymontana johnymontana merged commit bab8ab2 into neo4j-graphql:master Aug 12, 2019
@kraney
Copy link

kraney commented Aug 17, 2019

I'd just like to chime in that I do in fact use multiple labels and need this fix to support them. I really appreciate the work that has been done so far.

Also, not every node pointed to with the given relationship type would be candidates for the field. Some can be a completely different type that doesn't implement the interface. These are available on a different field referencing a different interface, but using the same relationship type. Does the default resolver handle this case?

E.g.

interface A {
}

type B implements A {
}

type C implements A {
}

interface X {
}

type Y implements X {
}

type Z implements X {
}

type Container {
  exes: [X!]! @relation(name: "CONTAINS", direction: "OUT")
  ays: [A!]! @relation(name: "CONTAINS", direction: "OUT")
}

...and where the nodes are labeled :A:B, :A:C, :X:Y, or :X:Z. Possibly with other labels as well.

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

Successfully merging this pull request may close these issues.

4 participants