Round-tripped log comparison fails if property bag contains DateTime

[ghost]

When the generated equality comparer (e.g., RunEqualityComparer) for a generated object model class compares the individual SerializedPropertyInfo objects in object's property bag, it does an object equality comparison instead of invoking SerializedPropertyInfo.ValueComparer.Equals. This causes the equality comparer's Equals method to incorrectly return false when comparing two objects with non-empty property bags, even if the property values are identical.

To see the problem, uncomment the line

originalLog.ValueEquals(deserializedLog).Should().BeTrue();

in PropertyBagConverter_RoundTrip.

Leaving that line uncommented could serve as a regression test for this bug.

@michaelcfanning @ScottLouvau FYI

[ghost]

The bug title "Generated code for property bag comparison is incorrect" is wrong. SerializedPropertyInfo doesn't have a ValueComparer, and it does have a perfectly good Equals method that performs a correct comparison. So calling object.Equals on a SerializedPropertyInfo object is the right thing to do, and it works fine.

What's actually going on is that we're comparing two DateTime-valued properties with the same SerializedPropertyInfo.SerializedValue ("\"2019-09-27T13:52:00\""), but one of them has IsString true and the other false. Here's how it happens:

The unit test creates a SarifLog object originalLog programmatically, including a DateTime-valued property:

run.SetProperty(dateTimePropertyName, dateTimePropertyValue);

This creates a SerializedPropertyInfo object with IsString == false and SerializedValue == "\"2019-09-27T13:52:00\"".

Now we round-trip the originalLog object (first serializing and then deserializing it). To deserialize, we call SarifUtilities.DeserializeObject to avoid bug #1577. (That bug would manifest if we instead called JsonConvert.DeserializeObject and then re-serialized the resulting SarifLog. Confusing, yes.)

SarifUtilties.DeserializeObject by design tells Newtonsoft.Json not to attempt to recognize string-valued properties that look like DateTimes, and instead to treat them as strings. As a result, the
"DateTime-valued" property in the round-tripped log object is now internally represented as a string (SerializedPropertyInfo.IsString == true), and so the comparison fails.

Nevertheless, the property still works as a DateTime, as this test on the round-tripped SarifLog object shows:

run.GetProperty<DateTime>(dateTimePropertyName).Should().Be(dateTimePropertyValue);

I propose that this is not worth fixing. IMO the more valuable round-tripping scenario is when you read a log file, modify it, and then write it back. In that scenario, you want the parts of the log file that you did not touch to be unaffected. The fix to #1577 ensures this: a property bag property that looks like a DateTime is unaffected by the round trip:

"someDate": "2019-09-27T13:52:00"

I don't see creating a SarifLog object, writing it out, reading it back, and verifying that you have an equivalent SarifLog object as a realistic product code scenario. It's a fine test scenario, and indeed it usually works. It's just that it fails whicen you have a DateTime-valued property bag property, as a result of the need for the workaround to the Newtonsoft.Json bug that we implemented in #1577.