-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Ensure IFormFile binding for nested properties works #12847
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
Changes from all commits
0119719
dfa8a82
d10a6fd
2476377
fb59968
455c165
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using Microsoft.AspNetCore.Http; | ||
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding | ||
| { | ||
| /// <summary> | ||
| /// An <see cref="IValueProvider"/> adapter for data stored in an <see cref="IFormFileCollection"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Unlike most <see cref="IValueProvider"/> instances, <see cref="FormFileValueProvider"/> does not provide any values, but | ||
| /// specifically responds to <see cref="ContainsPrefix(string)"/> queries. This allows the model binding system to | ||
| /// recurse in to deeply nested object graphs with only values for form files. | ||
| /// </remarks> | ||
| public sealed class FormFileValueProvider : IValueProvider | ||
| { | ||
| private readonly IFormFileCollection _files; | ||
| private PrefixContainer _prefixContainer; | ||
|
|
||
| /// <summary> | ||
| /// Creates a value provider for <see cref="IFormFileCollection"/>. | ||
| /// </summary> | ||
| /// <param name="files">The <see cref="IFormFileCollection"/>.</param> | ||
| public FormFileValueProvider(IFormFileCollection files) | ||
| { | ||
| _files = files ?? throw new ArgumentNullException(nameof(files)); | ||
| } | ||
|
|
||
| private PrefixContainer PrefixContainer | ||
| { | ||
| get | ||
| { | ||
| _prefixContainer ??= CreatePrefixContainer(_files); | ||
| return _prefixContainer; | ||
| } | ||
| } | ||
|
|
||
| private static PrefixContainer CreatePrefixContainer(IFormFileCollection formFiles) | ||
| { | ||
| var fileNames = new List<string>(); | ||
| var count = formFiles.Count; | ||
| for (var i = 0; i < count; i++) | ||
| { | ||
| var file = formFiles[i]; | ||
|
|
||
| // If there is an <input type="file" ... /> in the form and is left blank. | ||
pranavkm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // This matches the filtering behavior from FormFileModelBinder | ||
| if (file.Length == 0 && string.IsNullOrEmpty(file.FileName)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| fileNames.Add(file.Name); | ||
| } | ||
|
|
||
| return new PrefixContainer(fileNames); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool ContainsPrefix(string prefix) => PrefixContainer.ContainsPrefix(prefix); | ||
|
|
||
| /// <inheritdoc /> | ||
| public ValueProviderResult GetValue(string key) => ValueProviderResult.None; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Http; | ||
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding | ||
| { | ||
| /// <summary> | ||
| /// A <see cref="IValueProviderFactory"/> for <see cref="FormValueProvider"/>. | ||
| /// </summary> | ||
| public sealed class FormFileValueProviderFactory : IValueProviderFactory | ||
| { | ||
| /// <inheritdoc /> | ||
| public Task CreateValueProviderAsync(ValueProviderFactoryContext context) | ||
| { | ||
| if (context == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(context)); | ||
| } | ||
|
|
||
| var request = context.ActionContext.HttpContext.Request; | ||
| if (request.HasFormContentType) | ||
| { | ||
| // Allocating a Task only when the body is multipart form. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment isn't super duper accurate.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I changed the code, forgot to update the comment. |
||
| return AddValueProviderAsync(context, request); | ||
| } | ||
|
|
||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| private static async Task AddValueProviderAsync(ValueProviderFactoryContext context, HttpRequest request) | ||
| { | ||
| var formCollection = await request.ReadFormAsync(); | ||
| if (formCollection.Files.Count > 0) | ||
| { | ||
| var valueProvider = new FormFileValueProvider(formCollection.Files); | ||
| context.ValueProviders.Add(valueProvider); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>netcoreapp3.0</TargetFramework> | ||
| <RootNamespace>Microsoft.AspNetCore.Mvc</RootNamespace> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks 👏 for 👏 doing 👏 this |
||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc.Abstractions; | ||
| using Microsoft.AspNetCore.Routing; | ||
| using Microsoft.Extensions.Primitives; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding | ||
| { | ||
| public class FormFileValueProviderFactoryTest | ||
| { | ||
| [Fact] | ||
| public async Task CreateValueProviderAsync_DoesNotAddValueProvider_IfRequestDoesNotHaveFormContent() | ||
| { | ||
| // Arrange | ||
| var factory = new FormFileValueProviderFactory(); | ||
| var context = CreateContext("application/json"); | ||
|
|
||
| // Act | ||
| await factory.CreateValueProviderAsync(context); | ||
|
|
||
| // Assert | ||
| Assert.Empty(context.ValueProviders); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task CreateValueProviderAsync_DoesNotAddValueProvider_IfFileCollectionIsEmpty() | ||
| { | ||
| // Arrange | ||
| var factory = new FormFileValueProviderFactory(); | ||
| var context = CreateContext("multipart/form-data"); | ||
|
|
||
| // Act | ||
| await factory.CreateValueProviderAsync(context); | ||
|
|
||
| // Assert | ||
| Assert.Empty(context.ValueProviders); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task CreateValueProviderAsync_AddsValueProvider() | ||
| { | ||
| // Arrange | ||
| var factory = new FormFileValueProviderFactory(); | ||
| var context = CreateContext("multipart/form-data; boundary=----WebKitFormBoundarymx2fSWqWSd0OxQqq"); | ||
| var files = (FormFileCollection)context.ActionContext.HttpContext.Request.Form.Files; | ||
| files.Add(new FormFile(Stream.Null, 0, 10, "some-name", "some-name")); | ||
|
|
||
| // Act | ||
| await factory.CreateValueProviderAsync(context); | ||
|
|
||
| // Assert | ||
| Assert.Collection( | ||
| context.ValueProviders, | ||
| v => Assert.IsType<FormFileValueProvider>(v)); | ||
| } | ||
|
|
||
| private static ValueProviderFactoryContext CreateContext(string contentType) | ||
| { | ||
| var context = new DefaultHttpContext(); | ||
| context.Request.ContentType = contentType; | ||
| context.Request.Form = new FormCollection(new Dictionary<string, StringValues>(), new FormFileCollection()); | ||
| var actionContext = new ActionContext(context, new RouteData(), new ActionDescriptor()); | ||
|
|
||
| return new ValueProviderFactoryContext(actionContext); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.Extensions.Primitives; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding | ||
| { | ||
| public class FormFileValueProviderTest | ||
| { | ||
| [Fact] | ||
| public void ContainsPrefix_ReturnsFalse_IfFileIs0LengthAndFileNameIsEmpty() | ||
| { | ||
| // Arrange | ||
| var httpContext = new DefaultHttpContext(); | ||
| httpContext.Request.ContentType = "multipart/form-data"; | ||
| var formFiles = new FormFileCollection(); | ||
| formFiles.Add(new FormFile(Stream.Null, 0, 0, "file", fileName: null)); | ||
| httpContext.Request.Form = new FormCollection(new Dictionary<string, StringValues>(), formFiles); | ||
|
|
||
| var valueProvider = new FormFileValueProvider(formFiles); | ||
|
|
||
| // Act | ||
| var result = valueProvider.ContainsPrefix("file"); | ||
|
|
||
| // Assert | ||
| Assert.False(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ContainsPrefix_ReturnsTrue_IfFileExists() | ||
| { | ||
| // Arrange | ||
| var httpContext = new DefaultHttpContext(); | ||
| httpContext.Request.ContentType = "multipart/form-data"; | ||
| var formFiles = new FormFileCollection(); | ||
| formFiles.Add(new FormFile(Stream.Null, 0, 10, "file", "file")); | ||
| httpContext.Request.Form = new FormCollection(new Dictionary<string, StringValues>(), formFiles); | ||
|
|
||
| var valueProvider = new FormFileValueProvider(formFiles); | ||
|
|
||
| // Act | ||
| var result = valueProvider.ContainsPrefix("file"); | ||
|
|
||
| // Assert | ||
| Assert.True(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetValue_ReturnsNoneResult() | ||
| { | ||
| // Arrange | ||
| var httpContext = new DefaultHttpContext(); | ||
| httpContext.Request.ContentType = "multipart/form-data"; | ||
| var formFiles = new FormFileCollection(); | ||
| formFiles.Add(new FormFile(Stream.Null, 0, 10, "file", "file")); | ||
| httpContext.Request.Form = new FormCollection(new Dictionary<string, StringValues>(), formFiles); | ||
|
|
||
| var valueProvider = new FormFileValueProvider(formFiles); | ||
|
|
||
| // Act | ||
| var result = valueProvider.GetValue("file"); | ||
|
|
||
| // Assert | ||
| Assert.Equal(ValueProviderResult.None, result); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What motivated the choice to make this separate from
FormValueProvider? Is the idea that someone could remove this value provider if they don't want it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was one of the reasons. The other reason is that
FormValueProvideris meant to be derived, and it now makes user code much more complicated since they now have to track two separate prefix groups.