Change IValueProviderFactory.GetValueProvider to be synchronous. #528
Conversation
| .ToArray(); | ||
|
|
||
| return new ActionBindingContext( | ||
| var valueProviders = _valueProviderFactories.Select(factory => factory.GetValueProvider(requestContext)) |
There was a problem hiding this comment.
What happened to the ToArray here? Seems like that's a good thing to have since we don't want to call the factories multiple times.
There was a problem hiding this comment.
The CompositeValueProvider does that for us: https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/CompositeValueProvider.cs#L21-L22
There was a problem hiding this comment.
Can the CompositeValueProvider be replaced? In WebAPI we had the AsArray construct, that wouldn't force a copy, but will ensure that if the input is not an array it will do it then. Thus reducing the dependency/coupling of the two classes.
There was a problem hiding this comment.
You can always consume it as an IEnumerable though and AsArray seems like a premature perf optimization.
|
bump |
| } | ||
|
|
||
| private PrefixContainer PrefixContainer | ||
| public CultureInfo Culture |
There was a problem hiding this comment.
Consider just a field here is this doesn't need to be exposed
There was a problem hiding this comment.
Not really sure why this was ever a public property actually. If there's a good reason then just ignore my ill-informed-raving.
There was a problem hiding this comment.
I don't think so. I think it's just a convenience thing so you know what Culture your instance of ReadableString... is using
|
|
| @@ -29,14 +29,12 @@ public class DefaultActionBindingContextProvider : IActionBindingContextProvider | |||
| _validatorProviders = validatorProviders; | |||
| } | |||
|
|
|||
| public async Task<ActionBindingContext> GetActionBindingContextAsync(ActionContext actionContext) | |||
| public Task<ActionBindingContext> GetActionBindingContextAsync(ActionContext actionContext) | |||
There was a problem hiding this comment.
this will not return a faulted task when something fails along the way. Did you verify the behavior is still fine upstream when a failure happens?
There was a problem hiding this comment.
I think the context provider also needs to be sync. That said the type itself might need some redesign now that we have support for scoped services which was primarily what we designed this for.
There was a problem hiding this comment.
@yishaigalatzer Thoughts?
Follow up to #303