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

Update middleware with options and new design #774

Merged
merged 52 commits into from
May 20, 2022
Merged

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Apr 14, 2022

TODO

  • Read up on BatchedRequestsExecuteInParallel in some batching docs/specs
  • Implement BatchedRequestsConcurrencyLimit as a new feature in a separate PR or here.
  • Take a look at options regarding GraphQLRequestExecutionResult

@Shane32 Shane32 requested a review from sungam3r April 14, 2022 17:42
@Shane32 Shane32 self-assigned this Apr 14, 2022
@github-actions github-actions bot added the test label Apr 14, 2022
@Shane32
Copy link
Member Author

Shane32 commented Apr 14, 2022

If we merge #773 then it will reduce the diff here in this PR. Most changed files in this PR have file-scoped namespaces.

@Shane32
Copy link
Member Author

Shane32 commented Apr 14, 2022

@sungam3r This PR is a copy of my implementation with almost no changes. I have commented out the code that is needed to support websockets once we merge the updated websocket support. Until then it will still use the old websocket middleware.

If we want to make changes here, we should make them in a separate PR. For instance, if we want to reintroduce the CancellationToken virtual member (which is fine; it will not significantly change the code), let's make a separate PR to do so. I want to keep track of the diff between this and GraphQL.AspNetCore3.

@Shane32
Copy link
Member Author

Shane32 commented Apr 14, 2022

The few changes to tests involve mutation requests against a GET endpoint, and error message results.

@sungam3r
Copy link
Member

Need to update branch? Will nothing bad happen?

@sungam3r sungam3r added this to the v7 milestone Apr 14, 2022
@sungam3r
Copy link
Member

sungam3r commented Apr 14, 2022

@Shane32 Check description of https://github.com/graphql-dotnet/server/milestone/7

@sungam3r
Copy link
Member

I do not promise that I can review tomorrow. I had a challenge at work so most likely on the weekend.

@codecov-commenter
Copy link

Codecov Report

Merging #774 (b4c6501) into develop (6bba2cb) will decrease coverage by 0.22%.
The diff coverage is 56.90%.

@@             Coverage Diff             @@
##           develop     #774      +/-   ##
===========================================
- Coverage    47.80%   47.58%   -0.23%     
===========================================
  Files           50       59       +9     
  Lines         1548     1715     +167     
  Branches       174      204      +30     
===========================================
+ Hits           740      816      +76     
- Misses         752      822      +70     
- Partials        56       77      +21     
Impacted Files Coverage Δ
.../Transports.AspNetCore/Errors/AccessDeniedError.cs 0.00% <0.00%> (ø)
...NetCore/Errors/BatchedRequestsNotSupportedError.cs 0.00% <0.00%> (ø)
...re/Errors/WebSocketSubProtocolNotSupportedError.cs 0.00% <0.00%> (ø)
.../Extensions/GraphQLBuilderUserContextExtensions.cs 0.00% <0.00%> (ø)
...sions/GraphQLHttpEndpointRouteBuilderExtensions.cs 0.00% <0.00%> (ø)
src/Transports.AspNetCore/UserContextBuilder.cs 0.00% <0.00%> (ø)
src/Transports.AspNetCore/AuthorizationHelper.cs 11.42% <11.42%> (ø)
...ensions/GraphQLHttpApplicationBuilderExtensions.cs 31.25% <25.00%> (-10.42%) ⬇️
...rc/Transports.AspNetCore/HttpPostValidationRule.cs 42.85% <42.85%> (ø)
...ports.AspNetCore/Errors/InvalidContentTypeError.cs 50.00% <50.00%> (ø)
... and 11 more

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 6bba2cb...b4c6501. Read the comment docs.

@sungam3r
Copy link
Member

Finished! I resolved all conversation except one with subscriptions/websockets explanations as a reminder for me to read later.

@Shane32 Shane32 merged commit 2161872 into develop May 20, 2022
@Shane32 Shane32 deleted the update_middleware branch May 20, 2022 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants