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

Fix error message when no query is defined #3170

Merged
merged 3 commits into from
Jun 12, 2022
Merged

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jun 9, 2022

Currently a ARGUMENT_NULL error is returned to the client when a query request arrives for a schema with no root query type. This PR aligns the error message with the similar case for mutation and subscription types, until such time as we decide to disallow schemas without a root query type (in accordance with the spec).

Note that there is an upcoming RFC to the spec for a validation rule to address this condition:
graphql/graphql-spec#955

@Shane32 Shane32 self-assigned this Jun 9, 2022
@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Jun 9, 2022
@Shane32
Copy link
Member Author

Shane32 commented Jun 9, 2022

Problem: this means that without a query type, introspection queries cannot be performed anymore with this PR.

And we have a test that validates an introspection query for a schema without a query type !!!

@Shane32
Copy link
Member Author

Shane32 commented Jun 9, 2022

We should consider revising our schema introspection to require a query root type, as the spec requires -- and changing all of the tests as required.

@Shane32 Shane32 changed the title Fix error message when no query is defined [WIP] Fix error message when no query is defined Jun 9, 2022
@sungam3r
Copy link
Member

sungam3r commented Jun 9, 2022

We should consider revising our schema introspection to require a query root type, as the spec requires

I remember how I read discussions regarding it ). Hot topic.

@codecov-commenter
Copy link

Codecov Report

Merging #3170 (a35632c) into master (ec7cef0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3170      +/-   ##
==========================================
- Coverage   84.43%   84.43%   -0.01%     
==========================================
  Files         371      371              
  Lines       16087    16099      +12     
  Branches     2599     2604       +5     
==========================================
+ Hits        13583    13593      +10     
  Misses       1882     1882              
- Partials      622      624       +2     
Impacted Files Coverage Δ
src/GraphQL/Execution/ExecutionStrategy.cs 85.19% <100.00%> (+0.59%) ⬆️
src/GraphQL/GraphQLBuilderExtensions.cs 95.86% <0.00%> (-0.80%) ⬇️
src/GraphQL/Execution/Nodes/ExecutionNode.cs 83.51% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec7cef0...a35632c. Read the comment docs.

@Shane32 Shane32 changed the title [WIP] Fix error message when no query is defined Fix error message when no query is defined Jun 10, 2022
@Shane32
Copy link
Member Author

Shane32 commented Jun 10, 2022

@sungam3r I fixed the tests. I see you already approved this PR, but I'll wait to merge it until tomorrow, so you have a chance to see my additional changes if desired.

Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

I reviewed again. 👍

This dummy field for query looks pointless though. I'm still hope that spec wg will find better solution.

@sungam3r sungam3r added the enhancement New feature or request label Jun 10, 2022
Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@Shane32 Shane32 merged commit f1c8085 into master Jun 12, 2022
@Shane32 Shane32 deleted the missing_query_should_fail branch June 12, 2022 13:12
@Shane32 Shane32 added this to the 5.4 milestone Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants