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.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; 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/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 @@ - + 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); + } + } + } } 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/SchemaCommandLineTests.cs b/Schema.Test/SchemaCommandLineTests.cs index 9e03f9b..c8312af 100644 --- a/Schema.Test/SchemaCommandLineTests.cs +++ b/Schema.Test/SchemaCommandLineTests.cs @@ -199,6 +199,29 @@ 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 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 ce0a1c5..0c32c32 100644 --- a/Schema.Test/SchemaFormatVersionTests.cs +++ b/Schema.Test/SchemaFormatVersionTests.cs @@ -165,4 +165,184 @@ 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}"); + } + } + + /// + /// 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)); + } + + /// + /// 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.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; + } } /// diff --git a/Schema/SchemaSerializer.cs b/Schema/SchemaSerializer.cs index addcecf..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; @@ -18,6 +19,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 { @@ -176,9 +182,32 @@ 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), + }; + } + + 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() @@ -211,6 +240,112 @@ 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}", + }; + } + } + + /// + /// 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. + /// + /// + /// 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}'."; } /// diff --git a/docs/schema-format.md b/docs/schema-format.md index e7d7e15..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 @@ -36,6 +37,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.