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

System.Text.Json - Don't add to readonly collections - Take 2#40366

Merged
ahsonkhan merged 4 commits into
dotnet:masterfrom
khellang:no-readonly-add
Aug 20, 2019
Merged

System.Text.Json - Don't add to readonly collections - Take 2#40366
ahsonkhan merged 4 commits into
dotnet:masterfrom
khellang:no-readonly-add

Conversation

@khellang

@khellang khellang commented Aug 16, 2019

Copy link
Copy Markdown
Member

layomia
layomia suggested changes Aug 16, 2019
View reviewed changes
Comment thread src/System.Text.Json/tests/Serialization/Value.ReadTests.GenericCollections.cs Outdated
@bartonjs bartonjs added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Aug 16, 2019
@layomia layomia added this to the 5.0 milestone Aug 17, 2019
ahsonkhan
ahsonkhan reviewed Aug 20, 2019
View reviewed changes
Comment thread src/System.Text.Json/tests/Serialization/DictionaryTests.cs
ahsonkhan
ahsonkhan reviewed Aug 20, 2019
View reviewed changes
Comment thread src/System.Text.Json/tests/Serialization/TestClasses.GenericCollections.cs
@layomia

layomia commented Aug 20, 2019

Copy link
Copy Markdown
Contributor

how's the code coverage for these new branches?

100% sequence and branch coverage for the new branches, and the entire methods.
report.zip

@layomia layomia self-requested a review August 20, 2019 04:27
@ahsonkhan ahsonkhan requested a review from steveharter August 20, 2019 05:46
@ahsonkhan

ahsonkhan commented Aug 20, 2019

Copy link
Copy Markdown

LGTM. Since this is a breaking change, we need to port this to 3.0. @layomia - can you cherry-pick this change and submit a PR for this?

cc @ericstj

@ahsonkhan ahsonkhan merged commit f66beee into dotnet:master Aug 20, 2019
@khellang khellang deleted the no-readonly-add branch August 20, 2019 06:43
khellang added a commit to khellang/corefx that referenced this pull request Aug 20, 2019
@khellang
System.Text.Json - Don't add to readonly collections - Take 2 (dotnet…
0dd223e 
…#40366)

* Don't add to collection when IsReadOnly is true

* Throw NotSupportedException instead of JsonException

* Add test for readonly collections

* Revert back to InvalidCastException
This was referenced Aug 20, 2019
Merged
Merged
ericstj pushed a commit that referenced this pull request Aug 20, 2019
@khellang @ericstj
[release/3.0] Honor IsReadOnly when deserializing collections (#40438)
286e1df 
* System.Text.Json - Don't add to readonly collections - Take 2 (#40366)

* Don't add to collection when IsReadOnly is true

* Throw NotSupportedException instead of JsonException

* Add test for readonly collections

* Revert back to InvalidCastException

* Fix failing System.Text.Json test (#40439)
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@khellang @ahsonkhan
System.Text.Json - Don't add to readonly collections - Take 2 (dotnet…
31a9d03 
…/corefx#40366)

* Don't add to collection when IsReadOnly is true

* Throw NotSupportedException instead of JsonException

* Add test for readonly collections

* Revert back to InvalidCastException


Commit migrated from dotnet/corefx@f66beee
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@steveharter steveharter Awaiting requested review from steveharter
2 more reviewers
@ahsonkhan ahsonkhan ahsonkhan left review comments
@layomia layomia layomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

Serializer should throw NotSupportedException when collections can be instantiated but not populated

5 participants

@khellang @layomia @ahsonkhan @bartonjs @Dotnet-GitSync-Bot