Something with interfaces generates invalid cypher

[megatrond]

Describe the bug I'm not sure what actually triggers this error, but it triggers given the following schema

Type definitions

type SomeNode {
    id: ID! @id
    other: OtherNode! @relationship(type: "HAS_OTHER_NODES", direction: OUT)
}

type OtherNode {
    id: ID! @id
    interfaceField: MyInterface! @relationship(type: "HAS_INTERFACE_NODES", direction: OUT)
}

interface MyInterface {
    id: ID! @id
}

type MyImplementation implements MyInterface {
    id: ID! @id
}

To Reproduce Run the following query:

query {
  someNodes {
    id
    other {
      interfaceField {
        id
      }
    }
  }
}

and you should get an error, similar to #1535

Variable `interfaceField` not defined (line 2, column 117 (offset: 138))\n\"RETURN this { .id, other: head([ (this)-[:HAS_OTHER_NODES]->(this_other:OtherNode)   | this_other { interfaceField: interfaceField } ]) } as this\"\n                                                                                                                     ^

System (please complete the following information):

• OS: macOS
• Version: @neo4j/[email protected]
• Node.js version: [e.g. 14.16.0]14

[neo4j-team-graphql]

Many thanks for raising this bug report @megatrond. :bug: 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! :pray:

[megatrond]

Oddly enough, this error goes away if we change the schema to use an interface instead of a regular type for the Other type

type SomeNode {
    id: ID! @id
    other: OtherInterface! @relationship(type: "HAS_OTHER_NODES", direction: OUT)
}

interface OtherInterface {
    id: ID! @id
    interfaceField: MyInterface! @relationship(type: "HAS_INTERFACE_NODES", direction: OUT)
}

type OtherNode implements OtherInterface {
    id: ID! @id
    interfaceField: MyInterface! @relationship(type: "HAS_INTERFACE_NODES", direction: OUT)
}

interface MyInterface {
    id: ID! @id
}

type MyImplementation implements MyInterface {
    id: ID! @id
}

[tbwiss]

@megatrond thanks for raising this separate ticket!

Some notes when debugging this. With the first provided type definitions the library generates the following cypher:

MATCH (this:SomeNode)
RETURN this { .id, other: head([ (this)-[:HAS_OTHER_NODES]->(this_other:OtherNode)   | this_other { interfaceField: interfaceField } ]) } as this

It's clear that interfaceField is not defined.

With the second provided type definitions the library generates the following cypher:

MATCH (this:SomeNode)
WITH this
CALL {
WITH this
MATCH (this)-[:HAS_OTHER_NODES]->(this_OtherNode:OtherNode)
WITH this_OtherNode
CALL {
WITH this_OtherNode
MATCH (this_OtherNode)-[:HAS_INTERFACE_NODES]->(this_OtherNode_MyImplementation:MyImplementation)
RETURN { __resolveType: "MyImplementation", id: this_OtherNode_MyImplementation.id } AS interfaceField
}
RETURN { __resolveType: "OtherNode", interfaceField: interfaceField } AS other
}
RETURN this { .id, other: other } as this

Note the CALL subquery which returns the interfaceField

[neo4j-team-graphql]

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

[neo4j-team-graphql]

This bug report has been assigned high priority to fix. If you wish to contribute a fix, please branch from master and submit your PR with the base set to master. Thanks!

[tbwiss]

Hi @megatrond! Unfortunately, I have to put this bug fix aside for now. This issue and some other issues tied to interfaces need to be fixed in a holistic manner, including some architectural changes, and can therefore not be addressed as a bug fix. For this specific case here we would need to restructure the cypher query to use CALL subquery(ies), just like the cypher in the second provided type definition. This requires us to do some larger refactoring to get it working properly. I'm very sorry for any inconveniences this may cause.

[megatrond]

We worked around this by adding an interface with only one implementation to fix this for now, and we're looking at options to remove the interfaces from this particular place, so it doesn't affect us that much actually! Hopefully it's still valuable to you to have this bug in the open though! Thanks!

Mvh

Trond Sørås Developer 99500932

On Thu, Jun 16 2022 at 3:19 PM, Thomas Wiss @.***> wrote:

Hi @megatrond https://github.com/megatrond! Unfortunately, I have to put this bug fix aside for now. This issue and some other issues tied to interfaces need to be fixed in a holistic manner, including some architectural changes, and can therefore not be addressed as a bug fix. For this specific case here we would need to restructure the cypher query to use CALL subquery(ies), just like the cypher in the second provided type definition. This requires us to do some larger refactoring to get it working properly. I'm very sorry for any inconveniences this may cause.

— Reply to this email directly, view it on GitHub https://github.com/neo4j/graphql/issues/1536#issuecomment-1157653496, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALJ3NEYGFZCGYB2Y2AA3CTVPMSWPANCNFSM5YCS5E3A . You are receiving this because you were mentioned.Message ID: @.***>

[tbwiss]

Glad to hear that you found a workaround! It for sure helps to have this bug report to have knowledge about some actual use cases. Thank you!

[chrisdostert]

@tbwiss just mentioning, we have a similar issue and couldn't find a work around aside from using a graphql union type instead of an interface, which really limits the functionality. This seems to still be tagged as high priority, any idea on timeline for a fix? Your efforts are much appreciated.

[tbwiss]

@chrisdostert, sorry to hear that! That's on me that the bug still has a high-priority label, I simply forgot to remove it. Given your input and some changes in the library in the meantime, I'll also make sure that we in the team discuss and re-evaluate this bug report again, likely next week.

[chrisdostert]

@tbwiss / @darrellwarde thank you for your help triaging this. We're really struggling due to this bug; we have alot of interfaces in our model that we fundamentally can't query until this is fixed. Any assistance in getting a fix prioritized is greatly appreciated.

[litewarp]

So I could be way off base here, but I think this fix might be easier than once thought. Or at least it should be theoretically.

If I understand the spec correctly, the only difference between an interface and union is where you can query the types - in an interface, you can group them together; in a union, you have to list them individually. In other words, this is really a concern for how the client queries the graphql server, but it doesn't necessarily affect the query to the database.

To take the star wars example the difference between querying an interface and a union is about where you place the inline fragment.

# Example 1 - With interfaces

interface Character {
  id: ID!
  name: String!
}

type Human implements Character {
  id: ID!
  name: String!
  hasForce: Boolean!
}

type Droid implements Character {
  id: ID!
  name: String!
  hasRestrainingBolt: Boolean!
}

query {
  character {
    id
    name
    ... on Human {
       hasForce
    }
    ... on Droid {
      hasRestrainingBolt
    }
  }
}

----

# Example 2 - With unions

union Character = Human | Droid

type Human {
  id: ID!
  name: String!
  hasForce: Boolean!
}

type Droid {
  id: ID!
  name: String!
  hasRestrainingBolt: Boolean!
}

query {
  character {
    ... on Human {
      id
      name
      hasForce
    }
    ... on Droid {
      id
      name
      hasRestrainingBolt
    }
  }
}

In both cases, the database has to search for Droids that have id, name, and hasRestrainingBolt, and for Humans, id, name, and hasForce.

The way that create-projection-and-params currently handles unions is that it finds the nodes that comprise the union, and then filters out the ones that either don't have any fields projected or are filtered by the where clause and then constructs parallel subqueries for them joined by the UNION statement.

See, e.g.

            "MATCH (this:\`Movie\`)
            WHERE this.title = $param0
            CALL {
                WITH this
                CALL {
                    WITH this
                    MATCH (this)-[thisthis0:SEARCH]->(this_search:\`Genre\`)
                    WHERE (this_search.name = $thisparam0 AND apoc.util.validatePredicate(NOT ((this_search.name IS NOT NULL AND this_search.name = $thisparam1)), \\"@neo4j/graphql/FORBIDDEN\\", [0]))
                    WITH this_search  { __resolveType: \\"Genre\\",  .name } AS this_search
                    RETURN this_search AS this_search
                    UNION # <--------
                    WITH this
                    MATCH (this)-[thisthis1:SEARCH]->(this_search:\`Movie\`)
                    WHERE this_search.title = $thisparam2
                    WITH this_search  { __resolveType: \\"Movie\\",  .title } AS this_search
                    RETURN this_search AS this_search
                }
                WITH this_search
                SKIP 1
                LIMIT 10
                RETURN collect(this_search) AS this_search
            }
            RETURN this { search: this_search } as this"

However, since both abstract classes expect the same data returned, you could basically use the cypher built for unions to run the interface query (echoing a comment made waaaaay back in the day).

Most of the heavy lifting can be pulled from the relationField.union implementation linked above. You'd just have to change the logic to find all the Nodes that implement the Interface before passing it to the same subquery builder as used for unions. Then the resolver should accept the same shape of the returned data, but process it as an interface before returning it to the client.

If I had the time, I'd give it a try. But hope this helps someone else get started.

[angrykoala]

After some recent changes in the cypher projections, fixing this should indeed be easier. As @litewarp mentions, the logic is similar to unions, and in fact to normal relationships now that those are subqueries (#1971 and #1918)

I'll be making a PR soon that should fix this issue, thanks for all the info and sorry for the long time this has been on triage

[neo4j-team-graphql]

This bug report has been assigned high priority to fix. If you wish to contribute a fix, please branch from master and submit your PR with the base set to master. Thanks!

[chrisdostert]

@angrykoala @tbwiss Amazing work all and thank you for closing this. Is it possible to get a release with this fix? We're really eager for it, thanks again!

[litewarp]

@chrisdostert I believe that the team reported it might be a few weeks until the next release. In the meantime, I have a forked package at @litewarp/graphql and @litewarp/graphql-ogm that include all the commits to the dev branch through September 1 which should include the fixes for issues #2022 and #1536. I'll be deleting it when the revision comes out, but this was blocking me as well, so I published the fork.

[angrykoala]

@chrisdostert We are planning on a release this week, so hopefully you'll be unblocked pretty soon.

Thanks for the fork, @litewarp :clap: