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

Support multipart file uploads #1115

Merged
merged 13 commits into from
Jan 23, 2024
Merged

Support multipart file uploads #1115

merged 13 commits into from
Jan 23, 2024

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jan 22, 2024

Based on the GraphQL multipart request spec:

Readme has been updated, sample added, and tests have been added for 100% coverage of new code. API and endpoints are backwards compatible. Did not introduce an option to enable/disable new behavior because it is backwards compatible, and additional behavior should have no negative side-effects for existing users. Also added a new scalar called FormFileGraphType which can be used by schemas to ingest IFormFile values sent via multipart form posts. The scalar needs to be added via the .AddFormFileGraphType() builder method.

Note that GraphQL.NET Server 7.x is vulnerable to security implications of form-type POST requests being enabled by default. This is true with or without this feature, and is outlined in the readme. The next version of GraphQL.NET Server will have form-type POST requests disabled by default (as also noted in the readme).

I recommend reviewing the api diff, readme, and sample project to understand the extent of this PR.

Possible changes that could be made:

  • Make middleware partial class and move code to separate file to perform form parsing
  • Add type-first schema validation attribute to limit potential mime types that can be submitted
  • Add enum property to options that configures the form post parsing methods available

@Shane32 Shane32 self-assigned this Jan 22, 2024
src/Transports.AspNetCore/GraphQLHttpMiddleware.cs Dismissed Show dismissed Hide dismissed
samples/Samples.Upload/Mutation.cs Dismissed Show dismissed Hide dismissed
src/Transports.AspNetCore/GraphQLHttpMiddleware.cs Dismissed Show dismissed Hide dismissed
tests/Samples.Upload.Tests/EndToEndTests.cs Dismissed Show dismissed Hide dismissed
src/Transports.AspNetCore/GraphQLHttpMiddleware.cs Dismissed Show dismissed Hide dismissed
@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2462fdd) 93.45% compared to head (825c426) 93.81%.

Files Patch % Lines
src/Transports.AspNetCore/GraphQLHttpMiddleware.cs 99.15% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1115      +/-   ##
==========================================
+ Coverage   93.45%   93.81%   +0.35%     
==========================================
  Files          45       49       +4     
  Lines        2215     2360     +145     
  Branches      375      418      +43     
==========================================
+ Hits         2070     2214     +144     
  Misses        103      103              
- Partials       42       43       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gao-artur
Copy link
Contributor

Add type-first schema validation attribute to limit potential mime types that can be submitted

This looks important.

The feature looks very cool! It's not an official spec, though, right?

@Shane32
Copy link
Member Author

Shane32 commented Jan 23, 2024

Add type-first schema validation attribute to limit potential mime types that can be submitted

This looks important.

Ok then I suggest we do that as a subsequent PR. There are some questions in my mind which will be worth review. So long as the underlying feature (this PR) is fine, let's merge it in first.

The feature looks very cool! It's not an official spec, though, right?

The GraphQL working group hasn't ratified any GraphQL over HTTP protocol. They do have a proposal, which does not encompass this feature, nor the existing WebSockets protocols, persisted queries, or automatic persisted queries. However, as can be seen by the list in the spec's readme, many existing GraphQL clients and servers support this feature. There is an existing nuget package to extend GraphQL.NET Server to support this feature here, but it suffers from being a separate piece of ASP.NET Core middleware. I think we should add this as a first-class feature of GraphQL.NET Server.

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.

None yet

3 participants