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

Support for InlineFragments #103

Closed
Duske opened this issue Sep 11, 2018 · 21 comments · Fixed by #377
Closed

Support for InlineFragments #103

Duske opened this issue Sep 11, 2018 · 21 comments · Fixed by #377
Labels
Interface Issues relating to Interface types and InlineFragments

Comments

@Duske
Copy link
Contributor

Duske commented Sep 11, 2018

It would be awesome if we support inlineFragments in order to use interface appropriately.
I've been tinkering on a basic prototype in this fork and got inlineFragments for non-custom queries working.

The approach is not very clean, but maybe it can be a first step for InlineFragment support :)

What do you think?

@kipz
Copy link
Contributor

kipz commented Sep 20, 2018

I'm very keen to have this too - it's pretty crippling not being able to use interfaces.

@Duske
Copy link
Contributor Author

Duske commented Sep 20, 2018

@kipz You could try out the changes I've done in my fork, it works for non-custom queries.
Maybe you have the time to make it more than a prototype, but I guess @johnymontana insight is crucial for this kind of feature

@kipz
Copy link
Contributor

kipz commented Sep 21, 2018

Thanks @Duske, but I'm having a little difficulty rebasing your fork with current upstream/master as there have been changes to the same areas.

In no small part due to master being broken right now!

@Duske
Copy link
Contributor Author

Duske commented Sep 22, 2018

Ah, now I see the current code. Before the support of relation types the code was already hard to understand, but now the else part is huge. I don't know how to get the inlinefragments to work with this mess 😕

@johnymontana
Copy link
Contributor

I've taken a stab at this today. So far I'm able to generate the correct Cypher queries to resolve the data, however it seems graphql-js expects resolvers for interface types to return an object that it can call resolveType on:

Abstract type Attribute must resolve to an Object type at runtime for field Movie.genres with value \"[object Object]\", received \"undefined\". Either the Attribute type should provide a \"resolveType\" function or each possible types should provide an \"isTypeOf\" function.

with

type Genre implements Attribute {
  name: String
  movies: [Movie] @relation(name: "IN_GENRE", direction: "IN")
}

interface Attribute {
  name: String
}

@Duske did you encounter this in your implementation?

@Duske
Copy link
Contributor Author

Duske commented Sep 25, 2018

@johnymontana Thank you very much for looking into this issue :)

Yeah I encountered this problem as well and wrote a resolveType function.
But first, I added the type of the Fragment/specific implementation to the cypher result like here
So for example, querying for Genre returns Genre nodes with an additional FRAGMENT_TYPE: "Genre" property.

Then you can write a resolveType function like this:

const resolverMap = {
  Genre: {
    __resolveType(obj, context, info){
      return obj["FRAGMENT_TYPE"];  // which is "Genre"
    },
  //next implementation of Attribute
  },
};

Note that the FRAGMENT_TYPE property will be removed in the GraphQL response, so this side is clean.

@johnymontana
Copy link
Contributor

@Duske ah, cool. I'll give that a try. Thanks!

johnymontana added a commit that referenced this issue Sep 26, 2018
@johnymontana
Copy link
Contributor

I've added initial support for InlineFragments based on @Duske's suggestions above. Thanks @Duske! This is included in the recent v1.0.1 release on NPM.

A few notes:

  • Due to the required implementation of __resolveType resolvers, if you don't use the augmentSchema or makeAugmentedSchema functions exported by neo4j-graphql-js to create/augment your GraphQL schema, you'll need to implement a __resolveType for each InterfaceType implementation which returns obj['FRAGMENT_TYPE'] as @Duske describes above
  • This is an incomplete implementation. I haven't considered all cases, but wanted to get this initial approach out ASAP. Please test and report what breaks :-)

@Duske
Copy link
Contributor Author

Duske commented Oct 1, 2018

Thank you for working on this :)

I've tested the release in our project and noticed two bugs, which are mentioned and possibly solved in #114 and #115.

A feature which seems to be missing for now is to access the related subnodes of an inlinefragment.

Example schema

interface Resource {
    name: String!
    type: String!
}

type CSVResource implements Resource {
    uid: ID!
    name: String!
    type: String!
    columns: [Column] @relation(name: "HAS_COLUMN", direction: "OUT")
}

type Column {
    name: String!
    type: String
}

Example GraphQL- and Cypher-Query

query {
  MyQuery{ resources {
    name,
    ...on CSVResource { name, type, columns {name, type} }
  }}
}
... (resourcePool_resources:CSVResource) | resourcePool_resources {FRAGMENT_TYPE: "CSVResource", .name , .name , .type }]

As you can see, the columns property and subquery is missing in the cyper statement

@johnymontana
Copy link
Contributor

Thanks @Duske, I've merged PRs #114 and #115 and made a release. Those are included in v.1.0.2

@Duske
Copy link
Contributor Author

Duske commented Oct 3, 2018

Nice, thank you 👍

I've noticed another aspect we need to consider when implementing this feature. Let's suppose we have the following query added to the schema.

type Query {
  ...
  AllPersons: [Person]
}

So basically a query, which directly returns nodes implementing the interface Person, no subquery needed.

A GraphQL query could look like this:

query {AllPersons {
  name,
    ...on User {
        rated {
        timestamp
      }
    }
    }
}

The problem is here, that the cypher query will first match all nodes with label Person, which is already a problem as no node will match because they have not that label:

MATCH (person:Person {}) RETURN person { .name } AS person SKIP $offset

Instead, for each inlinefragment with type there should be a MATCH clause like MATCH (varname1:FragmentType1 {}) RETURN varname1 ..., MATCH (varname2:FragmentType2 {}) RETURN varname2 ... and so on. Then the results should be concatenated.

What do you think? Is this reasonable and/or feasible?

@obarbier
Copy link

@Duske This look like it Require some kind of Union support. https://graphql.org/learn/schema/#union-types Is this supported now?

@francescovenica
Copy link

francescovenica commented Sep 10, 2019

@obarbier did you find a solution for the union?

@jonasdumas
Copy link

+1 Union needed. Is it supported now?
Thanks for this amazing library

@JSv4
Copy link

JSv4 commented Oct 31, 2019

Hey all, is there any documentation on the neo4j-graphql-js Interface support to the extents it's implemented? Interfaces are still marked as unsupported in the Grandstack docs.

@flazoon
Copy link
Contributor

flazoon commented Nov 7, 2019

@Duske the issue in your latest comment might be fixed by #336

@Duske
Copy link
Contributor Author

Duske commented Nov 7, 2019

@flazoon This looks really good, unfortunately I cannot test it the following days. Feel free to close this issue if fixed, awesome work!

@benjamin-rood
Copy link

benjamin-rood commented Dec 4, 2019

@flazoon From my testing what @Duske wants to do is still broken, unless you know of the specific version of the library I should be testing against?

@flazoon
Copy link
Contributor

flazoon commented Dec 4, 2019

@benjamin-rood I guess I was referring to @Duske's comment from Oct 3, 2018. I guess that query should work with 2.9.3+.

@benjamin-rood
Copy link

@flazoon Unfortunately still broken for unions, but it shouldn't make a difference: in both cases there needs to be generated a resolver function for either the interface or union type __resolveType or a __isTypeOf function for each type implementing the interface or the types in the union.

Without this, how can inline fragments be working?

@johnymontana
Copy link
Contributor

Much more robust support for intefaces and unions was added in the most recent release v2.13.0, including generation of __resolveType resolvers based on multiple node labels in Neo4j. Please give that version a try and see the docs here for more info: https://grandstack.io/docs/graphql-interface-union-types.html

Closing this issue as it originally pertained to use of inline fragments, please open new issue if you're seeing specific problems related to interface and union types.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Interface Issues relating to Interface types and InlineFragments
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants