Skip to content
This repository was 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.

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

Successfully merging a pull request may close this issue.

9 participants