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

#436

Closed

Conversation

AlessandroFerrariFood
Copy link

@AlessandroFerrariFood AlessandroFerrariFood commented Jan 11, 2021

Using recent apollo-server-core populateASTNode(..) don't detect most columns if sqlColumn is not set.
Digging I found that recent apollo-server-core always define a field resolver (see
#3988 enablePluginsForSchemaResolvers() function
) so "!field.resolve" is not a good check for columns.
I found using parentTypeNode.constructor.name === 'GraphQLObjectType' is correct in all cases I can test.

…luginsForSchemaResolvers function, #3988) so "!field.resolve" is not a good check for columns.

Using parentTypeNode.constructor.name === 'GraphQLObjectType'
// is it just a column? if they specified a sqlColumn or they didn't define a resolver, yeah
} else if (fieldConfig.sqlColumn || !field.resolve) {
// or maybe it just depends on some SQL columns
} else if (fieldConfig.sqlDeps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move these around?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because fieldConfig.sqlDeps usually has field.resolve set and didn't match fieldConfig.sqlColumn || !field.resolve condition.
Now field.resolve is always set so fieldConfig.sqlDeps need higher priority.

@alexbbt
Copy link
Contributor

alexbbt commented Jan 11, 2021

Would you mind cleaning up your commit message? If you need more than can fit on one line, then do

subject
line1
line2

@AlessandroFerrariFood
Copy link
Author

AlessandroFerrariFood commented Jan 12, 2021

@alexbbt
No problem. I was just creating a draft so I didn't expect to be seen and get feedback.

I found cases where my solution didn't works as expected so I need to fix it (see failed test).
In my mind

favNums: {
type: new GraphQLList(GraphQLInt),
resolve: () => [1, 2, 3]
},

should be marked with "ignoreAll: true" because join-monster has nothing to do.
Am I wrong?

@alexbbt
Copy link
Contributor

alexbbt commented Jan 13, 2021

Ah got it, a draft was a good start.

Yes I think ignoreAll is correct

@zachrdz
Copy link
Contributor

zachrdz commented Apr 4, 2021

Any progress on this? I just upgraded to join-monster v3 from v2, and I was able to get everything working with express-graphql. Yet, when I tried to use apollo-server-express instead, all of the fields for my resolvers are always returning null due to sql query not containing any of the columns requested in the graphql query . I added in the fix from this PR and that fixed the issue, but I guess there is still work to be done to get a passing build?

@zachrdz
Copy link
Contributor

zachrdz commented Apr 4, 2021

I've created a new PR with minor adjustments to this code and passing tests here #444 . I believe it is ready to merge in its current state, waiting on review.

@alexbbt
Copy link
Contributor

alexbbt commented Apr 30, 2021

Closing in favor of the new PR

@alexbbt alexbbt closed this Apr 30, 2021
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

Successfully merging this pull request may close these issues.

None yet

3 participants