Serialiser_Engine: Added condition that the serialised value must be null if the deserialised result was null.

[Tom-Kingstone]

NOTE: Depends on

Issues addressed by this PR

Closes #3462

See the linked issue:
With help from @IsakNaslundBh in isolating the problem, added a condition to CanSetValueToProperty which ensures that if a deserialised property is null, then the serialised property must also have been null to pass.

Test files

See the linked issue for a script that demonstrated the original problem - There is still an error being recorded (as a value is still being deserialised to null) but the versioning upgrade is happening and the object contains the FileSettings object in a correct form (rather than null/empty)

There will be knock-on effects from this PR on historical versioning, which will likely snowball so testing for this will be ongoing. Running versioning will probably highlight some of these but as @IsakNaslundBh says, much will be missed and it is best to test thoroughly

Changelog

Additional comments

[Tom-Kingstone]

@BHoMBot check core
@BHoMBot check versioning

[IsakNaslundBh]

@BHoMBot check versioning

[bhombot-ci]

@Tom-Kingstone to confirm, the following actions are now queued:

• check core
• check versioning

There are 2 requests in the queue ahead of you.

[Tom-Kingstone]

@BHoMBot check unit-tests

[bhombot-ci]

@Tom-Kingstone to confirm, the following actions are now queued:

• check unit-tests

[Tom-Kingstone]

FYI

for BHoM_Engine\.ci\Datasets\Physical_Engine\Create\Material.json

[IsakNaslundBh]

To comment on this as well as I was part of the investigation and finding the solution.

By pure principle, I think this change is correct as getting a null back should in principle only be ok if the serialised. Need to investigate if this causes any fallout for various cases and seems like you have already found one case @Tom-Kingstone .

That to me highlights that we need to be quite stringent and really check this through as full as possible.

Given the failing UT-s are mine, I can look into them.

[Tom-Kingstone]

Given the failing UT-s are mine, I can look into them.

@IsakNaslundBh
I found the cause of the one that I commented about - Basically there is a property in the serialised object "Type" which contains the following:

{ "_t" : "System.Type", "Name" : "" }

Which when run through FromJson deserialises as null, so if there is a property that contains this, then it is not bson null and therefore fails the check, even though type being null in this case is correct - seems like a fairly sticky one to solve generally, but for specific cases we could just write an upgrader for them.

full json of the object so you can test yourself:

{
    "_t" : "BH.oM.Matter.Options.DensityExtractionOptions",
    "Type" : { "_t" : "System.Type", "Name" : "" },
    "AllowFallbackIfNoType" : false,
    "ExtractionType" : "AllMatching",
    "EqualTolerance" : 0.001,
    "IgnoreZeroValues" : false,
    "ZeroTolerance" : 9.9999999999999995E-07
}

[IsakNaslundBh]

Given the failing UT-s are mine, I can look into them.

@IsakNaslundBh I found the cause of the one that I commented about - Basically there is a property in the serialised object "Type" which contains the following:

{ "_t" : "System.Type", "Name" : "" }

Which when run through FromJson deserialises as null, so if there is a property that contains this, then it is not bson null and therefore fails the check, even though type being null in this case is correct - seems like a fairly sticky one to solve generally, but for specific cases we could just write an upgrader for them.

full json of the object so you can test yourself:

{
    "_t" : "BH.oM.Matter.Options.DensityExtractionOptions",
    "Type" : { "_t" : "System.Type", "Name" : "" },
    "AllowFallbackIfNoType" : false,
    "ExtractionType" : "AllMatching",
    "EqualTolerance" : 0.001,
    "IgnoreZeroValues" : false,
    "ZeroTolerance" : 9.9999999999999995E-07
}

Thanks Tom.

Looking into that json, it looks... wrong to me. Name should always be set in the Type. A null type should just be BsonNull, not set as a type with no name.

That said, need to understand the scope of this, if this is just a annoyance in these unit-tests, or if it is a propagating problem for many cases that has Type as a property...

[IsakNaslundBh]

Did a bit of digging, and the problem with the types is coming from our old serialiser:

BHoM_Engine/Serialiser_Engine/Objects/BsonSerializers/TypeSerializer.cs

Lines 52 to 68 in 43ee03d

	public override void Serialize(BsonSerializationContext context, BsonSerializationArgs args, Type value)
	{
	var bsonWriter = context.Writer;
	bsonWriter.WriteStartDocument();
	var discriminator = m_DiscriminatorConvention.GetDiscriminator(typeof(object), typeof(Type));
	bsonWriter.WriteName(m_DiscriminatorConvention.ElementName);
	BsonValueSerializer.Instance.Serialize(context, discriminator);
	if (value == null)
	{
	//Using context.Writer.WriteNull() leads to problem in the deserialisation.
	//We think that BSON think that the types will always be types to be deserialised rather than properties of objects.
	//If that type is null bson throws an exception believing that it wont be able to deserialise an object of type null, while for this case it is ment to be used as a property.
	bsonWriter.WriteName("Name");
	bsonWriter.WriteString("");
	}

The comment there about why it was done this way might be a good indication that this should be the only place bson nulls are skipped though, so one can at least hope that this is the only edgecase.

So, one solution to this might be to add a IsValidNull method accepting a BsonValue, were we can add this and any other edgecases we might find. Hope not to many of them though, but think we at least need to check through the old serialiser to check how things like methods etc are handled in there. From my initial checking though, I cant find any other place where nulls are checked for, and dealt with in another way then simply bsonWriter.WriteNull(); Example:

BHoM_Engine/Serialiser_Engine/Objects/BsonSerializers/MethodBaseSerializer.cs

Lines 52 to 59 in 43ee03d

	public override void Serialize(BsonSerializationContext context, BsonSerializationArgs args, MethodBase value)
	{
	var bsonWriter = context.Writer;
	if (value == null)
	{
	bsonWriter.WriteNull();
	return;
	}

Hence, a method, something like:

        private static bool IsValidNull(this BsonValue bsonValue)
        {
            if (bsonValue == null || bsonValue.IsBsonNull)
                return true;

            //Handle case for old type-serialiser storing null types by setting name to empty string
            if (bsonValue.IsBsonDocument)
            { 
                BsonDocument bsonDocument = (BsonDocument)bsonValue;
                if (bsonDocument.Contains("_t") && bsonDocument.Contains("Name"))
                {
                    BsonValue typeValue = bsonDocument["_t"];
                    BsonValue nameValue = bsonDocument["Name"];
                    if (typeValue.IsString && nameValue.IsString)
                    {
                        BsonString typeString = (BsonString)typeValue;
                        BsonString nameString = (BsonString)nameValue;
                        if (typeString.Value == "System.Type" && string.IsNullOrEmpty(nameString.Value))
                            return true;
                    }
                }
            }

            return false;
        }

Might be a start - to be expanded upon if need be!

Another option for this is to try to add versioning for this case, turning type documents of that format into nulls. Might be worth exploring as well.

[IsakNaslundBh]

@Tom-Kingstone had a brief chat with @adecler about this, and think this might require a bit of a zoomout before we just patch with the current PR.

If ok with you, will try to tackle this next sprint, making sure we set it up in a way that hopefully covers even more cases.

[Tom-Kingstone]

added parked status awaiting end of testing sprint