Enhance ModelReaderWriter with collection support #48102
Conversation
Updated ModelReaderWriter.cs to improve reading and writing functionality for various collection types, including List<> and Dictionary<>. Added error handling for unsupported types and enhanced exception messages. Updated ModelReaderWriterTests.cs with new test cases to validate the changes, covering edge cases and empty collections. Added JSON test data for comprehensive testing of availability set data. Included a new package reference for System.Memory.Data in project files. Minor corrections made to comments and variable names for clarity.
|
|
||
| private static readonly HashSet<Type> s_supportedCollectionTypes = | ||
| [ | ||
| typeof(List<>), |
There was a problem hiding this comment.
Should this use IList<> and IDictionary<>?
There was a problem hiding this comment.
we need concrete constructable types not interfaces here
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| @@ -12,6 +15,12 @@ namespace System.ClientModel.Primitives; | |||
| /// </summary> | |||
| public static class ModelReaderWriter | |||
| { | |||
| private static readonly HashSet<Type> s_supportedCollectionTypes = | |||
There was a problem hiding this comment.
Do we expect the supported types to grow over time? With just a few elements an iterative lookup can be faster than the hash.
There was a problem hiding this comment.
I think it would be easy to add things like Collection, SortedList, etc if we wanted to and this just centralized the checking to make it simpler than a series of if/else statements in multiple places.
Still in discussion on which types to support for output, but if we end up with two we can optimize this with a method call that does an iterative lookup.
There was a problem hiding this comment.
How does this work for ReadOnlyCollection<T> (or whatever type we instantiate for an IReadOnlyList<T> output collection)? Do we need to update this with every possible collection type we return? How does it work for internal shared source types like ChangeTrackingList<T>?
There was a problem hiding this comment.
For output (Read) the idea is we are limiting you to a set collection types (for input (Write) it should be unlimited). Right now if you wanted a ChangeTrackingList you would do this
ChangeTrackingList<MyType> list = new(ModelReaderWriter.Read<List<MyType>>(json));For ReadOnlycollection you would do
ReadOnlycollection<MyType> readonlyList = ModelReaderWriter.Read<List<MyType>>(json).AsReadOnly();We can add support to do this internally, but we would need to do something special for each of these that we want to add so its a balance of how many types we think should be in this supported list.
Updated ModelReaderWriter.cs to improve reading and writing functionality for various collection types, including List<> and Dictionary<>. Added error handling for unsupported types and enhanced exception messages.
Updated ModelReaderWriterTests.cs with new test cases to validate the changes, covering edge cases and empty collections. Added JSON test data for comprehensive testing of availability set data.
Included a new package reference for System.Memory.Data in project files. Minor corrections made to comments and variable names for clarity.
Revival of this old PR #40598
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.