'find' projection includes 'edges' and 'count' fields

[yoadsn]

Well, best connection implementation I have seen so far! I must say I did not completely understand yet all the logic that went into this but the stable cursors are brilliant.

I have noticed that the find query of the connection resolve is projecting also the count and edges fields. (which would not make sense of course on a 'relay' type of connection where they have a different semantic meaning)
The reason for that is probably at the connectionResolver.js file here:
https://github.com/nodkz/graphql-compose-connection/blob/86ddc88dcce544e1579494072c5d7fb72ffad44a/src/connectionResolver.js#L126

Where the top level projection is combined with the node specific projection. This is to account for some middleware adding fields via the resolveParams object. (According to the comment)

I think the workaround should be to omit the specific known fields from this top level projection when combining. Maybe add after line 128:

delete findManyParams.projection.count;
delete findManyParams.projection.edges;

Although it's not "pretty" - it should do the trick.

This is also related to another behavior which is unexpected (should I open another issue?) - if only asking for the count field of the connection - a find is also issued, again with the count field projected. this is due to:

https://github.com/nodkz/graphql-compose-connection/blob/86ddc88dcce544e1579494072c5d7fb72ffad44a/src/connectionResolver.js#L130

Which again uses the top level projection and later issues a find even if only count is needed. this could be optimized by checking if the projection only contains the count and avoiding a find altogether.

Maybe I have misunderstood the purpose of this logic?

[nodkz]

These logic related to easy projection definition via Resolver.wrapResolve, TC.addFilterArg and TC.addSortArg.
Here is a commit when I add this logic with tests 0824ab8#diff-1ff7e847fc9c25bfcdbf829393312528R145

How passing down projection works in action. Eg. we add filter arg for connection resolver:

  ShopTC
    .getResolver('connection')
    .addFilterArg({
      name: 'q',
      type: 'String',
      description: 'Fulltext search with mongodb stemming and weights',
      query: (query, value, resolveParams) => {
        resolveParams.args.sort = {
          score: { $meta: 'textScore' },
        };
        query.$text = { $search: value, $language: 'ru' };
        resolveParams.projection.score = { $meta: 'textScore' };
      },
    })

The right way to define projection for connection will be (without passing down projection from conection root):

  if (!resolveParams.projection.edges) resolveParams.projection.edges = {};
  if (!resolveParams.projection.edges.node) resolveParams.projection.edges.node = {};
  resolveParams.projection.edges.node.score = { $meta: 'textScore' };

  // Versus this is much clear variant
  resolveParams.projection.score = { $meta: 'textScore' };
  // and such definition may be used for findMany resolver   

As you correctly notice count and edges are pass down to findMany. And this is the least evil in the current implementation. Cause count and edges fields may be present in a document, and user can request them:

query {
  trackerConnection: {
    edges {
      node {
        id
        count # request `count` field from document
        edges # request `edges` field from document
      }
    }
  }
}

But if user make such request, graphql-js take care to remove them from response due it normal AST tree of query:

query {
  trackerConnection: {
    count
    edges {
      node {
        id
      }
    }
  }
}

I'll try to isolate graphql-compose-[plugins] from GraphQL infoAST as much as possible and keep it simple. Of course providing to developer easy way to read projection and extend it if needed.

So will be cool if count and edges will be removed, if they are absent in resolveParams.info. But need to go down by AST.
Sorry, but I do not have time right now to improve this.

---

What about unneeded find.

But we should be sure that in resolveParams.projection do not present extended/defined by wrappers by a developer. So I thought that it is a complicated task.
I think that your suggestion is awesome! If in projection present only count field - we may safely avoiding a find. 👍

It should be realized somehow like countPromise https://github.com/nodkz/graphql-compose-connection/blob/86ddc88dcce544e1579494072c5d7fb72ffad44a/src/connectionResolver.js#L176 and lines 112-118.

Do you want to make a PR?

[yoadsn]

@nodkz I have split the unnecessary find issue off.

Regarding the removal of the count and edges - I did not understand why the AST has to be consulted. Is it because you do want to allow higher level wrappers to add edges and count to the projection?
Also because this may make sense of a document has actually these fields and for resolution time the developer wants to access them?

In that case - I understand why we must know the "original" info contents. But, what if you always kept the original projection around and allowed the middleware to modify a copy of it? In that case any middleware can consult with the original if need to make decisions about it without looking at the AST as well as the actual data access code which could smartly look at original and modified and do clever things in terms of optimization.

Of course, this would somewhat change the API - for example:

ShopTC
    .getResolver('connection')
    .addFilterArg({
      name: 'fa',
      type: 'String',
      query: (query, value, resolveParams) => {
        let originalProjection = resolveParams.originalProjection; // An immutable / copy is returned
      },
    })

Of course there are better ways to do that.. just the concept..

[nodkz]

On both your questions answer is yes. edges and count can be added to the projection by a developer, cause such fields may exist in a document.

---

Also, I remember another case when a developer may extend projections. Eg. we add persons field to Organization type and want that this field resolves a list of Persons by some field in the organization document:

OrganizationTC.addRelation(
  'persons',
  () => ({
    resolver: PersonTC.getResolver('findByIds'),
    args: {
      _ids: source => source.personsIds,
    },
    projection: { personsIds: true },
  })
);

Following query returns Person names, even personsIds field is not requested in query. Thanks to extending of projection.

query {
  organization {
    persons {
      name
    }
  }
}

---

About keeping "original" projection. Your solution is pretty simple in realization. But has one downside: we pollute resolveParams.

By the way, projection is initialized here: https://github.com/nodkz/graphql-compose/blob/master/src/resolver.js#L210-L214 So there is no problem to deep clone and add originalProjection.

Anyway, let think a little bit more, how this task can be solved. Something confuses me in adding additional resolveParam. In most case when developers will debug resolvers and printing their params to console.log will see complete duplication of projection and originalProjection.
Also needs to imagine additional use case when originalProjection may be essential/required (except for connection)?

[yoadsn]

I also feel this is polluting the resolveParams. Maybe expose the getProjectionFromAST as a utility. since info is available already. If the use case is that rare - only handful of developers would need this utility anyway usually when building some plugin.
possible downside is that maybe it's problematic from performance point of view to "recalc" the projection?