SpatialPoint Serialization/Deserialization: Fixes spatial point serialization/deserialization bug#4801
SpatialPoint Serialization/Deserialization: Fixes spatial point serialization/deserialization bug#4801
Conversation
|
If you are still making the changes, you can convert this PR to "draft" mode. |
|
Looks like it needs to be rebased |
|
@dibahlfi, any updates? |
| Position min = null; | ||
| Position max = null; | ||
|
|
||
| JsonElement rootElement = JsonDocument.ParseValue(ref reader).RootElement; |
There was a problem hiding this comment.
It's different from existing Newtonsoft conversion. It seems the final form is not 1-1 JSON types conversion (like "prop" = "Value")
All converters need to exactly match existing Newton converts. And also lets please revisit the test coverage also.
There was a problem hiding this comment.
@kirankumarkolli Could you pls take a look again. I matched the STJ conversion to newtonsoft and updated the test cases.
…lization/deserilization bug
…lization/deserilization bug
|
@NaluTripician I've opened a new pull request, #5638, to work on those changes. Once the pull request is ready, I'll request review from you. |
…e instead of JsonSerializer.Deserialize(GetRawText()) in GeometrySTJConverter (#5638) All deserialization calls in `GeometrySTJConverter` were going through `JsonSerializer.Deserialize<T>(element.GetRawText(), options)`, which unnecessarily re-serializes the already-parsed `JsonElement` back to a UTF-16 string before deserializing it. ## Changes - **`GeometrySTJConverter.cs`**: Replaced all 9 occurrences with `element.Deserialize<T>(options)`, covering: - `bbox` → `BoundingBox` - `crs` → `Crs` - `coordinates`/`geometries` for all 7 geometry types (`Point`, `MultiPoint`, `LineString`, `MultiLineString`, `Polygon`, `MultiPolygon`, `GeometryCollection`) **Before:** ```csharp boundingBox = JsonSerializer.Deserialize<BoundingBox>(bboxElement.GetRawText(), options); crs = JsonSerializer.Deserialize<Crs>(crsElement.GetRawText(), options); Position pointCoordinates = JsonSerializer.Deserialize<Position>(rootElement.GetProperty("coordinates").GetRawText(), options); ``` **After:** ```csharp boundingBox = bboxElement.Deserialize<BoundingBox>(options); crs = crsElement.Deserialize<Crs>(options); Position pointCoordinates = rootElement.GetProperty("coordinates").Deserialize<Position>(options); ``` <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: NaluTripician <27316859+NaluTripician@users.noreply.github.com>
Addresses review feedback to add PatchItemAsync coverage since the original issue (#4744) specifically mentions PatchItemAsync having special handling where spatial converters were ignored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The STJ test uses JsonNamingPolicy.CamelCase, which serializes SpatialItem.Location as 'location' (lowercase) in the document. The PatchOperation path must match the serialized property name, so '/Location' is corrected to '/location'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Any reason there are not STJConverters for all of the spatial types? |
- Replace raw string literals in GeometrySTJConverter.cs with STJMetaDataFields constants - Replace custom error messages with RMResources.SpatialInvalidGeometryType - Add STJMetaDataFields.Bbox constant for GeoJSON bbox property - Revert whitespace-only changes in ClientTests.cs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All spatial types are covered. The |
Pull Request Template
Description
This PR adds System.Text.Json (STJ) serialization/deserialization support for all
Microsoft.Azure.Cosmos.Spatialgeometry types, resolving a bug where spatial types likePointcould not be serialized or deserialized when usingUseSystemTextJsonSerializerWithOptionsor a custom STJ-based serializer.closes #4744
Problem
The SDK's spatial types (
Point,LineString,Polygon, etc.) only had Newtonsoft.Json converters. When users configuredCosmosClientOptions.UseSystemTextJsonSerializerWithOptions, two failures occurred:System.NotSupportedExceptiononCreateItemAsync/ReadItemAsyncbecausePointlacks a parameterless constructor and had no STJJsonConverter.PatchItemAsyncproduced malformed JSON (e.g., raw CLR property names likeposition.longitudeinstead of GeoJSONcoordinatesarrays), because STJ fell back to default property serialization.Solution
A complete set of 8 new STJ converters mirrors the existing Newtonsoft converter architecture, producing GeoJSON-compliant output identical to the Newtonsoft serializers. Dual
[JsonConverter]attributes on each spatial model class allow the runtime to automatically select the correct converter based on the serializer in use.Design
Architecture
Key Design Decisions
JsonElementvsJToken); sharing code is not feasible.GeometrySTJConverterimplements fullWrite()GeometryJsonConverter(CanWrite = false), STJ requires explicit write logic for precise control over property order and formatting.[JsonConverter]attributes on model classes[Newtonsoft.Json.JsonConverter]and[System.Text.Json.Serialization.JsonConverter]coexist onGeometry,Position,BoundingBox,Crs, etc. The runtime picks the correct one based on which serializer is active.x.0; non-integers use"R"format for full precision. This ensuresAssert.AreEqual(newtonsoftJson, stjJson)passes in tests.STJMetaDataFieldsconstants"type","coordinates","geometries","bbox","crs", etc.) to prevent string-literal errors.GeoJSON Types Supported
All 7 GeoJSON geometry types per RFC 7946:
"geometries"key)CRS (Coordinate Reference System) Handling
{ "type": "name", "properties": { "name": "EPSG:4326" } }{ "type": "link", "properties": { "href": "...", "type": "..." } }nullType Discrimination
The
GeometrySTJConverteruses the"type"field in the GeoJSON payload to determine whichGeometrysubclass to instantiate during deserialization, mirroring the existingGeometryJsonConverterpattern.Type of change
Changes
New Files (8)
Spatial/Converters/STJConverters/GeometrySTJConverter.cs— polymorphic converter handling all geometry typesSpatial/Converters/STJConverters/PositionSTJConverter.cs—[lon, lat, alt?]array converterSpatial/Converters/STJConverters/BoundingBoxSTJConverter.cs— flat min/max coordinate arraySpatial/Converters/STJConverters/CrsSTJConverter.cs— Named, Linked, and Unspecified CRSSpatial/Converters/STJConverters/LinearRingSTJConverter.cs— closed position ringSpatial/Converters/STJConverters/LineStringCoordinatesSTJConverter.cs— multi-line coordinatesSpatial/Converters/STJConverters/PolygonCoordinatesSTJConverter.cs— multi-polygon coordinatesSpatial/Converters/STJConverters/STJMetaDataFields.cs— GeoJSON property-name constantsModified Files (17)
Point.cs,Position.cs,Geometry.cs,BoundingBox.cs,Crs.cs,LineString.cs,Polygon.cs,MultiPoint.cs,MultiLineString.cs,MultiPolygon.cs,GeometryCollection.cs,LinearRing.cs,LineStringCoordinates.cs,PolygonCoordinates.cs) — added[System.Text.Json.Serialization.JsonConverter]attributeCrsJsonConverter.cs— minor cleanupTest Files (2)
STJSpatialTest.cs— ~20+ test methods covering all geometry types, CRS variants, bounding boxes, additional properties, and round-trip fidelity (verifies STJ output exactly matches Newtonsoft output)ClientTests.cs— emulator integration tests for spatial types with STJ serialization