Add BindingSourceMetadataProvider #5750
Conversation
|
@rynowak Should we add two different types of |
| options.ModelMetadataDetailsProviders.Add(new SpecialParametersBindingMetadataProvider(typeof(CancellationToken))); | ||
| options.ModelMetadataDetailsProviders.Add(new SpecialParametersBindingMetadataProvider(typeof(IFormFile))); | ||
| options.ModelMetadataDetailsProviders.Add(new SpecialParametersBindingMetadataProvider(typeof(IFormCollection))); | ||
| options.ModelMetadataDetailsProviders.Add(new SpecialParametersBindingMetadataProvider(typeof(Stream))); |
There was a problem hiding this comment.
Can you figure out what the right set of types are to have Special applied?
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata | ||
| { | ||
| public class SpecialParametersBindingMetadataProvider : IBindingMetadataProvider |
There was a problem hiding this comment.
SpecialBindingSourceMetadataProvider
In general always avoid plurals like SpecialParameters in names.
| /// Creates a new <see cref="SpecialParametersBindingMetadataProvider"/> for the given <paramref name="type"/>. | ||
| /// </summary> | ||
| /// <param name="type"> | ||
| /// The <see cref="Type"/>. All properties with this <see cref="Type"/> will have |
There was a problem hiding this comment.
Careful here, this isn't what your code does.
|
@rynowak Updated |
jbagga
changed the title
jbagga
force-pushed
the
jbagga/ApiExplorerSpecialParams5673
branch
from
fbfe141 to
21f7fe1
Compare
|
Added unit tests |
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata | ||
| { | ||
| public class FormFileBindingSourceMetadataProvider : IBindingMetadataProvider |
There was a problem hiding this comment.
Why not also take the BindingSource as a parameter and then you don't have to make this class tied to the source you're setting up.
| throw new ArgumentNullException(nameof(context)); | ||
| } | ||
|
|
||
| if (Type.IsAssignableFrom(context.Key.ModelType)) |
There was a problem hiding this comment.
This doesn't do what the docs say 😆 this will set the source for the given type and anything assignable to the given type.
| public class FormFileBindingSourceMetadataProviderTests | ||
| { | ||
| [Fact] | ||
| public void ChecksParameterType_AssignsFormFileBindingSource() |
There was a problem hiding this comment.
https://github.com/aspnet/Home/wiki/Engineering-guidelines#unit-test-method-naming
Consider something like CreateBindingMetadata_ForMatchingType_SetsBindingSource.
if you don't have a lot to say then say a little.
jbagga
force-pushed
the
jbagga/ApiExplorerSpecialParams5673
branch
from
21f7fe1 to
4974ab3
Compare
|
@rynowak Updated. The swagger hides the |
|
|
||
| namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata | ||
| { | ||
| public class SpecialBindingSourceMetadataProvider : IBindingMetadataProvider |
There was a problem hiding this comment.
Just call this BindingSourceMetadataProvider. It's not as special as you think
| public class SpecialBindingSourceMetadataProvider : IBindingMetadataProvider | ||
| { | ||
| public Type Type { get; } | ||
| public BindingSource BindingSource { get; } |
|
|
||
| // ModelState | ||
| Assert.True(modelState.IsValid); | ||
| } |
There was a problem hiding this comment.
Is this just a test that the binding source has no effect on the runtime behavior?
@domaindrivendev - I believe this was your ideal behavior right? |
|
@rynowak Updated |
| var boundPerson = Assert.IsType<CancellationTokenBundle>(modelBindingResult.Model); | ||
| Assert.NotNull(boundPerson); | ||
| Assert.Equal("Fred", boundPerson.Name); | ||
| Assert.Equal(token, boundPerson.Token); |
jbagga
changed the title
jbagga
force-pushed
the
jbagga/ApiExplorerSpecialParams5673
branch
from
15c2f44 to
1b4c30d
Compare
In progress #5673
cc @rynowak