Discriminator warnings and Element concrete class

[anthonie-kramer]

BACKGROUND:

• Actually throwing errors during deserialization can make it very hard to debug an issue if the is a deserialization issue, even if that deserialization issue can be safely ignored. We eventually handle these exceptions anyways and surface messages, so this simply does that explicitly.
• This really only affects working with local Elements, but this is frequent enough to be annoying.
• Doesn't allow null values when deserializing

DESCRIPTION:

• Create a _deserializationDiscriminatorWarnings list that saves a list of deserialization issues encountered, rather than throwing for each one encountered.
• Makes Element a concrete class so we can instantiate it instead of returning null

TESTING:

• Updated tests in the ModelTests to turn unknown types into the base class of Element with Name and Id assigned and all AdditionalProperties are assigned.

FUTURE WORK:

• We could do things now when we don't have the right Element type for deserialization... we can still get at the properties of the base Element through AdditionalProperties

REQUIRED:

• All changes are up to date in CHANGELOG.md.

COMMENTS:

• Any other notes.

---

This change is 

[andrewheumann]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer and @wynged)

---

Elements/src/Serialization/JSON/JsonInheritanceConverter.cs line 317 at r2 (raw file):

                    Element element = new Element();

                    foreach (var prop in jObject.Properties())

I think we don't need to do this. See how the GeometricElement fallback was implemented here:
https://github.com/hypar-io/Elements/pull/369

I think we can add another clause there for falling back to Element which should work, without having to construct a new one ourselves. AdditionalProperties should be handled automatically. I think it's in those cases (where a fallback was used for a type) that we should add deserialization warnings.

[andrewheumann]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer and @wynged)

---

Elements/src/Serialization/JSON/JsonInheritanceConverter.cs line 317 at r2 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

I think we don't need to do this. See how the GeometricElement fallback was implemented here:
https://github.com/hypar-io/Elements/pull/369

I think we can add another clause there for falling back to Element which should work, without having to construct a new one ourselves. AdditionalProperties should be handled automatically. I think it's in those cases (where a fallback was used for a type) that we should add deserialization warnings.

something like

// If it's not in the type cache see if it's got a representation.
// Import it as a GeometricElement.
// TODO: this may not be a safe assumption now that we don't always serialize representations. 
if (jObject.TryGetValue("Representation", out _))
{
    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);
}

[wynged]

Reviewed 1 of 4 files at r1, 5 of 6 files at r2, all commit messages.
Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer)

---

Elements/src/Model.cs line 449 at r2 (raw file):

            JsonInheritanceConverter.Elements.Clear();
            // Don't leave null elements in the model.
            var goodElements = new Dictionary<Guid, Element>(model.Elements.Count());

I think that with this new strategy we won't end up with any nulls right? so maybe we should remove this bit of code.

---

Elements/test/ModelTests.cs line 88 at r2 (raw file):

            // and one base element (Elements.Baz)
            Assert.Equal(4, model.Elements.Count);
            Assert.Single(errors);

can we add a check that the thing that was Elements.Bas has it's discriminator saved as Elements.Baz in the additional properties?

[anthonie-kramer]

Kind of melding both approaches from @wynged 's original branch, and @andrewheumann 's approach to creating the base Element... I think we still want to return null on discriminator errors because of the thrown exception and debugging frustration...

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann and @wynged)

---

Elements/src/Model.cs line 449 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

I think that with this new strategy we won't end up with any nulls right? so maybe we should remove this bit of code.

I would like to keep this in since we will still be potentially returning null when a discriminator exception gets thrown (see updates from Andrew's comment)

---

Elements/src/Serialization/JSON/JsonInheritanceConverter.cs line 317 at r2 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

something like

// If it's not in the type cache see if it's got a representation.
// Import it as a GeometricElement.
// TODO: this may not be a safe assumption now that we don't always serialize representations. 
if (jObject.TryGetValue("Representation", out _))
{
    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);
}

Done.

---

Elements/test/ModelTests.cs line 88 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

can we add a check that the thing that was Elements.Bas has it's discriminator saved as Elements.Baz in the additional properties?

Done.

[wynged]

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann and @anthonie-kramer)

---

Elements/src/Model.cs line 449 at r2 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

I would like to keep this in since we will still be potentially returning null when a discriminator exception gets thrown (see updates from Andrew's comment)

but I think what you've done has basically made it impossible for a discriminator exception to be thrown except it truly unusual circumstances so this seems just like redundant code executing that we shouldn't need.

[anthonie-kramer]

Added a test and updated code to keep the discriminator when the object falls back to a base GeometricElement or Element

Reviewable status: 2 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann and @wynged)

---

Elements/src/Model.cs line 449 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

but I think what you've done has basically made it impossible for a discriminator exception to be thrown except it truly unusual circumstances so this seems just like redundant code executing that we shouldn't need.

Removed per conversation earlier. We want to know if nulls are assigned.

[andrewheumann]

LGTM, tho I haven't tested myself!

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @wynged)

[wynged]

Reviewed all commit messages.
Reviewable status: complete! 2 of 1 approvals obtained