Feature: Add Required Fields For Custom Resolver

[dmoree]

Description

Proposed solution for #204 that adds a require argument to @ignore thereby making the definition:

directive @ignore(
  require: [String!]
) on FIELD_DEFINITION

If any field that is marked with @ignore(require: ["field1", ...]) is found in the selection set of the parent node, then the fields in the require argument will be added to the fields in the resolveTree that ultimately get projected and only if those fields are not already in the selection set. This ensures that regardless if all fields are selected, they will be available on the parent object in the resolver.

Current Implementation of @ignore

Taking the example from the documentation:

const typeDefs = `
    type User {
        firstName: String!
        lastName: String!
        fullName: String! @ignore
    }
`;

const resolvers = {
    User: {
        fullName(source) {
            return `${source.firstName} ${source.lastName}`;
        },
    },
};

const neoSchema = new Neo4jGraphQL({
    typeDefs,
    resolvers,
});

It is accompanied by a note:

Due to the nature of the Cypher generation in this library, you must query any fields used in a custom resolver. For example, in the first example below calculating fullName, firstName and lastName must be included in the selection set when querying fullName. Without this being the case, firstName and lastName will be undefined in the custom resolver.

As @litewarp pointed out in the request, this is inefficient from a GraphQL perspective.

Proposed Implementation of @ignore

If the typeDefs were then defined as:

type User {
    firstName: String!
    lastName: String!
    fullName: String! @ignore(require: ["firstName", "lastName"])
}

The resolvers will then have access to the fields on the source object

const resolvers = {
    User: {
        fullName(source) {
            // source.firstName and source.lastName are always defined here
            return `${source.firstName} ${source.lastName}`;
        },
    },
};

Which means even if the query were for example:

query UserWithoutFirstAndLastName {
    users {
        fullName
    }
}
    

The field fullName would resolve correctly.

Issue

• Return Primitive Fields to GraphQL layer #204

Checklist

The following requirements should have been met (depending on the changes in the branch):

• Documentation has been updated
• TCK tests have been updated
• Integration tests have been updated
• New files have copyright header
• CLA (https://neo4j.com/developer/cla/) has been signed

[darrellwarde]

This is a really innovative solution @dmoree, excited to take a look at this during the week!

[darrellwarde]

Due to the nature of the Cypher generation in this library, you must query any fields used in a custom resolver. For example, in the first example below calculating fullName, firstName and lastName must be included in the selection set when querying fullName. Without this being the case, firstName and lastName will be undefined in the custom resolver.

Before I even jump into code review, it looks like this sentence hasn't been removed from the docs in this PR. Can you find and remove it? 🙂

[neo4j-team-graphql]

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

[mhlz]

I was working on something similar for a while, but had to stop because other things came up.

One thing I had in my implementation was to also be able to require nested fields:

type Offer {
  price: Money! // @relationship etc
  vat: Money! // @relationship etc
  
  gross: Int! @ignore(requires: [ "price.amount", "vat.amount" ])
}

type Money {
  amount: Int!
  currency: String!
}

This is just an example of course, but it would make a few use cases we have possible.

My code is here, but it's really not finished or cleaned up (and it might contain some other things I was working on).

[dmoree]

Thanks for the suggestion @mhlz. I really like the buildResolveTree that you're using which I'll come back to.

I considered something similar, but was using a selection set instead of the array in the PR. It would have looked something like:

type User {
    firstName: String!
    lastName: String!
    fullName: String!
        @ignore(
            require: """
            {
                firstName
                lastName
            }
            """
        )
}

This generalization would allow for similar use cases that you describe. Such that, if the schema is:

type User {
    id: ID!
    username: String!
    posts: [Post!]! @relationship(type: "HAS_POST", direction: OUT)
}

type Post {
    id: ID!
    title: String!
    content: String!
    author: User! @relationship(type: "HAS_POST", direction: IN)
    likes: [User!]! @relationship(type: "LIKED_POST", direction: IN)
}

one could add a field with a custom resolver that can traverse the graph:

extend type User {
    customResolver: String!
        @ignore(
            require: """
            {
                id
                posts {
                    id
                    title
                    likes {
                        id
                    }
                }
            }
            """
        )
}

I'm not sure why one would want this particular selection, but in any case the resolver:

const resolvers = {
  User: {
    customResolver: (source) => {
      // get unique user ids of those that have liked any post
      const userIds = Array.from(
        new Set(
          source.posts.map((post) => post.likes.map((like) => like.id)).flat()
        )
      )
      // do something with userIds
    },
  },
}

There were two immediate issues (probably more): validation and arguments. Both, I think, are solvable.

Validation

My attempt at validation was to use the validate function from graphql against a constructed query in the constructor of Neo4jGraphQL.

const ignoreValidationErrors = nodes
    .filter((node) => node.ignoredFields.length)
    .map((node) =>
        node.ignoredFields
            .filter((ignoredField) => ignoredField.require)
            .map((ignoredField) => validate(schema, gql`query { ${node.plural} ${ignoredField.require} }`))
            .flat()
    )
    .flat();

if (ignoreValidationErrors.length) {
    throw new Error(ignoreValidationErrors[0].message);
}

The drawback was that this is relying on a root users query that may not exist in the schema since one could always @exclude(operations: [READ]) on the User type. This would prevent the generation of users query thereby always throwing a validation error. One could exclude those fields after makeAugmentedSchema, but before makeExecutableSchema to ensure this works.

This was the approach because I couldn't find a way to validate a selection set against a particular type. It seems hacky. Ideally, it would be something like validateAgainstType('User', selectionSet) I tried searching through @graphql-tools but came up empty. It may already exist; I don't know. I'm sure at the end of the day it could be done.

If validation is accounted for, then buildResolveTree can go through the parsed selectionSet and construct the resolve tree. The library uses graphql-parse-resolve-info to get the initial resolveTree, but I don't see a way to achieve the same thing from a selection set. This should be doable, but it would have to be done to get the projection right. The only problem I see here is that what would happen if posts returned an interface? I'm not sure if fields of interfaces are kept track of like nodes are. And even then scalars would be fine, it would be an issue for related fields.

Arguments

If one were to query users with:

query ($postTitle: String!) {
  users {
    id
    username
    posts(where: { title: $postTitle }) {
      id
      title
      content
      likes {
        id
      }
    }
    customResolver
  }
}

This should throw an error because posts would have differing arguments since in require no arguments are present. A simple solution to this would be to require any non-scalar fields to be aliased within the selection set, but this also feels a little off, but it would solve the problem of merging fields in the selection set by having a completely different tree to project.

I apologize for the long post, but as you can see it ended up being a nontrivial problem. I don't want to trivialize this PR, but it is relatively straightforward and satisfies the initial request in #204. Perhaps in the future this could be revisited.

[mhlz]

Using SelectionSets is also a nice idea!

And I also agree that this doesn't have to be part of this PR, just wanted to put the idea somewhere :)

[angrykoala]

Great job. It is almost good to go IMO.
I've made a few comments regarding some areas where code complexity could be reduced, would love to get your thoughts on these before merging @dmoree

[angrykoala]

LGTM

[oskarhane]

I'm not sure I like the naming of @ignore(require: [""]).
Not sure what it is, but those words combined makes no sense to me. :)

Is it only me?

Personally I think @ignore(dependsOn: [""]) or @ignore(dependencies: [""]) reads better and is easier to understand what it means.

[dmoree]

It's not just you. I went back and forth on required requires and require. But really I have no strong opinions either way; I'm willing to change it to wherever the consensus lands.

[oskarhane]

I think the real issue for me here is @ignore. I think @computed would be a better name for that directive.

type Person {
  firstName: String!
  lastName: String!
  fullName: String! @computed(requires: ["firstName", "lastName"])
}

is much more clear imo.

Sorry for the curve ball here @angrykoala and @dmoree.
I just happened to see this in one of my feeds and figure I'd mention it before v.3 goes out (while you still can break API:s).

[dmoree]

No worries at all. Naming is important.

If the directive were to change, it should be more along the lines of @custom because require isn't required and

type Person {
  firstName: String!
  lastName: String!
  customField: String! @computed
}

seems like the library should be doing something as opposed to the user taking responsibility for the field.

(I always thought @computed should have been used for @cypher fields returning a Scalar type.)

[oskarhane]

Yeah, you're right on the @computed, it might be too narrow in this case.
@custom makes sense to me as well.

Not sure what the team thinks though.

[angrykoala]

@dmoree @oskarhane
I agree on require name change (and now it's a great time to rename stuff!).
I'm not sure about ignore to computed though, so maybe we could go with @ignore(dependsOn)? Thoughts @darrellwarde @danstarns @tbwiss ?

[darrellwarde]

This is tricky! Whilst we do have 3.0.0 coming up, we should still keep breaking changes to a minimum if possible, and renaming @ignore to @custom will result in a breaking change for everybody that uses this directive, as opposed to the non-breaking manner of this feature as it is.

I always look at our directives as "instructions" to our library - so always saw it that the @ignore directive was instructing the library to ignore the field. Personally, I would go with @ignore(dependsOn: [...]) but I can understand all of the arguments here.

[dmoree]

I always look at our directives as "instructions" to our library - so always saw it that the @ignore directive was instructing the library to ignore the field.

I think this may be slightly confusing here because it is true that @ignore is telling the library to ignore the field, but I'm not so sure one would want to totally ignore the field. The library should ignore the field for the purposes of schema generation, but not ignore the field when it comes to projection. This, I think, is the basis for issues like #204 and must come with a note in the documentation due to the ignoring of the field in projections.

I like the concept of directives as "instructions", I'm not so sure if @ignore is it. Personally, I think a different directive should be used that captures the distinction in generating the schema and generating the projection.

[angrykoala]

Hi @dmoree , @oskarhane @darrellwarde
The conversation has creeped completely out of scope for this PR. We should keep this PR only for the newly added feature. I opened issue #937 so we can move any discussion regarding improvements to the @ignore directive there, as it doesn't make sense to block this PR for this kind of discussion.

As of now, it seems that dependsOn is the preferred name for this new argument, so if there are no objections to that, I'm happy to merge this PR once that rename is made.

[dmoree]

@angrykoala @darrellwarde @oskarhane
Refactored require to dependsOn in 2b4091a