graphql-nexus / nexus

Code-First, Type-Safe, GraphQL Schema Construction
https://nexusjs.org
MIT License
3.4k stars 275 forks source link

required extendConnection field resolvers for connectionPlugin connection when providing field resolve function #399

Open Luka4ever opened 4 years ago

Luka4ever commented 4 years ago

When using the connectionPlugin and adding an additional field via extendConnection, according to the typings you have to provide a resolve function for the additional fields, which is fine if you're using the nodes resolver. But when using the resolve function instead of the nodes function for the connection, the return type for the resolve function includes the additional field, but according to the typings you still have to provide a resolver for the additional field, which seems like a bug.

connectionPlugin({ extendConnection: { aggregate: { type: 'ConnectionAggregate' } } })

export const connectionAggregate = objectType({
  name: 'ConnectionAggregate',
  description: 'Aggregate counts for the connection',
  definition: (t) => {
    t.int('count', { resolve: (aggregate) => aggregate.count });
  },
  rootTyping: { name: 'ConnectionAggregate', path: __filename },
});

export type ConnectionAggregate = {
  count: number;
};

t.connectionField('usersConnection', {
  type: 'User',
  resolve: () => ({
    aggregate: { count: 0 }, // resolved additional field
    pageInfo: { hasNextPage: false, hasPreviousPage: false, endCursor: null, startCursor: null },
    edges: [],
  }),
  aggregate: () => ({ count: 0 }), // must be specified according to typeings, but already resolved in the provided connection field resolve function
});
dpekkle commented 4 years ago

Seconding I have run into this exact issue

makeSchema snippet

connectionPlugin({
    typePrefix: 'Filtered',
    nexusFieldName: 'filteredConnectionField',
    extendConnection: {
        totalCount: { type: 'Int' },
    },
    additionalArgs: {
        filters: FilterGroupInput,
    },
}),

Schema definition

export const Foo= objectType({
    name: "Foo",
        ...
});

export const FooByFilters = queryField((t) => {
    t.filteredConnectionField("issues", {
        type: Foo,
        inheritAdditionalArgs: true,
        totalCount() {
                        //ignored entirely, won't even be called, but required and must be an Int
            return 1;
        },
        async resolve(root, args, ctx) {
            const results = await ctx.dataSources.issueSource.getIssuesByFilters(args);
            console.log(results);
                        //{ totalCount: 477, pageInfo: {...}, edges: [...]};
                        return results;
        }
    });
});

Generated typescript

declare global {
     ...
     filteredConnectionField<FieldName extends string>(
            fieldName: FieldName, 
            config: connectionPluginCore.ConnectionFieldConfig<TypeName, FieldName>  & { totalCount: core.SubFieldResolver<TypeName, FieldName, "totalCount"> }
          ): void
  }
}

I will also note that this code actually works, and will return the totalCount that the resolve function returns (477 in this case).

The totalCount() function is never actually called, and in fact if the resolve function does NOT return a totalCount it will actually runtime error on a query saying that totalCount cannot be null.

tgriesser commented 4 years ago

The reason for this enforced separation by the API was that you'd want the ability to resolving the aggregate extension fields without needing to execute the full resolve of the connection:

issues {
  totalCount
}

If you ever did ^ in your application then you'd have an issue because you're currently relying on resolve executing to get this value, and then you'd see 1, correct?

In your case, what I would do is separate the two - and then at the data layer optimize the situation in which they're both called:

t.filteredConnectionField("issues", {
  type: Foo,
  inheritAdditionalArgs: true,
  totalCount(root, args, ctx) {
    return ctx.dataSources.issueSource.getIssuesCountByFilters(args)
  },
  async resolve(root, args, ctx) {
    return ctx.dataSources.issueSource.getIssuesByFilters(args);
  }
});

Let me know what you think of this.

dpekkle commented 4 years ago

If you ever did ^ in your application then you'd have an issue because you're currently relying on resolve executing to get this value, and then you'd see 1, correct?

That doesn't appear to be the case

image

image

And in general resolver is logged.

Your reasoning makes sense, and if this were working your suggestion to optimize both being called may be tenable, though in my situation non-trivial as I have GraphQL layered over REST endpoints, where each datasource function is hitting a dedicated endpoint.

It made sense to gather all the connection information in one hit, thinking about how to avoid the two hits may need to involve something akin to the dataloader pattern that intercepts and defers the execution of that HTTP request, but I'd be a bit concerned about the complexity of cleanly abstracting that.

Realistically (time constraints and all) I'd likely end up just extending the field at the connection level, though obviously not the preferred solution e.g.

t.filteredConnectionField("issues", {
    type: Foo,
    extendConnection(t) {
        t.int("totalCount");
    },
    async resolve(root, args, ctx) {
        return ctx.dataSources.issueSource.getIssuesByFilters(args);
    }
});
dpekkle commented 4 years ago

One thing I do find somewhat peculiar is that, within the SDL, "totalCount" is a child field of the connection.

type FilteredFooConnection {
  edges: [FilteredFooEdge]
  pageInfo: PageInfo!
  totalCount: Int!
}

In any other case the child field's resolver's first argument would be the resolved parent (i.e. what is returned by resolve), so in supporting the case of executing a child resolver without the parent resolver it does make it somewhat difficult to parse the behaviour. Perhaps there is precedence for this though.

tgriesser commented 4 years ago

Ah right, I forgot - when you're doing the resolve yourself, you'd need to do the work yourself if you want to be able to defer it. Here's a better demonstration:

t.filteredConnectionField("issues", {
  type: Foo,
  inheritAdditionalArgs: true,
  totalCount(root, args, ctx) {
    console.log('In specific resolver')
    return ctx.dataSources.issueSource.getIssuesCountByFilters(args)
  },
  async resolve(root, args, ctx) {
    let exec: ReturnType<typeof ctx.dataSources.issueSource.getIssuesByFilters>;
    return {
       get edges() {
          console.log('In edges resolver')
          exec = exec || ctx.dataSources.issueSource.getIssuesByFilters(args);
          return exec.then(val => val.edges)
       },
       get pageInfo() {
          console.log('In pageInfo resolver')
          exec = exec || ctx.dataSources.issueSource.getIssuesByFilters(args);
          return exec.then(val => val.pageInfo)
       }
    }
  }
});

I suppose I could add an option to the config to make this not required from a type perspective if you know it's always going to be available if the connection is hit, e.g.:

extendConnection: {
  totalCount: { type: 'Int', fromResolve: true },
},

In which case the nodes API for connection would be made unavailable.

tgriesser commented 4 years ago

In any other case the child field's resolver's first argument would be the resolved parent (i.e. what is returned by resolve), so in supporting the case of executing a child resolver without the parent resolver it does make it somewhat difficult to parse the behaviour.

I'm not certain, but you may be referring to https://github.com/graphql-nexus/schema/issues/402 which is an issue I'd like to get fixed soon.

dpekkle commented 4 years ago

First off thanks for getting back to me, very late here so I can't tinker much but will later. I still haven't found a situation where the resolver for totalCount is ever called, so what we're discussing doesn't quite line up with what I'm seeing. Is it behaving as you'd expect?

Here's a better demonstration

Good to know. Though to be clear, if possible, I don't want getIssuesByFilters and getIssuesCountByFilters to both make dataSource calls when both totalCount and edges || pageInfo fields are asked for. My concern there would be that it would involve twice the REST calls, redundant processing of pagination/filter args (on node and the remote server), then twice the database hits, which is what I was considering deferring.

That would be the most common use case in my application, so I'd prefer optimizing for that case.

I suppose I could add an option to the config

I'd be fine with losing the nodes API.

In any other case the child field's resolver's first argument would be the resolved parent (i.e. what is returned by resolve), so in supporting the case of executing a child resolver without the parent resolver it does make it somewhat difficult to parse the behaviour.

I'm not certain, but you may be referring to #402 which is an issue I'd like to get fixed soon.

Looks right, the root coming in to totalCounts does seem strange.

tgriesser commented 4 years ago

Oh, yeah if you're wrapping a rest endpoint this doesn't make sense. I'll look at adding the config option to the extendConnection field.

dpekkle commented 4 years ago

Hey @tgriesser, is this still on your radar?

tgriesser commented 4 years ago

@dpekkle It is, yes. Have a few things I've been meaning get to here - I'll try and block off some time this week to get to this.