From c72ce721c4539c3479286f332c1e95e7853c8314 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:07:27 +0000 Subject: [PATCH 1/7] Move the editor onto ktsu.UndoRedo 3 so the ImGui 3.75 packages build ktsu.ImGui.NodeEditor 3.75 depends on ktsu.UndoRedo 3.4, which absorbed ktsu.UndoRedo.Core. Referencing both put every undo type in the editor's compilation twice (CS0433), which is why the NodeEditor bump has been red. The namespaces and API are unchanged, so swapping the reference is enough. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- Directory.Packages.props | 4 ++-- Schema.Editor/Schema.Editor.csproj | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 6422e4f..97d755c 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -15,7 +15,7 @@ - + @@ -25,7 +25,7 @@ - + diff --git a/Schema.Editor/Schema.Editor.csproj b/Schema.Editor/Schema.Editor.csproj index e02b24f..033699c 100644 --- a/Schema.Editor/Schema.Editor.csproj +++ b/Schema.Editor/Schema.Editor.csproj @@ -26,7 +26,7 @@ - + From a35b467549ae2339344f0098c942aa2191fb15e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:10:55 +0000 Subject: [PATCH 2/7] Save schemas through a temporary file so a failed write keeps the last save TrySave wrote over the target with File.WriteAllText, which truncates before writing, so a full disk or a dropped share left the only saved copy empty or partial. The content now goes to a temporary file beside the target and is swapped in with File.Replace (or File.Move when there is nothing to replace); the temporary file is removed on any failure. Fixes #292. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- Schema.Editor.Test/SchemaFileTests.cs | 66 +++++++++++++++++++++++++++ Schema.Editor/SchemaFile.cs | 55 +++++++++++++++++++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/Schema.Editor.Test/SchemaFileTests.cs b/Schema.Editor.Test/SchemaFileTests.cs index b6e9184..96275c2 100644 --- a/Schema.Editor.Test/SchemaFileTests.cs +++ b/Schema.Editor.Test/SchemaFileTests.cs @@ -72,6 +72,72 @@ public void SavingCreatesTheDirectoryTheFileGoesIn() Assert.IsTrue(File.Exists(path)); } + /// + /// A write that fails partway leaves the previous save as it was. Writing straight over the target + /// truncated it first, so a full disk or a dropped share destroyed the only saved copy. + /// + [TestMethod] + public void AFailedWriteLeavesThePreviousSaveIntact() + { + AbsoluteFilePath path = ScratchFile("kept.schema.json"); + Schema original = new(); + original.AddClass("User".As()); + Assert.IsTrue(SchemaFile.TrySave(original, path)); + byte[] before = File.ReadAllBytes(path); + + Schema edited = new(); + edited.AddClass("Order".As()); + bool saved = SchemaFile.TrySave(edited, path, static (target, content) => + { + File.WriteAllText(target, content[..(content.Length / 2)]); + throw new IOException("The disk is full."); + }); + + Assert.IsFalse(saved); + Assert.AreSequenceEqual(before, File.ReadAllBytes(path)); + } + + /// + /// The temporary file a failed save wrote into is removed, so failures do not litter the folder + /// the schema lives in. + /// + [TestMethod] + public void AFailedWriteLeavesNoTemporaryFileBehind() + { + AbsoluteFilePath path = ScratchFile("tidy.schema.json"); + + bool saved = SchemaFile.TrySave(new Schema(), path, static (target, content) => + { + File.WriteAllText(target, content); + throw new IOException("The share went away."); + }); + + Assert.IsFalse(saved); + Assert.HasCount(0, Directory.GetFiles(scratchDirectory)); + } + + /// + /// Saving over an existing file replaces its content and leaves only that file in the folder. + /// + [TestMethod] + public void SavingOverAnExistingFileReplacesIt() + { + AbsoluteFilePath path = ScratchFile("replaced.schema.json"); + Schema first = new(); + first.AddClass("User".As()); + Schema second = new(); + second.AddClass("Order".As()); + + Assert.IsTrue(SchemaFile.TrySave(first, path)); + Assert.IsTrue(SchemaFile.TrySave(second, path)); + Assert.IsTrue(SchemaFile.TryLoad(path, out Schema? loaded)); + + Assert.IsNotNull(loaded); + Assert.IsNotNull(loaded.GetClass("Order".As())); + Assert.IsNull(loaded.GetClass("User".As())); + Assert.HasCount(1, Directory.GetFiles(scratchDirectory)); + } + [TestMethod] public void SavingWithNoPathFails() => Assert.IsFalse(SchemaFile.TrySave(new Schema(), new AbsoluteFilePath())); diff --git a/Schema.Editor/SchemaFile.cs b/Schema.Editor/SchemaFile.cs index b8b7666..0c63521 100644 --- a/Schema.Editor/SchemaFile.cs +++ b/Schema.Editor/SchemaFile.cs @@ -85,7 +85,23 @@ public static SchemaLoadResult Load(AbsoluteFilePath filePath) /// The schema to save. /// The path to save the schema to. /// True if the schema was saved successfully, false otherwise. - public static bool TrySave(Schema schema, AbsoluteFilePath filePath) + public static bool TrySave(Schema schema, AbsoluteFilePath filePath) => + TrySave(schema, filePath, File.WriteAllText); + + /// + /// Saves a Schema to a file path, writing the content through . + /// + /// + /// The content goes to a temporary file beside the target and is swapped into place only once it + /// has been written whole, so a write that fails partway - a full disk, a dropped network share - + /// leaves the previous save intact rather than truncated. is the seam + /// a test uses to make that write fail; it is only ever given the temporary path. + /// + /// The schema to save. + /// The path to save the schema to. + /// Writes the given text to the given path. + /// True if the schema was saved successfully, false otherwise. + internal static bool TrySave(Schema schema, AbsoluteFilePath filePath, Action writeFile) { if (schema is null || string.IsNullOrEmpty(filePath)) { @@ -102,7 +118,7 @@ public static bool TrySave(Schema schema, AbsoluteFilePath filePath) Directory.CreateDirectory(directory); } - File.WriteAllText(filePath, json); + WriteAtomically(filePath, json, writeFile); return true; } catch (JsonException) @@ -116,4 +132,39 @@ public static bool TrySave(Schema schema, AbsoluteFilePath filePath) return false; } + + /// + /// Writes to a temporary file in the target's directory, then moves it + /// over the target, so the target only ever holds a complete previous save or a complete new one. + /// + /// + /// The temporary file is in the same directory so the final step is a rename on one volume rather + /// than a copy. It is deleted on any failure, which is what the finally is for: an + /// exception the caller does not catch still must not leave it behind. + /// + private static void WriteAtomically(string targetPath, string content, Action writeFile) + { + string temporaryPath = $"{targetPath}.tmp-{Guid.NewGuid():N}"; + + try + { + writeFile(temporaryPath, content); + + if (File.Exists(targetPath)) + { + File.Replace(temporaryPath, targetPath, destinationBackupFileName: null); + } + else + { + File.Move(temporaryPath, targetPath); + } + } + finally + { + if (File.Exists(temporaryPath)) + { + File.Delete(temporaryPath); + } + } + } } From b5684b522d382b1d09065b4fe8088c56643746ff Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:13:47 +0000 Subject: [PATCH 3/7] Refuse schema documents with root properties the model does not read A PascalCase or legacy file loaded as an empty schema and was reported as a successful migration from format version 0, so validation passed and saving from the editor replaced the file with nothing. Load now refuses any root property the serializer's own contract does not name, and says so when the only problem is casing. Fixes #278 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- Schema.Test/SchemaFormatVersionTests.cs | 73 +++++++++++++++++++++++++ Schema/SchemaSerializer.cs | 52 ++++++++++++++++++ docs/schema-format.md | 7 +++ 3 files changed, 132 insertions(+) diff --git a/Schema.Test/SchemaFormatVersionTests.cs b/Schema.Test/SchemaFormatVersionTests.cs index ce0a1c5..a636c0f 100644 --- a/Schema.Test/SchemaFormatVersionTests.cs +++ b/Schema.Test/SchemaFormatVersionTests.cs @@ -165,4 +165,77 @@ public void TestDocumentedRootPropertiesAreAllPresent() Assert.IsTrue(json.Contains($"\"{property}\"", StringComparison.Ordinal), $"Root property '{property}' is missing from {json}"); } } + + /// + /// A file whose root the model does not read is refused rather than loaded as an empty schema. + /// + /// + /// Loading the legacy files reported "Migrated from format version 0" with nothing in the + /// schema, so validation passed and saving from the editor replaced the file with an empty one. + /// + [TestMethod] + public void TestALegacyFileIsRefusedRatherThanLoadedEmpty() + { + string path = Path.Join(LegacySchemaMigrationTests.SamplesDirectory, "legacy", "dungeoneer", "weapon.schema.json"); + + SchemaLoadResult result = SchemaSerializer.Load(File.ReadAllText(path)); + + Assert.AreEqual(SchemaLoadStatus.InvalidJson, result.Status, result.Message); + Assert.IsNull(result.Schema); + Assert.Contains("'Classes'", result.Message); + Assert.Contains("'classes'", result.Message); + } + + /// + /// No committed legacy file loads as a success, so none of them can be mistaken for an empty schema. + /// + [TestMethod] + public void TestNoLegacyFileLoads() + { + string[] files = Directory.GetFiles(Path.Join(LegacySchemaMigrationTests.SamplesDirectory, "legacy"), "*.schema.json", SearchOption.AllDirectories); + Assert.IsNotEmpty(files); + + foreach (string file in files) + { + Assert.IsFalse(SchemaSerializer.Load(File.ReadAllText(file)).IsSuccess, file); + } + } + + /// + /// A root property with no camelCase counterpart is named without a casing hint. + /// + [TestMethod] + public void TestAnUnknownRootPropertyIsNamed() + { + SchemaLoadResult result = SchemaSerializer.Load("""{ "formatVersion": 1, "widgets": [] }"""); + + Assert.AreEqual(SchemaLoadStatus.InvalidJson, result.Status); + Assert.Contains("'widgets'", result.Message); + Assert.DoesNotContain("camelCase", result.Message); + } + + /// + /// A file from a newer version is still told apart from one this build cannot read, even when + /// the newer version added a root property this build has never heard of. + /// + [TestMethod] + public void TestAFutureFileWithANewRootPropertyIsStillReportedAsFuture() + { + SchemaLoadResult result = SchemaSerializer.Load($$"""{ "formatVersion": {{Schema.CurrentFormatVersion + 1}}, "somethingNew": [] }"""); + + Assert.AreEqual(SchemaLoadStatus.UnsupportedFutureVersion, result.Status); + } + + /// + /// Every committed modern sample still loads, so the root check refuses nothing the format allows. + /// + [TestMethod] + public void TestEveryModernSampleStillLoads() + { + foreach (string file in Directory.GetFiles(LegacySchemaMigrationTests.SamplesDirectory, "*.schema.json")) + { + SchemaLoadResult result = SchemaSerializer.Load(File.ReadAllText(file)); + Assert.IsTrue(result.IsSuccess, $"{file}: {result.Message}"); + } + } } diff --git a/Schema/SchemaSerializer.cs b/Schema/SchemaSerializer.cs index addcecf..a8f5242 100644 --- a/Schema/SchemaSerializer.cs +++ b/Schema/SchemaSerializer.cs @@ -176,6 +176,17 @@ public static SchemaLoadResult Load(string json) }; } + string? unrecognised = FindUnrecognisedRootProperty(json); + if (unrecognised is not null) + { + return new() + { + Status = SchemaLoadStatus.InvalidJson, + FileFormatVersion = declaredVersion, + Message = DescribeUnrecognisedRootProperty(unrecognised), + }; + } + try { Schema? schema = JsonSerializer.Deserialize(json, JsonOptions); @@ -213,6 +224,47 @@ public static SchemaLoadResult Load(string json) } } + /// + /// The names a schema document's root may hold, as the serializer itself spells them. + /// + /// + /// Read from the serializer's own contract rather than listed here, so a property added to + /// is accepted the moment it can be read. + /// + private static readonly Lazy> RootPropertyNames = new(() => + [.. JsonOptions.GetTypeInfo(typeof(Schema)).Properties.Select(property => property.Name)]); + + /// + /// Finds the first property at the document root that the model does not read, if there is one. + /// + /// + /// The serializer skips a property it does not recognise, so a document written with + /// Classes rather than classes used to load as an empty schema reported as a + /// successful migration, and saving it from the editor then replaced the file with nothing. + /// Refusing the document is what keeps a file this build cannot read from being taken for an + /// empty one. A file from a newer version is refused before this runs, with its own message, + /// so an unrecognised property here is never one a newer writer added. + /// + private static string? FindUnrecognisedRootProperty(string json) + { + using JsonDocument document = JsonDocument.Parse(json); + HashSet known = RootPropertyNames.Value; + + return document.RootElement.EnumerateObject() + .Select(property => property.Name) + .FirstOrDefault(name => !known.Contains(name)); + } + + private static string DescribeUnrecognisedRootProperty(string name) + { + string? differentlyCased = RootPropertyNames.Value + .FirstOrDefault(known => string.Equals(known, name, StringComparison.OrdinalIgnoreCase)); + + return differentlyCased is null + ? $"The schema has a property '{name}' that this build does not recognise, so it cannot be read without losing data." + : $"The schema has a property '{name}' that this build does not recognise: property names are camelCase, so it would be '{differentlyCased}'."; + } + /// /// Reads just the format version from the document root. /// diff --git a/docs/schema-format.md b/docs/schema-format.md index e7d7e15..883f022 100644 --- a/docs/schema-format.md +++ b/docs/schema-format.md @@ -36,6 +36,13 @@ lose information. All six collections are always written, empty or not. +Property names are camelCase and matched exactly. A file whose root holds a property this list does +not name - `Classes` rather than `classes`, say, or anything a hand edit added - is refused rather +than loaded with that property skipped, because loading it would look like a successful load of a +schema with something missing, and saving it would then write the omission back over the file. A +file declaring a newer `formatVersion` is refused as newer before this check runs, so a property a +later version added is reported as a version this build cannot read rather than as an unknown name. + Every named element also carries a `description`, a free-text string that is always written even when empty. It is the natural source for a doc comment in generated code. From 52855ce4d179d746dee347f45afbda82c25cf736 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:16:01 +0000 Subject: [PATCH 4/7] Refuse a null in the C++ options file instead of throwing {"existingTypes": null}, or a null entry in it, reached IsSpelled and threw a NullReferenceException with nothing pointing at the file. The options now respect nullable annotations, so a null where a value is needed is refused by the deserializer, and a null existing-type entry is refused by name. Fixes #291 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- .../CppGeneratorOptionsFileTests.cs | 38 +++++++++++++++++++ Schema.Cpp/CppGeneratorOptionsFile.cs | 10 ++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/Schema.Cpp.Test/CppGeneratorOptionsFileTests.cs b/Schema.Cpp.Test/CppGeneratorOptionsFileTests.cs index 0e7c9ac..eca90fb 100644 --- a/Schema.Cpp.Test/CppGeneratorOptionsFileTests.cs +++ b/Schema.Cpp.Test/CppGeneratorOptionsFileTests.cs @@ -148,6 +148,44 @@ public void RefusesAnExistingTypeWithNoName() Assert.Contains("Kilograms", message, StringComparison.Ordinal); } + /// + /// A null in place of the whole table is refused by name rather than escaping as a + /// NullReferenceException. + /// + [TestMethod] + public void RefusesANullExistingTypesTable() + { + const string json = """{ "existingTypes": null }"""; + + Assert.IsFalse(CppGeneratorOptionsFile.TryParse(json, out CppGeneratorOptions? _, out string? message)); + Assert.Contains("existingTypes", message, StringComparison.Ordinal); + } + + /// The same, for one entry in the table. + [TestMethod] + public void RefusesANullExistingType() + { + const string json = """{ "existingTypes": { "Kilograms": null } }"""; + + Assert.IsFalse(CppGeneratorOptionsFile.TryParse(json, out CppGeneratorOptions? _, out string? message)); + Assert.Contains("existingTypes.Kilograms", message, StringComparison.Ordinal); + } + + /// + /// A null anywhere else the options need a value is refused too, so no other key reaches the + /// generator as a null it was never written to expect. + /// + [TestMethod] + [DataRow("""{ "generatedBy": null }""", "generatedBy")] + [DataRow("""{ "headerExtension": null }""", "headerExtension")] + [DataRow("""{ "vector3": { "name": "holo::Vec3", "include": null } }""", "include")] + [DataRow("""{ "quantities": { "namespace": "holo", "include": "q.hpp", "storage": null } }""", "storage")] + public void RefusesANullWhereAValueIsNeeded(string json, string key) + { + Assert.IsFalse(CppGeneratorOptionsFile.TryParse(json, out CppGeneratorOptions? _, out string? message)); + Assert.Contains(key, message, StringComparison.OrdinalIgnoreCase); + } + /// /// Malformed JSON reports where it went wrong, which is the whole of what makes a message about /// a hand-written file useful. diff --git a/Schema.Cpp/CppGeneratorOptionsFile.cs b/Schema.Cpp/CppGeneratorOptionsFile.cs index 2d55de5..cab372d 100644 --- a/Schema.Cpp/CppGeneratorOptionsFile.cs +++ b/Schema.Cpp/CppGeneratorOptionsFile.cs @@ -62,6 +62,12 @@ public static class CppGeneratorOptionsFile ReadCommentHandling = JsonCommentHandling.Skip, AllowTrailingCommas = true, UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow, + + // A JSON null in a property the options declare non-nullable would otherwise be written + // straight in, and fail later as a NullReferenceException with nothing pointing at the file. + // This covers properties and constructor parameters; dictionary values are checked in + // IsSpelled, because the serializer does not apply annotations to a type argument. + RespectNullableAnnotations = true, Converters = { new JsonStringEnumConverter() }, }; @@ -130,9 +136,9 @@ private static bool IsSpelled(CppGeneratorOptions options, [NotNullWhen(false)] } } - foreach ((string schemaName, CppTypeSpelling spelling) in options.ExistingTypes) + foreach ((string schemaName, CppTypeSpelling? spelling) in options.ExistingTypes) { - if (string.IsNullOrWhiteSpace(spelling.Name)) + if (spelling is null || string.IsNullOrWhiteSpace(spelling.Name)) { message = $"'existingTypes.{schemaName}' has no name."; return false; From 3be5e5b567d8cc095e287d77589cfdbf52542494 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:19:18 +0000 Subject: [PATCH 5/7] Read a type whose discriminator is not first, and refuse one with none A "TypeName" or "DefaultKind" written after the object's other properties threw NotSupportedException out of SchemaSerializer.Load, which promises a result. The serializer now accepts metadata in any position, and a type with no discriminator it recognises is reported as InvalidJson, so the CLI says "Could not open" instead of crashing. Fixes #287 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- Schema.Test/SchemaCommandLineTests.cs | 13 ++++++ Schema.Test/SchemaFormatVersionTests.cs | 57 +++++++++++++++++++++++++ Schema/SchemaSerializer.cs | 17 ++++++++ 3 files changed, 87 insertions(+) diff --git a/Schema.Test/SchemaCommandLineTests.cs b/Schema.Test/SchemaCommandLineTests.cs index 9e03f9b..8276082 100644 --- a/Schema.Test/SchemaCommandLineTests.cs +++ b/Schema.Test/SchemaCommandLineTests.cs @@ -199,6 +199,19 @@ public void TestUnreadableSchemaIsAFailure() Assert.IsTrue(result.Error.Contains("Could not open", StringComparison.Ordinal), result.Error); } + [TestMethod] + public void TestASchemaWithATypeMissingItsDiscriminatorIsAFailure() + { + string path = WriteSchema("no-discriminator.schema.json", ValidSchema.Replace( + "{ \"TypeName\": \"String\" }", + "{ \"typeName\": \"String\" }", + StringComparison.Ordinal)); + Result result = Run("validate", path); + + Assert.AreEqual(SchemaCommandLine.Failure, result.ExitCode); + Assert.IsTrue(result.Error.Contains("Could not open", StringComparison.Ordinal), result.Error); + } + [TestMethod] public void TestSchemaFromANewerVersionSaysSoRatherThanCallingItCorrupt() { diff --git a/Schema.Test/SchemaFormatVersionTests.cs b/Schema.Test/SchemaFormatVersionTests.cs index a636c0f..dcd2313 100644 --- a/Schema.Test/SchemaFormatVersionTests.cs +++ b/Schema.Test/SchemaFormatVersionTests.cs @@ -238,4 +238,61 @@ public void TestEveryModernSampleStillLoads() Assert.IsTrue(result.IsSuccess, $"{file}: {result.Message}"); } } + + /// + /// A type discriminator written after the type's other properties still loads. + /// + [TestMethod] + public void TestADiscriminatorThatIsNotFirstStillLoads() + { + const string json = """ + { + "classes": [ + { + "name": "Body", + "description": "", + "members": [ + { + "name": "mass", + "description": "", + "type": { "storage": { "TypeName": "Double" }, "quantityName": "Mass", "TypeName": "Quantity" }, + "defaultValue": { "value": 1.0, "DefaultKind": "NumberDefault" } + } + ] + } + ] + } + """; + + SchemaLoadResult result = SchemaSerializer.Load(json); + + Assert.AreEqual(SchemaLoadStatus.Success, result.Status, result.Message); + SchemaMember? member = result.Schema!.GetClass("Body".As())?.Members[0]; + Assert.IsNotNull(member); + Assert.IsInstanceOfType(member.Type); + Assert.IsNotNull(member.DefaultValue); + } + + /// + /// A type with no discriminator the serializer recognises is a failed load rather than an exception. + /// + [TestMethod] + [DataRow("""{ "typeName": "Int" }""", DisplayName = "Discriminator in camelCase")] + [DataRow("""{}""", DisplayName = "No discriminator")] + public void TestATypeWithNoRecognisedDiscriminatorIsReportedAsInvalid(string type) + { + string json = $$""" + { + "classes": [ + { "name": "User", "description": "", "members": [ { "name": "Name", "description": "", "type": {{type}} } ] } + ] + } + """; + + SchemaLoadResult result = SchemaSerializer.Load(json); + + Assert.AreEqual(SchemaLoadStatus.InvalidJson, result.Status, result.Message); + Assert.IsNull(result.Schema); + Assert.IsFalse(string.IsNullOrWhiteSpace(result.Message)); + } } diff --git a/Schema/SchemaSerializer.cs b/Schema/SchemaSerializer.cs index a8f5242..82e7030 100644 --- a/Schema/SchemaSerializer.cs +++ b/Schema/SchemaSerializer.cs @@ -18,6 +18,11 @@ public static class SchemaSerializer WriteIndented = true, DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + + // A type's "TypeName" and a default's "DefaultKind" are ordinary properties to anyone + // editing a file by hand, and JSON objects are unordered. Without this, a discriminator + // that is not the first property threw NotSupportedException out of Load. + AllowOutOfOrderMetadataProperties = true, Converters = { new RoundTripStringJsonConverterFactory() }, TypeInfoResolver = new DefaultJsonTypeInfoResolver { @@ -222,6 +227,18 @@ public static SchemaLoadResult Load(string json) Message = $"The schema is not valid JSON: {ex.Message}", }; } + catch (NotSupportedException ex) + { + // The serializer reports a shape it cannot construct - a polymorphic object with no + // discriminator, or one it does not recognise - as NotSupportedException rather than + // JsonException. The file is no less unreadable for that, and Load promises a result. + return new() + { + Status = SchemaLoadStatus.InvalidJson, + FileFormatVersion = declaredVersion, + Message = $"The schema could not be read: {ex.Message}", + }; + } } /// From 7bb6bdd5d95269d9a2cd88c8c4cb91eedd4c36c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:23:56 +0000 Subject: [PATCH 6/7] Read a null property as omitted, and refuse a null list element by its path A null in a hand-written file reached the model as a null its non-nullable properties never expect, so Load or Validate threw. Load now drops null properties before deserializing, so they take the same default an omitted property does, and reports a null list element as InvalidJson naming it. Fixes #242 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- Schema.Test/SchemaCommandLineTests.cs | 10 ++++ Schema.Test/SchemaFormatVersionTests.cs | 50 ++++++++++++++++++ Schema/SchemaSerializer.cs | 68 ++++++++++++++++++++++++- docs/schema-format.md | 3 +- 4 files changed, 129 insertions(+), 2 deletions(-) diff --git a/Schema.Test/SchemaCommandLineTests.cs b/Schema.Test/SchemaCommandLineTests.cs index 8276082..c8312af 100644 --- a/Schema.Test/SchemaCommandLineTests.cs +++ b/Schema.Test/SchemaCommandLineTests.cs @@ -212,6 +212,16 @@ public void TestASchemaWithATypeMissingItsDiscriminatorIsAFailure() Assert.IsTrue(result.Error.Contains("Could not open", StringComparison.Ordinal), result.Error); } + [TestMethod] + public void TestASchemaWithANullClassIsAFailure() + { + string path = WriteSchema("null-class.schema.json", """{ "formatVersion": 1, "classes": [null] }"""); + Result result = Run("validate", path); + + Assert.AreEqual(SchemaCommandLine.Failure, result.ExitCode); + Assert.IsTrue(result.Error.Contains("Could not open", StringComparison.Ordinal), result.Error); + } + [TestMethod] public void TestSchemaFromANewerVersionSaysSoRatherThanCallingItCorrupt() { diff --git a/Schema.Test/SchemaFormatVersionTests.cs b/Schema.Test/SchemaFormatVersionTests.cs index dcd2313..0c32c32 100644 --- a/Schema.Test/SchemaFormatVersionTests.cs +++ b/Schema.Test/SchemaFormatVersionTests.cs @@ -295,4 +295,54 @@ public void TestATypeWithNoRecognisedDiscriminatorIsReportedAsInvalid(string typ Assert.IsNull(result.Schema); Assert.IsFalse(string.IsNullOrWhiteSpace(result.Message)); } + + /// + /// A null element in any list is refused by its path rather than throwing out of the load. + /// + [TestMethod] + [DataRow("""{ "classes": [null] }""", "$.classes[0]", DisplayName = "Null class")] + [DataRow("""{ "codeGenerators": [null] }""", "$.codeGenerators[0]", DisplayName = "Null code generator")] + [DataRow("""{ "classes": [ { "name": "A", "members": [ null ] } ] }""", "$.classes[0].members[0]", DisplayName = "Null member")] + [DataRow("""{ "enums": [ { "name": "E", "values": [ "One", null ] } ] }""", "$.enums[0].values[1]", DisplayName = "Null enum value")] + public void TestANullElementIsReportedByItsPath(string json, string path) + { + SchemaLoadResult result = SchemaSerializer.Load(json); + + Assert.AreEqual(SchemaLoadStatus.InvalidJson, result.Status, result.Message); + Assert.IsNull(result.Schema); + Assert.Contains(path, result.Message); + } + + /// + /// A null property reads as an omitted one, so the load succeeds and validation, which must not + /// throw, says what is missing. + /// + [TestMethod] + [DataRow("""{ "classes": null }""", DisplayName = "Null classes")] + [DataRow("""{ "enums": null, "interfaces": null, "dataSources": null, "codeGenerators": null }""", DisplayName = "Null collections")] + [DataRow("""{ "classes": [ { "name": "A", "members": null } ] }""", DisplayName = "Null members")] + [DataRow("""{ "classes": [ { "name": null } ] }""", DisplayName = "Null class name")] + [DataRow("""{ "classes": [ { "name": "A", "members": [ { "name": "x", "type": null } ] } ] }""", DisplayName = "Null member type")] + [DataRow("""{ "classes": [ { "name": "A", "members": [ { "name": null, "type": { "TypeName": "Int" } } ] } ] }""", DisplayName = "Null member name")] + [DataRow("""{ "interfaces": [ { "name": "I", "functions": [ { "name": "F", "parameters": null, "returnType": null } ] } ] }""", DisplayName = "Null function parts")] + public void TestANullPropertyLoadsAsIfOmitted(string json) + { + SchemaLoadResult result = SchemaSerializer.Load(json); + + Assert.AreEqual(SchemaLoadStatus.Success, result.Status, result.Message); + Assert.IsNotNull(result.Schema); + _ = result.Schema.Validate(); + } + + /// + /// A null name is reported by validation the same way an omitted one is. + /// + [TestMethod] + public void TestANullClassNameIsAValidationError() + { + SchemaLoadResult result = SchemaSerializer.Load("""{ "classes": [ { "name": null } ] }"""); + + Assert.IsTrue(result.IsSuccess, result.Message); + Assert.IsTrue(result.Schema!.Validate().Any(issue => issue.Message.Contains("empty name", StringComparison.Ordinal))); + } } diff --git a/Schema/SchemaSerializer.cs b/Schema/SchemaSerializer.cs index 82e7030..b836b9c 100644 --- a/Schema/SchemaSerializer.cs +++ b/Schema/SchemaSerializer.cs @@ -3,6 +3,7 @@ namespace ktsu.Schema.Models; using System.Text.Json; +using System.Text.Json.Nodes; using System.Text.Json.Serialization; using System.Text.Json.Serialization.Metadata; using ktsu.RoundTripStringJsonConverter; @@ -192,9 +193,21 @@ public static SchemaLoadResult Load(string json) }; } + JsonNode? document = JsonNode.Parse(json); + string? nullElement = RemoveNulls(document, "$"); + if (nullElement is not null) + { + return new() + { + Status = SchemaLoadStatus.InvalidJson, + FileFormatVersion = declaredVersion, + Message = $"The schema has a null at {nullElement}, where a list holds only elements.", + }; + } + try { - Schema? schema = JsonSerializer.Deserialize(json, JsonOptions); + Schema? schema = document.Deserialize(JsonOptions); if (schema is null) { return new() @@ -241,6 +254,59 @@ public static SchemaLoadResult Load(string json) } } + /// + /// Reads every null property in a document as an omitted one, and finds the first null element. + /// + /// + /// The serializer never writes a null, so a null in a file was written by hand. Read as given, a + /// null collection, name or type reached the model as a null its non-nullable properties never + /// expect, and or then threw + /// rather than reporting anything. Omitting a property already means "the default": an empty + /// collection, an empty name the validator reports, an unfinished type. A null element in a list + /// has no default to stand for, so it is refused by its path. + /// + /// The node to clean, which is modified in place. + /// Where sits in the document. + /// The path of the first null element, or when there is none. + private static string? RemoveNulls(JsonNode? node, string path) + { + switch (node) + { + case JsonObject jsonObject: + foreach (string name in jsonObject.Where(property => property.Value is null).Select(property => property.Key).ToList()) + { + jsonObject.Remove(name); + } + + foreach (KeyValuePair property in jsonObject) + { + string? found = RemoveNulls(property.Value, $"{path}.{property.Key}"); + if (found is not null) + { + return found; + } + } + + return null; + + case JsonArray jsonArray: + for (int index = 0; index < jsonArray.Count; index++) + { + string elementPath = $"{path}[{index}]"; + string? found = jsonArray[index] is null ? elementPath : RemoveNulls(jsonArray[index], elementPath); + if (found is not null) + { + return found; + } + } + + return null; + + default: + return null; + } + } + /// /// The names a schema document's root may hold, as the serializer itself spells them. /// diff --git a/docs/schema-format.md b/docs/schema-format.md index 883f022..095c5e4 100644 --- a/docs/schema-format.md +++ b/docs/schema-format.md @@ -5,7 +5,8 @@ serializer actually emits, the version field and its migration path, and what ma future release. The format is JSON, written by `System.Text.Json` with camel-cased property names and indentation. -Nulls are omitted. Round-tripping is the contract: loading a file and saving it again must not +Nulls are omitted, and a null written by hand reads as if the property were omitted; a null element +in a list is refused, with its path in the message. Round-tripping is the contract: loading a file and saving it again must not lose information. ## Root From 96f4dabd1b97132edf76639b328e8c0a75bcb233 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 05:28:56 +0000 Subject: [PATCH 7/7] Refuse a class that holds itself by value A class that contains itself directly, or through a chain of other classes, an Optional or a Result, has no finite size and cannot be generated in any target. Validation now reports one error per cycle, naming the route, and points at an Array or a Handle as the way to write a recursive structure. Fixes #293 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01B1AYPNe9rDK8FtuQgiKTnT --- Schema.Test/ContainmentCycleTests.cs | 128 +++++++++++++++++++++++++++ Schema.Test/TravelsAsBytesTests.cs | 8 +- Schema/Models/Schema.Validation.cs | 94 ++++++++++++++++++++ 3 files changed, 228 insertions(+), 2 deletions(-) create mode 100644 Schema.Test/ContainmentCycleTests.cs diff --git a/Schema.Test/ContainmentCycleTests.cs b/Schema.Test/ContainmentCycleTests.cs new file mode 100644 index 0000000..2277766 --- /dev/null +++ b/Schema.Test/ContainmentCycleTests.cs @@ -0,0 +1,128 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Schema.Tests; + +using System.Collections.ObjectModel; + +using ktsu.Schema.Models; +using ktsu.Schema.Models.Names; +using ktsu.Schema.Models.Types; +using ktsu.Semantics.Strings; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Covers the refusal of a class that holds itself by value, which no generator can turn into a +/// type of finite size. +/// +[TestClass] +public sealed class ContainmentCycleTests +{ + /// + /// A class holding itself directly is the commonest way to write this, and it is reported at + /// the member that does it. + /// + [TestMethod] + public void AClassHoldingItselfIsAnError() + { + Schema schema = new(); + SchemaClass node = schema.AddClass("TreeNode".As())!; + node.AddMember("Parent".As())!.SetType(ObjectOf("TreeNode")); + + SchemaValidationIssue error = Errors(schema).Single(); + + Assert.AreEqual("TreeNode.Parent", error.Path); + StringAssert.Contains(error.Message, "TreeNode.Parent → TreeNode"); + } + + /// + /// A cycle through other classes is one issue, naming every step of it, rather than one per + /// class on it. + /// + [TestMethod] + public void ACycleThroughSeveralClassesIsOneErrorNamingTheRoute() + { + Schema schema = new(); + schema.AddClass("A".As())!.AddMember("b".As())!.SetType(ObjectOf("B")); + schema.AddClass("B".As())!.AddMember("c".As())!.SetType(ObjectOf("C")); + schema.AddClass("C".As())!.AddMember("a".As())!.SetType(ObjectOf("A")); + + SchemaValidationIssue error = Errors(schema).Single(); + + Assert.AreEqual("C.a", error.Path); + StringAssert.Contains(error.Message, "A.b → B.c → C.a → A"); + } + + /// + /// An optional and a result hold what they wrap in place, so a cycle through either is still + /// a cycle. + /// + /// The wrapper the self-reference goes through. + [TestMethod] + [DataRow("Optional")] + [DataRow("Result")] + public void ACycleThroughAnInlineWrapperIsAnError(string wrapper) + { + BaseType element = ObjectOf("TreeNode"); + BaseType type = wrapper == "Optional" ? new Optional { ElementType = element } : new Result { ElementType = element }; + + Schema schema = new(); + schema.AddClass("TreeNode".As())!.AddMember("Parent".As())!.SetType(type); + + StringAssert.Contains(Errors(schema).Single(issue => issue.Message.Contains("holds itself", StringComparison.Ordinal)).Message, "holds itself by value"); + } + + /// + /// An array and a handle reach their element by indirection, which is how a recursive + /// structure is meant to be written, so neither is a cycle. + /// + /// The indirection the self-reference goes through. + [TestMethod] + [DataRow("Array")] + [DataRow("Handle")] + public void AReferenceThroughAnIndirectionIsNotACycle(string indirection) + { + BaseType element = ObjectOf("TreeNode"); + BaseType type = indirection == "Array" ? new Models.Types.Array { ElementType = element } : new Handle { ElementType = element }; + + Schema schema = new(); + schema.AddClass("TreeNode".As())!.AddMember("Children".As())!.SetType(type); + + Assert.IsFalse( + schema.Validate().Any(i => i.Message.Contains("holds itself by value", StringComparison.Ordinal)), + string.Join("; ", schema.Validate().Select(i => i.Message))); + } + + /// + /// Two classes holding the same third class is a shared part, not a cycle. + /// + [TestMethod] + public void ADiamondIsNotACycle() + { + Schema schema = new(); + schema.AddClass("Point".As())!.AddMember("X".As())!.SetType(new Float()); + SchemaClass line = schema.AddClass("Line".As())!; + line.AddMember("Start".As())!.SetType(ObjectOf("Point")); + line.AddMember("End".As())!.SetType(ObjectOf("Point")); + schema.AddClass("Shape".As())!.AddMember("Edge".As())!.SetType(ObjectOf("Line")); + + Assert.IsEmpty(Errors(schema), string.Join("; ", Errors(schema).Select(e => e.Message))); + } + + /// + /// A class named by a member but not declared is reported elsewhere, and is not walked into. + /// + [TestMethod] + public void AnUnresolvedClassIsNotWalked() + { + Schema schema = new(); + schema.AddClass("A".As())!.AddMember("b".As())!.SetType(ObjectOf("Missing")); + + Assert.IsFalse(schema.Validate().Any(i => i.Message.Contains("holds itself by value", StringComparison.Ordinal))); + } + + private static Models.Types.Object ObjectOf(string className) => new() { ClassName = className.As() }; + + private static Collection Errors(Schema schema) => + [.. schema.Validate().Where(i => i.Severity == SchemaValidationSeverity.Error)]; +} diff --git a/Schema.Test/TravelsAsBytesTests.cs b/Schema.Test/TravelsAsBytesTests.cs index 9dc60a7..e9892fb 100644 --- a/Schema.Test/TravelsAsBytesTests.cs +++ b/Schema.Test/TravelsAsBytesTests.cs @@ -171,7 +171,9 @@ public void AClassThatMakesThePromiseIsAccepted() /// /// Two classes holding each other is a schema someone can write, and validating it must not - /// hang: the promise is read off each class rather than walked through it. + /// hang. It is refused, once, because two structs holding each other have no finite size; + /// the promise itself is read off each class rather than walked through it, so it adds + /// nothing more. /// [TestMethod] public void TwoClassesHoldingEachOtherDoNotHangValidation() @@ -184,7 +186,9 @@ public void TwoClassesHoldingEachOtherDoNotHangValidation() left.AddMember("Other".As())!.SetType(new Models.Types.Object { ClassName = "Right".As() }); right.AddMember("Other".As())!.SetType(new Models.Types.Object { ClassName = "Left".As() }); - Assert.IsEmpty(Errors(schema), string.Join("; ", Errors(schema))); + Collection errors = Errors(schema); + Assert.HasCount(1, errors, string.Join("; ", errors.Select(e => e.Message))); + StringAssert.Contains(errors[0].Message, "holds itself by value"); } /// diff --git a/Schema/Models/Schema.Validation.cs b/Schema/Models/Schema.Validation.cs index 3d60667..306841a 100644 --- a/Schema/Models/Schema.Validation.cs +++ b/Schema/Models/Schema.Validation.cs @@ -116,6 +116,100 @@ private void ValidateClasses(Collection issues) ValidateTravelsAsBytes(issues, schemaClass, member, memberPath); } } + + ValidateContainmentCycles(issues); + } + + /// + /// Reports every class that holds itself by value, directly or through other classes. + /// + /// + /// A class that contains itself has no finite size. Generated C# either constructs one member + /// inside the next until the stack overflows, which no handler can catch, or fails CS0523 when + /// the classes are structs; generated C++ declares a field of an incomplete type. So the + /// schema is refused here rather than at whichever compiler or process meets it first. + /// + /// The walk passes through an and a , which hold + /// what they wrap inline, and stops at an , a and an + /// , each of which reaches its element by indirection and is how a + /// cycle is meant to be written. Each class is explored once and the classes on the current + /// path are tracked, so a cycle is one reported issue at the member that closes it, and + /// validating it cannot hang. + /// + /// + private void ValidateContainmentCycles(Collection issues) + { + HashSet explored = []; + List<(SchemaClass Class, SchemaMember Member)> path = []; + + foreach (SchemaClass schemaClass in ClassesInternal) + { + FindContainmentCycles(issues, schemaClass, explored, path); + } + } + + private void FindContainmentCycles(Collection issues, SchemaClass schemaClass, HashSet explored, List<(SchemaClass Class, SchemaMember Member)> path) + { + if (!explored.Add(schemaClass)) + { + return; + } + + foreach (SchemaMember member in schemaClass.Members) + { + foreach (SchemaClass held in ClassesHeldByValue(member.Type)) + { + path.Add((schemaClass, member)); + + int start = path.FindIndex(step => ReferenceEquals(step.Class, held)); + if (start >= 0) + { + ReportContainmentCycle(issues, [.. path.Skip(start)], held); + } + else + { + FindContainmentCycles(issues, held, explored, path); + } + + path.RemoveAt(path.Count - 1); + } + } + } + + private static void ReportContainmentCycle(Collection issues, List<(SchemaClass Class, SchemaMember Member)> cycle, SchemaClass closedOn) + { + (SchemaClass closingClass, SchemaMember closingMember) = cycle[^1]; + string route = string.Join(" → ", cycle.Select(step => $"{step.Class.Name}.{step.Member.Name}")); + Report( + issues, + $"{PathSegment(closingClass.Name)}.{PathSegment(closingMember.Name)}", + closingMember, + $"Class '{closedOn.Name}' holds itself by value ({route} → {closedOn.Name}), so it has no finite size. Hold one of these through an Array or a Handle instead."); + } + + /// + /// The classes a member's type contains inline, looking through the wrappers that hold what + /// they wrap in place. + /// + private IEnumerable ClassesHeldByValue(BaseType type) + { + switch (type) + { + case Types.Object objectType when TryGetClass(objectType.ClassName, out SchemaClass? held) && held is not null: + yield return held; + break; + + case Optional or Result: + foreach (SchemaClass held in ClassesHeldByValue(((WrapperType)type).ElementType)) + { + yield return held; + } + + break; + + default: + break; + } } ///