Skip to content

Commit

Permalink
Merge pull request #1058 from hypar-io/discriminator-warnings-ak
Browse files Browse the repository at this point in the history
Discriminator warnings and Element concrete class (#1058)

Co-Authored-By: Eric Wassail <[email protected]>
  • Loading branch information
anthonie-kramer and wynged authored Nov 17, 2023
2 parents 1db3e1f + ed459b4 commit 1d0020d
Show file tree
Hide file tree
Showing 8 changed files with 485 additions and 11 deletions.
10 changes: 10 additions & 0 deletions Elements.Benchmarks/Serialization.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Linq;
using BenchmarkDotNet.Attributes;
using Elements.Geometry.Profiles;
Expand Down Expand Up @@ -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()
{
Expand Down
31 changes: 31 additions & 0 deletions Elements/Elements.sln
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion Elements/src/Element.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions Elements/src/Model.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down
36 changes: 33 additions & 3 deletions Elements/src/Serialization/JSON/JsonInheritanceConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ public static Dictionary<Guid, Element> Elements
}
}

private static List<string> _deserializationWarnings = new List<string>();

public JsonInheritanceConverter()
{
_discriminator = DefaultDiscriminatorName;
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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];
}

Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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.
Expand Down
27 changes: 23 additions & 4 deletions Elements/test/ModelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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}}}";
Expand All @@ -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>
Expand Down Expand Up @@ -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()
{
Expand Down
119 changes: 119 additions & 0 deletions Elements/test/models/Elements/spaceinputs.json
Original file line number Diff line number Diff line change
@@ -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"
}
}
}
Loading

0 comments on commit 1d0020d

Please sign in to comment.