-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add IReadOnlySet support to System.Text.Json #120306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add IReadOnlySet support to System.Text.Json #120306
Conversation
This is based on existing test cases for IReadOnlyList and ISet
I'm pretty sure this is not the right direction, but it helps me with being able to run tests against an initial working implementation I'll work on next. I'll discuss the current #IF !NETFRAMEWORK code setup with the team when the PR is ready for a first review
This seems useful for code that deserializes to a type that deserializes to a Set type that implements both interfaces.
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.
Pull Request Overview
This PR adds support for IReadOnlySet<T> serialization/deserialization to System.Text.Json. The implementation creates a new converter that uses HashSet<T> as the concrete type for IReadOnlySet<T> interface deserialization.
- Adds
IReadOnlySetOfTConverter<TCollection, TElement>converter that deserializes JSON arrays toHashSet<T>instances - Integrates the new converter into the converter factory with appropriate conditional compilation
- Adds comprehensive test coverage for both generic and object-typed
IReadOnlySet<T>scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| IReadOnlySetOfTConverter.cs | New converter implementation for IReadOnlySet<T> using HashSet<T> as backing type |
| IEnumerableConverterFactory.cs | Integrates IReadOnlySet<T> converter into the factory with conditional compilation |
| System.Text.Json.csproj | Adds the new converter file to the project |
| ThrowHelper.Serialization.cs | Fixes typo in method name from "Factorty" to "Factory" |
| JsonConverterFactory.cs | Updates method call to use corrected spelling |
| Array.ReadTests.cs | Adds test methods for IReadOnlySet<T> deserialization |
| ReferenceHandlerTests.cs | Adds JsonSerializable attributes for IReadOnlySet<T> test classes |
| TestData.cs | Includes IReadOnlySet<T> test cases in success test data |
| TestClasses.cs | Adds test class definitions and minor formatting change to dictionary initialization |
...xt.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlySetOfTConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.cs
Outdated
Show resolved
Hide resolved
...Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactory.cs
Outdated
Show resolved
Hide resolved
...xt.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlySetOfTConverter.cs
Outdated
Show resolved
Hide resolved
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 would also require updates to the source generator so IReadOnlySet is recognized and relevant code gets generated.
@eiriktsarpalis |
|
Sure, you can get started by looking at how runtime/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs Lines 844 to 848 in ca29ebb
|
Also exclude the entirety of IReadOnlySetOfTConverter by moving the #IF up.
…ck to csproj, which is what @huoyaoyuan (most likely) meant in his PR feedback. As far as I can see, there isn't a modern .NET identifier. Therefore, a check for .NETCoreApp is not enough, because that also includes < NET 5.0. Because of this, I also added a check that we are on .NET 5 or higher. Only then will the file be compiled.
…gh my editor says it is required
|
@eiriktsarpalis I've made changes to the source generators. Can you provide a check to see if this was done to your satisfaction? Also: If the unit tests pass, can I then assume the source generator implementation is succesful? I am not really sure how to test source generators this in the repo itself, so any tips on that would be great (if succesful unit tests aren't enough). Thanks for your time! |
src/libraries/System.Text.Json/tests/Common/JsonCreationHandlingTests.Generic.cs
Outdated
Show resolved
Hide resolved
I am rather sure these tests shouldn't succeed anyway. when I compare them to IReadOnlyList, (some) of these tests actually assert an exception. My goal currently is to get a successful test run, and the reviewers can tell me which tests should still be added.
…y types. Therefore, this test does not belong there.
…ssages. While ISetOfTConverter does not have this, IReadOnlyDictionaryOfTKeyTValueConverter *does*. I am not sure if there is any logic for when we do or do not include this check, but clearer exception messages seem like a plus to me.
A lot of these were part of mutable collection type tests. Previously, I added test cases for readonly collection types, and it seems like that those succeed, so I do not believe I am lowering the code coverage here.
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.
Pull Request Overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.GenericCollections.cs
Outdated
Show resolved
Hide resolved
...xt.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ReferenceHandlerTests.cs
Show resolved
Hide resolved
|
Hi @eiriktsarpalis and @huoyaoyuan ! I've spent quite a bit of time on this PR in the last 2 weeks. I am fairly certain support for IReadOnlySet is solid. When I create a little test project and run I have added tests for more in-depth coverage, but there's still some tests (especially for source generation) that fail, and I can't really figure out why. I've tried to dive deeper into these, but when I attach a debugger to the tests, nothing happens, and I've tried on WSL, Dev Container and Visual Studio. Anyway, I believe this PR is at the point where it can get a review by the team. My goal is to obtain the following:
Final question: |
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.
It's prioritized to finalize .NET 10 work in this month. I'm not a member of the team and can only provide auxiliary review.
src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Read.cs
Outdated
Show resolved
Hide resolved
|
I've been looking at the PR from @jozkee that added support for So, I'll need a PR review from a fellow contributor/team member with some guidance on how to fix the last tests. (@eiriktsarpalis ). The last 6(!) tests that fail are copies of the same 2, regarding to source generation. Some help here would be welcome. I believe it might just be related to how I set up the tests. Looking forward to continue working on this PR after that is done! |
| [JsonSerializable(typeof(ClassWithReadOnlyPropertyISetOfInt_BackedBy_StructSetOfIntWithNumberHandling))] | ||
| [JsonSerializable(typeof(ClassWithReadOnlyPropertyISetOfInt_BackedBy_StructSetOfIntWithNumberHandlingWithAttributeOnType))] | ||
| [JsonSerializable(typeof(ClassWithReadOnlyPropertyISetOfInt_BackedBy_StructSetOfIntWithNumberHandlingWithoutPopulateAttribute))] | ||
| // TODO: Should these cases also be tested (L121 - L129) for IReadOnlySet? |
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.
TODO: I'd like to check this with the team. I'll try to find some time this week/next week to write the tests to check if they succeed or not. :)
This pull request adds support for serializing and deserializing
IReadOnlySet<T>collections in theSystem.Text.Jsonlibrary. The changes include updates to the codebase to recognizeIReadOnlySet<T>as a collection type, introduce a dedicated converter for it, and provide comprehensive tests to ensure correct behavior. This enhancement aligns the library's handling ofIReadOnlySet<T>with other collection interfaces likeISet<T>andICollection<T>.Core serialization/deserialization support
IReadOnlySetOfTConverter<TCollection, TElement>to handle serialization and deserialization ofIReadOnlySet<T>, including logic to deserialize asHashSet<T>when appropriate. (src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlySetOfTConverter.cs)IReadOnlySet<T>. (src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactory.cs,src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Collections.cs) [1] [2]src/libraries/System.Text.Json/src/System.Text.Json.csproj)Type recognition and source generator integration
IReadOnlySet<T>, enabling source generator and parser logic to identify and process this collection type. (src/libraries/System.Text.Json/gen/Helpers/KnownTypeSymbols.cs,src/libraries/System.Text.Json/gen/Model/CollectionType.cs,src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs,src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs) [1] [2] [3] [4]CreateIReadOnlySetInfo<TCollection, TElement>. (src/libraries/System.Text.Json/ref/System.Text.Json.cs,src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Collections.cs) [1] [2]Test coverage
IReadOnlySet<T>, covering primitive types, nested sets, arrays, and nullable scenarios. (src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Read.cs,src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs) [1] [2] [3] [4] [5]Minor fixes
JsonConverterFactoryReturnsJsonConverterFactorty→JsonConverterFactoryReturnsJsonConverterFactory). (src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs,src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs) [1] [2]This pull request adds support for serializing and deserializingIReadOnlySet<T>collections inSystem.Text.Jsonfor .NET versions greater or equal to 5.0. The changes are carefully scoped to only apply when targeting compatible frameworks, and include updates to the source generator, converter infrastructure, and test coverage.Support for IReadOnlySet collections:
IReadOnlySetOfTConverter, to handle serialization and deserialization ofIReadOnlySet<T>types, falling back toHashSet<T>for deserialization. (src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlySetOfTConverter.cs)IReadOnlySet<T>, including new APIs likeCreateIReadOnlySetInfoand corresponding enum values and method mappings. (src/libraries/System.Text.Json/gen/Helpers/KnownTypeSymbols.cs,src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs,src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs,src/libraries/System.Text.Json/gen/Model/CollectionType.cs,src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Collections.cs,src/libraries/System.Text.Json/ref/System.Text.Json.cs) [1] [2] [3] [4] [5] [6]IReadOnlySet<T>types are automatically handled when encountered. (src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactory.cs)src/libraries/System.Text.Json/src/System.Text.Json.csproj)Test coverage for IReadOnlySet:
IReadOnlySet<T>and related scenarios, including inheritance and custom interfaces. (src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Read.cs) [1] [2] [3] [4]Minor fixes:
ReturnsJsonConverterFactortytoReturnsJsonConverterFactory. (src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs,src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs) [1] [2]Fixes #91875