-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix #1579 - Bind top-level collections as an empty collection #2583
Conversation
var isTopLevelObject = bindingContext.ModelMetadata.ContainerType == null; | ||
var hasExplicitAlias = bindingContext.BinderModelName != null; | ||
|
||
if (isTopLevelObject && (hasExplicitAlias || bindingContext.ModelName == string.Empty)) |
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.
This is a simplified version of the logic used by MutableObjectModelBinder
- simplified because we don't have to handle as many cases.
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.
Working on adding a few unit tests for this logic.
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.
You're done "working on" those tests, correct?
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.
yep.
@dougbu updated with some unit tests |
return null; | ||
} | ||
|
||
private static Type GetCollectionBinder(Type modelType) | ||
private static GenericModelBindingInfo GetCollectionBinder(Type modelType) |
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.
Suggest renaming this method and similar ones below.
GetCollectionBinder => GetCollectionBinderInfo
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.
👍
⌚ |
@dougbu Addressed most feedback, have one idea we should discuss. |
bindingContext.ModelMetadata, | ||
model); | ||
|
||
return new ModelBindingResult(model, bindingContext.ModelName, true, validationNode); |
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.
pls name the true
argument
This change treats 'top-level' collection-type models similarly to top-level POCO model - namely that they will always be instantiated even if there's no data to put inside.
Updated. Moved the model-creation down into the CollectionModelBinder classes. |
// Caller (GenericModelBinder) was able to resolve a binder type and will create a ModelBindingResult | ||
// that exits current ModelBinding loop. | ||
// If this is the fallback case, and we failed to find data as a top-level model, then generate a | ||
// default 'empty' model and return 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.
Odd case. Curious whether Web API 2 or MVC 5 pass default(KeyValuePair<TKey, TValue>)
into an action. Of course it's fine to support here.
This change treats 'top-level' collection-type models similarly to
top-level POCO model - namely that they will always be instantiated even
if there's no data to put inside.