Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Projection of the edges field on wrapped type #75

Open
yoadsn opened this issue May 17, 2021 · 6 comments
Open

Projection of the edges field on wrapped type #75

yoadsn opened this issue May 17, 2021 · 6 comments

Comments

@yoadsn
Copy link
Contributor

yoadsn commented May 17, 2021

@nodkz Funny - we talked about this already back in 2016 :)
So now that the mongoose composter flattens the projection to improve performance - it also flattens the non required fields from the connection structure.

This means that for schema Post { title } and User { posts } where posts is a connection you would get for a query:
{ user { posts { edges { node { title } } } } }

This projection on mongodb "posts" collection find:
{ 'edges.node.title': true, title: true }

Also, on the "users" collection you can expect this find projection:
{'posts.edges.nodes.title': true }

This used to be a small problem since there was no "flattening" so "edges" would be the only wrong field used. now it's much more. The more fields are used from the connected type the more duplication exists. I am not sure if mongodb has any performance implications but it might.

Since i know for sure my schema is not using "edges" and "node" fields on internal types - I wanted to remove this.
I thought of working around this in multiple ways:

  • Wrapping the resolver for the "find user" and clearing the "posts" field from the projection sent to the resolver. (of course working on a copy of RP). This works, and I can think of a way to build a generic wrapper.
  • Wrapping findMany of the connection is not possible since there is only way to provide customization opts to connection resolver factory.
  • No customization to connection resolver factory allows the ability to "let the connection" know those fields are "OK" to be removed before using them on the findMany resolver. For example I would make up a parameter like allowEdgeNodeFieldsInProjection which by default is off.

What do you think - I revisited all the code involved but could not find a very clean way to let the projection ignore those fields - it must be the responsibility of the "connection" plugin I feel.

@nodkz
Copy link
Member

nodkz commented May 18, 2021

I completely agree about the responsibility of the "connection" plugin. And I think that we can remove edges key from projection. If somebody has edges fields in Mongoose models then they will be provided in node projection.

@yoadsn can you try 8.1.1-alpha.1 version? If it works well for you then I merge it with master and publish a stable package.

@nodkz
Copy link
Member

nodkz commented May 18, 2021

You may copy-paste https://unpkg.com/browse/graphql-compose-connection@8.1.1-alpha.1/lib/connection.js to your node_modules for fast testing.

@yoadsn
Copy link
Contributor Author

yoadsn commented May 18, 2021

Thanks @nodkz The above fix seems to solve it for the internal connection findMany. I think it's a great improvement.

Of course, the extra fields on the top level access (in my example "users" collection) still had all the "posts.edgest.*" flattened projections.

As I see it a type (User) was added a relation (to [Post] on the fields "posts") using a "connection" resolver which took care of the projection cleanup all the levels below it.
Now, I think that the addRelation which better knows that "posts" is a connection compared to all other resolvers on the "User" level - would need to make sure the projection of parent excludes "edges.*" when the "User" mongodb resolver is executed. This is most likely NOT the responsibility of this plugin since it only provides the resolver and down. not up.

Where should such cleanup be if we built it? I don't know.
Maybe the same way addRelation specifies "projection" to force parent to include some fields it could specify "excludeProjection" to prevent parent from loading some field. 😵

@yoadsn
Copy link
Contributor Author

yoadsn commented May 20, 2021

@nodkz Should we at least close the fix on this plugin since it's already providing great benefit as-is?

nodkz added a commit that referenced this issue May 25, 2021
@nodkz
Copy link
Member

nodkz commented May 25, 2021

@yoadsn can you provide a repro case of your problem?
You may use this sandbox https://codesandbox.io/s/github/yurtaev/graphql-compose-mongoose-codesandbox/tree/master/?file=/index.js

PS. A new version will be published in minutes.

@yoadsn
Copy link
Contributor Author

yoadsn commented May 26, 2021

@nodkz Thank you so much! (This sandbox is so convenient).

Here is a very small repro using the User and Post schemas I mentioned in my original example.
https://codesandbox.io/s/compassionate-christian-21qge?file=/index.js
When running this query:

query example {
        users(limit: 1) {
          name
          posts {
            edges {
              node {
                title
              }
            }
          }
        }
      }

The queries on the DB are:

Mongoose: users.find({}, { limit: 1, projection: { name: true, 'posts.edges.node.title': true, _id: true }})
Mongoose: posts.find({ author: ObjectId("a00000000000000000000000") }, { limit: 21, sort: { _id: -1 }, projection:{ 'edges.node.title': true, title: true, _id: true }})

And I was referring to the 'posts.edges.node.title': true projection on the users collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants