Renaming of @ignore directive

[angrykoala]

As discussed in #842, the naming (and, potentially behaviour) of the @ignore directive is not intuitive enough. This issue exists to discuss the following:

• @ignore name (e.g. @custom, @computed ...)
• Optional argument naming (dependsOn)
• Any behaviour improvements regarding projections

[nelsonpecora]

FWIW, when I implemented a similar feature for another library, I ended up going with @computed(from: ["list", "of", "fields"]). Here's my full directive definition, if it's helpful when thinking about behavior improvements:

# Computed fields tell the query parser to fetch certain fields
# from the database when querying for data.
directive @computed(
  from: [String!] # field(s) to fetch from db instead of the api-layer field name
  terminating: Boolean # don't query the db for any further down in the tree (useful when using dataloaders to fetch external data)
) on FIELD_DEFINITION

As an example of the terminating arg, we have user data stored in a separate service, but link user ids to other records in our database. For a node like { id: "someid", createdById: 2 } the SDL looks like:

createdBy: User! @computed(from: ["createdById"], terminating: true)

This tells our query parser to fetch createdById from the database. The fetching of actual user data (everything in the User type) happens in a dataloader in the createdBy field resolver.

Someone mentioned it in the other thread, but we also included (limited) support for deeper querysets by allowing dot notation in the fields argument, e.g. @computed(from: ["label", "tags.title"]). That functionality only ended up being used in a handful of places, but it was pretty helpful when we needed it for adding human-readable name fields to interfaces.

Hope this is helpful!

EDIT: One other thing I should mention: In our API, some of the fields with @computed are supposed to be available in input types, but others are not. I think a similar thing could be achieved by combining this directive with something like @exclude(operations: [CREATE, UPDATE, DELETE]) where applicable, but I'm not sure how composable those two directives are (obviously, something like @exclude(operations: [READ]) wouldn't play nicely with this logic, but maybe that's not a problem?). Anyway, hope this is helpful as a real-world use case.

[dmoree]

@oskarhane @darrellwarde @angrykoala
The original attempt to modify the @ignore directive was to add an argument for a selection set. This comment (#842 (comment)) outlined some initial hurdles. I believe I may have solved the problems presented there by rebuilding graphql-parse-resolve-info to suit the specific needs of the directive.

I'm aware that this is a wholly different direction, but the previous attempt only allowed for querying primitive fields. I believe that with this generalization a custom resolver will now have access to the entire graph using the source node as a root.

From #842 one could define a custom resolver with:

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

The proposed way would be written as:

type User {
    id: ID!
    firstName: String!
    lastName: String!
    fullName: String @ignore(dependsOn: "{ firstName lastName }")
}

This will make it possible to have more complex requirements such as:

type User {
    id: ID!
    firstName: String!
    lastName: String!
    friends: [User!]! @relationship(type: "HAS_FRIEND", direction: OUT)
    friendsOfFriendsIds: [ID!]!
      @ignore(
        dependsOn: """
        {
          friends {
            id
            friends {
              id
            }
          }
        }
        """
      )
}

Here is a comparison:
dev...dmoree:feature/ignore-depends-on-selection-set

[dmoree]

@nelsonpecora Thanks for the suggestions. I mentioned in the other thread that to me @computed seems as if the library should be responsible for the value given how @cypher is available. Perhaps

directive @custom(
  from: String # selection set to determine what field(s) to fetch from db
) on FIELD_DEFINITION

What utility do you think that selection sets offer? I don't want to overgeneralize the directive, but I think this direction may be better. I recently pushed a commit to the above branch dev...dmoree:feature/ignore-depends-on-selection-set that allows for variables in the custom resolver such that something like this is possible (using the above definition):

interface FriendsProperties {
    since: DateTime!
}

type User {
    id: ID!
    firstName: String!
    lastName: String!
    friends: [User!]! @relationship(type: "FRIENDS_WITH", direction: OUT, properties: "FriendsProperties")
    newestFriendIds(since: DateTime!): [ID!]!
        @custom(
            from: """
            {
                friendsConnection(where: { edge: { since_GT: $since } }) {
                    totalCount
                    edges {
                        node {
                            id
                        }
                    }
                }
            }
            """
        )
}

I didn't make it explicit in the comment above, but this will take care of validation to make sure that the selection set is valid against the parent node. It also takes care of any argument mismatch by throwing an error about the need to alias. Although a good practice would be to simply circumvent that on the server by always aliasing the field in the selection set. I haven't yet found, however, a simple solution of validating the argument of the field against that of the selection set (since above).

There is a PR graphql/graphql-js#3152 that seeks to implement fragment arguments which would solve this problem. The syntax would require the variable declarations on the fragment itself. It would then make sense to change how the selection set is defined and make it explicit to the user that this is a fragment on the parent node. Meaning the above would actually need to be defined as:

interface FriendsProperties {
    since: DateTime!
}

type User {
    id: ID!
    firstName: String!
    lastName: String!
    friends: [User!]! @relationship(type: "FRIENDS_WITH", direction: OUT, properties: "FriendsProperties")
    newestFriendIds(since: DateTime!): [ID!]!
        @custom(
            from: """
            fragment User_newestFriendIds on User {
                friendsConnection(where: { edge: { since_GT: $since } }) {
                    totalCount
                    edges {
                        node {
                            id
                        }
                    }
                }
            }
            """
        )
}

Then, if that PR were to be implemented validation would be as simple as writing:

fragment User_newestFriendIds($since: DateTime!) on User 

You also get fragment spreads making possible:

extend type User {
    newestFriendIds(since: DateTime!): [ID!]!
        @custom(
            from: """
            fragment User_newestFriendIds on User {
                friendsConnection(where: { edge: { since_GT: $since } }) {
                    totalCount
                    edges {
                        node {
                            ...UserInfo
                        }
                    }
                }
            }
            fragment UserInfo on User {
                id
                firstName
                lastName
            }
            """
        )
}

This would be more useful if there was one place where UserInfo was defined and it was dropped in anywhere in the SDL it was needed using string interpolation. It can then be modified as requirements change.

I don't want to over engineer this, but it appears quite flexible.

[nelsonpecora]

Oh wow, this looks great! I agree about avoiding overengineering, and those flexible querysets seem like a great solution. I don't think my own use cases are complicated enough for fragments (they'd look like @custom(from: "{ relation { field }}") at most), but I could see that being useful for folks.

@custom also seems like a reasonable name!

[darrellwarde]

Ignoring (no pun intended) the extended use cases above, we have made a start on this by renaming this directive as per the recommendation of @nelsonpecora in #979. Comments welcomed!

[darrellwarde]

We're going to close this issue. We're satisfied that the rename of @ignore to @computed offers more clarity over its purpose, and at the moment we don't have the appetite to implemented "nested GraphQL" in any of our directives due to the complexity that this would add, especially when it comes to type definition validation.