Request for comment: System.Text.Json compatibility by arendvw · Pull Request #10 · bertt/cityjson

arendvw · 2025-12-23T16:16:49Z

Since cityjson files tend to be huge and system.text.json promises to be more performant in many places, here's a refactor of the code that uses system.text.json.

At this point, all unit tests run ok, but there are breaking changes in the api or usage.

Most prominently:

System.Text.JSON is case sensitive by default, and Newtonsoft is not
Decoding requires non-case sensitivity passed as options
Marking things as required has a different impact

TODO:

Benchmarking to see if it actually matters
More thorough testing.

So the main question: is System.Text.Json interesting to you as well? There were also other clear reasons, upstream or downstream, to choose Newtonsoft.Json over stj.

Happy holidays!

- Updated to .netstandard 2.0 for cityjson - Added small fix for net48 compatiblity in cj2glb.csproj - Note: cj2glb is not compatible with net48 because of signing issues with SharpGLTF and AnyOf (SharpGLTF is signed, AnyOf is not), but the code on this project's side is fully compatible if built against a locally checked out Gltf project

System.Text.JSON behaves a bit differently and does not allow global overrides, so deserialization should be done with CityJsonSystemTextJson.Options. All tests run completely, but there might be other trouble.

bertt · 2025-12-23T19:53:52Z

yes interested in this, I shall add a basic benchmark test so we can do a comparison

Copilot

Pull request overview

Refactors the CityJSON library and related tools/tests to use System.Text.Json instead of Newtonsoft.Json for (de)serialization, aiming for better performance and aligning behavior via shared serializer options.

Changes:

Replace Newtonsoft.Json usage across src/, tests/, and tools/ with System.Text.Json, including new STJ converters/options.
Update model annotations to STJ attributes (JsonPropertyName, JsonRequired) and refactor custom converters to STJ (Utf8JsonReader/Utf8JsonWriter).
Minor tool/test/project updates (dependency bump, net48 dev note, .gitignore, test csproj cleanup).

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tools/cj2glb/cj2glb.csproj	Bumps triangulator dependency version.
tools/cj2glb/TexturedGltfCreator.cs	Adds TF-specific `Vector2` construction to support older targets.
tools/cj2glb/README.net48.md	Documents .NET Framework 4.8 strong-name runtime issue/workaround.
tools/cj2glb/Program.cs	Switches CityJSON read + tileset write paths to `System.Text.Json`.
tools/cj2glb.tests/UnitTest1.cs	Updates cj2glb tests to deserialize with STJ options.
tests/tests.csproj	Updates language version settings (currently duplicated).
tests/SimpleGeometriesTests.cs	Switches deserialization to STJ options.
tests/CityJsonWriteTests.cs	Switches read/write roundtrip tests to STJ options.
tests/CityJsonTests.cs	Switches deserialization to STJ options and adjusts an attribute assertion for STJ value types.
src/cityjson.csproj	Replaces Newtonsoft.Json package with `System.Text.Json`.
src/Vertex.cs	Updates usings for STJ + keeps custom converter via attribute.
src/Transform.cs	Migrates JSON attributes to STJ (`JsonPropertyName`, `JsonRequired`).
src/Texture.cs	Migrates JSON attributes to STJ (`JsonPropertyName`, `JsonRequired`).
src/Geometry/SolidGeometry.cs	Migrates `texture` JSON property naming to STJ.
src/Geometry/MultiSurfaceGeometry.cs	Migrates `texture` JSON property naming to STJ.
src/Geometry/MultiSolidGeometry.cs	Migrates `texture` JSON property naming to STJ.
src/Geometry/Geometry.cs	Switches to STJ and keeps polymorphic converter via `[JsonConverter]`.
src/Geometry/CompositeSurfaceGeometry.cs	Migrates `texture` JSON property naming to STJ.
src/Geometry/CompositeSolidGeometry.cs	Migrates `texture` JSON property naming to STJ.
src/Convertors/VertexConvertor.cs	Rewrites `VertexConverter` from Newtonsoft to STJ.
src/Convertors/GeometryConverter.cs	Rewrites polymorphic geometry converter from Newtonsoft to STJ.
src/CityObject.cs	Replaces `JToken` address with `JsonNode?`.
src/CityJsonWriter.cs	Replaces Newtonsoft-based writer with STJ options.
src/CityJsonSystemText.cs	Introduces shared STJ `JsonSerializerOptions`.
src/CityJsonSeqWriter.cs	Replaces Newtonsoft-based JSON-seq writer with STJ options.
src/CityJsonSeqReader.cs	Replaces Newtonsoft-based JSON-seq reader with STJ options.
src/Appearance.cs	Migrates `vertices-texture` JSON property naming to STJ.
.gitignore	Ignores `.idea/*`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-27T21:49:12Z

@@ -0,0 +1,19 @@
+# .NET Framework 4.8 Runtime issue
+Note, this tool does not need to be compiled against net48, and is not released as such, 
+this is purely for development or other adoptation purposes.


Typo in documentation: "adoptation" should be "adaptation".

Suggested change

this is purely for development or other adoptation purposes.

this is purely for development or other adaptation purposes.

Copilot · 2026-02-27T21:49:13Z

+        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
+        Converters =
+        {
+            new JsonStringEnumConverter(JsonNamingPolicy.CamelCase, allowIntegerValues: true)


JsonStringEnumConverter(JsonNamingPolicy.CamelCase, ...) will serialize enums as camelCase (e.g., buildingInstallation), while the fixtures and typical CityJSON use PascalCase values (e.g., BuildingInstallation). If this options instance is intended for writing as well, consider using the default naming (no policy) to preserve casing, while keeping case-insensitive reads.

Suggested change

new JsonStringEnumConverter(JsonNamingPolicy.CamelCase, allowIntegerValues: true)

new JsonStringEnumConverter(allowIntegerValues: true)

Copilot · 2026-02-27T21:49:13Z

    {
-        Formatting = Formatting.Indented,
-        ContractResolver = new DefaultContractResolver
-        {
-            NamingStrategy = new CamelCaseNamingStrategy
-            {
-                ProcessDictionaryKeys = false,
-                OverrideSpecifiedNames = false
-            }
-        },
-        NullValueHandling = NullValueHandling.Ignore
+        WriteIndented = true,                         // Formatting.Indented
+        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
+        // DictionaryKeyPolicy = null;                // keep dictionary keys unchanged
+        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull


CityJsonWriter defines its own JsonSerializerOptions instead of reusing CityJsonSystemTextJson.Options, which can lead to subtle serialization differences (notably enum handling via JsonStringEnumConverter, trailing commas tolerance, case-insensitive reads, etc.). Consider reusing the shared options (or explicitly mirroring the relevant settings/converters) so read/write behavior stays consistent across the library.

Copilot · 2026-02-27T21:49:13Z

    {
-        Formatting = Formatting.None,
-        ContractResolver = new DefaultContractResolver
-        {
-            NamingStrategy = new CamelCaseNamingStrategy
-            {
-                ProcessDictionaryKeys = false,
-                OverrideSpecifiedNames = false
-            }
-        },
-        NullValueHandling = NullValueHandling.Ignore
+        WriteIndented = false,                       // Formatting.None
+        PropertyNamingPolicy = JsonNamingPolicy.CamelCase, // CamelCaseNamingStrategy
+        // DictionaryKeyPolicy = null;               // keep dictionary keys unchanged (matches ProcessDictionaryKeys = false)
+        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull // NullValueHandling.Ignore


CityJsonSeqWriter duplicates JsonSerializerOptions that may drift from the library’s shared CityJsonSystemTextJson.Options (e.g., enum string handling). Consider reusing the shared options (or a shared base options factory) to keep JSON output consistent across writers.

Copilot · 2026-02-27T21:49:14Z

+                    Boundaries = ReadRequired(root, "boundaries").Deserialize<int[][][][][]>(options)
+                                 ?? throw new JsonException("Invalid 'boundaries' for CompositeSolid.")
+                },
+                _ => null


Read(...) returns null for unknown geometry type (_ => null). This can lead to returning a null Geometry instance (and a subsequent NullReferenceException when lod is set) instead of failing fast. Consider throwing a JsonException for unsupported types so deserialization errors are explicit and non-nullable return contracts are honored.

Suggested change

_ => null

_ => throw new JsonException($"Unsupported geometry type '{type}'.")

Copilot · 2026-02-27T21:49:14Z

+            if (root.TryGetProperty("lod", out var lodEl) && lodEl.ValueKind != JsonValueKind.Null)
            {
-                case "solid":
-                    {
-                        var boundaries = jsonObject["boundaries"].ToObject<int[][][][]>();
-                        obj = new SolidGeometry() { Type = GeometryType.Solid, Boundaries = boundaries };
-
-                        if (textureObject != null)
-                        {
-                            ((SolidGeometry)obj).Texture = ReadTextureSolid(jsonObject["texture"].ToObject<Dictionary<string, object>>());
-                        }
-                        break;
-                    }
-
-                case "compositesurface":
-                    {
-                        var boundaries = jsonObject["boundaries"].ToObject<int[][][]>();
-                        obj = new CompositeSurfaceGeometry() { Type = GeometryType.CompositeSurface, Boundaries = boundaries };
-                        break;
-                    }
-
-                case "multisurface":
-                    {
-                        var boundaries = jsonObject["boundaries"].ToObject<int[][][]>();
-                        obj = new MultiSurfaceGeometry() { Type = GeometryType.MultiSurface, Boundaries = boundaries };
-                        if(textureObject != null)
-                        {
-                            ((MultiSurfaceGeometry)obj).Texture = ReadTexture(jsonObject["texture"].ToObject<Dictionary<string, object>>());
-                        }   
-                        break;
-                    }
-                case "multisolid":
-                    {
-                        var boundaries = jsonObject["boundaries"].ToObject<int[][][][][]>();
-                        obj = new MultiSolidGeometry() { Type = GeometryType.MultiSolid, Boundaries = boundaries };
-                        break;
-                    }
-                case "compositesolid":
-                    {
-                        var boundaries = jsonObject["boundaries"].ToObject<int[][][][][]>();
-                        obj = new CompositeSolidGeometry() { Type = GeometryType.CompositeSolid, Boundaries = boundaries };
-                        break;
-                    }
-
-            }
-
-            if (jsonObject["lod"] != null)
-            {
-                obj.Lod = jsonObject["lod"].ToString();
+                obj.Lod = lodEl.ValueKind switch
+                {
+                    JsonValueKind.String => lodEl.GetString(),
+                    JsonValueKind.Number => lodEl.GetRawText(), // "2"
+                    _ => throw new JsonException("'lod' must be string or number.")
+                };


obj.Lod = ... assumes obj is non-null, but obj can be null if an unsupported geometry type is encountered. After changing the default case to throw, this becomes safe; otherwise add a null-check/throw before assigning Lod.

Copilot · 2026-02-27T21:49:14Z

        while (!reader.EndOfStream)
        {
            var line = reader.ReadLine();
-            var cityJsonSecond = JsonConvert.DeserializeObject<CityJsonDocument>(line);
+            var cityJsonSecond = JsonSerializer.Deserialize<CityJsonDocument>(line, CityJsonSystemTextJson.Options);
            result.Add(cityJsonSecond);


StreamReader isn’t disposed, and JsonSerializer.Deserialize(...) can return null (or ReadLine() can return null/empty), which will lead to nulls in the returned list or a NullReferenceException later. Use a using/using var for the reader and skip/guard empty lines; also validate the deserialize result before adding to result.

Copilot · 2026-02-27T21:49:14Z

-
    <IsPackable>false</IsPackable>
-
+    <LangVersion>latest</LangVersion> 


There are two <LangVersion>latest</LangVersion> entries in the same PropertyGroup, and one includes trailing whitespace. Remove the duplicate to avoid confusion about which value should apply.

Suggested change

<LangVersion>latest</LangVersion>

arendvw added 2 commits December 23, 2025 15:09

Experimental system.text.json port of bertt's cityjson library.

913703d

System.Text.JSON behaves a bit differently and does not allow global overrides, so deserialization should be done with CityJsonSystemTextJson.Options. All tests run completely, but there might be other trouble.

Merge branch 'master' into feature/system-text-json

fe82488

bertt requested a review from Copilot February 27, 2026 21:44

Copilot started reviewing on behalf of bertt February 27, 2026 21:44 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request for comment: System.Text.Json compatibility#10

Request for comment: System.Text.Json compatibility#10
arendvw wants to merge 3 commits intobertt:masterfrom
arendvw:feature/system-text-json

arendvw commented Dec 23, 2025

Uh oh!

bertt commented Dec 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Copilot AI Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	this is purely for development or other adoptation purposes.
	this is purely for development or other adaptation purposes.

	new JsonStringEnumConverter(JsonNamingPolicy.CamelCase, allowIntegerValues: true)
	new JsonStringEnumConverter(allowIntegerValues: true)

	_ => null
	_ => throw new JsonException($"Unsupported geometry type '{type}'.")


		<IsPackable>false</IsPackable>

		<LangVersion>latest</LangVersion>

Conversation

arendvw commented Dec 23, 2025

Uh oh!

bertt commented Dec 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants