neo4j-graphql / neo4j-graphql-js

NOTE: This project is no longer actively maintained. Please consider using the official Neo4j GraphQL Library (linked in README).
Other
609 stars 148 forks source link

Confusing Logical Default in Relationship Filters #556

Open Phylodome opened 3 years ago

Phylodome commented 3 years ago

TLDR: Default relationName filters appear indistinguishable from relationName_every filters, though this is not explained well in the documentation, and the intuitive behavior would be more like relationName_some.

Explanation:

Given a simplified graph:

(p0:Person)-[r0:ASSOCIATED_WITH {rejected: false, accepted: false]->(p1:Person)
(p0:Person)-[r1:ASSOCIATED_WITH {rejected: true, accepted: false]->(p2:Person)

A relation type ConnectionRelation:

type ConnectionRelation @relation(name: "ASSOCIATED_WITH") {
  from: Person!
  to: Person!
  status: RelationStatus!
  accepted: Boolean!
  rejected: Boolean!
  blocking: [ID!]!
}

and a Filtered Query getConnections:

query getConnections($id: ID!) {
  connections: Person(
    filter: {
      connections: {
        to: {
          Person: { id: $id }
          rejected: false
          accepted: false
        }
      }
    }
  ) {
    id
    email
    name
    avatar {
      ...AvatarFields
    }
  }
}

One would think–to the extent that my thought process is representatively similar to other library users–that such a query would return ANY Person with an outgoing relation that satisfies the property filter constraints.

After all, the documentation states that the default relation filter: "Matches nodes based on a filter of the related node"

But the library generates the query:

MATCH (`person`:`Person`)
  WHERE ((
    EXISTS((`person`)-[:ASSOCIATED_WITH]->(:Person)) 
     AND 
     ALL(`person_filter_person` IN [(`person`)-[`_person_filter_person`:ASSOCIATED_WITH]->(:Person) | `_person_filter_person`]
      WHERE (`person_filter_person`.accepted = $filter.connections.to.accepted) 
      AND (`person_filter_person`.rejected = $filter.connections.to.rejected)
      AND (
        ALL(`person` IN [(`person`)-[`person_filter_person`]->(`_person`:Person) | `_person`]
          WHERE (`person`.id = $filter.connections.to.Person.id)
        )
      )
    )
  ))

RETURN `person` {
  .id ,
  .email , 
  .name , 
  .avatar {...}
} AS `person`

Turn your attention to the use of ALL within the first WHERE clause.

Per documentation, "ALL returns true if the predicate is true for all elements in the list.". But... the list in question is a set of relationships that may possess heterogeneous property values for the filter parameters across each relationship element.

In this simple example, person_filter_person.accepted and person_filter_person.rejected generate a set of 4 possible states, any of which may reflect the state of a list element [[:ASSOCIATED_WITH], ... ]:

So, despite the fact that I am looking for ANY relation satisfying the filter criterion, I get no results if there exists any property variance amidst the matched relations.

This can, of course, be solved by using connections_some, but in terms of developer ergonomics and adoption, the choice of ALL rather than ANY as a default appears a deeply counterintuitive design decision.

Discussion:

Consider that when someone asks: "does Alice have a connection to Bob that's neither accepted nor rejected?"

They are almost certainly not asking: "Does Alice possess only connections that are neither accepted nor rejected and are Bob?"

This logic generalizes to essentially any example concerning variable properties: the default conceptual expectation is that such a filter would return any entities related via the specified relation type that match all property constraints.

So if the default is going to map to either [relation]_some or [relation]_every, the choice of "every" seems...less than ideal.

Is there a reason I'm missing that y'all chose "every / ALL" as the default approach?

Suggestion

If there's no explicit reason, I'd submit that in the next round of otherwise breaking changes, that the default logic for the default relation filter is changed to that of "some / ANY".

michaeldgraham commented 3 years ago

https://github.com/neo4j-graphql/neo4j-graphql-js/issues/608