Fix #1673 (property bag testing); Fix #1687 (NuGet packaging warning) #1688

ghost · 2019-09-26T23:49:38Z

I added a comprehensive end-to-end test of round-tripping property bag properties. I also added a DateTime test to the PropertyBagConverterTests functional tests.

The problem that @ScottLouvau ran into with the DateTime test is related to my fix for #1577. I don't mean that my fix caused his problem; I mean that to avoid the problem, we need to do the same thing I did to fix #1577. For an explanation, see this comment in SarifUtilities.cs.

In short, when Newtonsoft.Json deserializes a string-valued property that looks to it like a DateTime, its default behavior is to convert it to a DateTime object. That behavior breaks our property round-tripping. To avoid it, we need to call JsonConvert.DeserializeObject with a JsonSerializerSettings object that specifies DateParseHandling.None. That tells Newtonsoft.Json to leave string-valued properties alone and let us handle the DateTime-iness. In #1577, I introduced SarifUtilities.DeserializeObject to do exactly that.

@michaelcfanning and I have discussed this and we know it's not the end of the story. It's too easy to forget (or not to know about) the need to call the SarifUtilities method, which is exactly what happened in that existing unit test. But as far as verifying that our handling of property bag properties is good, I believe we now have sufficient test coverage to be confident of that.

Also:

Fix a NuGet packaging warning.
Improve our internal handling of property bag properties with null values.

…ber.

ghost · 2019-09-26T23:50:43Z

src/build.props

    <!--
    Don't complain about SemVer 2.0.0-compatible version strings.
    See https://github.com/NuGet/Home/issues/4687#issuecomment-393302779.
    -->
    <NoWarn>NU5105</NoWarn>
  </PropertyGroup>

+  <ItemGroup>
+    <None Include="..\..\triskelion.png" Pack="true" PackagePath="\"/>


None [](start = 5, length = 4)

This fixes the warning NU5048 ("PackageIconUrl is deprecated") that we get as we build each package. #ByDesign

ghost · 2019-09-26T23:53:12Z

src/Test.UnitTests.Sarif/TestData/RoundTripping/RoundTripping.sarif

+            "dateArray": [
+  "2019-09-26T15:52Z",
+  "2019-09-26T15:54Z"
+],


This is how Formatting.Indented renders the arrays and sub-objects within properties. I don't know why. #ByDesign

Oh, it's because the PropertyBagConverter has taken over the serialization at this point and it doesn't retain the indentation.

In reply to: 328869713 [](ancestors = 328869713)

Also: - Fix crash when deserializing a null string-valued property.

... by simplifying the way SetProperty deals with null values.

ghost · 2019-09-27T19:30:13Z

src/Sarif/Core/PropertyBagHolder.cs

            if (!Properties[propertyName].IsString)
            {
                throw new InvalidOperationException(SdkResources.CallGenericGetProperty);
            }

            string value = Properties[propertyName].SerializedValue;

-            // Remove the quotes around the serialized value ("x" => x) -- unless it's null.
-            return value.Equals(NullValue, StringComparison.Ordinal)
-                ? null


null [](start = 18, length = 4)

This null check is no longer needed because we simplified the way that SetProperty deals with null values (SetProperty("aProp", null)). Rather than creating a SerializedPropertyInfo object with SerializedValue set to null and placing that object into Properties["aProp"], we simply set Properties["aProp"] to null.

This is consistent with what the PropertyBagConverter does when it encounters a null valued property. Before this change, a null valued property read from the log file had a different internal representation from a property you created programatically with SetProperty("aProp", null).

This is one of those explanatory comments that doesn't have a natural place in the source code. I'm explaining why some code no longer appears. But, come to think of it, I can add a comment on the null path in SetProperty, so I'll do that. #ByDesign

ScottLouvau · 2019-09-30T16:09:05Z

src/Sarif/Core/PropertyBagHolder.cs

-            {
-                Properties.Remove(propertyName);
-            }
+                Properties?.Remove(propertyName);


It looks like the design is settled here, but I wonder if we should've thrown in this case. Callers could try to clean up properties on the wrong objects in the SARIF hierarchy and not realize nothing is actually happening underneath. #WontFix

It's a good point. Changing this behavior would of course be breaking (not that that's ever stopped us before ;-)).

In reply to: 329663092 [](ancestors = 329663092)

ScottLouvau · 2019-09-30T16:10:31Z

src/Test.FunctionalTests.Sarif/PropertyBagConverterTests.cs

                }
            };

            var settings = new JsonSerializerSettings { Formatting = Formatting.Indented };
            string originalLogText = JsonConvert.SerializeObject(originalLog, settings);

-            SarifLog deserializedLog = JsonConvert.DeserializeObject<SarifLog>(originalLogText);
+            SarifLog deserializedLog = SarifUtilities.DeserializeObject<SarifLog>(originalLogText);
            run = deserializedLog.Runs[0];

            int integerProperty = run.GetProperty<int>(intPropertyName);


Is there a ValueComparer you can use to verify the entire Run object seems to match the Deserialized one? I think it's good to have the per-property checks for clarity, but an overall compare might be good redundant verification. #Pending

Yes, each generated class (Run) has an associated generated equality comparer class (RunEqualityComparer) with a static Instance property. The generated class has a static ValueComparer field that is set to that instance, and a ValueEquals method that invokes Equals on the instance:

public partial class Run : PropertyBagHolder, ISarifNode { public static IEqualityComparer<Run> ValueComparer => RunEqualityComparer.Instance; public bool ValueEquals(Run other) => ValueComparer.Equals(this, other);

But the equality comparison fails because of a code generation bug! When comparing the individual SerializedPropertyInfo objects in the property bag, the generated comparison code does an object equality comparison instead of invoking SerializedPropertyInfo.ValueComparer.Equals.

I filed #1689, "Generated code for property bag comparison is incorrect".

I will not fix that bug in this PR.

In reply to: 329663713 [](ancestors = 329663713)

ScottLouvau · 2019-09-30T16:11:19Z

src/Test.UnitTests.Sarif/RoundTrippingTests.cs

+            intProperty.Should().Be(42);
+
+            long longProperty = holder.GetProperty<long>("long");
+            longProperty.Should().Be(5000000000);


Comment affirming that this example is over int.MaxValue? #ByDesign

That comment appears on Line 55.

In reply to: 329664092 [](ancestors = 329664092)

ScottLouvau · 2019-09-30T16:11:48Z

src/Test.UnitTests.Sarif/RoundTrippingTests.cs

+            DateTime[] dateTimeArray = holder.GetProperty<DateTime[]>("dateTimeArray");
+            dateTimeArray.Length.Should().Be(2);
+            dateTimeArray[0].Should().Be(ParseUtcDateTime("2019-09-26T15:52:02Z"));
+            dateTimeArray[1].Should().Be(ParseUtcDateTime("2019-09-26T15:54Z"));


How much precision do we emit for DateTimes? Does UtcNow roundtrip properly? #Resolved

Does UtcNow round-trip properly? Yes.

How much precision do we emit for DateTimes?

For DateTimes in "first-class" properties, we emit three decimal digits. Example: log.Runs[0].Invocations[0].StartTimeUtc = DateTime.UtcNow; => "startTimeUtc": "2019-09-30T17:10:20.185Z".

For DateTimes in property bag properties, we emit seven decimal digits. Example: log.Runs[0].SetProperty("now", DateTime.UtcNow) => "properties": { "now": "2019-09-30T17:10:20.1851531Z" } .

The reason for the difference is subtle. First-class DateTime-valued properties are decorated with [JsonConvert(typeof(DateTimeConverter)], and DateTimeConverter uses SarifUtilities.SarifDateTimeFormatMillisecondsPrecision. But PropertyBagHolder.SetProperty calls JsonConvert.SerializeObject without a contract resolver, so the value is serialized with Newtonsoft.Json's default DateTime serialization, which (apparently) uses more precision.

SetProperty doesn't set a contract resolver because it doesn't know whether it's working with a v1 or a v2 log, so it doesn't know whether to use the v1 or v2 contract resolver. However, I can add code to use the DateTimeConverter for DateTime-valued properties. Then everything will serialize with millisecond precision.

In reply to: 329664303 [](ancestors = 329664303)

Ugh. Doing that breaks the unit test PropertyBagHolder_SetProperty_SetsDateTimeProperty, which has the comment:

DateTime instances should not be converted to UTC when persisted to a property bag. The reason is that we can't be sure that the user has an actual UTC time in hand, or even the ability to convert the property date time to UTC.

Nightmare. I need to think more about this. After lunch.

In reply to: 329717572 [](ancestors = 329717572,329664303)

Ok, I've thought about it. I can live with DateTime-valued property bag properties that are created with SetProperty having more digits than first-class DateTime-valued properties. It's not great but IMO we're at the point of diminishing returns on this.

In reply to: 329725369 [](ancestors = 329725369,329717572,329664303)

Larry Golding added 2 commits September 26, 2019 16:12

Demonstrate property bag round-tripping.

458d8bf

Fix packaging warning; update release history and package version num…

ee9b484

…ber.

ghost commented Sep 26, 2019

View reviewed changes

Larry Golding added 4 commits September 26, 2019 17:01

Verify null property and more arrays.

2c8e93c

Exhaustively test deserialized values.

7620746

Also: - Fix crash when deserializing a null string-valued property.

Add a comment.

d4c7ed3

Remove unnecessary null test.

98cf7eb

... by simplifying the way SetProperty deals with null values.

ghost commented Sep 27, 2019

View reviewed changes

Larry Golding added 3 commits September 27, 2019 12:34

Add a comment.

9c4e43f

Remove what I now think is an unnecessary comment.

f051120

Show that Scott's test works with the necessary serializer settings.

8110b4e

ghost requested review from michaelcfanning and ScottLouvau September 27, 2019 21:15

ghost marked this pull request as ready for review September 29, 2019 00:42

ScottLouvau approved these changes Sep 30, 2019

View reviewed changes

Larry Golding added 2 commits September 30, 2019 13:56

Address PR feedback.

9779734

Add test for floating point property.

2cc8318

ghost merged commit 68182f0 into master Sep 30, 2019

ghost deleted the users/lgolding/bug-1673 branch September 30, 2019 21:12

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1673 (property bag testing); Fix #1687 (NuGet packaging warning) #1688

Fix #1673 (property bag testing); Fix #1687 (NuGet packaging warning) #1688

ghost commented Sep 26, 2019 •

edited by ghost

Loading

ghost Sep 26, 2019 •

edited by ghost

Loading

ghost Sep 26, 2019 •

edited by ghost

Loading

ghost Sep 29, 2019

ghost Sep 27, 2019 •

edited by ghost

Loading

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019 •

edited by ghost

Loading

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019

ghost Sep 30, 2019

Fix #1673 (property bag testing); Fix #1687 (NuGet packaging warning) #1688

Fix #1673 (property bag testing); Fix #1687 (NuGet packaging warning) #1688

Conversation

ghost commented Sep 26, 2019 • edited by ghost Loading

ghost Sep 26, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 26, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 29, 2019

Choose a reason for hiding this comment

ghost Sep 27, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ScottLouvau Sep 30, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 30, 2019

Choose a reason for hiding this comment

ScottLouvau Sep 30, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 30, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ScottLouvau Sep 30, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 30, 2019

Choose a reason for hiding this comment

ScottLouvau Sep 30, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 30, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Sep 30, 2019

Choose a reason for hiding this comment

ghost Sep 30, 2019

Choose a reason for hiding this comment

ghost commented Sep 26, 2019 •

edited by ghost

Loading

ghost Sep 26, 2019 •

edited by ghost

Loading

ghost Sep 26, 2019 •

edited by ghost

Loading

ghost Sep 27, 2019 •

edited by ghost

Loading

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019 •

edited by ghost

Loading

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ScottLouvau Sep 30, 2019 •

edited by ghost

Loading

ghost Sep 30, 2019 •

edited by ghost

Loading