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 thrown for invalid operation name #3106

Merged
merged 5 commits into from
Apr 27, 2022
Merged

Fix error thrown for invalid operation name #3106

merged 5 commits into from
Apr 27, 2022

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Apr 26, 2022

When a request is executed with an operation name that is not listed in the document, a "client" (validation) error should be returned to the caller with friendly description, rather than a "server" error being thrown which triggers the unhandled exception delegate and returns a masked error to the caller.

@Shane32 Shane32 requested a review from sungam3r April 26, 2022 21:15
@Shane32 Shane32 self-assigned this Apr 26, 2022
@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Apr 26, 2022
@Shane32 Shane32 added the bugfix Pull request that fixes a bug label Apr 26, 2022
result.Errors[0].InnerException.ShouldBeOfType<InvalidOperationException>();
result.Errors[0].InnerException.Message.ShouldBe($"Query does not contain operation '{operationName}'.");
result.Errors[0].ShouldBeOfType<InvalidOperationError>();
result.Errors[0].Message.ShouldBe("Query does not contain operation '" + operationName + "'.");
Copy link
Member

Choose a reason for hiding this comment

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

Check InnerException for null ?

@@ -65,7 +64,7 @@ public static IServiceProvider CurrentServiceProvider
{
var method = _currentMethod.Value;
return method == null || !_diAdapters.TryGetValue(method, out var stack) || stack == null || stack.Count == 0
? throw new InvalidOperationError("Attempt to access IServiceProvider out of prepared DependencyInjection context.")
Copy link
Member

Choose a reason for hiding this comment

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

oops

@@ -147,7 +147,7 @@ public virtual async Task<ExecutionResult> ExecuteAsync(ExecutionOptions options

if (operation == null)
{
throw new InvalidOperationException($"Query does not contain operation '{options.OperationName}'.");
throw new InvalidOperationError($"Query does not contain operation '{options.OperationName}'.");
Copy link
Member

Choose a reason for hiding this comment

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

Let's move metrics.SetOperationName(operation?.Name); after this check.

@codecov-commenter
Copy link

Codecov Report

Merging #3106 (a79dbfc) into master (01bf0ac) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3106      +/-   ##
==========================================
+ Coverage   84.31%   84.34%   +0.02%     
==========================================
  Files         362      362              
  Lines       15884    15884              
  Branches     2580     2579       -1     
==========================================
+ Hits        13393    13397       +4     
+ Misses       1875     1871       -4     
  Partials      616      616              
Impacted Files Coverage Δ
src/GraphQL/Execution/DocumentExecuter.cs 90.00% <100.00%> (ø)
src/GraphQL/Execution/InvalidOperationError.cs 100.00% <0.00%> (+100.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 01bf0ac...a79dbfc. Read the comment docs.

@Shane32 Shane32 merged commit cd42366 into master Apr 27, 2022
@Shane32 Shane32 deleted the fix_error branch April 27, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug 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