Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[release/3.1] Support dictionaries that don't implement non-generic IDictionary - #42050

Merged
steveharter merged 1 commit into
dotnet:release/3.1from
steveharter:PortBetterDictionarySupport
Oct 23, 2019
Merged

[release/3.1] Support dictionaries that don't implement non-generic IDictionary#42050
steveharter merged 1 commit into
dotnet:release/3.1from
steveharter:PortBetterDictionarySupport

Conversation

@steveharter

Copy link
Copy Markdown
Contributor

note: this PR was created and made stand-alone since the previous PR that included this fix takes a dependency on collection PR that will likely not be accepted.

Ports #41903

Issue:

Support dictionaries that implement IDictionary<TKey, TValue> but do not implement the non-generic IDictionary

Description

An assumption was made during serialization that a call to a dictionary's GetEnumerator() (either through the IEnumerable or IDictionary interfaces) returns an IDictionaryEnumerable (which is required for types that implement IDictionary). However, there are certain "newer" generic collection types outside of the BCL that don't implement IDictionary or return IDictionaryEnumerable from their IEnumerable.GetEnumerator() method (note that IDictionary<TKey, TValue> does not implement IDictionary). This caused a InvalidCastException at runtime. The fix is to assume IEnumerable<KeyValuePair<TKey, TValue>> instead of IDictionaryEnumerable for generic dictionaries.

Customer Impact

Without this PR, there is no support for generic dictionaries that implement IDictionary<TKey, TValue> but don't support the older non-generic IDictionary. This is a community-reported issue and is blocking adoption.

Regression?

No. The InvalidCastException is no longer encountered.

Risk

Low. Dictionaries have good coverage already and additional tests were added. A community member verified a fix based on the sames changes to master.

@steveharter steveharter added this to the 3.1 milestone Oct 23, 2019
@steveharter steveharter self-assigned this Oct 23, 2019
@danmoseley danmoseley added the api-approved API was approved in API review, it can be implemented label Oct 23, 2019
@danmoseley

Copy link
Copy Markdown

Thanks for breaking this out. Approved: customer reported, impeding adoption, relatively low risk and localized, good tests, verified by customer.

Please merge when green/signed off and hopefully by 4pm

@safern

safern commented Oct 23, 2019

Copy link
Copy Markdown
Member

/azp run corefx-ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ahsonkhan

Copy link
Copy Markdown

@danmosemsft - I am assuming this is supposed to have "servicing-approved" label rather than "api-approved".

@ahsonkhan ahsonkhan added Servicing-approved Approved for servicing release and removed api-approved API was approved in API review, it can be implemented labels Oct 23, 2019
@danmoseley

Copy link
Copy Markdown

Thank you.

@steveharter
steveharter merged commit f91669a into dotnet:release/3.1 Oct 23, 2019
@steveharter
steveharter deleted the PortBetterDictionarySupport branch October 23, 2019 20:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.Json Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants