Fixes #237 - support multi-dimensional arrays#238
Conversation
|
@philnach & @markjbrown How do I request a review? |
|
@mlankamp, thanks for submitting the PR and reaching out. you've done the right thing by opening a PR. You could in the Reviewers panel on the PR request specific people Requesting reviews from collaborators and organization members to review your code. With that said, I'll take a look. |
philnach
left a comment
philnach
left a comment
There was a problem hiding this comment.
Could you add tests for this scenario to ensure the behavior works? Tests would go into this file: https://github.com/AzureCosmosDB/data-migration-desktop-tool/blob/main/Interfaces/Cosmos.DataTransfer.Common.UnitTests/DataItemJsonConverterTests.cs
|
Of course! Will add them tomorrow |
| } | ||
| else if (arrayItem is not string && arrayItem is IEnumerable childs) | ||
| { | ||
| WriteFieldValue(writer, "", childs, includeNullFields); |
There was a problem hiding this comment.
Thanks for the PR — adding support for nested (multi-dimensional) arrays makes sense.
That said, I’m a bit concerned about the current approach of handling nested arrays by calling:
WriteFieldValue(writer, "", childs, includeNullFields);relies on:
if (!string.IsNullOrWhiteSpace(propertyName.Value)) { writer.WriteStartArray(propertyName); }
else { writer.WriteStartArray(); }Could we refactor nested-array writing so it doesn’t depend on passing fieldName == "" and the propertyName.Value check?
For example:
introduce a dedicated helper that writes an array value (no property name), e.g. WriteArrayValue(Utf8JsonWriter writer, IEnumerable items, ...)
use that helper for nested arrays so it always calls writer.WriteStartArray() / writer.WriteEndArray()
(Optional)
The nested-array detection (arrayItem is not string && arrayItem is IEnumerable) is very broad and may accidentally treat things like byte[] as arrays of numbers. It might be worth tightening this to the specific collection types we expect to see here.
|
Hi, By any chance this PR can be merged soon? Thanks, |
|
Hi @mlankamp — thanks again for the original fix and your patience. I've opened #243 which builds on your work and addresses the two review concerns:
I also added a comprehensive test suite (17 new tests) covering the GeoJSON scenarios from #237, edge cases with nulls / mixed types / dictionaries inside nested arrays, regression guards for property-name handling, and at/over-max-depth tests for both arrays and objects. Credit goes to you — I added a cc @thucnguyen77 — this should also fix the GeoJSON scenario you mentioned. |
…sonConverter Refactor WriteFieldValue to handle nested (multi-dimensional) arrays via a dedicated WriteArrayItems / WriteArrayItemValue pair instead of recursing back into WriteFieldValue with a sentinel empty-string field name. This: - Restores GeoJSON support: Polygon/MultiPolygon `coordinates` arrays (depth 3+) now serialize correctly instead of falling back to `System.Collections.Generic.List`1[System.Object]`. - Removes the implicit `IsNullOrWhiteSpace(propertyName)` sentinel so legitimate empty-string and whitespace property names are preserved. - Adds a MaxJsonDepth (64) guard that throws InvalidOperationException for pathological / recursively nested input instead of stack-overflowing the host process. Tests: - 17 new tests in DataItemJsonConverterTests covering GeoJSON Point/LineString/ Polygon, the full issue-237 document, mixed scalars/nulls/IDataItems inside nested arrays, dictionary-of-dictionary nested arrays, wide-array smoke, empty/whitespace property-name regression guards, deserialize/re-serialize round-trip, and depth-guard at and over the limit (arrays, objects, mixed). - All 125 Common.UnitTests pass; full solution test suite is green. Supersedes AzureCosmosDB#238. Original fix authored by Mart Lankamp - thanks! Closes AzureCosmosDB#237 Co-authored-by: Mart Lankamp <mart.lankamp@users.noreply.github.com>
3 participants
Extended the
WriteFieldValuefunction to support multi-dimensional arraysFixes #237