Discriminator warnings and Element concrete class

Elements.Benchmarks/Serialization.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Linq;
 using BenchmarkDotNet.Attributes;
 using Elements.Geometry.Profiles;
@@ -35,6 +36,15 @@ public void DeserializeFromJSON()
             Model.FromJson(_json);
         }
 
+        [Benchmark(Description = "Deserialize invalid Elements from JSON.")]
+        public void DeserializeInvalidsFromJSON()
+        {
+            var guids = Enumerable.Range(0, 1000000).Select(i => Guid.NewGuid().ToString()).ToArray();
+            var basElements = String.Join(",", guids.Select(g => $"'{g}':{{'discriminator':'Elements.Baz'}}").ToArray());
+            var toDeserialize = _json.Replace("'Elements':{", $"'Elements':{{{basElements},");
+            Model.FromJson(toDeserialize);
+        }
+
         [Benchmark(Description = "Serialize to glTF.")]
         public void SerializeToGlTF()
         {

Elements/Elements.sln

@@ -0,0 +1,31 @@
+
+Microsoft Visual Studio Solution File, Format Version 12.00
+# Visual Studio Version 17
+VisualStudioVersion = 17.5.002.0
+MinimumVisualStudioVersion = 10.0.40219.1
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Elements", "src\Elements.csproj", "{5DC7E525-3EED-4C3C-AC9F-2C95ACDD6F4D}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Elements.Tests", "test\Elements.Tests.csproj", "{0E2EE823-6892-4574-A62D-407A8EC21F6D}"
+EndProject
+Global
+	GlobalSection(SolutionConfigurationPlatforms) = preSolution
+		Debug|Any CPU = Debug|Any CPU
+		Release|Any CPU = Release|Any CPU
+	EndGlobalSection
+	GlobalSection(ProjectConfigurationPlatforms) = postSolution
+		{5DC7E525-3EED-4C3C-AC9F-2C95ACDD6F4D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{5DC7E525-3EED-4C3C-AC9F-2C95ACDD6F4D}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{5DC7E525-3EED-4C3C-AC9F-2C95ACDD6F4D}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{5DC7E525-3EED-4C3C-AC9F-2C95ACDD6F4D}.Release|Any CPU.Build.0 = Release|Any CPU
+		{0E2EE823-6892-4574-A62D-407A8EC21F6D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{0E2EE823-6892-4574-A62D-407A8EC21F6D}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{0E2EE823-6892-4574-A62D-407A8EC21F6D}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{0E2EE823-6892-4574-A62D-407A8EC21F6D}.Release|Any CPU.Build.0 = Release|Any CPU
+	EndGlobalSection
+	GlobalSection(SolutionProperties) = preSolution
+		HideSolutionNode = FALSE
+	EndGlobalSection
+	GlobalSection(ExtensibilityGlobals) = postSolution
+		SolutionGuid = {9CA8779B-A3A1-4A64-BF1D-4A44D8C5725B}
+	EndGlobalSection
+EndGlobal

Elements/src/Element.cs

@@ -8,7 +8,7 @@ namespace Elements
     /// An object which is identified with a unique identifier and a name.
     /// </summary>
     [JsonConverter(typeof(Elements.Serialization.JSON.JsonInheritanceConverter), "discriminator")]
-    public abstract class Element : System.ComponentModel.INotifyPropertyChanged
+    public class Element : System.ComponentModel.INotifyPropertyChanged
     {
         private System.Guid _id;
         private string _name;

Elements/src/Model.cs

@@ -228,7 +228,7 @@ public IEnumerable<T> AllElementsOfType<T>() where T : Element
         /// is an interface.
         /// </summary>
         /// <typeparam name="T">The type of the element from which returned elements derive.</typeparam>
-        /// <returns>A collection of elements derived from the specified type.</returns>
+        /// <returns>A collection of elements derived from the specified type.</returns>1
         public IEnumerable<T> AllElementsAssignableFromType<T>() where T : Element
         {
             return Elements.Values.Where(e => typeof(T).IsAssignableFrom(e.GetType())).Cast<T>();
@@ -421,7 +421,7 @@ internal Model CreateExportModel(bool gatherSubElements, bool updateElementsRepr
         /// </summary>
         /// <param name="json">The JSON representing the model.</param>
         /// <param name="errors">A collection of deserialization errors.</param>
-        /// <param name="forceTypeReload">Option to force reloading the inernal type cache. Use if you add types dynamically in your code.</param>
+        /// <param name="forceTypeReload">Option to force reloading the internal type cache. Use if you add types dynamically in your code.</param>
         public static Model FromJson(string json, out List<string> errors, bool forceTypeReload = false)
         {
             // When user elements have been loaded into the app domain, they haven't always been
@@ -442,6 +442,7 @@ public static Model FromJson(string json, out List<string> errors, bool forceTyp
                     args.ErrorContext.Handled = true;
                 }
             });
+            deserializationErrors.AddRange(JsonInheritanceConverter.GetAndClearDeserializationWarnings());
             errors = deserializationErrors;
             JsonInheritanceConverter.Elements.Clear();
             return model;

Elements/src/Serialization/JSON/JsonInheritanceConverter.cs

@@ -50,6 +50,8 @@ public static Dictionary<Guid, Element> Elements
             }
         }
 
+        private static List<string> _deserializationWarnings = new List<string>();
+
         public JsonInheritanceConverter()
         {
             _discriminator = DefaultDiscriminatorName;
@@ -163,13 +165,22 @@ public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value,
                 else
                 {
                     var jObject = Newtonsoft.Json.Linq.JObject.FromObject(value, serializer);
+
+                    var discriminatorName = GetDiscriminatorName(value);
+
                     if (jObject.TryGetValue(_discriminator, out JToken token))
                     {
-                        ((JValue)token).Value = GetDiscriminatorName(value);
+                        // Don't update the discriminator value if it is a base class of `GeometricElement` or `Element`.
+                        // This means that the type was likely set due to fallback when a type wasn't found when deserializing.
+                        // So, we should keep the discriminator value until another serializer can handle it.
+                        if (discriminatorName != "Elements.GeometricElement" && discriminatorName != "Elements.Element")
+                        {
+                            ((JValue)token).Value = discriminatorName;
+                        }
                     }
                     else
                     {
-                        jObject.AddFirst(new Newtonsoft.Json.Linq.JProperty(_discriminator, GetDiscriminatorName(value)));
+                        jObject.AddFirst(new Newtonsoft.Json.Linq.JProperty(_discriminator, discriminatorName));
                     }
                     writer.WriteToken(jObject.CreateReader());
                 }
@@ -234,13 +245,25 @@ public override bool CanConvert(System.Type objectType)
             return true;
         }
 
+        public static List<string> GetAndClearDeserializationWarnings()
+        {
+            var warnings = _deserializationWarnings.ToList();
+            _deserializationWarnings.Clear();
+            return warnings;
+        }
+
         public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
         {
             // The serialized value is an identifier, so the expectation is
             // that the element with that id has already been deserialized.
             if (typeof(Element).IsAssignableFrom(objectType) && !WritingTopLevelElement(reader.Path) && reader.Value != null)
             {
                 var id = Guid.Parse(reader.Value.ToString());
+                if (!Elements.ContainsKey(id))
+                {
+                    _deserializationWarnings.Add($"Element {id} was not found during deserialization. Check for other deserialization errors.");
+                    return null;
+                }
                 return Elements[id];
             }
 
@@ -295,7 +318,9 @@ public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type o
 
                 if (discriminator != null)
                 {
-                    throw new Exception($"An object with the discriminator, {discriminator}, could not be deserialized. {baseMessage} {moreInfoMessage}", ex);
+                    _deserializationWarnings.Add($"An object with the discriminator, {discriminator}, could not be deserialized. {baseMessage}");
+                    return null;
+
                 }
                 else
                 {
@@ -332,6 +357,11 @@ private System.Type GetObjectSubtype(System.Type objectType, string discriminato
             {
                 return typeof(GeometricElement);
             }
+            // If nothing else has worked, see if it has an ID and treat it as a generic element
+            if (jObject.TryGetValue("Id", out _))
+            {
+                return typeof(Element);
+            }
 
             // The default behavior for this converter, as provided by nJSONSchema
             // is to return the base objectType if a derived type can't be found.

Elements/test/ModelTests.cs

@@ -72,7 +72,7 @@ public void HasOriginAfterSerialization()
         }
 
         [Fact]
-        public void SkipsUnknownTypesDuringDeserialization()
+        public void UnknownTypesConvertToBaseElementDuringDeserialization()
         {
             // We've changed an Elements.Beam to Elements.Foo
             var modelStr = "{'Transform':{'Matrix':{'Components':[1.0,0.0,0.0,0.0,0.0,1.0,0.0,0.0,0.0,0.0,1.0,0.0]}},'Elements':{'c6d1dc68-f800-47c1-9190-745b525ad569':{'discriminator':'Elements.Baz'}, '37f161d6-a892-4588-ad65-457b04b97236':{'discriminator':'Elements.Geometry.Profiles.WideFlangeProfile','d':1.1176,'tw':0.025908,'bf':0.4064,'tf':0.044958,'Perimeter':{'discriminator':'Elements.Geometry.Polygon','Vertices':[{'X':-0.2032,'Y':0.5588,'Z':0.0},{'X':-0.2032,'Y':0.51384199999999991,'Z':0.0},{'X':-0.012954,'Y':0.51384199999999991,'Z':0.0},{'X':-0.012954,'Y':-0.51384199999999991,'Z':0.0},{'X':-0.2032,'Y':-0.51384199999999991,'Z':0.0},{'X':-0.2032,'Y':-0.5588,'Z':0.0},{'X':0.2032,'Y':-0.5588,'Z':0.0},{'X':0.2032,'Y':-0.51384199999999991,'Z':0.0},{'X':0.012954,'Y':-0.51384199999999991,'Z':0.0},{'X':0.012954,'Y':0.51384199999999991,'Z':0.0},{'X':0.2032,'Y':0.51384199999999991,'Z':0.0},{'X':0.2032,'Y':0.5588,'Z':0.0}]},'Voids':null,'Id':'37f161d6-a892-4588-ad65-457b04b97236','Name':'W44x335'},'6b77d69a-204e-40f9-bc1f-ed84683e64c6':{'discriminator':'Elements.Material','Color':{'Red':0.60000002384185791,'Green':0.5,'Blue':0.5,'Alpha':1.0},'SpecularFactor':0.0,'GlossinessFactor':0.0,'Id':'6b77d69a-204e-40f9-bc1f-ed84683e64c6','Name':'steel'},'fd35bd2c-0108-47df-8e6d-42cc43e4eed0':{'discriminator':'Elements.Foo','Curve':{'discriminator':'Elements.Geometry.Arc','Center':{'X':0.0,'Y':0.0,'Z':0.0},'Radius':2.0,'StartAngle':0.0,'EndAngle':90.0},'StartSetback':0.25,'EndSetback':0.25,'Profile':'37f161d6-a892-4588-ad65-457b04b97236','Transform':{'Matrix':{'Components':[1.0,0.0,0.0,0.0,0.0,1.0,0.0,0.0,0.0,0.0,1.0,0.0]}},'Material':'6b77d69a-204e-40f9-bc1f-ed84683e64c6','Representation':{'SolidOperations':[{'discriminator':'Elements.Geometry.Solids.Sweep','Profile':'37f161d6-a892-4588-ad65-457b04b97236','Curve':{'discriminator':'Elements.Geometry.Arc','Center':{'X':0.0,'Y':0.0,'Z':0.0},'Radius':2.0,'StartAngle':0.0,'EndAngle':90.0},'StartSetback':0.25,'EndSetback':0.25,'Rotation':0.0,'IsVoid':false}]},'Id':'fd35bd2c-0108-47df-8e6d-42cc43e4eed0','Name':null}}}";
@@ -83,9 +83,11 @@ public void SkipsUnknownTypesDuringDeserialization()
             }
 
             // We expect three geometric elements,
-            // but the baz will not deserialize.
-            Assert.Equal(3, model.Elements.Count);
-            Assert.Equal(2, errors.Count);
+            // and one base element (Elements.Baz)
+            Assert.Equal(4, model.Elements.Count);
+            bool containsBaz = model.Elements.Select(x => x.Value.AdditionalProperties["discriminator"]).Contains("Elements.Baz");
+            Assert.True(containsBaz);
+            Assert.Single(errors);
         }
 
         /// <summary>
@@ -213,6 +215,23 @@ public void CoreElementTransformsAreIdempotentDuringSerialization()
             Assert.Equal(wall.Transform, newWall.Transform);
         }
 
+
+        [Fact]
+        public void SerializationKeepsDiscriminatorOnFallbackBaseElementTypes()
+        {
+            // Reading in a model with a SpaceBoundary input
+            var json = File.ReadAllText("../../../models/Elements/spaceinputs.json");
+            var model = Model.FromJson(json);
+
+            // Since SpaceBoundary isn't in Elements, it will be deserialized as a `GeometricElement`
+            Assert.True(model.Elements.First().Value.GetType() == typeof(GeometricElement));
+
+            var newModel = model.ToJson();
+
+            // When we serialize again, we should still have the discriminator of `Elements.SpaceBoundary`
+            Assert.Contains("Elements.SpaceBoundary", newModel);
+        }
+
         [Fact]
         public void DeserializationSkipsUnknownProperties()
         {

Elements/test/models/Elements/spaceinputs.json

@@ -0,0 +1,119 @@
+{
+  "Elements": {
+    "5e2f9c73-1d6a-4861-97e1-fbba56e90c55": {
+      "discriminator": "Elements.SpaceBoundary",
+      "Level Add Id": "dummy-level-volume",
+      "Relative Position": {
+        "X": 2,
+        "Y": 2,
+        "Z": 0
+      },
+      "ProgramName": "Open Office",
+      "Boundary": {
+        "Perimeter": {
+          "discriminator": "Elements.Geometry.Polygon",
+          "Vertices": [
+            {
+              "X": 0,
+              "Y": 0,
+              "Z": 0
+            },
+            {
+              "X": 4,
+              "Y": 0,
+              "Z": 0
+            },
+            {
+              "X": 4,
+              "Y": 4,
+              "Z": 0
+            },
+            {
+              "X": 0,
+              "Y": 4,
+              "Z": 0
+            }
+          ]
+        },
+        "Voids": [],
+        "EdgeThickness": [
+          [
+            0.0762,
+            0.0762
+          ],
+          [
+            0.0762,
+            0.0762
+          ],
+          [
+            0.0762,
+            0.0762
+          ],
+          [
+            0.0762,
+            0.0762
+          ]
+        ],
+        "Id": "0bb37606-1110-4a6d-801c-1b4f5de83127",
+        "discriminator": "Elements.Geometry.Profile"
+      },
+      "Area": 16,
+      "Height": 1,
+      "Program Type": "Open Office",
+      "Level": "603042dd-0336-43ea-b31b-13794fb410ad",
+      "Level Layout": {
+        "discriminator": "Elements.LevelLayout",
+        "Profiles": [],
+        "Add Id": "dummy-level-volume-layout",
+        "Levels": [
+          "603042dd-0336-43ea-b31b-13794fb410ad"
+        ],
+        "Id": "19794924-e0f1-4335-8b27-5c03f3ddfdd0",
+        "Name": " Layout"
+      },
+      "Hypar Space Type": "Open Office",
+      "Transform": {
+        "Matrix": {
+          "Components": [
+            1,
+            0,
+            0,
+            0,
+            0,
+            1,
+            0,
+            0,
+            0,
+            0,
+            1,
+            0
+          ]
+        }
+      },
+      "Material": {
+        "discriminator": "Elements.Material",
+        "Color": {
+          "Red": 0.435,
+          "Green": 0.627,
+          "Blue": 0.745,
+          "Alpha": 0.5
+        },
+        "SpecularFactor": 0.1,
+        "GlossinessFactor": 0.1,
+        "Unlit": false,
+        "DoubleSided": false,
+        "RepeatTexture": true,
+        "InterpolateTexture": true,
+        "EmissiveFactor": 0,
+        "Draw In Front": false,
+        "EdgeDisplaySettings": null,
+        "Id": "3086c9b7-db48-4604-a59f-d4d0630fccd7",
+        "Name": "Open Office"
+      },
+      "Representation": null,
+      "IsElementDefinition": false,
+      "Id": "5e2f9c73-1d6a-4861-97e1-fbba56e90c55",
+      "Name": "Space"
+    }
+  }
+}