Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Some Dictionaries do not get model bound #1418

Closed
pranavkm opened this issue Oct 21, 2014 · 17 comments
Closed

Some Dictionaries do not get model bound #1418

pranavkm opened this issue Oct 21, 2014 · 17 comments

Comments

@pranavkm
Copy link
Contributor

Consider the model

public class Foo
{
    public Dictionary<string, string> Values { get; set; }
}

The name generated by Html.NameFor(v => v.Values["height"]) is Values[height] however this expression does not get model bound.

There's a gap in the collection model binder where it only attempts to bind numeric keys. The right fix for this would be to call into IEnumerableValueProvider and then bind each key and value independently and add the result to the dictionary.

@pranavkm pranavkm added the bug label Oct 21, 2014
@pranavkm
Copy link
Contributor Author

cc @kichalla This issue seems to repro in WebAPI 2.3.2, so we might be able to port this fix back to WebAPI.

@danroth27
Copy link
Member

@pranavkm What is the behavior in MVC 5?

@dougbu
Copy link
Member

dougbu commented Oct 23, 2014

may also need to change @Html.NameFor() to get this working end-to-end. the name Values[height] is ambiguous, covering for example indexers using a variable named height. shouldn't the generated name be Values["height"] in this case?
since @Html.Namefor() defers naming of the indexer argument to Func<object, object>.ToString(), might not be simple to finish this scenario.

@rynowak
Copy link
Member

rynowak commented Nov 20, 2014

See: RoundTrippedValues_GetsModelBound_ForStringIndexedProperties for a functional test that needs to be updated

@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0-rc1, 6.0.0-beta2 Dec 12, 2014
@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0-beta3, 6.0.0-rc1 Jan 11, 2015
@yishaigalatzer
Copy link
Contributor

@pranavkm did you do the comparison to MVC5?

@kichalla
Copy link
Member

Just FYI...I have tried in MVC 5 and it also didn't support this..

@dougbu
Copy link
Member

dougbu commented Mar 20, 2015

what about Web API 3?

@kichalla
Copy link
Member

Pranav and I investigated earlier(please take a look at the earlier comment in this bug) about this for 2.3.2 and it was not supported there too..

@danroth27
Copy link
Member

Clearing milestone so we look at this again.

@Eilon Eilon added this to the 6.0.0-beta5 milestone May 8, 2015
@Eilon
Copy link
Member

Eilon commented May 8, 2015

We need to make sure we can model bind everything that our HTML helpers generate.

@Eilon Eilon modified the milestones: 6.0.0-beta6, 6.0.0-beta5 May 18, 2015
@Eilon Eilon modified the milestones: 6.0.0-beta6, 6.0.0-beta7 Jul 1, 2015
@danroth27 danroth27 modified the milestones: 6.0.0-beta6, 6.0.0-beta7 Jul 6, 2015
@danroth27 danroth27 modified the milestones: 6.0.0-beta6, 6.0.0-beta7 Jul 15, 2015
@dougbu
Copy link
Member

dougbu commented Jul 16, 2015

I'm surprised to say MVC 5 supports binding to Dictionary<string, TValue> properties. It doesn't support for example Dictionary<int, string> but submitted values for such dictionaries are visible in ModelState. MVC 6 has regressed in both ways i.e. w.r.t. ModelState entries as well as model dictionary entries.

dougbu added a commit that referenced this issue Jul 20, 2015
…entries

- #1418
- add new fallback binding in `DictionaryModelBinder`
 - similar to MVC 5 approach but more explicit and with better key conversion support
- fix bugs in `PrefixContainer` encountered while adding new tests of #1418 scenarios
 - did not handle entries like "[key]" or "prefix.key[index]" correctly
 - refactor part of `GetKeyFromEmptyPrefix()` into `IndexOfDelimiter()`; share with `GetKeyFromNonEmptyPrefix()`
 - extend `ReadableStringCollectionValueProviderTest` to cover bracketed key segments

nits:
- remove use of "foo", "bar", and "baz" in affected test classes
- `""` -> `string.Empty`
- `vpResult` -> `result`
dougbu added a commit that referenced this issue Jul 20, 2015
…entries

- #1418
- add new fallback binding in `DictionaryModelBinder`
 - similar to MVC 5 approach but more explicit and with better key conversion support
- fix bugs in `PrefixContainer` encountered while adding new tests of #1418 scenarios
 - did not handle entries like "[key]" or "prefix.key[index]" correctly
 - refactor part of `GetKeyFromEmptyPrefix()` into `IndexOfDelimiter()`; share with `GetKeyFromNonEmptyPrefix()`
 - extend `ReadableStringCollectionValueProviderTest` to cover bracketed key segments

nits:
- remove use of "foo", "bar", and "baz" in affected test classes
- `""` -> `string.Empty`
- `vpResult` -> `result`
dougbu added a commit that referenced this issue Jul 20, 2015
…entries

- #1418
- add new fallback binding in `DictionaryModelBinder`
 - similar to MVC 5 approach but more explicit and with better key conversion support
- fix bugs in `PrefixContainer` encountered while adding new tests of #1418 scenarios
 - did not handle entries like "[key]" or "prefix.key[index]" correctly
 - refactor part of `GetKeyFromEmptyPrefix()` into `IndexOfDelimiter()`; share with `GetKeyFromNonEmptyPrefix()`
 - extend `ReadableStringCollectionValueProviderTest` to cover bracketed key segments

nits:
- remove use of "foo", "bar", and "baz" in affected test classes
- `""` -> `string.Empty`
- `vpResult` -> `result`
@dougbu
Copy link
Member

dougbu commented Jul 20, 2015

79a2982

@dougbu dougbu closed this as completed Jul 20, 2015
@dougbu dougbu changed the title Dictionaries do not get model bound Some Dictionaries do not get model bound Jul 20, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants